Re: [Intel-gfx] [PATCH 1/3] drm/i915: Disallow plane x+w>stride on ilk+ with X-tiling

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 09, 2021 at 09:22:09AM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2021-02-09 02:19:16)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > ilk+ planes get notably unhappy when the plane x+w exceeds
> > the stride. This wasn't a problem previously because we
> > always aligned SURF to the closest tile boundary so the
> > x offset never got particularly large. But now with async
> > flips we have to align to 256KiB instead and thus this
> > becomes a real issue.
> > 
> > On ilk/snb/ivb it looks like the accesses just just wrap
> > early to the next tile row when scanout goes past the
> > SURF+n*stride boundary, hsw/bdw suffer more heavily and
> > start to underrun constantly. i965/g4x appear to be immune.
> > vlv/chv I've not yet checked.
> > 
> > Let's borrow another trick from the skl+ code and search
> > backwards for a better SURF offset in the hopes of getting the
> > x offset below the limit. IIRC when I ran into a similar issue
> > on skl years ago it was causing the hardware to fall over
> > pretty hard as well.
> > 
> > And let's be consistent and include i965/g4x in the check
> > as well, just in case I just got super lucky somehow when
> > I wasn't able to reproduce the issue. Not that it really
> > matters since we still use 4k SURF alignment for i965/g4x
> > anyway.
> > 
> > Fixes: 6ede6b0616b2 ("drm/i915: Implement async flips for vlv/chv")
> > Fixes: 4bb18054adc4 ("drm/i915: Implement async flip for ilk/snb")
> > Fixes: 2a636e240c77 ("drm/i915: Implement async flip for ivb/hsw")
> > Fixes: cda195f13abd ("drm/i915: Implement async flips for bdw")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/i9xx_plane.c | 27 +++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > index 0523e2c79d16..8a52beaed2da 100644
> > --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > @@ -255,6 +255,33 @@ int i9xx_check_plane_surface(struct intel_plane_state *plane_state)
> >         else
> >                 offset = 0;
> >  
> > +       /*
> > +        * When using an X-tiled surface the plane starts to
> > +        * misbehave if the x offset + width exceeds the stride.
> > +        * hsw/bdw: underrun galore
> > +        * ilk/snb/ivb: wrap to the next tile row mid scanout
> > +        * i965/g4x: so far appear immune to this
> > +        * vlv/chv: TODO check
> > +        *
> > +        * Linear surfaces seem to work just fine, even on hsw/bdw
> > +        * despite them not using the linear offset anymore.
> > +        */
> > +       if (INTEL_GEN(dev_priv) >= 4 && fb->modifier == I915_FORMAT_MOD_X_TILED) {
> > +               u32 alignment = intel_surf_alignment(fb, 0);
> > +               int cpp = fb->format->cpp[0];
> > +
> > +               while ((src_x + src_w) * cpp > plane_state->color_plane[0].stride) {
> > +                       if (offset == 0) {
> > +                               drm_dbg_kms(&dev_priv->drm,
> > +                                           "Unable to find suitable display surface offset due to X-tiling\n");
> > +                               return -EINVAL;
> > +                       }
> > +
> > +                       offset = intel_plane_adjust_aligned_offset(&src_x, &src_y, plane_state, 0,
> > +                                                                  offset, offset - alignment);
> 
> As offset decreases, src_x goes up; but modulus the pitch. So long as
> the alignment is not a multiple of the pitch, src_x will change on each
> iteration. And after the adjustment, the offset is stored in
> plane_state.
> 
> So this loop would fail for any power-of-two stride, but at the same
> time that would put the src_x + src_w out-of-bounds in the supplied
> coordinates. The only way src_x + src_w would exceed stride legally is
> if we have chosen an aligned offset that causes that, thus there should
> exist an offset where src_x + src_w does not exceed the stride.
> 
> The reason for choosing a nearby tile offset was to reduce src_x/src_y
> to fit within the crtc limits. While remapping could be used to solve
> that, the aligned_offset computation allows reuse of a single view.
> 
> Since offset, src_x are a function of the plane input parameters, this
> should be possible to exercise with carefully selected framebuffers and
> modesetting. Right? Is there a test case for this?

My idea was to extend kms_big_fb for these sort of things.
While I originally made it purely to test remapping it should
be possible to extend it for non-remapped fbs as well. IIRC 
J-P did at least some work towards that goal, but I guess
it's only in the internal copy for whatever reason.

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Ta.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx