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

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

Hi,

On 16/01/2025 12:17, Geert Uytterhoeven wrote:
> On Thu, Jan 16, 2025 at 11:03 AM Tomi Valkeinen
> <tomi.valkeinen@xxxxxxxxxxxxxxxx> 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