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@xxxxxxxxxxxxxxx wrote: > >>>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >>>> > >>>> 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@xxxxxxxxxxxx> > >>>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >>>> --- > >>>> > >>>> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
