[PATCHv4 05/36] drm/gem-fb-helper: Add generic afbc size checks

[Liviu Dudau]

On Mon, Dec 16, 2019 at 07:41:23PM +0100, Andrzej Pietrasiewicz wrote:
> Hi Liviu,
> 
> W dniu 16.12.2019 o 18:19, Liviu Dudau pisze:
> > Hi Andrzej,
> > 
> > On Fri, Dec 13, 2019 at 04:58:36PM +0100, Andrzej Pietrasiewicz wrote:
> > > Extend the size-checking special function to handle afbc.
> > > 
> > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p at collabora.com>
> > > ---
> > >   drivers/gpu/drm/drm_fourcc.c                 | 10 +++-
> > >   drivers/gpu/drm/drm_framebuffer.c            |  3 +
> > >   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 60 ++++++++++++++++++--
> > >   include/drm/drm_gem_framebuffer_helper.h     | 16 ++++++
> > >   4 files changed, 82 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > > index d14dd7c86020..9ac2175c5bee 100644
> > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > @@ -323,8 +323,14 @@ drm_get_format_info(struct drm_device *dev,
> > >   {
> > >   	const struct drm_format_info *info = NULL;
> > > -	if (dev->mode_config.funcs->get_format_info)
> > > -		info = dev->mode_config.funcs->get_format_info(mode_cmd);
> > > +	/* bypass driver callback if afbc */
> > > +	if (!drm_is_afbc(mode_cmd->modifier[0]))
> > > +		if (dev->mode_config.funcs->get_format_info) {
> > > +			const struct drm_mode_config_funcs *funcs;
> > > +
> > > +			funcs = dev->mode_config.funcs;
> > > +			info = funcs->get_format_info(mode_cmd);
> > > +		}
> > 
> > What has this change to do with the rest of the patch? Also, I think this goes
> > against the idea that an AFBC-aware driver might return better data about the format
> > info than the drm_format_info() code.
> > 
> 
> The reason is the conclusion of my talk with danvet on irc:
> 
> https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2019-11-13&show_html=true
> 
> I followed his advice - if I understood him correctly, that is.

Yeah, I don't necessarily agree with danvet here. I think a better approach is to
still let the driver have a say in getting the format info, but if the hook is not
present or if it returns NULL then apply the AFBC code before (or as an alternative to)
the rest of the generic code.

> 
> > As a bikeshed, I know it is useful for debugging to turn the oneliner into 3, but it
> > feels like not necessary here.
> 
> 80 chars per line. If kept in one line, the limit is exceeded
> with an additional indentation level present.

DRM subsystem has never enforced that and there are plenty of instances in the core DRM 
code where that rule gets ignored. We know that mode_config.funcs is never NULL and
that the get_format_info() hook is always populated, so we don't really gain anything
from splitting it into multiple lines.

Best regards,
Liviu

> 
> Regards,
> 
> Andrzej

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