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

[Ville Syrjälä]

On Mon, Nov 21, 2016 at 03:42:34PM +0200, Laurent Pinchart wrote:
> 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 ?

I think I already did everything except the "mark as deprecated" part.
And adding that last bit into the patch would be trivial.

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

I suppose drivers could just embed the drm_format_info into some bigger
structure which has additional information. One thing that makes that a
little harder is the fact that the regular format info structures aren't
exported to drivers in a way that they could just:

static const struct my_format_info format_info_foo {
	.base = drm_format_info_foo,
	...
};

So they'd have to duplicate the information which is maybe a little
error prone. OTOH that problem already exists to some degree with my
.get_format_info() hook (which somewhat makes me think we should just
put all of that stuff into drm_fourcc.c).

But I suppose exporting the drm_format_info structs could be done as
well, and then drivers could do whatever they want.

-- 
Ville Syrjälä
Intel OTC