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

Commit Message

Boris Brezillon July 25, 2018, 3:32 p.m. UTC

From: Eric Anholt <[e-mail]>

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.

Signed-off-by: Eric Anholt <[e-mail]>
Signed-off-by: Boris Brezillon <[e-mail]>
---
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.

It's been tested with a modified modetest (one that supports generating
T-format buffers) and it seems to work fine with any kind of negative
X/Y offset (both when starting on an even or an odd tile row).

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}.

Regards,

Boris
---
 drivers/gpu/drm/vc4/vc4_plane.c | 51 +++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 7 deletions(-)

Comments

Eric Anholt July 27, 2018, 8:46 p.m. UTC | #1

Boris Brezillon <[e-mail]> writes:

> From: Eric Anholt <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Boris Brezillon <[e-mail]>
> ---
> 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 <[e-mail]>

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!

Boris Brezillon July 27, 2018, 8:59 p.m. UTC | #2

On Fri, 27 Jul 2018 13:46:31 -0700
Eric Anholt <[e-mail]> wrote:

> Boris Brezillon <[e-mail]> writes:
> 
> > From: Eric Anholt <[e-mail]>
> >
> > 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.

Not so sure. I've done so many tests that I don't remember which exact
setups were working and which ones were not. I'll test again before
sending v2 and adjust the commit message accordingly.

> 
> > Signed-off-by: Eric Anholt <[e-mail]>
> > Signed-off-by: Boris Brezillon <[e-mail]>
> > ---
> > 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.
> 

I will. Thanks.

> > 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 <[e-mail]>
> 
> I have a comment on patch 3 I'd like to sort out,

I'll address that one in v2.

> but other than that
> I'm pleased with this whole series.  Thanks for persevering on it!

Boris Brezillon Aug. 3, 2018, 9:06 a.m. UTC | #3

On Fri, 27 Jul 2018 13:46:31 -0700
Eric Anholt <[e-mail]> wrote:

> Boris Brezillon <[e-mail]> writes:
> 
> > From: Eric Anholt <[e-mail]>
> >
> > 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?

I think I was talking about crtc_x/y, while you were probably talking
about src_x/y. If you have src_x/y = 0 and crtc_x/y >= 0 it works fine,
if you have src_x/y = 0 and crtc_x/y < 0 it doesn't work, and if you
have src_x/y != 0 it doesn't work either.

> Because it wasn't for me, and I think the tile_h_mask change is
> important for making positive work.

Yes, for positive src_x/y it does matter. I'll clarify that in my
commit message.

> 
> > Signed-off-by: Eric Anholt <[e-mail]>
> > Signed-off-by: Boris Brezillon <[e-mail]>
> > ---
> > 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.

Okay, I'll drop the second sentence then.

> 
> Other than that,
> 
> Reviewed-by: Eric Anholt <[e-mail]>
> 
> 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!

Patch

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;
+		/* 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 tiles_w = fb->pitches[0] >> (tile_size_shift - tile_h_shift);
+		u32 tiles_l = src_x >> tile_w_shift;
+		u32 tiles_r = tiles_w - tiles_l;
+		u32 tiles_t = src_y >> tile_h_shift;
+		/* Intra-tile offsets, which modify the base address (the
+		 * SCALER_PITCH0_TILE_Y_OFFSET tells HVS how to walk from that
+		 * base address).
+		 */
+		u32 tile_y = (src_y >> 4) & 1;
+		u32 subtile_y = (src_y >> 2) & 3;
+		u32 utile_y = src_y & 3;
+		u32 x_off = src_x & tile_w_mask;
+		u32 y_off = src_y & tile_h_mask;
 
 		tiling = SCALER_CTL0_TILING_256B_OR_T;
+		pitch0 = (VC4_SET_FIELD(x_off, SCALER_PITCH0_SINK_PIX) |
+			  VC4_SET_FIELD(y_off, SCALER_PITCH0_TILE_Y_OFFSET) |
+			  VC4_SET_FIELD(tiles_l, SCALER_PITCH0_TILE_WIDTH_L) |
+			  VC4_SET_FIELD(tiles_r, SCALER_PITCH0_TILE_WIDTH_R));
+		vc4_state->offsets[0] += tiles_t * (tiles_w << tile_size_shift);
+		vc4_state->offsets[0] += subtile_y << 8;
+		vc4_state->offsets[0] += utile_y << 4;
+
+		/* Rows of tiles alternate left-to-right and right-to-left. */
+		if (tiles_t & 1) {
+			pitch0 |= SCALER_PITCH0_TILE_INITIAL_LINE_DIR;
+			vc4_state->offsets[0] += (tiles_w - tiles_l) <<
+						 tile_size_shift;
+			vc4_state->offsets[0] -= (1 + !tile_y) << 10;
+		} else {
+			vc4_state->offsets[0] += tiles_l << tile_size_shift;
+			vc4_state->offsets[0] += tile_y << 10;
+		}
 
-		pitch0 = (VC4_SET_FIELD(0, SCALER_PITCH0_TILE_Y_OFFSET) |
-			  VC4_SET_FIELD(0, SCALER_PITCH0_TILE_WIDTH_L) |
-			  VC4_SET_FIELD(tiles_w, SCALER_PITCH0_TILE_WIDTH_R));
 		break;
 	}