Re: [Intel-gfx] [PATCH 36/37] drm: Add mode_config .get_format_info() hook
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.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä > > > > 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 > > Cc: intel-gfx@lists.freedesktop.org > > Signed-off-by: Ville Syrjälä > > --- > > > > 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 w
Re: [Intel-gfx] [PATCH 36/37] drm: Add mode_config .get_format_info() hook
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.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > 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 > Cc: intel-gfx@lists.freedesktop.org > Signed-off-by: Ville Syrjälä > --- > > 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 36/37] drm: Add mode_config .get_format_info() hook
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.syrj...@linux.intel.com wrote: > > >> From: Ville Syrjälä > > >> > > >> 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 > > >> Cc: intel-gfx@lists.freedesktop.org > > >> Signed-off-by: Ville Syrjälä > > >> --- > > >> > > >> 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); > >
Re: [Intel-gfx] [PATCH 36/37] drm: Add mode_config .get_format_info() hook
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.syrj...@linux.intel.com wrote: > >> From: Ville Syrjälä > >> > >> 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 > >> Cc: intel-gfx@lists.freedesktop.org > >> Signed-off-by: Ville Syrjälä > >> --- > >> > >> 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 36/37] drm: Add mode_config .get_format_info() hook
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.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä > > > > 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 > > Cc: intel-gfx@lists.freedesktop.org > > Signed-off-by: Ville Syrjälä > > --- > > 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 i
Re: [Intel-gfx] [PATCH 36/37] drm: Add mode_config .get_format_info() hook
Hi Ville, Thank you for the patch. On Friday 18 Nov 2016 21:53:12 ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > 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 > Cc: intel-gfx@lists.freedesktop.org > Signed-off-by: Ville Syrjälä > --- > 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 > @@ -