[Intel-gfx] [PATCH 1/2] drm/i915: Add display WA #1175 for planes ending close to right screen edge

[Chris Wilson]

Quoting Imre Deak (2018-01-15 13:20:37)
> On Fri, Jan 12, 2018 at 03:01:59PM +0000, Chris Wilson wrote:
> > Quoting Imre Deak (2018-01-12 14:54:36)
> > > As described in the WA on GLK and CNL planes on the right edge of the
> > > screen that have less than 4 pixels visible from the beginning of the
> > > plane to the edge of the screen can cause FIFO underflow and display
> > > corruption.
> > > 
> > > On GLK/CNL I could trigger the problem only if the plane was at the same
> > > time also aligned to the top edge of the screen (after clipping) and
> > > there were exactly 2 pixels visible from the start of the plane to the
> > > right edge of the screen (so couldn't trigger it with 1 or 3 pixels
> > > visible). Nevertheless, to be sure, I also applied the WA for these cases.
> > > 
> > > I also couldn't see any problem with the cursor plane and later Art
> > > confirmed that it's not affected, so the WA is applied only for the
> > > other plane types.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++----
> > >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
> > >  drivers/gpu/drm/i915/intel_sprite.c  |  2 +-
> > >  3 files changed, 27 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 221e3a183d36..3d931b652795 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2917,14 +2917,19 @@ static bool skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state
> > >         return true;
> > >  }
> > >  
> > > -static int skl_check_main_surface(struct intel_plane_state *plane_state)
> > > +static int skl_check_main_surface(const struct intel_crtc_state *crtc_state,
> > > +                                 struct intel_plane_state *plane_state)
> > >  {
> > > +       struct drm_i915_private *dev_priv =
> > > +               to_i915(plane_state->base.plane->dev);
> > >         const struct drm_framebuffer *fb = plane_state->base.fb;
> > >         unsigned int rotation = plane_state->base.rotation;
> > >         int x = plane_state->base.src.x1 >> 16;
> > >         int y = plane_state->base.src.y1 >> 16;
> > >         int w = drm_rect_width(&plane_state->base.src) >> 16;
> > >         int h = drm_rect_height(&plane_state->base.src) >> 16;
> > > +       int dst_x = plane_state->base.dst.x1;
> > > +       int pipe_src_w = crtc_state->pipe_src_w;
> > >         int max_width = skl_max_plane_width(fb, 0, rotation);
> > >         int max_height = 4096;
> > >         u32 alignment, offset, aux_offset = plane_state->aux.offset;
> > > @@ -2935,6 +2940,20 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> > >                 return -EINVAL;
> > >         }
> > >  
> > > +       /*
> > > +        * Display WA #1175: cnl,glk
> > > +        * Planes other than the cursor may cause FIFO underflow and display
> > > +        * corruption if starting less than 4 pixels from the right edge of
> > > +        * the screen.
> > > +        */
> > > +       if ((IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) &&
> > > +           dst_x > pipe_src_w - 4) {
> > > +               DRM_DEBUG_KMS("requested plane X start position %d invalid (valid range %d-%d)\n",
> > > +                             dst_x,
> > > +                             0, pipe_src_w - 4);
> > 
> > You are rejecting user input, so this should be DRM_DEBUG() (or whatever
> > the future user channel will be).
> 
> What's the rational for a user channel? Not having to build the rest of
> debugging stuff, or a simpler user interface?

So that the right information goes to the right user. The only person
who should be able to see such error messages is the person making the
mistake (we're leaking information about the caller into a general
purpose message log). Plus we really want some other means for getting
accurate error messages about mistakes in using the ABI back to the user
without having to use dmesg.
-Chris