From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH 5/5] drm/vc4: Fix negative X/Y positioning of planes using T_TILES modifier Date: Fri, 27 Jul 2018 13:46:31 -0700 Message-ID: <87bmas1k60.fsf@anholt.net> References: <20180725153209.14366-1-boris.brezillon@bootlin.com> <20180725153209.14366-6-boris.brezillon@bootlin.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0476201140==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 7FFAD6E946 for ; Fri, 27 Jul 2018 20:46:34 +0000 (UTC) In-Reply-To: <20180725153209.14366-6-boris.brezillon@bootlin.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Cc: David Airlie , Boris Brezillon , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0476201140== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Boris Brezillon writes: > From: Eric Anholt > > X/Y positioning of T-format buffers is quite tricky and the current > implementation was failing to position a plane using this format > correctly when the X, Y or both X and Y offsets were negative. Wait, were things working for you with even postivie X/Y offsets on T? Because it wasn't for me, and I think the tile_h_mask change is important for making positive work. > Signed-off-by: Eric Anholt > Signed-off-by: Boris Brezillon > --- > Hi Eric, > > I kept the SoB and authorship since you're the original author, but > I also significantly reworked the code, so I'd be more confident if you > could have a close look at this code. I think you should definitely grab authorship on this one. You did the work to make it actually work. > Also, I intentionally did not add a Fixes and Cc-stable to this commit > because it depends on a rework we've done in > vc4_plane_setup_clipping_and_scaling() which cannot be easily > backported. > What I could do though is add a patch that rejects all negative > crtc_{x,y}. Agreed. Given that we're trying to fix a bug that nobody else has reported to me yet, I think we can skip dealing with this for stable. > --- > drivers/gpu/drm/vc4/vc4_plane.c | 51 +++++++++++++++++++++++++++++++++++= ------ > 1 file changed, 44 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_pl= ane.c > index 2b8ba1c412be..ade47c3f65d1 100644 > --- a/drivers/gpu/drm/vc4/vc4_plane.c > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > @@ -539,22 +539,59 @@ static int vc4_plane_mode_set(struct drm_plane *pla= ne, > (i ? h_subsample : 1) * > fb->format->cpp[i]; > } > + > break; >=20=20 > case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED: { > - /* For T-tiled, the FB pitch is "how many bytes from > - * one row to the next, such that pitch * tile_h =3D=3D > - * tile_size * tiles_per_row." > - */ > u32 tile_size_shift =3D 12; /* T tiles are 4kb */ > + /* Whole-tile offsets, mostly for setting the pitch. */ > + u32 tile_w_shift =3D fb->format->cpp[0] =3D=3D 2 ? 6 : 5; > u32 tile_h_shift =3D 5; /* 16 and 32bpp are 32 pixels high */ > + u32 tile_w_mask =3D (1 << tile_w_shift) - 1; > + /* The height mask on 32-bit-per-pixel tiles is 63, i.e. 2 > + * times the height (in pixels) of a 4k tile. I just assumed > + * this is also true for other RGB formats, but maybe it's not. > + */ > + u32 tile_h_mask =3D (2 << tile_h_shift) - 1; Only 2 and 4-byte formats are supported for T format, and tiles are 32 pixels high for both of those. Other than that, Reviewed-by: Eric Anholt I have a comment on patch 3 I'd like to sort out, but other than that I'm pleased with this whole series. Thanks for persevering on it! --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAltbhKgACgkQtdYpNtH8 nujKGQ/6Azp7UYTqdf8A2sOL4sn+GC83SdFP9MfzYukV2MsaLWCG1PnZUmMzrP8l OF8shu+//8psy47cW1w7AXNLtA4dYwdPx++tn9UClvdwCBVWdRyvr6qKKAg5ksmn NpZFYqnC3zq7+uu7cL0RtDSlrnq87AAHpLVIqnR+pXp1KHpM6rhWaBRExapG8pIk 6Pp6OyOpUZOwIGE+REnKRp9Bshr9eXeXn7OIACJ7sT46nS+jhy/2YfpD8S/M8Wbq M3gcgIjhoZrFx9vTKk6UyZGTza1z9VFLI6k1nVJ416Q4ITMbXevxzuAI5ToURxyM dFTCqHP9g0u1NMiRd37JoZO2eYB3vpIt1dlgFws9bdPEpuBHUIkPcwfvakkLR2QP rGS/9ZOuFmYSbEiacUt1gYJ9ox9cwZgtC8Eqs7+/xSIaERQVchD6l4D7hsxCIMUR DlBfYEo48eyGrFsabEaGM/PNfhNzHS2RG/O+gvwAy96cznIBQtuZyCF9tZ9t/sWH FjHK8GNdgueSgMWu9xr0lSKxzvbmbwDKmCGxFCsoo6MdY9Nfk3Tf1zhmRlY5IPPu YNLb+g5gn+z1nLM9iP+6feQc72+jQ5vl1hzQVCcllWxDquX4w2w+mRBhiFtgSZ/G MDnTwLoaDQwt4s/17O9Ecu084eend4XNqmIC7vbOEz6ZaagxpGQ= =rAIz -----END PGP SIGNATURE----- --=-=-=-- --===============0476201140== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0476201140==--