| Message ID | 1479498793-31021-37-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ville, Thank you for the patch. On Friday 18 Nov 2016 21:53:12 ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@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@bwidawsk.net> > Cc: intel-gfx@lists.freedesktop.org > Signed-off-by: Ville Syrjälä <ville.syrjala@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(-) > > 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() > + * @dev: DRM device Do we need the dev pointer ? > + * @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. > + */ > +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
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@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@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@bwidawsk.net> > > Cc: intel-gfx@lists.freedesktop.org > > Signed-off-by: Ville Syrjälä <ville.syrjala@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(-) > > > > 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
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@linux.intel.com wrote: > >> From: Ville Syrjälä <ville.syrjala@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@bwidawsk.net> > >> Cc: intel-gfx@lists.freedesktop.org > >> Signed-off-by: Ville Syrjälä <ville.syrjala@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(-) > >> > >> 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);
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@linux.intel.com wrote: > > >> From: Ville Syrjälä <ville.syrjala@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@bwidawsk.net> > > >> Cc: intel-gfx@lists.freedesktop.org > > >> Signed-off-by: Ville Syrjälä <ville.syrjala@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(-) > > >> > > >> 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
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@linux.intel.com wrote: > >>>> From: Ville Syrjälä <ville.syrjala@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@bwidawsk.net> > >>>> Cc: intel-gfx@lists.freedesktop.org > >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@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 ? > > 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.
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@linux.intel.com wrote: > > >>>> From: Ville Syrjälä <ville.syrjala@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@bwidawsk.net> > > >>>> Cc: intel-gfx@lists.freedesktop.org > > >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@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.
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 + * @dev: DRM device + * @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. + */ +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