On Sun, Nov 20, 2016 at 10:13:10AM +0200, Laurent Pinchart wrote:
> Hi Ville,
>
> Thank you for the patch.
>
> 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.
>
> > + * @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.
>
> > + */
> > +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);
> > +
> > +/**
> > * drm_format_num_planes - get the number of planes for format
> > * @format: pixel format (DRM_FORMAT_*)
> > *
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c
> > b/drivers/gpu/drm/drm_framebuffer.c index 94ddab41f24f..292930a5dcc2 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -126,11 +126,13 @@ int drm_mode_addfb(struct drm_device *dev,
> > return 0;
> > }
> >
> > -static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> > +static int framebuffer_check(struct drm_device *dev,
> > + const struct drm_mode_fb_cmd2 *r)
> > {
> > const struct drm_format_info *info;
> > int i;
> >
> > + /* check if the format is supported at all */
> > info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
> > if (!info) {
> > struct drm_format_name_buf format_name;
> > @@ -140,6 +142,9 @@ static int framebuffer_check(const struct
> > drm_mode_fb_cmd2 *r) return -EINVAL;
> > }
> >
> > + /* now let the driver pick its own format info */
> > + info = drm_get_format_info(dev, r);
> > +
> > if (r->width == 0 || r->width % info->hsub) {
> > DRM_DEBUG_KMS("bad framebuffer width %u\n", r->width);
> > return -EINVAL;
> > @@ -263,7 +268,7 @@ drm_internal_framebuffer_create(struct drm_device *dev,
> > return ERR_PTR(-EINVAL);
> > }
> >
> > - ret = framebuffer_check(r);
> > + ret = framebuffer_check(dev, r);
> > if (ret)
> > return ERR_PTR(ret);
> >
> > diff --git a/drivers/gpu/drm/drm_modeset_helper.c
> > b/drivers/gpu/drm/drm_modeset_helper.c index 5b051859b8d3..f78df06a940d
> > 100644
> > --- a/drivers/gpu/drm/drm_modeset_helper.c
> > +++ b/drivers/gpu/drm/drm_modeset_helper.c
> > @@ -75,7 +75,7 @@ void drm_helper_mode_fill_fb_struct(struct drm_device
> > *dev, int i;
> >
> > fb->dev = dev;
> > - fb->format = drm_format_info(mode_cmd->pixel_format);
> > + fb->format = drm_get_format_info(dev, mode_cmd);
> > fb->width = mode_cmd->width;
> > fb->height = mode_cmd->height;
> > for (i = 0; i < 4; i++) {
> > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > index fcc08da850c8..6942e84b6edd 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -25,6 +25,9 @@
> > #include <linux/types.h>
> > #include <uapi/drm/drm_fourcc.h>
> >
> > +struct drm_device;
> > +struct drm_mode_fb_cmd2;
> > +
> > /**
> > * struct drm_format_info - information about a DRM format
> > * @format: 4CC format identifier (DRM_FORMAT_*)
> > @@ -55,6 +58,9 @@ struct drm_format_name_buf {
> >
> > const struct drm_format_info *__drm_format_info(u32 format);
> > const struct drm_format_info *drm_format_info(u32 format);
> > +const struct drm_format_info *
> > +drm_get_format_info(struct drm_device *dev,
> > + const struct drm_mode_fb_cmd2 *mode_cmd);
> > uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
> > int drm_format_num_planes(uint32_t format);
> > int drm_format_plane_cpp(uint32_t format, int plane);
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index bf9991b20611..80bb7e49bf8e 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -34,6 +34,7 @@ struct drm_file;
> > struct drm_device;
> > struct drm_atomic_state;
> > struct drm_mode_fb_cmd2;
> > +struct drm_format_info;
> >
> > /**
> > * struct drm_mode_config_funcs - basic driver provided mode setting
> > functions @@ -70,6 +71,20 @@ struct drm_mode_config_funcs {
> > const struct drm_mode_fb_cmd2
> *mode_cmd);
> >
> > /**
> > + * @get_format_info:
> > + *
> > + * Allows a driver to return custom format information for special
> > + * fb layouts (eg. ones with auxiliary compresssion control planes).
> > + *
> > + * RETURNS:
> > + *
> > + * The format information specific to the given fb metadata, or
> > + * NULL if none is found.
> > + */
> > + const struct drm_format_info *(*get_format_info)(struct drm_device
> *dev,
> > + const struct
> drm_mode_fb_cmd2 *mode_cmd);
> > +
> > + /**
> > * @output_poll_changed:
> > *
> > * Callback used by helpers to inform the driver of output
> configuration
>
> --
> Regards,
>
> Laurent Pinchart
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
