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 ? 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.
> >> + */
> >> +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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
