| Message ID | 20181205164529.12791-1-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/vc4: Fix negative X/Y positioning on SAND planes | expand |
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
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.
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
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. */
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(-)