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

[Laurent Pinchart]

Hi Ville,

On Monday 21 Nov 2016 15:31:57 Ville Syrjälä wrote:
> On Mon, Nov 21, 2016 at 03:23:19PM +0200, Laurent Pinchart wrote:
> > 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(-)

[snip]

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

How about at least making drm_get_format_info() the default but converting 
what can be converted with coccinelle, and marking drm_format_info() as 
deprecated ?

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

That's an interesting point I wanted to also mention. We have drivers that 
include formats information tables duplicating the one in the DRM core, with 
additional driver-specific information (see rcar_du_format_info() in 
drivers/gpu/drm/rcar-du/rcar_du_kms.c for instance). I wonder whether it would 
be possible to come up with a simple API that would allow providing those 
driver-specific data to the core, and get them back from the 
drm_get_format_info() function.

-- 
Regards,

Laurent Pinchart