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

[Sam Ravnborg <sam@ravnborg.org>]

Hi Geert.

> > >
> > > 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?
if char_per_block[] > 1 AND block_w[] > 0 AND block_h[] > 0 should be
enough.

> 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().

drm_client_buffer_create() and drm_client_buffer_addfb() both get their
format from  drm_mode_legacy_fb_format() which do not produce any blocky
formats - so they are good.

drm_mode_getfb() looks up a framebuffer originally created using one of
the above (I think), so here it should also be fine.
I do not see the need to push this to fixes, so it has a full cycle to
mature if it causes issues.

	Sam