| Message ID | 20180112145437.16204-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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). -Chris
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@intel.com> > --- > + /* > + * 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); > + return -EINVAL; Should this be -ERANGE or -ENOSPC? -Chris
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@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? The rest of places failing due to user input on this path also use DRM_DEBUG_KMS(), so we could also convert all as a follow-up. --Imre
On Fri, Jan 12, 2018 at 03:13:19PM +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@intel.com> > > --- > > + /* > > + * 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); > > + return -EINVAL; > > Should this be -ERANGE or -ENOSPC? I don't mind changing it to -ERANGE. (I suppose it'd be used by user space as detection of platforms with the problem, falling back to something else.) --Imre
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@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
On Mon, Jan 15, 2018 at 01:26:48PM +0000, Chris Wilson wrote: > 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@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. Ok, sounds reasonable, but I think the conversion should be done for all places as a follow-up. At least I can't see how changing the above to DRM_DEBUG alone would help now, as I understand you'd need some per-process log buffer in addition. --Imre
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); + return -EINVAL; + } + intel_add_fb_offsets(&x, &y, plane_state, 0); offset = intel_compute_tile_offset(&x, &y, plane_state, 0); alignment = intel_surf_alignment(fb, 0); @@ -3067,7 +3086,8 @@ static int skl_check_ccs_aux_surface(struct intel_plane_state *plane_state) return 0; } -int skl_check_plane_surface(struct intel_plane_state *plane_state) +int skl_check_plane_surface(const struct intel_crtc_state *crtc_state, + struct intel_plane_state *plane_state) { const struct drm_framebuffer *fb = plane_state->base.fb; unsigned int rotation = plane_state->base.rotation; @@ -3107,7 +3127,7 @@ int skl_check_plane_surface(struct intel_plane_state *plane_state) plane_state->aux.y = 0; } - ret = skl_check_main_surface(plane_state); + ret = skl_check_main_surface(crtc_state, plane_state); if (ret) return ret; @@ -12766,7 +12786,7 @@ intel_check_primary_plane(struct intel_plane *plane, return 0; if (INTEL_GEN(dev_priv) >= 9) { - ret = skl_check_plane_surface(state); + ret = skl_check_plane_surface(crtc_state, state); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 731dc36d7129..5e23da04837e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1588,7 +1588,8 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state); u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane, unsigned int rotation); -int skl_check_plane_surface(struct intel_plane_state *plane_state); +int skl_check_plane_surface(const struct intel_crtc_state *crtc_state, + struct intel_plane_state *plane_state); int i9xx_check_plane_surface(struct intel_plane_state *plane_state); /* intel_csr.c */ diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index dd485f59eb1d..b8b617ba0c0c 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1027,7 +1027,7 @@ intel_check_sprite_plane(struct intel_plane *plane, dst->y2 = crtc_y + crtc_h; if (INTEL_GEN(dev_priv) >= 9) { - ret = skl_check_plane_surface(state); + ret = skl_check_plane_surface(crtc_state, state); if (ret) return ret;
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@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(-)