On Fri, 27 Jul 2018 13:46:31 -0700
Eric Anholt <eric@xxxxxxxxxx> wrote:
> Boris Brezillon <boris.brezillon@xxxxxxxxxxx> writes:
>
> > From: Eric Anholt <eric@xxxxxxxxxx>
> >
> > 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 <eric@xxxxxxxxxx>
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> > ---
> > 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 <eric@xxxxxxxxxx>
>
> 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!
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
