Re: [PATCH 36/37] drm: Add mode_config .get_format_info() hook

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 21, 2016 at 03:23:19PM +0200, Laurent Pinchart wrote:
> 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@linux.intel.com wrote:
> > >> From: Ville Syrjälä <ville.syrjala@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@bwidawsk.net>
> > >> Cc: intel-gfx@lists.freedesktop.org
> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@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 ?

The ones that my cocci stuff couldn't convert over to fb->format.

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

fb->format is probably the right choice in most cases. But some drivers
seemed to have some kind of internal format info struct instead which
was in the way of doing a trivial conversion. I didn't want to start
doing non-trivial conversions since the series was already way too big
as is.

> 
> > >> + */
> > >> +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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx