From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH 2/3] drm: Make sure at least one plane supports the fb format Date: Fri, 16 Mar 2018 15:07:18 -0700 Message-ID: <877eqbad49.fsf@anholt.net> References: <20180312204035.31387-1-ville.syrjala@linux.intel.com> <20180312204035.31387-2-ville.syrjala@linux.intel.com> <87woydgrra.fsf@anholt.net> <20180315174802.GZ5453@intel.com> <20180315180344.GA5453@intel.com> <20180315184817.GC5453@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0576097000==" Return-path: In-Reply-To: <20180315184817.GC5453@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ville =?utf-8?B?U3lyasOkbMOk?= Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0576097000== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Ville Syrj=C3=A4l=C3=A4 writes: > On Thu, Mar 15, 2018 at 08:03:44PM +0200, Ville Syrj=C3=A4l=C3=A4 wrote: >> On Thu, Mar 15, 2018 at 07:48:02PM +0200, Ville Syrj=C3=A4l=C3=A4 wrote: >> > On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote: >> > > Ville Syrjala writes: >> > >=20 >> > > > From: Ville Syrj=C3=A4l=C3=A4 >> > > > >> > > > 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 cre= ating >> > > > 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 t= he >> > > > new .fb_modifier() hook this check can now be done whether the req= uest >> > > > 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 >> > > > Testcase: igt/kms_addfb_basic/expected-formats >> > > > Signed-off-by: Ville Syrj=C3=A4l=C3=A4 >> > > > --- >> > > > 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/d= rm_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); >> > > > } >> > > >=20=20 >> > > > +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) =3D= =3D 0) >> > > > + return true; >> > > > + } >> > > > + >> > > > + return false; >> > > > +} >> > >=20 >> > > 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 bi= ts >> > > as a bit of metadata (like how we have horizontal stride passed outs= ide >> > > of the modifier) and you can't list all of the possible values in an >> > > array on the plane. >> >=20 >> > Hmm. drm_atomic_plane_check() etc. call this thing as well. How does >> > that manage to work currently? >>=20 >> Maybe it doesn't. I added the modifier checks in >>=20 >> commit 23163a7d4b032489d375099d56571371c0456980 >> Author: Ville Syrj=C3=A4l=C3=A4 >> AuthorDate: Fri Dec 22 21:22:30 2017 +0200 >> Commit: Ville Syrj=C3=A4l=C3=A4 >> CommitDate: Mon Feb 26 16:29:47 2018 +0200 >>=20 >> drm: Check that the plane supports the request format+modifier combo >>=20 >> Maybe that broke vc4? >>=20 >> 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... >>=20 >> I guess relying on .format_mod_supported() might be what we need to=20 >> 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. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlqsQBcACgkQtdYpNtH8 nuhQlA/+LNh+Ihqr6pTkfHgcICDZLW9xgl9xRd61ogr2BxmAaBGHLNL7Myu9+iMB YNPSXmUYadlMErw5OPxHcvtDifomshsHv2bg6a06PTBGWaNpQ8wYuh7vKNG/B+v7 U4TsdmbtQyNhczZGKrWyMAnxdG2gUBY8m9YtxOnQnxfaQKF7RR+mEfgjAIBW9OaM HmtOO1UfOorY3Wbp2iatDj7vdhzCkY+zm3JV9WfOlusEjA3owVkFPdax53UMuGIn JDmKHI0PJ86T0QnA6RoR29Ix4zAJJX98IWU4CjJtSsP2dUhLvrhmFIwfalxxGs3q o6w90s7WR0O+0qx5+7MujL1NqZu6l0nA9VyfA7xTfu+ZA+o8fECcQT/mb3AvsgIg WGKh7zJsI6pxP9u3uiBFFkpnAh5nzpb8tuar2pfplEOEPXKvvfFIJGo4COS/V9ij 5yzt9v62iflGG+1IYuIypYhSNCs5f2VtN4oaLPBOnA+4ZKZVopL5eLjPVcYG0/Wp QR+ekPVXMkGngQNsJyfj07JbQqIA7OtCHrIjictauXCZrazLJQbqQO2CEFDHuV7p ZMrM3a0Z0o7MqVVHn8QlLLFl47aHFvmW6ENSSy8XTkmjq2lvibM2IL+z/y/1DqY7 ZdUPlRLPOre+3K5LoGuKHOQcd3cTjaj/z2BRUR2cW05Km3/D004= =zu+n -----END PGP SIGNATURE----- --=-=-=-- --===============0576097000== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0576097000==--