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

[Geert Uytterhoeven]

Hi Sam,

On Thu, Aug 11, 2022 at 8:29 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
> 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:
> > > 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).

You mean return 0 if char_per_block[] > 1?
I'm not sure it's actually safe to do so (and make this change this late
in the development cycle), as this is used in drm_client_buffer_create(),
drm_client_buffer_addfb(), and drm_mode_getfb().  Some of them do
rely on bpp to be non-zero.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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