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

[Eric Anholt]

Ville Syrjälä <ville.syrjala at linux.intel.com> writes:

> 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 at linux.intel.com> writes:
>> > > 
>> > > > From: Ville Syrjälä <ville.syrjala at 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 at anholt.net>
>> > > > Testcase: igt/kms_addfb_basic/expected-formats
>> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at 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 at linux.intel.com>
>> AuthorDate: Fri Dec 22 21:22:30 2017 +0200
>> Commit:     Ville Syrjälä <ville.syrjala at 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().

I think just trusting format_mod_supported's result makes more sense.
See the patch series I just sent.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20180316/1fe9df1e/attachment.sig>