[PATCH v2 1/2] drm/vc4: Fix negative X/Y positioning on SAND planes

[PATCH v2 1/2] drm/vc4: Fix negative X/Y positioning on SAND planes

[Boris Brezillon]

Commit 3e407417b192 ("drm/vc4: Fix X/Y positioning of planes using
T_TILES modifier") fixed the problem with T_TILES format, but left
things in a non-working state for SAND formats. Address that now.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Hi Eric,

So, I finally spent time debugging my SANDXXX pattern generator and
could validate that negative X/Y positioning does not work (which I
was expecting :-)). The fix turns out to be simpler than I thought
(much simpler than on T-tiles), and we now have negative X/Y
positioning working on all kind of formats.

Regards,

Boris

Changes in v2:
- New patch
---
 drivers/gpu/drm/vc4/vc4_plane.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 75db62cbe468..3132f5e1d16a 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -595,6 +595,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	case DRM_FORMAT_MOD_BROADCOM_SAND128:
 	case DRM_FORMAT_MOD_BROADCOM_SAND256: {
 		uint32_t param = fourcc_mod_broadcom_param(fb->modifier);
+		u32 tile_w, tile, x_off, pix_per_tile;
 
 		/* Column-based NV12 or RGBA.
 		 */
@@ -614,12 +615,15 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 		switch (base_format_mod) {
 		case DRM_FORMAT_MOD_BROADCOM_SAND64:
 			tiling = SCALER_CTL0_TILING_64B;
+			tile_w = 64;
 			break;
 		case DRM_FORMAT_MOD_BROADCOM_SAND128:
 			tiling = SCALER_CTL0_TILING_128B;
+			tile_w = 128;
 			break;
 		case DRM_FORMAT_MOD_BROADCOM_SAND256:
 			tiling = SCALER_CTL0_TILING_256B_OR_T;
+			tile_w = 256;
 			break;
 		default:
 			break;
@@ -630,6 +634,28 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 			return -EINVAL;
 		}
 
+		pix_per_tile = tile_w / fb->format->cpp[0];
+		tile = vc4_state->src_x / pix_per_tile;
+		x_off = vc4_state->src_x % pix_per_tile;
+
+		/* Adjust the base pointer to the first pixel to be scanned
+		 * out.
+		 */
+		for (i = 0; i < num_planes; i++) {
+			vc4_state->offsets[i] += param * tile_w * tile;
+			vc4_state->offsets[i] += (vc4_state->src_y /
+						  (i ? v_subsample : 1)) *
+						 tile_w;
+		}
+
+		/*
+		 * SCALER_PITCH0_SINK_PIX does not seem to work for SAND
+		 * formats. Specify a negative START_X instead, even if it's
+		 * less efficient.
+		 */
+		if (x_off)
+			vc4_state->crtc_x = -x_off;
+
 		pitch0 = VC4_SET_FIELD(param, SCALER_TILE_HEIGHT);
 		break;
 	}
@@ -655,7 +681,8 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	vc4_state->pos0_offset = vc4_state->dlist_count;
 	vc4_dlist_write(vc4_state,
 			VC4_SET_FIELD(state->alpha >> 8, SCALER_POS0_FIXED_ALPHA) |
-			VC4_SET_FIELD(vc4_state->crtc_x, SCALER_POS0_START_X) |
+			VC4_SET_FIELD(vc4_state->crtc_x & SCALER_POS0_START_X_MASK,
+				      SCALER_POS0_START_X) |
 			VC4_SET_FIELD(vc4_state->crtc_y, SCALER_POS0_START_Y));
 
 	/* Position Word 1: Scaled Image Dimensions. */
-- 
2.17.1

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

[PATCH v2 2/2] drm/vc4: Add support for X/Y reflection

[Boris Brezillon]

Add support for X/Y reflection when the plane is using linear or T-tiled
formats. X/Y reflection hasn't been tested on SAND formats, so we reject
them until proper testing/debugging has been done.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Eric, I dropped your Reviewed-by as this version also contains support
for rotation/reflection on SAND formats.

