Re: [PATCH 5/5] drm/vc4: Fix negative X/Y positioning of planes using T_TILES modifier

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 2945 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> From: Eric Anholt <eric@anholt.net>
>
> 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 <eric@anholt.net>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> 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_plane.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 *plane,
>  						 (i ? h_subsample : 1) *
>  						 fb->format->cpp[i];
>  		}
> +
>  		break;
>  
>  	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 ==
> -		 * tile_size * tiles_per_row."
> -		 */
>  		u32 tile_size_shift = 12; /* T tiles are 4kb */
> +		/* Whole-tile offsets, mostly for setting the pitch. */
> +		u32 tile_w_shift = fb->format->cpp[0] == 2 ? 6 : 5;
>  		u32 tile_h_shift = 5; /* 16 and 32bpp are 32 pixels high */
> +		u32 tile_w_mask = (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 = (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 <eric@anholt.net>

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!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel