Boris Brezillon <boris.brezillon@xxxxxxxxxxx> writes:
> From: Eric Anholt <eric@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
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:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
