Re: [PATCH 2/3] drm: Make sure at least one plane supports the fb format

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Mar 15, 2018 at 08:03:44PM +0200, Ville Syrjälä wrote:
> 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.

And that last apporach would be annoying for i915. So I think the other
apporach is better.

Fortunately it seems your SAND modifiers aren't upstream yet so I didn't
break them :) I had a quick look at your github and it looks like the
first apporach would work.

Something like this is what I had in mind. Loosk like you could plug in
fourcc_mod_broadcom_mod() almost directly into .base_modifier().

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index a5d1fc7e8a37..250f66d51c2c 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -552,6 +552,8 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 int drm_plane_check_pixel_format(struct drm_plane *plane,
 				 u32 format, u64 modifier)
 {
+	struct drm_device *dev = plane->dev;
+	u64 base_modifier;
 	unsigned int i;
 
 	for (i = 0; i < plane->format_count; i++) {
@@ -564,8 +566,13 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 	if (!plane->modifier_count)
 		return 0;
 
+	if (dev->mode_config.funcs->base_modifier)
+		base_modifier = dev->mode_config.funcs->base_modifier(dev, modifier);
+	else
+		base_modifier = modifier;
+
 	for (i = 0; i < plane->modifier_count; i++) {
-		if (modifier == plane->modifiers[i])
+		if (base_modifier == plane->modifiers[i])
 			break;
 	}
 	if (i == plane->modifier_count)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index c74aed292b58..2ff2438263ef 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -45,6 +45,19 @@ struct drm_display_mode;
  * involve drivers.
  */
 struct drm_mode_config_funcs {
+	/**
+	 * @base_modifier:
+	 *
+	 * Reduce the passed in modifier to its base form. This optional
+	 * hook needs to be provided by any driver that embeds extra
+	 * metadata within the format modifier.
+	 *
+	 * RETURNS:
+	 * The base form of the modifier with any extra metadata
+	 * cleared out.
+	 */
+	u64 (*base_modifier)(struct drm_device *dev, u64 modifier);
+
 	/**
 	 * @fb_modifier:
 	 *
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index f7bf4a48b1c3..5e26d2a5f6ab 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -489,8 +489,6 @@ enum drm_plane_type {
  * @format_types: array of formats supported by this plane
  * @format_count: number of formats supported
  * @format_default: driver hasn't supplied supported formats for the plane
- * @modifiers: array of modifiers supported by this plane
- * @modifier_count: number of modifiers supported
  * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by
  * 	drm_mode_set_config_internal() to implement correct refcounting.
  * @funcs: helper functions
@@ -524,7 +522,15 @@ struct drm_plane {
 	unsigned int format_count;
 	bool format_default;
 
+	/**
+	 * @modifiers: Array of modifiers supported by this plane. If
+	 * the driver embeds any extra metadata within modifiers these
+	 * modifiers must be in their base form.
+	 */
 	uint64_t *modifiers;
+	/**
+	 * @modifier_count: number of modifiers supported
+	 */
 	unsigned int modifier_count;
 
 	/**
-- 
2.16.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx