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

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Fri, 27 Jul 2018 13:46:31 -0700
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@bootlin.com> writes:
> 
> > From: Eric Anholt <eric@anholt.net>
> >
> > 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 <eric@anholt.net>
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> > 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 <eric@anholt.net>
> 
> 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!

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