| Message ID | 15de89a3-0305-8e15-f8de-b972ea8532ca@daenzer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 26 Sep 2016 10:57:50 +0200, Michel D4nzer wrote: > > On 23/09/16 10:06 PM, Takashi Iwai wrote: > > radeon_cursor_move_unlock() contains a workaround for AVIVO chips that > > are older than DCE6 when the cursor ends on 128 pixel boundary. It > > decreases the position when the calculated end position is on 128 > > pixel boundary. However, it hits also the condition where x=-1 and > > width=1 are passed, since cursor_end is 0 (which is aligned with 128, > > too). Then the value gets decreased and it hits WARN_ON_ONCE() > > below, eventually screws up the GPU. > > > > The fix is simply to skip the workaround when x is already zero. > > Also, remove the superfluous WARN_ON_ON() for the negative value check > > that wouldn't happen any longer after this change. > > > > Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1000433 > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > drivers/gpu/drm/radeon/radeon_cursor.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/radeon/radeon_cursor.c b/drivers/gpu/drm/radeon/radeon_cursor.c > > index 2a10e24b34b1..4e436eb49a56 100644 > > --- a/drivers/gpu/drm/radeon/radeon_cursor.c > > +++ b/drivers/gpu/drm/radeon/radeon_cursor.c > > @@ -193,10 +193,8 @@ static int radeon_cursor_move_locked(struct drm_crtc *crtc, int x, int y) > > if (w <= 0) { > > w = 1; > > cursor_end = x - xorigin + w; > > - if (!(cursor_end & 0x7f)) { > > + if (x > 0 && !(cursor_end & 0x7f)) > > x--; > > - WARN_ON_ONCE(x < 0); > > - } > > } > > The problem with this change is that the horizontal cursor end position > can again and up as a multiple of 128, which this code is trying to > avoid, because it can break the cursor with some versions of AVIVO / DCE > display engines. > > Attached is a proof-of-concept (only compile tested) alternative fix > which hides the cursor while it's out of bounds. I'll test it, clean it > up and split it up into multiple patches when I get a chance, but maybe > you or the reporter of the referenced bug can test it in the meantime. Sure, I'll ask the reporter to test with your tentative fix. Thanks! Takashi
On Mon, 26 Sep 2016 11:42:16 +0200, Takashi Iwai wrote: > > On Mon, 26 Sep 2016 10:57:50 +0200, > Michel D4nzer wrote: > > > > On 23/09/16 10:06 PM, Takashi Iwai wrote: > > > radeon_cursor_move_unlock() contains a workaround for AVIVO chips that > > > are older than DCE6 when the cursor ends on 128 pixel boundary. It > > > decreases the position when the calculated end position is on 128 > > > pixel boundary. However, it hits also the condition where x=-1 and > > > width=1 are passed, since cursor_end is 0 (which is aligned with 128, > > > too). Then the value gets decreased and it hits WARN_ON_ONCE() > > > below, eventually screws up the GPU. > > > > > > The fix is simply to skip the workaround when x is already zero. > > > Also, remove the superfluous WARN_ON_ON() for the negative value check > > > that wouldn't happen any longer after this change. > > > > > > Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1000433 > > > Cc: <stable@vger.kernel.org> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > --- > > > drivers/gpu/drm/radeon/radeon_cursor.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/radeon/radeon_cursor.c b/drivers/gpu/drm/radeon/radeon_cursor.c > > > index 2a10e24b34b1..4e436eb49a56 100644 > > > --- a/drivers/gpu/drm/radeon/radeon_cursor.c > > > +++ b/drivers/gpu/drm/radeon/radeon_cursor.c > > > @@ -193,10 +193,8 @@ static int radeon_cursor_move_locked(struct drm_crtc *crtc, int x, int y) > > > if (w <= 0) { > > > w = 1; > > > cursor_end = x - xorigin + w; > > > - if (!(cursor_end & 0x7f)) { > > > + if (x > 0 && !(cursor_end & 0x7f)) > > > x--; > > > - WARN_ON_ONCE(x < 0); > > > - } > > > } > > > > The problem with this change is that the horizontal cursor end position > > can again and up as a multiple of 128, which this code is trying to > > avoid, because it can break the cursor with some versions of AVIVO / DCE > > display engines. > > > > Attached is a proof-of-concept (only compile tested) alternative fix > > which hides the cursor while it's out of bounds. I'll test it, clean it > > up and split it up into multiple patches when I get a chance, but maybe > > you or the reporter of the referenced bug can test it in the meantime. > > Sure, I'll ask the reporter to test with your tentative fix. Through a quick test by Tomas (added to Cc), the new patch seems working. thanks, Takashi
On 26/09/16 09:09 PM, Takashi Iwai wrote: > On Mon, 26 Sep 2016 11:42:16 +0200, Takashi Iwai wrote: >> On Mon, 26 Sep 2016 10:57:50 +0200, Michel D4nzer wrote: >>> On 23/09/16 10:06 PM, Takashi Iwai wrote: >>>> radeon_cursor_move_unlock() contains a workaround for AVIVO chips that >>>> are older than DCE6 when the cursor ends on 128 pixel boundary. It >>>> decreases the position when the calculated end position is on 128 >>>> pixel boundary. However, it hits also the condition where x=-1 and >>>> width=1 are passed, since cursor_end is 0 (which is aligned with 128, >>>> too). Then the value gets decreased and it hits WARN_ON_ONCE() >>>> below, eventually screws up the GPU. >>>> >>>> The fix is simply to skip the workaround when x is already zero. >>>> Also, remove the superfluous WARN_ON_ON() for the negative value check >>>> that wouldn't happen any longer after this change. >>>> >>>> Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1000433 >>>> Cc: <stable@vger.kernel.org> >>>> Signed-off-by: Takashi Iwai <tiwai@suse.de> >>>> --- >>>> drivers/gpu/drm/radeon/radeon_cursor.c | 4 +--- >>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/radeon/radeon_cursor.c b/drivers/gpu/drm/radeon/radeon_cursor.c >>>> index 2a10e24b34b1..4e436eb49a56 100644 >>>> --- a/drivers/gpu/drm/radeon/radeon_cursor.c >>>> +++ b/drivers/gpu/drm/radeon/radeon_cursor.c >>>> @@ -193,10 +193,8 @@ static int radeon_cursor_move_locked(struct drm_crtc *crtc, int x, int y) >>>> if (w <= 0) { >>>> w = 1; >>>> cursor_end = x - xorigin + w; >>>> - if (!(cursor_end & 0x7f)) { >>>> + if (x > 0 && !(cursor_end & 0x7f)) >>>> x--; >>>> - WARN_ON_ONCE(x < 0); >>>> - } >>>> } >>> >>> The problem with this change is that the horizontal cursor end position >>> can again and up as a multiple of 128, which this code is trying to >>> avoid, because it can break the cursor with some versions of AVIVO / DCE >>> display engines. >>> >>> Attached is a proof-of-concept (only compile tested) alternative fix >>> which hides the cursor while it's out of bounds. I'll test it, clean it >>> up and split it up into multiple patches when I get a chance, but maybe >>> you or the reporter of the referenced bug can test it in the meantime. >> >> Sure, I'll ask the reporter to test with your tentative fix. > > Through a quick test by Tomas (added to Cc), the new patch seems > working. Thanks for testing, Tomáš! I sent an updated patch series to the amd-gfx mailing list and put you guys in Cc.
On Thu, 27 Oct 2016 11:02:30 +0200, Michel D4nzer wrote: > > On 26/09/16 09:09 PM, Takashi Iwai wrote: > > On Mon, 26 Sep 2016 11:42:16 +0200, Takashi Iwai wrote: > >> On Mon, 26 Sep 2016 10:57:50 +0200, Michel D4nzer wrote: > >>> On 23/09/16 10:06 PM, Takashi Iwai wrote: > >>>> radeon_cursor_move_unlock() contains a workaround for AVIVO chips that > >>>> are older than DCE6 when the cursor ends on 128 pixel boundary. It > >>>> decreases the position when the calculated end position is on 128 > >>>> pixel boundary. However, it hits also the condition where x=-1 and > >>>> width=1 are passed, since cursor_end is 0 (which is aligned with 128, > >>>> too). Then the value gets decreased and it hits WARN_ON_ONCE() > >>>> below, eventually screws up the GPU. > >>>> > >>>> The fix is simply to skip the workaround when x is already zero. > >>>> Also, remove the superfluous WARN_ON_ON() for the negative value check > >>>> that wouldn't happen any longer after this change. > >>>> > >>>> Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1000433 > >>>> Cc: <stable@vger.kernel.org> > >>>> Signed-off-by: Takashi Iwai <tiwai@suse.de> > >>>> --- > >>>> drivers/gpu/drm/radeon/radeon_cursor.c | 4 +--- > >>>> 1 file changed, 1 insertion(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/radeon/radeon_cursor.c b/drivers/gpu/drm/radeon/radeon_cursor.c > >>>> index 2a10e24b34b1..4e436eb49a56 100644 > >>>> --- a/drivers/gpu/drm/radeon/radeon_cursor.c > >>>> +++ b/drivers/gpu/drm/radeon/radeon_cursor.c > >>>> @@ -193,10 +193,8 @@ static int radeon_cursor_move_locked(struct drm_crtc *crtc, int x, int y) > >>>> if (w <= 0) { > >>>> w = 1; > >>>> cursor_end = x - xorigin + w; > >>>> - if (!(cursor_end & 0x7f)) { > >>>> + if (x > 0 && !(cursor_end & 0x7f)) > >>>> x--; > >>>> - WARN_ON_ONCE(x < 0); > >>>> - } > >>>> } > >>> > >>> The problem with this change is that the horizontal cursor end position > >>> can again and up as a multiple of 128, which this code is trying to > >>> avoid, because it can break the cursor with some versions of AVIVO / DCE > >>> display engines. > >>> > >>> Attached is a proof-of-concept (only compile tested) alternative fix > >>> which hides the cursor while it's out of bounds. I'll test it, clean it > >>> up and split it up into multiple patches when I get a chance, but maybe > >>> you or the reporter of the referenced bug can test it in the meantime. > >> > >> Sure, I'll ask the reporter to test with your tentative fix. > > > > Through a quick test by Tomas (added to Cc), the new patch seems > > working. > > Thanks for testing, Tomáš! I sent an updated patch series to the amd-gfx > mailing list and put you guys in Cc. Thanks! I updated the test package with your latest patchset for openSUSE Tumbleweed: https://bugzilla.suse.com/show_bug.cgi?id=1000433 Feel free to join to the bugzilla. Takashi
diff --git a/drivers/gpu/drm/radeon/radeon_cursor.c b/drivers/gpu/drm/radeon/radeon_cursor.c index 2a10e24..a656410 100644 --- a/drivers/gpu/drm/radeon/radeon_cursor.c +++ b/drivers/gpu/drm/radeon/radeon_cursor.c @@ -90,6 +90,9 @@ static void radeon_show_cursor(struct drm_crtc *crtc) struct radeon_crtc *radeon_crtc = to_radeon_crtc(crtc); struct radeon_device *rdev = crtc->dev->dev_private; + if (radeon_crtc->cursor_out_of_bounds) + return; + if (ASIC_IS_DCE4(rdev)) { WREG32(EVERGREEN_CUR_SURFACE_ADDRESS_HIGH + radeon_crtc->crtc_offset, upper_32_bits(radeon_crtc->cursor_addr)); @@ -150,15 +153,6 @@ static int radeon_cursor_move_locked(struct drm_crtc *crtc, int x, int y) } DRM_DEBUG("x %d y %d c->x %d c->y %d\n", x, y, crtc->x, crtc->y); - if (x < 0) { - xorigin = min(-x, radeon_crtc->max_cursor_width - 1); - x = 0; - } - if (y < 0) { - yorigin = min(-y, radeon_crtc->max_cursor_height - 1); - y = 0; - } - /* fixed on DCE6 and newer */ if (ASIC_IS_AVIVO(rdev) && !ASIC_IS_DCE6(rdev)) { int i = 0; @@ -180,27 +174,36 @@ static int radeon_cursor_move_locked(struct drm_crtc *crtc, int x, int y) if (i > 1) { int cursor_end, frame_end; - cursor_end = x - xorigin + w; + cursor_end = x + w; frame_end = crtc->x + crtc->mode.crtc_hdisplay; if (cursor_end >= frame_end) { w = w - (cursor_end - frame_end); if (!(frame_end & 0x7f)) w--; - } else { - if (!(cursor_end & 0x7f)) - w--; + } else if (cursor_end <= 0) { + goto out_of_bounds; + } else if (!(cursor_end & 0x7f)) { + w--; } if (w <= 0) { - w = 1; - cursor_end = x - xorigin + w; - if (!(cursor_end & 0x7f)) { - x--; - WARN_ON_ONCE(x < 0); - } + goto out_of_bounds; } } } + if (x <= -w || y <= -radeon_crtc->cursor_height || + x >= crtc->mode.crtc_hdisplay || y >= crtc->mode.crtc_vdisplay) + goto out_of_bounds; + + if (x < 0) { + xorigin = min(-x, radeon_crtc->max_cursor_width - 1); + x = 0; + } + if (y < 0) { + yorigin = min(-y, radeon_crtc->max_cursor_height - 1); + y = 0; + } + if (ASIC_IS_DCE4(rdev)) { WREG32(EVERGREEN_CUR_POSITION + radeon_crtc->crtc_offset, (x << 16) | y); WREG32(EVERGREEN_CUR_HOT_SPOT + radeon_crtc->crtc_offset, (xorigin << 16) | yorigin); @@ -232,6 +235,19 @@ static int radeon_cursor_move_locked(struct drm_crtc *crtc, int x, int y) radeon_crtc->cursor_x = x; radeon_crtc->cursor_y = y; + if (radeon_crtc->cursor_out_of_bounds) { + radeon_crtc->cursor_out_of_bounds = false; + if (radeon_crtc->cursor_bo) + radeon_show_cursor(crtc); + } + + return 0; + + out_of_bounds: + if (!radeon_crtc->cursor_out_of_bounds) { + radeon_hide_cursor(crtc); + radeon_crtc->cursor_out_of_bounds = true; + } return 0; } @@ -259,6 +275,7 @@ int radeon_crtc_cursor_set2(struct drm_crtc *crtc, struct radeon_device *rdev = crtc->dev->dev_private; struct drm_gem_object *obj; struct radeon_bo *robj; + int x, y; int ret; if (!handle) { @@ -302,19 +319,18 @@ int radeon_crtc_cursor_set2(struct drm_crtc *crtc, radeon_lock_cursor(crtc, true); + x = radeon_crtc->cursor_x; + y = radeon_crtc->cursor_y; if (hot_x != radeon_crtc->cursor_hot_x || hot_y != radeon_crtc->cursor_hot_y) { - int x, y; - - x = radeon_crtc->cursor_x + radeon_crtc->cursor_hot_x - hot_x; - y = radeon_crtc->cursor_y + radeon_crtc->cursor_hot_y - hot_y; - - radeon_cursor_move_locked(crtc, x, y); + x += radeon_crtc->cursor_hot_x - hot_x; + y += radeon_crtc->cursor_hot_y - hot_y; radeon_crtc->cursor_hot_x = hot_x; radeon_crtc->cursor_hot_y = hot_y; } + radeon_cursor_move_locked(crtc, x, y); radeon_show_cursor(crtc); radeon_lock_cursor(crtc, false); diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h index bb75201a..f1da484 100644 --- a/drivers/gpu/drm/radeon/radeon_mode.h +++ b/drivers/gpu/drm/radeon/radeon_mode.h @@ -330,6 +330,7 @@ struct radeon_crtc { u16 lut_r[256], lut_g[256], lut_b[256]; bool enabled; bool can_tile; + bool cursor_out_of_bounds; uint32_t crtc_offset; struct drm_gem_object *cursor_bo; uint64_t cursor_addr;