Nouveau - Jan 2025 - [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

On Thu, Jan 16, 2025 at 11:03?AM Tomi Valkeinen
<tomi.valkeinen at ideasonboard.com> wrote:
> On 16/01/2025 10:09, Thomas Zimmermann wrote:
> > Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
> > [...]
> >>
> >> My point is that we have the current UAPI, and we have userspace
using
> >> it, but we don't have clear rules what the ioctl does with
specific
> >> parameters, and we don't document how it has to be used.
> >>
> >> Perhaps the situation is bad, and all we can really say is that
> >> CREATE_DUMB only works for use with simple RGB formats, and the
> >> behavior for all other formats is platform specific. But I think
even
> >> that would be valuable in the UAPI docs.
> >
> > To be honest, I would not want to specify behavior for anything but
the
> > linear RGB formats. If anything, I'd take Daniel's reply mail
for
> > documentation as-is. Anyone stretching the UAPI beyond RGB is on their
own.
> >
> >> Thinking about this, I wonder if this change is good for omapdrm
or
> >> xilinx (probably other platforms too that support non-simple
non-RGB
> >> formats via dumb buffers): without this patch, in both drivers,
the
> >> pitch calculations just take the bpp as bit-per-pixels, align it
up,
> >> and that's it.
> >>
> >> With this patch we end up using drm_driver_color_mode_format(),
and
> >> aligning buffers according to RGB formats figured out via
heuristics.
> >> It does happen to work, for the formats I tested, but it sounds
like
> >> something that might easily not work, as it's doing
adjustments based
> >> on wrong format.
> >>
> >> Should we have another version of drm_mode_size_dumb() which just
> >> calculates using the bpp, without the
drm_driver_color_mode_format()
> >> path? Or does the drm_driver_color_mode_format() path provide some
> >> value for the drivers that do not currently do anything similar?
> >
> > With the RGB-only rule, using drm_driver_color_mode_format() makes
> > sense. It aligns dumb buffers and video=, provides error checking, and
> > overall harmonizes code. The fallback is only required because of the
> > existing odd cases that already bend the UAPI's rules.
>
> I have to disagree here.
>
> On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
> buffers are the only buffers you can get from the DRM driver. The dumb
> buffers have been used to allocate linear and multiplanar YUV buffers
> for a very long time on those platforms.
>
> I tried to look around, but I did not find any mentions that CREATE_DUMB
> should only be used for RGB buffers. Is anyone outside the core
> developers even aware of it?
>
> If we don't use dumb buffers there, where do we get the buffers? Maybe
> from a v4l2 device or from a gpu device, but often you don't have
those.
> DMA_HEAP is there, of course.
Why can't there be a variant that takes a proper fourcc format instead of
an imprecise bpp value?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at
linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or
something like that.
                                -- Linus Torvalds

Hi,

On 16/01/2025 12:17, Geert Uytterhoeven wrote:
> On Thu, Jan 16, 2025 at 11:03?AM Tomi Valkeinen
> <tomi.valkeinen at ideasonboard.com> wrote:
>> On 16/01/2025 10:09, Thomas Zimmermann wrote:
>>> Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
>>> [...]
>>>>
>>>> My point is that we have the current UAPI, and we have
userspace using
>>>> it, but we don't have clear rules what the ioctl does with
specific
>>>> parameters, and we don't document how it has to be used.
>>>>
>>>> Perhaps the situation is bad, and all we can really say is that
>>>> CREATE_DUMB only works for use with simple RGB formats, and the
>>>> behavior for all other formats is platform specific. But I
think even
>>>> that would be valuable in the UAPI docs.
>>>
>>> To be honest, I would not want to specify behavior for anything but
the
>>> linear RGB formats. If anything, I'd take Daniel's reply
mail for
>>> documentation as-is. Anyone stretching the UAPI beyond RGB is on
their own.
>>>
>>>> Thinking about this, I wonder if this change is good for
omapdrm or
>>>> xilinx (probably other platforms too that support non-simple
non-RGB
>>>> formats via dumb buffers): without this patch, in both drivers,
the
>>>> pitch calculations just take the bpp as bit-per-pixels, align
it up,
>>>> and that's it.
>>>>
>>>> With this patch we end up using drm_driver_color_mode_format(),
and
>>>> aligning buffers according to RGB formats figured out via
heuristics.
>>>> It does happen to work, for the formats I tested, but it sounds
like
>>>> something that might easily not work, as it's doing
adjustments based
>>>> on wrong format.
>>>>
>>>> Should we have another version of drm_mode_size_dumb() which
just
>>>> calculates using the bpp, without the
drm_driver_color_mode_format()
>>>> path? Or does the drm_driver_color_mode_format() path provide
some
>>>> value for the drivers that do not currently do anything
similar?
>>>
>>> With the RGB-only rule, using drm_driver_color_mode_format() makes
>>> sense. It aligns dumb buffers and video=, provides error checking,
and
>>> overall harmonizes code. The fallback is only required because of
the
>>> existing odd cases that already bend the UAPI's rules.
>>
>> I have to disagree here.
>>
>> On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
>> buffers are the only buffers you can get from the DRM driver. The dumb
>> buffers have been used to allocate linear and multiplanar YUV buffers
>> for a very long time on those platforms.
>>
>> I tried to look around, but I did not find any mentions that
CREATE_DUMB
>> should only be used for RGB buffers. Is anyone outside the core
>> developers even aware of it?
>>
>> If we don't use dumb buffers there, where do we get the buffers?
Maybe
>> from a v4l2 device or from a gpu device, but often you don't have
those.
>> DMA_HEAP is there, of course.
> 
> Why can't there be a variant that takes a proper fourcc format instead
of
> an imprecise bpp value?
There can, but it's somewhat a different topic, although it's been 
covered a bit in this thread.

My specific concern here is the current CREATE_DUMB, and (not) changing 
how it behaves.

  Tomi

On Thu, Jan 16, 2025 at 11:17:50AM +0100, Geert Uytterhoeven
wrote:
> On Thu, Jan 16, 2025 at 11:03?AM Tomi Valkeinen
> <tomi.valkeinen at ideasonboard.com> wrote:
> > On 16/01/2025 10:09, Thomas Zimmermann wrote:
> > > Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
> > > [...]
> > >>
> > >> My point is that we have the current UAPI, and we have
userspace using
> > >> it, but we don't have clear rules what the ioctl does
with specific
> > >> parameters, and we don't document how it has to be used.
> > >>
> > >> Perhaps the situation is bad, and all we can really say is
that
> > >> CREATE_DUMB only works for use with simple RGB formats, and
the
> > >> behavior for all other formats is platform specific. But I
think even
> > >> that would be valuable in the UAPI docs.
> > >
> > > To be honest, I would not want to specify behavior for anything
but the
> > > linear RGB formats. If anything, I'd take Daniel's reply
mail for
> > > documentation as-is. Anyone stretching the UAPI beyond RGB is on
their own.
> > >
> > >> Thinking about this, I wonder if this change is good for
omapdrm or
> > >> xilinx (probably other platforms too that support non-simple
non-RGB
> > >> formats via dumb buffers): without this patch, in both
drivers, the
> > >> pitch calculations just take the bpp as bit-per-pixels, align
it up,
> > >> and that's it.
> > >>
> > >> With this patch we end up using
drm_driver_color_mode_format(), and
> > >> aligning buffers according to RGB formats figured out via
heuristics.
> > >> It does happen to work, for the formats I tested, but it
sounds like
> > >> something that might easily not work, as it's doing
adjustments based
> > >> on wrong format.
> > >>
> > >> Should we have another version of drm_mode_size_dumb() which
just
> > >> calculates using the bpp, without the
drm_driver_color_mode_format()
> > >> path? Or does the drm_driver_color_mode_format() path provide
some
> > >> value for the drivers that do not currently do anything
similar?
> > >
> > > With the RGB-only rule, using drm_driver_color_mode_format()
makes
> > > sense. It aligns dumb buffers and video=, provides error
checking, and
> > > overall harmonizes code. The fallback is only required because of
the
> > > existing odd cases that already bend the UAPI's rules.
> >
> > I have to disagree here.
> >
> > On the platforms I have been using (omap, tidss, xilinx, rcar) the
dumb
> > buffers are the only buffers you can get from the DRM driver. The dumb
> > buffers have been used to allocate linear and multiplanar YUV buffers
> > for a very long time on those platforms.
> >
> > I tried to look around, but I did not find any mentions that
CREATE_DUMB
> > should only be used for RGB buffers. Is anyone outside the core
> > developers even aware of it?
> >
> > If we don't use dumb buffers there, where do we get the buffers?
Maybe
> > from a v4l2 device or from a gpu device, but often you don't have
those.
> > DMA_HEAP is there, of course.
> 
> Why can't there be a variant that takes a proper fourcc format instead
of
> an imprecise bpp value?
Backwards compatibility. We can add an IOCTL for YUV / etc. But
userspace must be able to continue allocating YUV buffers through
CREATE_DUMB.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at
linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
But
> when I'm talking to journalists I just say "programmer" or
something like that.
>                                 -- Linus Torvalds
-- 
With best wishes
Dmitry

Hi,

On 2025/1/16 18:17, Geert Uytterhoeven wrote:
> On Thu, Jan 16, 2025 at 11:03?AM Tomi Valkeinen
> <tomi.valkeinen at ideasonboard.com> wrote:
>> On 16/01/2025 10:09, Thomas Zimmermann wrote:
>>> Am 15.01.25 um 15:20 schrieb Tomi Valkeinen:
>>> [...]
>>>> My point is that we have the current UAPI, and we have
userspace using
>>>> it, but we don't have clear rules what the ioctl does with
specific
>>>> parameters, and we don't document how it has to be used.
>>>>
>>>> Perhaps the situation is bad, and all we can really say is that
>>>> CREATE_DUMB only works for use with simple RGB formats, and the
>>>> behavior for all other formats is platform specific. But I
think even
>>>> that would be valuable in the UAPI docs.
>>> To be honest, I would not want to specify behavior for anything but
the
>>> linear RGB formats. If anything, I'd take Daniel's reply
mail for
>>> documentation as-is. Anyone stretching the UAPI beyond RGB is on
their own.
>>>
>>>> Thinking about this, I wonder if this change is good for
omapdrm or
>>>> xilinx (probably other platforms too that support non-simple
non-RGB
>>>> formats via dumb buffers): without this patch, in both drivers,
the
>>>> pitch calculations just take the bpp as bit-per-pixels, align
it up,
>>>> and that's it.
>>>>
>>>> With this patch we end up using drm_driver_color_mode_format(),
and
>>>> aligning buffers according to RGB formats figured out via
heuristics.
>>>> It does happen to work, for the formats I tested, but it sounds
like
>>>> something that might easily not work, as it's doing
adjustments based
>>>> on wrong format.
>>>>
>>>> Should we have another version of drm_mode_size_dumb() which
just
>>>> calculates using the bpp, without the
drm_driver_color_mode_format()
>>>> path? Or does the drm_driver_color_mode_format() path provide
some
>>>> value for the drivers that do not currently do anything
similar?
>>> With the RGB-only rule, using drm_driver_color_mode_format() makes
>>> sense. It aligns dumb buffers and video=, provides error checking,
and
>>> overall harmonizes code. The fallback is only required because of
the
>>> existing odd cases that already bend the UAPI's rules.
>> I have to disagree here.
>>
>> On the platforms I have been using (omap, tidss, xilinx, rcar) the dumb
>> buffers are the only buffers you can get from the DRM driver. The dumb
>> buffers have been used to allocate linear and multiplanar YUV buffers
>> for a very long time on those platforms.
>>
>> I tried to look around, but I did not find any mentions that
CREATE_DUMB
>> should only be used for RGB buffers. Is anyone outside the core
>> developers even aware of it?
>>
>> If we don't use dumb buffers there, where do we get the buffers?
Maybe
>> from a v4l2 device or from a gpu device, but often you don't have
those.
>> DMA_HEAP is there, of course.
> Why can't there be a variant that takes a proper fourcc format instead
of
> an imprecise bpp value?
The 'flags' parameter of the 'struct drm_mode_create_dumb'
doesn't gets
in used so far, I guess the situation will be much better if passing a
correct fourcc code from the user-space to kernel is allowed.

> Gr{oetje,eeting}s,
>
>                          Geert
>
-- 
Best regards,
Sui