Re: [PATCH v3 01/10] drm/fourcc: Add drm_format_info_bpp() helper

[Sam Ravnborg]

Hi Geert, Daniel.

On Thu, Aug 11, 2022 at 06:11:40PM +0200, Daniel Vetter wrote:
> On Thu, Aug 11, 2022 at 09:59:39AM +0200, Geert Uytterhoeven wrote:
> > Hi Daniel,
> > 
> > On Wed, Aug 10, 2022 at 5:59 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
> > > On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote:
> > > > Add a helper to retrieve the actual number of bits per pixel for a
> > > > plane, taking into account the number of characters and pixels per
> > > > block for tiled formats.
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > > Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> > 
> > > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > >  }
> > > >  EXPORT_SYMBOL(drm_format_info_block_height);
> > > >
> > > > +/**
> > > > + * drm_format_info_bpp - number of bits per pixel
> > > > + * @info: pixel format info
> > > > + * @plane: plane index
> > > > + *
> > > > + * Returns:
> > > > + * The actual number of bits per pixel, depending on the plane index.
> > > > + */
> > > > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane)
> > > > +{
> > > > +     if (!info || plane < 0 || plane >= info->num_planes)
> > > > +             return 0;
> > > > +
> > > > +     return info->char_per_block[plane] * 8 /
> > > > +            (drm_format_info_block_width(info, plane) *
> > > > +             drm_format_info_block_height(info, plane));
> > >
> > > Do we really needs this for blocky formats where this is potentially
> > > ill-defined? I think if there's no need then this should also return 0
> > > when block_width/height != 1, it doesn't make much sense to compute bpp
> > > when it's not really bits per _pixel_.
> > 
> > Yes, we do need this.  For low-color formats, the number of bits
> > per pixel is less than eight, and block_width is larger than one.
> > That is actually the point of this patch.
> 
> Hm right, I didn't realize that this is how we have to describe the
> formats with less than 8 bpp.
> 
> I think we can include them easily with a check for char_per_block == 1
> and then making sure that the division does not have a reminder (just in
> case someone does something really funny, it could e.g. be a 332 layout or
> something like that for 3 pixels).
> 
> > > Minimally this needs to check whether the division actually makes sense or
> > > whether there's a reminder, and if there's  reminder, then fail. But that
> > > feels like a bad hack and I think we should avoid it if it's not
> > > absolutely necessary.
> > 
> > Looking at drivers/gpu/drm/drm_fourcc.c, the only supported format
> > where there can be a remainder is P030, which has 2 spare bits per
> > 32-bit word, and thus is special anyway.
> > Still, 4 * 8 / 3 = 10, so you get the correct numbers of bits for
> > the first plane.  For the second plane, you get 8 * 8 / 3 = 21,
> > but as .is_yuv = true, you have to divide that result by two again,
> > so you get 10 again.
> 
> Yeah I don't think we should describe these with bpp or cpp or anything
> like that. bpp < 8 makes sense since that's how this has been done since
> decades, but trying to extend these to funny new formats is a bad idea.
> This is also why cpp and depth refuse to compute these (or at least
> should).

Daniel and I discussed this on irc. Let me try to recap here.
Using the bits per pixel info from drm_format_info is something we shall
try to avoid as this is often a sign of the wrong abstraction/design (my
words based on the irc talk).
So we shall limit the use of drm_format_info_bpp() to what we need now,
thus blocky formats should not be supported - to try to avoid seeing
this used more than necessary.

Daniel suggested a rename to drm_format_info_legacy_bpp() to make it
obvious that this is often/always the wrong solution. I did not jump on
doing the rename as I do not know stuff good enough to tell people what
to use when this is not the right solution. The rename is simple, it is
the follow-up that keep me away.

On top of this there is a few formats in drm_drourcc that has a depth
field set which should be dropped. .depth is only for the few legacy
formats where it is used today.

We would also like to convert the fbdev helpers to drm_format_info,
and doing so will likely teach us a bit more what we need and what we
can drop.

Geert - can you give drm_format_info_bpp() a spin so it is limited to
the formats used now (not the blocky ones).

	Sam