Changes in v2:
- Use drm_rotation_simplify() to support ROTATE_180
- Support rotation/reflection on SAND formats
---
 drivers/gpu/drm/vc4/vc4_plane.c | 59 ++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 3132f5e1d16a..335aaf815290 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -492,8 +492,9 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	bool mix_plane_alpha;
 	bool covers_screen;
 	u32 scl0, scl1, pitch0;
-	u32 tiling;
+	u32 tiling, src_y;
 	u32 hvs_format = format->hvs;
+	unsigned int rotation;
 	int ret, i;
 
 	if (vc4_state->dlist_initialized)
@@ -520,6 +521,16 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	h_subsample = drm_format_horz_chroma_subsampling(format->drm);
 	v_subsample = drm_format_vert_chroma_subsampling(format->drm);
 
+	rotation = drm_rotation_simplify(state->rotation,
+					 DRM_MODE_ROTATE_0 |
+					 DRM_MODE_REFLECT_X |
+					 DRM_MODE_REFLECT_Y);
+
+	/* We must point to the last line when Y reflection is enabled. */
+	src_y = vc4_state->src_y;
+	if (rotation & DRM_MODE_REFLECT_Y)
+		src_y += vc4_state->src_h[0] - 1;
+
 	switch (base_format_mod) {
 	case DRM_FORMAT_MOD_LINEAR:
 		tiling = SCALER_CTL0_TILING_LINEAR;
@@ -529,9 +540,10 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 		 * out.
 		 */
 		for (i = 0; i < num_planes; i++) {
-			vc4_state->offsets[i] += vc4_state->src_y /
+			vc4_state->offsets[i] += src_y /
 						 (i ? v_subsample : 1) *
 						 fb->pitches[i];
+
 			vc4_state->offsets[i] += vc4_state->src_x /
 						 (i ? h_subsample : 1) *
 						 fb->format->cpp[i];
@@ -557,22 +569,38 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 		u32 tiles_w = fb->pitches[0] >> (tile_size_shift - tile_h_shift);
 		u32 tiles_l = vc4_state->src_x >> tile_w_shift;
 		u32 tiles_r = tiles_w - tiles_l;
-		u32 tiles_t = vc4_state->src_y >> tile_h_shift;
+		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 = (vc4_state->src_y >> 4) & 1;
-		u32 subtile_y = (vc4_state->src_y >> 2) & 3;
-		u32 utile_y = vc4_state->src_y & 3;
+		u32 tile_y = (src_y >> 4) & 1;
+		u32 subtile_y = (src_y >> 2) & 3;
+		u32 utile_y = src_y & 3;
 		u32 x_off = vc4_state->src_x & tile_w_mask;
-		u32 y_off = vc4_state->src_y & tile_h_mask;
+		u32 y_off = src_y & tile_h_mask;
+
+		/* When Y reflection is requested we must set the
+		 * SCALER_PITCH0_TILE_LINE_DIR flag to tell HVS that all lines
+		 * after the initial one should be fetched in descending order,
+		 * which makes sense since we start from the last line and go
+		 * backward.
+		 * Don't know why we need y_off = max_y_off - y_off, but it's
+		 * definitely required (I guess it's also related to the "going
+		 * backward" situation).
+		 */
+		if (rotation & DRM_MODE_REFLECT_Y) {
+			y_off = tile_h_mask - y_off;
+			pitch0 = SCALER_PITCH0_TILE_LINE_DIR;
+		} else {
+			pitch0 = 0;
+		}
 
 		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));
