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

[Eric Anholt <eric@anholt.net>]

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