[Intel-gfx] [PATCH 36/37] drm: Add mode_config .get_format_info() hook

[Laurent Pinchart]

Hi Ville,

On Monday 21 Nov 2016 15:18:23 Ville Syrjälä wrote:
> On Sun, Nov 20, 2016 at 10:13:10AM +0200, Laurent Pinchart wrote:
> > On Friday 18 Nov 2016 21:53:12 ville.syrjala at linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> 
> >> Allow drivers to return a custom drm_format_info structure for special
> >> fb layouts. We'll use this for the compression control surface in i915.
> >> 
> >> Cc: Ben Widawsky <ben at bwidawsk.net>
> >> Cc: intel-gfx at lists.freedesktop.org
> >> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/drm_fb_cma_helper.c  |  2 +-
> >>  drivers/gpu/drm/drm_fourcc.c         | 25 +++++++++++++++++++++++++
> >>  drivers/gpu/drm/drm_framebuffer.c    |  9 +++++++--
> >>  drivers/gpu/drm/drm_modeset_helper.c |  2 +-
> >>  include/drm/drm_fourcc.h             |  6 ++++++
> >>  include/drm/drm_mode_config.h        | 15 +++++++++++++++
> >>  6 files changed, 55 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> >> b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..d7f8876cf5e9
> >> 100644
> >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> >> @@ -186,7 +186,7 @@ struct drm_framebuffer
> >> *drm_fb_cma_create_with_funcs(struct drm_device *dev, int ret;
> >> 
> >>  	int i;
> >> 
> >> -	info = drm_format_info(mode_cmd->pixel_format);
> >> +	info = drm_get_format_info(dev, mode_cmd);
> >>  	if (!info)
> >>  		return ERR_PTR(-EINVAL);
> >> 
> >> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> >> index 90d2cc8da8eb..7cfaee689f0c 100644
> >> --- a/drivers/gpu/drm/drm_fourcc.c
> >> +++ b/drivers/gpu/drm/drm_fourcc.c
> >> @@ -199,6 +199,31 @@ const struct drm_format_info *drm_format_info(u32
> >> format)
> >>  EXPORT_SYMBOL(drm_format_info);
> >> 
> >>  /**
> >> + * drm_format_info - query information for a given framebuffer
> >> configuration
> > 
> > I assume you meant drm_get_format_info()
> 
> Yes.
> 
> >> + * @dev: DRM device
> > 
> > Do we need the dev pointer ?
> 
> Not at the moment. I was thinking we might allow drivers to return a
> different set of formats based on the device type, but I'm not sure
> that's all that useful since drivers will have to check for unsupported
> formats anyway in .fb_create(). The only use case might be if you need
> to select between two different format info structs based on the device
> type, because you simply can't tell the formats apart based on the
> mode_cmd. But that sort of thing feels like a bad idea to me, and might
> as well just require that you must be able to tell formats that require
> different format intos apart based on the mode_cmd (eg. by having
> different modifiers on them).
> 
> So I guess we could just drop the 'dev' argument to make it harder for
> people to make that sort of mistake.

I think that's a good idea, yes.

> >> + * @mode_cmd: metadata from the userspace fb creation request
> >> + *
> >> + * Returns:
> >> + * The instance of struct drm_format_info that describes the pixel
> >> format, or
> >> + * NULL if the format is unsupported.
> > 
> > It would be useful to document how this function differs from
> > drm_format_info(). I also wonder whether it would make sense to completely
> > replace drm_format_info() to avoid keeping two separate but very similar
> > functions.
> 
> Yeah, that is basically what I was thinking. But I didn't feel like
> doing that myself as it looked like that might involve actual work
> in some of the drivers. I figured I'd leave it up to whoever cares
> about said drivers.

Which driver(s) are you thinking about ? If we want to make 
drm_get_format_info() the default we obviously need to pass modifiers 
directly, as in most cases we won't have a struct drm_mode_fb_cmd2 to pass to 
the function. If we remove the dev argument you could just pass NULL modifiers 
in most cases, I don't think that would involve much rework in drivers.

> >> + */
> >> +const struct drm_format_info *
> >> +drm_get_format_info(struct drm_device *dev,
> >> +		    const struct drm_mode_fb_cmd2 *mode_cmd)
> >> +{
> >> +	const struct drm_format_info *info = NULL;
> >> +
> >> +	if (dev->mode_config.funcs->get_format_info)
> >> +		info = dev->mode_config.funcs->get_format_info(dev, mode_cmd);
> >> +
> >> +	if (!info)
> >> +		info = drm_format_info(mode_cmd->pixel_format);
> >> +
> >> +	return info;
> >> +}
> >> +EXPORT_SYMBOL(drm_get_format_info);

-- 
Regards,

Laurent Pinchart