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@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(-)
> > >>
> > >> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
