| Message ID | 20180312204035.31387-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ville Syrjala <ville.syrjala@linux.intel.com> writes: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > To make life easier for drivers, let's have the core check that the > requested pixel format is supported by at least one plane when creating > a new framebuffer. > > This eases the burden on drivers by making sure they'll never get > requests to create fbs with unsupported pixel formats. Thanks to the > new .fb_modifier() hook this check can now be done whether the request > specifies the modifier directly or driver has to deduce it from the > gem bo tiling (or via any other method really). > > v0: Accept anyformat if the driver doesn't do planes (Eric) > s/planes_have_format/any_plane_has_format/ (Eric) > Check the modifier as well since we already have a function > that does both > v3: Don't do the check in the core since we may not know the > modifier yet, instead export the function and let drivers > call it themselves > v4: Unexport the functiona and put the format_default check back > since this will again be called by the core, ie. undo v3 ;) > > Cc: Eric Anholt <eric@anholt.net> > Testcase: igt/kms_addfb_basic/expected-formats > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 21d3d51eb261..e618a6b728d4 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -152,6 +152,26 @@ static int fb_plane_height(int height, > return DIV_ROUND_UP(height, format->vsub); > } > > +static bool any_plane_has_format(struct drm_device *dev, > + u32 format, u64 modifier) > +{ > + struct drm_plane *plane; > + > + drm_for_each_plane(plane, dev) { > + /* > + * In case the driver doesn't really do > + * planes we have to accept any format here. > + */ > + if (plane->format_default) > + return true; > + > + if (drm_plane_check_pixel_format(plane, format, modifier) == 0) > + return true; > + } > + > + return false; > +} This drm_plane_check_pixel_format() will always fail for VC4's SAND modifiers or VC5's UIF modifiers, where we're using the middle 48 bits as a bit of metadata (like how we have horizontal stride passed outside of the modifier) and you can't list all of the possible values in an array on the plane.
On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote: > Ville Syrjala <ville.syrjala@linux.intel.com> writes: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > To make life easier for drivers, let's have the core check that the > > requested pixel format is supported by at least one plane when creating > > a new framebuffer. > > > > This eases the burden on drivers by making sure they'll never get > > requests to create fbs with unsupported pixel formats. Thanks to the > > new .fb_modifier() hook this check can now be done whether the request > > specifies the modifier directly or driver has to deduce it from the > > gem bo tiling (or via any other method really). > > > > v0: Accept anyformat if the driver doesn't do planes (Eric) > > s/planes_have_format/any_plane_has_format/ (Eric) > > Check the modifier as well since we already have a function > > that does both > > v3: Don't do the check in the core since we may not know the > > modifier yet, instead export the function and let drivers > > call it themselves > > v4: Unexport the functiona and put the format_default check back > > since this will again be called by the core, ie. undo v3 ;) > > > > Cc: Eric Anholt <eric@anholt.net> > > Testcase: igt/kms_addfb_basic/expected-formats > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > index 21d3d51eb261..e618a6b728d4 100644 > > --- a/drivers/gpu/drm/drm_framebuffer.c > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > @@ -152,6 +152,26 @@ static int fb_plane_height(int height, > > return DIV_ROUND_UP(height, format->vsub); > > } > > > > +static bool any_plane_has_format(struct drm_device *dev, > > + u32 format, u64 modifier) > > +{ > > + struct drm_plane *plane; > > + > > + drm_for_each_plane(plane, dev) { > > + /* > > + * In case the driver doesn't really do > > + * planes we have to accept any format here. > > + */ > > + if (plane->format_default) > > + return true; > > + > > + if (drm_plane_check_pixel_format(plane, format, modifier) == 0) > > + return true; > > + } > > + > > + return false; > > +} > > This drm_plane_check_pixel_format() will always fail for VC4's SAND > modifiers or VC5's UIF modifiers, where we're using the middle 48 bits > as a bit of metadata (like how we have horizontal stride passed outside > of the modifier) and you can't list all of the possible values in an > array on the plane. Hmm. drm_atomic_plane_check() etc. call this thing as well. How does that manage to work currently?
On Thu, Mar 15, 2018 at 07:48:02PM +0200, Ville Syrjälä wrote: > On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote: > > Ville Syrjala <ville.syrjala@linux.intel.com> writes: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > To make life easier for drivers, let's have the core check that the > > > requested pixel format is supported by at least one plane when creating > > > a new framebuffer. > > > > > > This eases the burden on drivers by making sure they'll never get > > > requests to create fbs with unsupported pixel formats. Thanks to the > > > new .fb_modifier() hook this check can now be done whether the request > > > specifies the modifier directly or driver has to deduce it from the > > > gem bo tiling (or via any other method really). > > > > > > v0: Accept anyformat if the driver doesn't do planes (Eric) > > > s/planes_have_format/any_plane_has_format/ (Eric) > > > Check the modifier as well since we already have a function > > > that does both > > > v3: Don't do the check in the core since we may not know the > > > modifier yet, instead export the function and let drivers > > > call it themselves > > > v4: Unexport the functiona and put the format_default check back > > > since this will again be called by the core, ie. undo v3 ;) > > > > > > Cc: Eric Anholt <eric@anholt.net> > > > Testcase: igt/kms_addfb_basic/expected-formats > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++ > > > 1 file changed, 30 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > > index 21d3d51eb261..e618a6b728d4 100644 > > > --- a/drivers/gpu/drm/drm_framebuffer.c > > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > > @@ -152,6 +152,26 @@ static int fb_plane_height(int height, > > > return DIV_ROUND_UP(height, format->vsub); > > > } > > > > > > +static bool any_plane_has_format(struct drm_device *dev, > > > + u32 format, u64 modifier) > > > +{ > > > + struct drm_plane *plane; > > > + > > > + drm_for_each_plane(plane, dev) { > > > + /* > > > + * In case the driver doesn't really do > > > + * planes we have to accept any format here. > > > + */ > > > + if (plane->format_default) > > > + return true; > > > + > > > + if (drm_plane_check_pixel_format(plane, format, modifier) == 0) > > > + return true; > > > + } > > > + > > > + return false; > > > +} > > > > This drm_plane_check_pixel_format() will always fail for VC4's SAND > > modifiers or VC5's UIF modifiers, where we're using the middle 48 bits > > as a bit of metadata (like how we have horizontal stride passed outside > > of the modifier) and you can't list all of the possible values in an > > array on the plane. > > Hmm. drm_atomic_plane_check() etc. call this thing as well. How does > that manage to work currently? Maybe it doesn't. I added the modifier checks in commit 23163a7d4b032489d375099d56571371c0456980 Author: Ville Syrjälä <ville.syrjala@linux.intel.com> AuthorDate: Fri Dec 22 21:22:30 2017 +0200 Commit: Ville Syrjälä <ville.syrjala@linux.intel.com> CommitDate: Mon Feb 26 16:29:47 2018 +0200 drm: Check that the plane supports the request format+modifier combo Maybe that broke vc4? Hmm. So either we need to stop checking against the modifiers array and rely purely or .format_mod_supported(), or we need to somehow get the driver to reduce the modifier to its base form. I guess we could make .fb_modifier() do that and call it also for addfb with modifiers. And I'd need to undo some of the modifier[0] vs. deduced modifier changes I did to framebuffer_check(), and we'd need to preserve the original modifier in the request for .fb_create(). Oh, but that wouldn't allow returning a non-base modifier from .fb_modifuer() for the !modifiers case. This is turning slightly more tricky than I had hoped... I guess relying on .format_mod_supported() might be what we need to do. Unfortunately it does mean that the .format_mod_supported() implementations must be prepared for modifiers that were not registered with the plane. Which does feel quite a bit more fragile.
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 21d3d51eb261..e618a6b728d4 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -152,6 +152,26 @@ static int fb_plane_height(int height, return DIV_ROUND_UP(height, format->vsub); } +static bool any_plane_has_format(struct drm_device *dev, + u32 format, u64 modifier) +{ + struct drm_plane *plane; + + drm_for_each_plane(plane, dev) { + /* + * In case the driver doesn't really do + * planes we have to accept any format here. + */ + if (plane->format_default) + return true; + + if (drm_plane_check_pixel_format(plane, format, modifier) == 0) + return true; + } + + return false; +} + static int framebuffer_check(struct drm_device *dev, struct drm_mode_fb_cmd2 *r, u64 modifier) @@ -183,6 +203,16 @@ static int framebuffer_check(struct drm_device *dev, return -EINVAL; } + if (!any_plane_has_format(dev, r->pixel_format, modifier)) { + struct drm_format_name_buf format_name; + + DRM_DEBUG_KMS("unsupported pixel format %s / modifier 0x%llx\n", + drm_get_format_name(r->pixel_format, + &format_name), + modifier); + return -EINVAL; + } + for (i = 0; i < info->num_planes; i++) { unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i);