+		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;
@@ -643,7 +671,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 		 */
 		for (i = 0; i < num_planes; i++) {
 			vc4_state->offsets[i] += param * tile_w * tile;
-			vc4_state->offsets[i] += (vc4_state->src_y /
+			vc4_state->offsets[i] += (src_y /
 						  (i ? v_subsample : 1)) *
 						 tile_w;
 		}
@@ -669,6 +697,8 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	/* Control word */
 	vc4_dlist_write(vc4_state,
 			SCALER_CTL0_VALID |
+			(rotation & DRM_MODE_REFLECT_X ? SCALER_CTL0_HFLIP : 0) |
+			(rotation & DRM_MODE_REFLECT_Y ? SCALER_CTL0_VFLIP : 0) |
 			VC4_SET_FIELD(SCALER_CTL0_RGBA_EXPAND_ROUND, SCALER_CTL0_RGBA_EXPAND) |
 			(format->pixel_order << SCALER_CTL0_ORDER_SHIFT) |
 			(hvs_format << SCALER_CTL0_PIXEL_FORMAT_SHIFT) |
@@ -1150,6 +1180,11 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
 	drm_plane_helper_add(plane, &vc4_plane_helper_funcs);
 
 	drm_plane_create_alpha_property(plane);
+	drm_plane_create_rotation_property(plane, DRM_MODE_ROTATE_0,
+					   DRM_MODE_ROTATE_0 |
+					   DRM_MODE_ROTATE_180 |
+					   DRM_MODE_REFLECT_X |
+					   DRM_MODE_REFLECT_Y);
 
 	return plane;
 }
-- 
2.17.1

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

Re: [PATCH v2 1/2] drm/vc4: Fix negative X/Y positioning on SAND planes

[Eric Anholt]

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

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

> Commit 3e407417b192 ("drm/vc4: Fix X/Y positioning of planes using
> T_TILES modifier") fixed the problem with T_TILES format, but left
> things in a non-working state for SAND formats. Address that now.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Hi Eric,
>
> So, I finally spent time debugging my SANDXXX pattern generator and
> could validate that negative X/Y positioning does not work (which I
> was expecting :-)). The fix turns out to be simpler than I thought
> (much simpler than on T-tiles), and we now have negative X/Y
> positioning working on all kind of formats.
>
> Regards,
>
> Boris
>
> Changes in v2:
> - New patch
> ---
>  drivers/gpu/drm/vc4/vc4_plane.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index 75db62cbe468..3132f5e1d16a 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -595,6 +595,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
>  	case DRM_FORMAT_MOD_BROADCOM_SAND128:
>  	case DRM_FORMAT_MOD_BROADCOM_SAND256: {
>  		uint32_t param = fourcc_mod_broadcom_param(fb->modifier);
> +		u32 tile_w, tile, x_off, pix_per_tile;
>  
>  		/* Column-based NV12 or RGBA.
>  		 */
> @@ -614,12 +615,15 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
>  		switch (base_format_mod) {
>  		case DRM_FORMAT_MOD_BROADCOM_SAND64:
>  			tiling = SCALER_CTL0_TILING_64B;
> +			tile_w = 64;
>  			break;
>  		case DRM_FORMAT_MOD_BROADCOM_SAND128:
>  			tiling = SCALER_CTL0_TILING_128B;
> +			tile_w = 128;
>  			break;
>  		case DRM_FORMAT_MOD_BROADCOM_SAND256:
>  			tiling = SCALER_CTL0_TILING_256B_OR_T;
> +			tile_w = 256;
>  			break;
>  		default:
>  			break;
> @@ -630,6 +634,28 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
>  			return -EINVAL;
>  		}
>  
> +		pix_per_tile = tile_w / fb->format->cpp[0];
> +		tile = vc4_state->src_x / pix_per_tile;
> +		x_off = vc4_state->src_x % pix_per_tile;
> +
> +		/* Adjust the base pointer to the first pixel to be scanned
> +		 * out.
> +		 */
> +		for (i = 0; i < num_planes; i++) {
> +			vc4_state->offsets[i] += param * tile_w * tile;
> +			vc4_state->offsets[i] += (vc4_state->src_y /
> +						  (i ? v_subsample : 1)) *
> +						 tile_w;
> +		}
> +
> +		/*
> +		 * SCALER_PITCH0_SINK_PIX does not seem to work for SAND
> +		 * formats. Specify a negative START_X instead, even if it's
> +		 * less efficient.
> +		 */
> +		if (x_off)
> +			vc4_state->crtc_x = -x_off;

Wait. If we were supposed to start at a nonzero x position within the
FB, then we instead put the image off the left hand side of the screen?
That seems wrong.

Did you test if we can just vc4_state->offsets[i] += x_off * cpp?

> +
>  		pitch0 = VC4_SET_FIELD(param, SCALER_TILE_HEIGHT);
>  		break;
>  	}
> @@ -655,7 +681,8 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
>  	vc4_state->pos0_offset = vc4_state->dlist_count;
>  	vc4_dlist_write(vc4_state,
>  			VC4_SET_FIELD(state->alpha >> 8, SCALER_POS0_FIXED_ALPHA) |
> -			VC4_SET_FIELD(vc4_state->crtc_x, SCALER_POS0_START_X) |
> +			VC4_SET_FIELD(vc4_state->crtc_x & SCALER_POS0_START_X_MASK,
> +				      SCALER_POS0_START_X) |
>  			VC4_SET_FIELD(vc4_state->crtc_y, SCALER_POS0_START_Y));
>  
>  	/* Position Word 1: Scaled Image Dimensions. */
> -- 
> 2.17.1

[-- 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

Re: [PATCH v2 2/2] drm/vc4: Add support for X/Y reflection

[Eric Anholt]

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

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

> Add support for X/Y reflection when the plane is using linear or T-tiled
> formats. X/Y reflection hasn't been tested on SAND formats, so we reject
> them until proper testing/debugging has been done.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Eric, I dropped your Reviewed-by as this version also contains support
> for rotation/reflection on SAND formats.
>
> Changes in v2:
> - Use drm_rotation_simplify() to support ROTATE_180
> - Support rotation/reflection on SAND formats

This one still looks good to me.

[-- 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

Re: [PATCH v2 1/2] drm/vc4: Fix negative X/Y positioning on SAND planes

[Boris Brezillon]

On Thu, 06 Dec 2018 10:59:17 -0800
Eric Anholt <eric@anholt.net> wrote:

> > +
> > +		/*
> > +		 * SCALER_PITCH0_SINK_PIX does not seem to work for SAND
> > +		 * formats. Specify a negative START_X instead, even if it's
> > +		 * less efficient.
> > +		 */
> > +		if (x_off)
> > +			vc4_state->crtc_x = -x_off;  
> 
> Wait. If we were supposed to start at a nonzero x position within the
> FB, then we instead put the image off the left hand side of the screen?
> That seems wrong.

Yep, I overlooked this case.

> 
> Did you test if we can just vc4_state->offsets[i] += x_off * cpp?

Yep, I tried, and it doesn't seem to work. We have to tell the HVS that
some pixels must be skipped at the beginning of each line of the first
tile. That's what SINK_PIX(x_off) is supposed to do, but it seems to be
broken for 2 reasons:

1/ the max value of SINK_PIX is 63, but the HVS supports 128 and 256
   bytes tiles.
2/ looks like the UV plane is truncated when we specify a SINK_PIX!=0.
   Don't know why exactly.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm/vc4: Fix negative X/Y positioning on SAND planes

[Boris Brezillon]

On Thu, 6 Dec 2018 20:29:19 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Thu, 06 Dec 2018 10:59:17 -0800
> Eric Anholt <eric@anholt.net> wrote:
> 
> > > +
> > > +		/*
> > > +		 * SCALER_PITCH0_SINK_PIX does not seem to work for SAND
> > > +		 * formats. Specify a negative START_X instead, even if it's
> > > +		 * less efficient.
> > > +		 */
> > > +		if (x_off)
> > > +			vc4_state->crtc_x = -x_off;    
> > 
> > Wait. If we were supposed to start at a nonzero x position within the
> > FB, then we instead put the image off the left hand side of the screen?
> > That seems wrong.  
> 
> Yep, I overlooked this case.
> 
> > 
> > Did you test if we can just vc4_state->offsets[i] += x_off * cpp?  
> 
> Yep, I tried, and it doesn't seem to work.

Okay, looks like you were right. It seems the HVS engine knows that it
must skip pixels on each new line when the initial offset is not
aligned on a tile. I must have tested something slightly different.

Thanks,

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