* [PATCH] drm/amd/display: Fix overlay validation by considering cursors @ 2021-05-14 11:47 Rodrigo Siqueira 2021-05-14 14:54 ` Mark Yacoub ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Rodrigo Siqueira @ 2021-05-14 11:47 UTC (permalink / raw) To: amd-gfx Cc: Harry Wentland, Tianci . Yin, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas, Mark Yacoub A few weeks ago, we saw a two cursor issue in a ChromeOS system. We fixed it in the commit: drm/amd/display: Fix two cursor duplication when using overlay (read the commit message for more details) After this change, we noticed that some IGT subtests related to kms_plane and kms_plane_scaling started to fail. After investigating this issue, we noticed that all subtests that fail have a primary plane covering the overlay plane, which is currently rejected by amdgpu dm. Fail those IGT tests highlight that our verification was too broad and compromises the overlay usage in our drive. This patch fixes this issue by ensuring that we only reject commits where the primary plane is not fully covered by the overlay when the cursor hardware is enabled. With this fix, all IGT tests start to pass again, which means our overlay support works as expected. Cc: Tianci.Yin <tianci.yin@amd.com> Cc: Harry Wentland <harry.wentland@amd.com> Cc: Nicholas Choi <nicholas.choi@amd.com> Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> Cc: Mark Yacoub <markyacoub@google.com> Cc: Daniel Wheeler <daniel.wheeler@amd.com> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index ccd67003b120..9c2537a17a7b 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) int i; struct drm_plane *plane; struct drm_plane_state *old_plane_state, *new_plane_state; - struct drm_plane_state *primary_state, *overlay_state = NULL; + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL; /* Check if primary plane is contained inside overlay */ for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) if (!primary_state->crtc) return 0; + /* check if cursor plane is enabled */ + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); + if (IS_ERR(cursor_state)) + return PTR_ERR(cursor_state); + + if (drm_atomic_plane_disabling(plane->state, cursor_state)) + return 0; + /* Perform the bounds check to ensure the overlay plane covers the primary */ if (primary_state->crtc_x < overlay_state->crtc_x || primary_state->crtc_y < overlay_state->crtc_y || -- 2.25.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-05-14 11:47 [PATCH] drm/amd/display: Fix overlay validation by considering cursors Rodrigo Siqueira @ 2021-05-14 14:54 ` Mark Yacoub 2021-05-14 15:18 ` Wheeler, Daniel 2021-05-14 15:27 ` Harry Wentland 2 siblings, 0 replies; 13+ messages in thread From: Mark Yacoub @ 2021-05-14 14:54 UTC (permalink / raw) To: Rodrigo Siqueira Cc: Harry Wentland, Tianci . Yin, amd-gfx list, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas On Fri, May 14, 2021 at 7:48 AM Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote: > > A few weeks ago, we saw a two cursor issue in a ChromeOS system. We > fixed it in the commit: > > drm/amd/display: Fix two cursor duplication when using overlay > (read the commit message for more details) > > After this change, we noticed that some IGT subtests related to > kms_plane and kms_plane_scaling started to fail. After investigating > this issue, we noticed that all subtests that fail have a primary plane > covering the overlay plane, which is currently rejected by amdgpu dm. > Fail those IGT tests highlight that our verification was too broad and > compromises the overlay usage in our drive. This patch fixes this issue nit: driver* > by ensuring that we only reject commits where the primary plane is not > fully covered by the overlay when the cursor hardware is enabled. With > this fix, all IGT tests start to pass again, which means our overlay > support works as expected. > > Cc: Tianci.Yin <tianci.yin@amd.com> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Nicholas Choi <nicholas.choi@amd.com> > Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> > Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> > Cc: Mark Yacoub <markyacoub@google.com> > Cc: Daniel Wheeler <daniel.wheeler@amd.com> > > Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index ccd67003b120..9c2537a17a7b 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) > int i; > struct drm_plane *plane; > struct drm_plane_state *old_plane_state, *new_plane_state; > - struct drm_plane_state *primary_state, *overlay_state = NULL; > + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL; > > /* Check if primary plane is contained inside overlay */ > for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { > @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) > if (!primary_state->crtc) > return 0; > > + /* check if cursor plane is enabled */ > + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); > + if (IS_ERR(cursor_state)) > + return PTR_ERR(cursor_state); > + > + if (drm_atomic_plane_disabling(plane->state, cursor_state)) > + return 0; > + > /* Perform the bounds check to ensure the overlay plane covers the primary */ > if (primary_state->crtc_x < overlay_state->crtc_x || > primary_state->crtc_y < overlay_state->crtc_y || > -- > 2.25.1 > Happy this was caught earlier. Would not have been fun debugging it on Chrome haha. _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-05-14 11:47 [PATCH] drm/amd/display: Fix overlay validation by considering cursors Rodrigo Siqueira 2021-05-14 14:54 ` Mark Yacoub @ 2021-05-14 15:18 ` Wheeler, Daniel 2021-05-14 15:27 ` Harry Wentland 2 siblings, 0 replies; 13+ messages in thread From: Wheeler, Daniel @ 2021-05-14 15:18 UTC (permalink / raw) To: Siqueira, Rodrigo, amd-gfx Cc: Wentland, Harry, Yin, Tianci (Rico), Choi, Nicholas, Lakha, Bhawanpreet, Kazlauskas, Nicholas, Mark Yacoub [AMD Public Use] The tests that failed previously for me were: amdgpu/amd_plane -> mpo-swizzle-toggle kms_atomic -> plane-overlay-legacy kms_plane -> plane-position-covered-pipe-a-planes kms_plane -> plane-position-covered-pipe-c-planes kms_plane -> plane-position-hole-dpms-pipe-a-planes kms_plane -> plane-position-hole-dpms-pipe-c-planes kms_plane -> plane-position-hole-pipe-a-planes kms_plane -> plane-position-hole-pipe-c-planes kms_plane_scaling -> pipe-a-plane-scaling kms_plane_scaling -> pipe-c-plane-scaling After testing this patch on a HP Envy 360, with Ryzen 5 4500u, a Sapphire Pulse RX5700XT, and an AMD Reference RX6800, these tests all now pass. Tested-by: Daniel Wheeler <daniel.wheeler@amd.com> Thank you, Dan Wheeler Technologist | AMD SW Display ------------------------------------------------------------------------------------------------------------------ 1 Commerce Valley Dr E, Thornhill, ON L3T 7X6 Facebook | Twitter | amd.com Thank you, Dan Wheeler Technologist | AMD SW Display O +(1) 905-882-2600 ext. 74665 ------------------------------------------------------------------------------------------------------------------ 1 Commerce Valley Dr E, Thornhill, ON L3T 7X6 Facebook | Twitter | amd.com -----Original Message----- From: Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com> Sent: May 14, 2021 7:48 AM To: amd-gfx@lists.freedesktop.org Cc: Yin, Tianci (Rico) <Tianci.Yin@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Choi, Nicholas <Nicholas.Choi@amd.com>; Lakha, Bhawanpreet <Bhawanpreet.Lakha@amd.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Mark Yacoub <markyacoub@google.com>; Wheeler, Daniel <Daniel.Wheeler@amd.com> Subject: [PATCH] drm/amd/display: Fix overlay validation by considering cursors A few weeks ago, we saw a two cursor issue in a ChromeOS system. We fixed it in the commit: drm/amd/display: Fix two cursor duplication when using overlay (read the commit message for more details) After this change, we noticed that some IGT subtests related to kms_plane and kms_plane_scaling started to fail. After investigating this issue, we noticed that all subtests that fail have a primary plane covering the overlay plane, which is currently rejected by amdgpu dm. Fail those IGT tests highlight that our verification was too broad and compromises the overlay usage in our drive. This patch fixes this issue by ensuring that we only reject commits where the primary plane is not fully covered by the overlay when the cursor hardware is enabled. With this fix, all IGT tests start to pass again, which means our overlay support works as expected. Cc: Tianci.Yin <tianci.yin@amd.com> Cc: Harry Wentland <harry.wentland@amd.com> Cc: Nicholas Choi <nicholas.choi@amd.com> Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> Cc: Mark Yacoub <markyacoub@google.com> Cc: Daniel Wheeler <daniel.wheeler@amd.com> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index ccd67003b120..9c2537a17a7b 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) int i; struct drm_plane *plane; struct drm_plane_state *old_plane_state, *new_plane_state; - struct drm_plane_state *primary_state, *overlay_state = NULL; + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = +NULL; /* Check if primary plane is contained inside overlay */ for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) if (!primary_state->crtc) return 0; + /* check if cursor plane is enabled */ + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); + if (IS_ERR(cursor_state)) + return PTR_ERR(cursor_state); + + if (drm_atomic_plane_disabling(plane->state, cursor_state)) + return 0; + /* Perform the bounds check to ensure the overlay plane covers the primary */ if (primary_state->crtc_x < overlay_state->crtc_x || primary_state->crtc_y < overlay_state->crtc_y || -- 2.25.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-05-14 11:47 [PATCH] drm/amd/display: Fix overlay validation by considering cursors Rodrigo Siqueira 2021-05-14 14:54 ` Mark Yacoub 2021-05-14 15:18 ` Wheeler, Daniel @ 2021-05-14 15:27 ` Harry Wentland 2021-05-14 16:31 ` Mark Yacoub 2 siblings, 1 reply; 13+ messages in thread From: Harry Wentland @ 2021-05-14 15:27 UTC (permalink / raw) To: Rodrigo Siqueira, amd-gfx Cc: Tianci . Yin, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas, Mark Yacoub On 2021-05-14 7:47 a.m., Rodrigo Siqueira wrote: > A few weeks ago, we saw a two cursor issue in a ChromeOS system. We > fixed it in the commit: > > drm/amd/display: Fix two cursor duplication when using overlay > (read the commit message for more details) > > After this change, we noticed that some IGT subtests related to > kms_plane and kms_plane_scaling started to fail. After investigating > this issue, we noticed that all subtests that fail have a primary plane > covering the overlay plane, which is currently rejected by amdgpu dm. > Fail those IGT tests highlight that our verification was too broad and > compromises the overlay usage in our drive. This patch fixes this issue > by ensuring that we only reject commits where the primary plane is not > fully covered by the overlay when the cursor hardware is enabled. With > this fix, all IGT tests start to pass again, which means our overlay > support works as expected. > > Cc: Tianci.Yin <tianci.yin@amd.com> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Nicholas Choi <nicholas.choi@amd.com> > Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> > Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> > Cc: Mark Yacoub <markyacoub@google.com> > Cc: Daniel Wheeler <daniel.wheeler@amd.com> > > Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index ccd67003b120..9c2537a17a7b 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) > int i; > struct drm_plane *plane; > struct drm_plane_state *old_plane_state, *new_plane_state; > - struct drm_plane_state *primary_state, *overlay_state = NULL; > + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL; > > /* Check if primary plane is contained inside overlay */ > for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { > @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) > if (!primary_state->crtc) > return 0; > > + /* check if cursor plane is enabled */ > + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); > + if (IS_ERR(cursor_state)) > + return PTR_ERR(cursor_state); > + > + if (drm_atomic_plane_disabling(plane->state, cursor_state)) > + return 0; > + I thought this breaks Chrome's compositor since it can't handle an atomic commit rejection based solely on whether a cursor is enabled or not. Harry > /* Perform the bounds check to ensure the overlay plane covers the primary */ > if (primary_state->crtc_x < overlay_state->crtc_x || > primary_state->crtc_y < overlay_state->crtc_y || > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-05-14 15:27 ` Harry Wentland @ 2021-05-14 16:31 ` Mark Yacoub 2021-05-14 16:37 ` Mark Yacoub 0 siblings, 1 reply; 13+ messages in thread From: Mark Yacoub @ 2021-05-14 16:31 UTC (permalink / raw) To: Mark Yacoub Cc: Harry Wentland, Tianci . Yin, Rodrigo Siqueira, amd-gfx list, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas On Fri, May 14, 2021 at 11:28 AM Harry Wentland <harry.wentland@amd.com> wrote: > > On 2021-05-14 7:47 a.m., Rodrigo Siqueira wrote: > > A few weeks ago, we saw a two cursor issue in a ChromeOS system. We > > fixed it in the commit: > > > > drm/amd/display: Fix two cursor duplication when using overlay > > (read the commit message for more details) > > > > After this change, we noticed that some IGT subtests related to > > kms_plane and kms_plane_scaling started to fail. After investigating > > this issue, we noticed that all subtests that fail have a primary plane > > covering the overlay plane, which is currently rejected by amdgpu dm. > > Fail those IGT tests highlight that our verification was too broad and > > compromises the overlay usage in our drive. This patch fixes this issue > > by ensuring that we only reject commits where the primary plane is not > > fully covered by the overlay when the cursor hardware is enabled. With > > this fix, all IGT tests start to pass again, which means our overlay > > support works as expected. > > > > Cc: Tianci.Yin <tianci.yin@amd.com> > > Cc: Harry Wentland <harry.wentland@amd.com> > > Cc: Nicholas Choi <nicholas.choi@amd.com> > > Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> > > Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> > > Cc: Mark Yacoub <markyacoub@google.com> > > Cc: Daniel Wheeler <daniel.wheeler@amd.com> > > > > Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > > --- > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > index ccd67003b120..9c2537a17a7b 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) > > int i; > > struct drm_plane *plane; > > struct drm_plane_state *old_plane_state, *new_plane_state; > > - struct drm_plane_state *primary_state, *overlay_state = NULL; > > + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL; > > > > /* Check if primary plane is contained inside overlay */ > > for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { > > @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) > > if (!primary_state->crtc) > > return 0; > > > > + /* check if cursor plane is enabled */ > > + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); > > + if (IS_ERR(cursor_state)) > > + return PTR_ERR(cursor_state); > > + > > + if (drm_atomic_plane_disabling(plane->state, cursor_state)) > > + return 0; > > + > > I thought this breaks Chrome's compositor since it can't handle an atomic commit rejection > based solely on whether a cursor is enabled or not. > > Harry > > > /* Perform the bounds check to ensure the overlay plane covers the primary */ > > if (primary_state->crtc_x < overlay_state->crtc_x || > > primary_state->crtc_y < overlay_state->crtc_y || > > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-05-14 16:31 ` Mark Yacoub @ 2021-05-14 16:37 ` Mark Yacoub 2021-05-18 18:58 ` Rodrigo Siqueira 0 siblings, 1 reply; 13+ messages in thread From: Mark Yacoub @ 2021-05-14 16:37 UTC (permalink / raw) To: Mark Yacoub Cc: Harry Wentland, Tianci . Yin, Rodrigo Siqueira, amd-gfx list, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas On Fri, May 14, 2021 at 12:31 PM Mark Yacoub <markyacoub@google.com> wrote: > > On Fri, May 14, 2021 at 11:28 AM Harry Wentland <harry.wentland@amd.com> wrote: > > > > On 2021-05-14 7:47 a.m., Rodrigo Siqueira wrote: > > > A few weeks ago, we saw a two cursor issue in a ChromeOS system. We > > > fixed it in the commit: > > > > > > drm/amd/display: Fix two cursor duplication when using overlay > > > (read the commit message for more details) > > > > > > After this change, we noticed that some IGT subtests related to > > > kms_plane and kms_plane_scaling started to fail. After investigating > > > this issue, we noticed that all subtests that fail have a primary plane > > > covering the overlay plane, which is currently rejected by amdgpu dm. > > > Fail those IGT tests highlight that our verification was too broad and > > > compromises the overlay usage in our drive. This patch fixes this issue nit: s/drive/driver? > > > by ensuring that we only reject commits where the primary plane is not > > > fully covered by the overlay when the cursor hardware is enabled. With > > > this fix, all IGT tests start to pass again, which means our overlay > > > support works as expected. > > > > > > Cc: Tianci.Yin <tianci.yin@amd.com> > > > Cc: Harry Wentland <harry.wentland@amd.com> > > > Cc: Nicholas Choi <nicholas.choi@amd.com> > > > Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> > > > Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> > > > Cc: Mark Yacoub <markyacoub@google.com> > > > Cc: Daniel Wheeler <daniel.wheeler@amd.com> > > > > > > Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > > > --- > > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > index ccd67003b120..9c2537a17a7b 100644 > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) > > > int i; > > > struct drm_plane *plane; > > > struct drm_plane_state *old_plane_state, *new_plane_state; > > > - struct drm_plane_state *primary_state, *overlay_state = NULL; > > > + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL; > > > > > > /* Check if primary plane is contained inside overlay */ > > > for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { > > > @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) > > > if (!primary_state->crtc) > > > return 0; > > > > > > + /* check if cursor plane is enabled */ > > > + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); > > > + if (IS_ERR(cursor_state)) > > > + return PTR_ERR(cursor_state); > > > + > > > + if (drm_atomic_plane_disabling(plane->state, cursor_state)) > > > + return 0; > > > + > > > > I thought this breaks Chrome's compositor since it can't handle an atomic commit rejection > > based solely on whether a cursor is enabled or not. For context: To use overlays (the old/current async way), Chrome tests if an overlay strategy will work by doing an atomic commit with a TEST flag to check if the combination would work. If it works, it flags this planes configuration as valid. As it's valid, it performs an atomic page flip (which could also include the cursor). So to Harry's point, you would pass an atomic test but fail on an atomic page flip with the exact same configuration that's been flagged as valid. Failing a pageflip causes Chrome to crash (Reset the GPU process cause something went horribly wrong when it shouldn't). > > > > Harry > > > > > /* Perform the bounds check to ensure the overlay plane covers the primary */ > > > if (primary_state->crtc_x < overlay_state->crtc_x || > > > primary_state->crtc_y < overlay_state->crtc_y || > > > > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-05-14 16:37 ` Mark Yacoub @ 2021-05-18 18:58 ` Rodrigo Siqueira 2021-06-07 18:19 ` Sean Paul 0 siblings, 1 reply; 13+ messages in thread From: Rodrigo Siqueira @ 2021-05-18 18:58 UTC (permalink / raw) To: Mark Yacoub, seanpaul Cc: Harry Wentland, Tianci . Yin, amd-gfx list, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas, Mark Yacoub [-- Attachment #1.1: Type: text/plain, Size: 5178 bytes --] On 05/14, Mark Yacoub wrote: > On Fri, May 14, 2021 at 12:31 PM Mark Yacoub <markyacoub@google.com> wrote: > > > > On Fri, May 14, 2021 at 11:28 AM Harry Wentland <harry.wentland@amd.com> wrote: > > > > > > On 2021-05-14 7:47 a.m., Rodrigo Siqueira wrote: > > > > A few weeks ago, we saw a two cursor issue in a ChromeOS system. We > > > > fixed it in the commit: > > > > > > > > drm/amd/display: Fix two cursor duplication when using overlay > > > > (read the commit message for more details) > > > > > > > > After this change, we noticed that some IGT subtests related to > > > > kms_plane and kms_plane_scaling started to fail. After investigating > > > > this issue, we noticed that all subtests that fail have a primary plane > > > > covering the overlay plane, which is currently rejected by amdgpu dm. > > > > Fail those IGT tests highlight that our verification was too broad and > > > > compromises the overlay usage in our drive. This patch fixes this issue > nit: s/drive/driver? > > > > by ensuring that we only reject commits where the primary plane is not > > > > fully covered by the overlay when the cursor hardware is enabled. With > > > > this fix, all IGT tests start to pass again, which means our overlay > > > > support works as expected. > > > > > > > > Cc: Tianci.Yin <tianci.yin@amd.com> > > > > Cc: Harry Wentland <harry.wentland@amd.com> > > > > Cc: Nicholas Choi <nicholas.choi@amd.com> > > > > Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> > > > > Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> > > > > Cc: Mark Yacoub <markyacoub@google.com> > > > > Cc: Daniel Wheeler <daniel.wheeler@amd.com> > > > > > > > > Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > > > > --- > > > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > index ccd67003b120..9c2537a17a7b 100644 > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) > > > > int i; > > > > struct drm_plane *plane; > > > > struct drm_plane_state *old_plane_state, *new_plane_state; > > > > - struct drm_plane_state *primary_state, *overlay_state = NULL; > > > > + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL; > > > > > > > > /* Check if primary plane is contained inside overlay */ > > > > for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { > > > > @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) > > > > if (!primary_state->crtc) > > > > return 0; > > > > > > > > + /* check if cursor plane is enabled */ > > > > + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); > > > > + if (IS_ERR(cursor_state)) > > > > + return PTR_ERR(cursor_state); > > > > + > > > > + if (drm_atomic_plane_disabling(plane->state, cursor_state)) > > > > + return 0; > > > > + > > > > > > I thought this breaks Chrome's compositor since it can't handle an atomic commit rejection > > > based solely on whether a cursor is enabled or not. > For context: To use overlays (the old/current async way), Chrome tests > if an overlay strategy will work by doing an atomic commit with a TEST > flag to check if the combination would work. If it works, it flags > this planes configuration as valid. As it's valid, it performs an > atomic page flip (which could also include the cursor). > So to Harry's point, you would pass an atomic test but fail on an > atomic page flip with the exact same configuration that's been flagged > as valid. Failing a pageflip causes Chrome to crash (Reset the GPU > process cause something went horribly wrong when it shouldn't). Hi Mark and Sean, I don't know if I fully comprehended the scenario which in my patch might cause a ChromeOS crash, but this is what I understood: There is a chance that we pass the atomic check (DRM_MODE_ATOMIC_TEST_ONLY - TEST) but fails in the atomic page flip because, after use TEST, the compositor might slightly change the commit config by adding the cursor. The reason behind that came from the assumption that adds the cursor in the atomic commit config after the test is harmless. Is my understand correct? Please, correct me if I'm wrong. If the above reasoning is correct, I think the compositor should not assume anything extra from what it got from the atomic check. Best Regards, Siqueira > > > > > > Harry > > > > > > > /* Perform the bounds check to ensure the overlay plane covers the primary */ > > > > if (primary_state->crtc_x < overlay_state->crtc_x || > > > > primary_state->crtc_y < overlay_state->crtc_y || > > > > > > > -- Rodrigo Siqueira https://siqueira.tech [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-05-18 18:58 ` Rodrigo Siqueira @ 2021-06-07 18:19 ` Sean Paul 2021-06-07 18:37 ` Harry Wentland 0 siblings, 1 reply; 13+ messages in thread From: Sean Paul @ 2021-06-07 18:19 UTC (permalink / raw) To: Rodrigo Siqueira Cc: Mark Yacoub, Tianci . Yin, amd-gfx list, Daniel Wheeler, Nicholas Choi, Harry Wentland, Bhawanpreet Lakha, Nicholas Kazlauskas, Mark Yacoub On Tue, May 18, 2021 at 2:58 PM Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> wrote: > > On 05/14, Mark Yacoub wrote: > > On Fri, May 14, 2021 at 12:31 PM Mark Yacoub <markyacoub@google.com> wrote: > > > > > > On Fri, May 14, 2021 at 11:28 AM Harry Wentland <harry.wentland@amd.com> wrote: > > > > > > > > On 2021-05-14 7:47 a.m., Rodrigo Siqueira wrote: > > > > > A few weeks ago, we saw a two cursor issue in a ChromeOS system. We > > > > > fixed it in the commit: > > > > > > > > > > drm/amd/display: Fix two cursor duplication when using overlay > > > > > (read the commit message for more details) > > > > > > > > > > After this change, we noticed that some IGT subtests related to > > > > > kms_plane and kms_plane_scaling started to fail. After investigating > > > > > this issue, we noticed that all subtests that fail have a primary plane > > > > > covering the overlay plane, which is currently rejected by amdgpu dm. > > > > > Fail those IGT tests highlight that our verification was too broad and > > > > > compromises the overlay usage in our drive. This patch fixes this issue > > nit: s/drive/driver? > > > > > by ensuring that we only reject commits where the primary plane is not > > > > > fully covered by the overlay when the cursor hardware is enabled. With > > > > > this fix, all IGT tests start to pass again, which means our overlay > > > > > support works as expected. > > > > > > > > > > Cc: Tianci.Yin <tianci.yin@amd.com> > > > > > Cc: Harry Wentland <harry.wentland@amd.com> > > > > > Cc: Nicholas Choi <nicholas.choi@amd.com> > > > > > Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> > > > > > Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> > > > > > Cc: Mark Yacoub <markyacoub@google.com> > > > > > Cc: Daniel Wheeler <daniel.wheeler@amd.com> > > > > > > > > > > Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > > > > > --- > > > > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- > > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > index ccd67003b120..9c2537a17a7b 100644 > > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > > @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) > > > > > int i; > > > > > struct drm_plane *plane; > > > > > struct drm_plane_state *old_plane_state, *new_plane_state; > > > > > - struct drm_plane_state *primary_state, *overlay_state = NULL; > > > > > + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL; > > > > > > > > > > /* Check if primary plane is contained inside overlay */ > > > > > for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { > > > > > @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) > > > > > if (!primary_state->crtc) > > > > > return 0; > > > > > > > > > > + /* check if cursor plane is enabled */ > > > > > + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); > > > > > + if (IS_ERR(cursor_state)) > > > > > + return PTR_ERR(cursor_state); > > > > > + > > > > > + if (drm_atomic_plane_disabling(plane->state, cursor_state)) > > > > > + return 0; > > > > > + > > > > > > > > I thought this breaks Chrome's compositor since it can't handle an atomic commit rejection > > > > based solely on whether a cursor is enabled or not. > > For context: To use overlays (the old/current async way), Chrome tests > > if an overlay strategy will work by doing an atomic commit with a TEST > > flag to check if the combination would work. If it works, it flags > > this planes configuration as valid. As it's valid, it performs an > > atomic page flip (which could also include the cursor). > > So to Harry's point, you would pass an atomic test but fail on an > > atomic page flip with the exact same configuration that's been flagged > > as valid. Failing a pageflip causes Chrome to crash (Reset the GPU > > process cause something went horribly wrong when it shouldn't). > > Hi Mark and Sean, > > I don't know if I fully comprehended the scenario which in my patch > might cause a ChromeOS crash, but this is what I understood: > Chrome compositor only does an atomic test when the layout geometry changes (ie: plane is added/removed/resized/moved). This does not take into consideration the cursor. Once the atomic test has been validated for a given layout geometry (set of planes/fbs along with their sizes and locations), Chrome assumes this will continue to be valid. So the situation I'm envisioning is if the cursor is hidden, an overlay-based strategy will pass in the kernel. If at any time the cursor becomes visible, the kernel will start failing commits which causes Chrome's GPU process to crash. In general I'm uneasy with using the cursor in the atomic check since it feels like it could be racy independent of the issue above. I haven't dove into the helper code enough to prove it, just a spidey-sense. Sean > There is a chance that we pass the atomic check > (DRM_MODE_ATOMIC_TEST_ONLY - TEST) but fails in the atomic page flip > because, after use TEST, the compositor might slightly change the commit > config by adding the cursor. The reason behind that came from the > assumption that adds the cursor in the atomic commit config after the > test is harmless. Is my understand correct? Please, correct me if I'm > wrong. > > If the above reasoning is correct, I think the compositor should not > assume anything extra from what it got from the atomic check. > > Best Regards, > Siqueira > > > > > > > > > Harry > > > > > > > > > /* Perform the bounds check to ensure the overlay plane covers the primary */ > > > > > if (primary_state->crtc_x < overlay_state->crtc_x || > > > > > primary_state->crtc_y < overlay_state->crtc_y || > > > > > > > > > > > -- > Rodrigo Siqueira > https://siqueira.tech _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-06-07 18:19 ` Sean Paul @ 2021-06-07 18:37 ` Harry Wentland 2021-06-07 18:45 ` Sean Paul 0 siblings, 1 reply; 13+ messages in thread From: Harry Wentland @ 2021-06-07 18:37 UTC (permalink / raw) To: Sean Paul, Rodrigo Siqueira Cc: Mark Yacoub, Tianci . Yin, amd-gfx list, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas, Mark Yacoub On 2021-06-07 2:19 p.m., Sean Paul wrote: > On Tue, May 18, 2021 at 2:58 PM Rodrigo Siqueira > <Rodrigo.Siqueira@amd.com> wrote: >> >> On 05/14, Mark Yacoub wrote: >>> On Fri, May 14, 2021 at 12:31 PM Mark Yacoub <markyacoub@google.com> wrote: >>>> >>>> On Fri, May 14, 2021 at 11:28 AM Harry Wentland <harry.wentland@amd.com> wrote: >>>>> >>>>> On 2021-05-14 7:47 a.m., Rodrigo Siqueira wrote: >>>>>> A few weeks ago, we saw a two cursor issue in a ChromeOS system. We >>>>>> fixed it in the commit: >>>>>> >>>>>> drm/amd/display: Fix two cursor duplication when using overlay >>>>>> (read the commit message for more details) >>>>>> >>>>>> After this change, we noticed that some IGT subtests related to >>>>>> kms_plane and kms_plane_scaling started to fail. After investigating >>>>>> this issue, we noticed that all subtests that fail have a primary plane >>>>>> covering the overlay plane, which is currently rejected by amdgpu dm. >>>>>> Fail those IGT tests highlight that our verification was too broad and >>>>>> compromises the overlay usage in our drive. This patch fixes this issue >>> nit: s/drive/driver? >>>>>> by ensuring that we only reject commits where the primary plane is not >>>>>> fully covered by the overlay when the cursor hardware is enabled. With >>>>>> this fix, all IGT tests start to pass again, which means our overlay >>>>>> support works as expected. >>>>>> >>>>>> Cc: Tianci.Yin <tianci.yin@amd.com> >>>>>> Cc: Harry Wentland <harry.wentland@amd.com> >>>>>> Cc: Nicholas Choi <nicholas.choi@amd.com> >>>>>> Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> >>>>>> Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> >>>>>> Cc: Mark Yacoub <markyacoub@google.com> >>>>>> Cc: Daniel Wheeler <daniel.wheeler@amd.com> >>>>>> >>>>>> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> >>>>>> --- >>>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- >>>>>> 1 file changed, 9 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>>> index ccd67003b120..9c2537a17a7b 100644 >>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>>> @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) >>>>>> int i; >>>>>> struct drm_plane *plane; >>>>>> struct drm_plane_state *old_plane_state, *new_plane_state; >>>>>> - struct drm_plane_state *primary_state, *overlay_state = NULL; >>>>>> + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL; >>>>>> >>>>>> /* Check if primary plane is contained inside overlay */ >>>>>> for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { >>>>>> @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) >>>>>> if (!primary_state->crtc) >>>>>> return 0; >>>>>> >>>>>> + /* check if cursor plane is enabled */ >>>>>> + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); >>>>>> + if (IS_ERR(cursor_state)) >>>>>> + return PTR_ERR(cursor_state); >>>>>> + >>>>>> + if (drm_atomic_plane_disabling(plane->state, cursor_state)) >>>>>> + return 0; >>>>>> + >>>>> >>>>> I thought this breaks Chrome's compositor since it can't handle an atomic commit rejection >>>>> based solely on whether a cursor is enabled or not. >>> For context: To use overlays (the old/current async way), Chrome tests >>> if an overlay strategy will work by doing an atomic commit with a TEST >>> flag to check if the combination would work. If it works, it flags >>> this planes configuration as valid. As it's valid, it performs an >>> atomic page flip (which could also include the cursor). >>> So to Harry's point, you would pass an atomic test but fail on an >>> atomic page flip with the exact same configuration that's been flagged >>> as valid. Failing a pageflip causes Chrome to crash (Reset the GPU >>> process cause something went horribly wrong when it shouldn't). >> >> Hi Mark and Sean, >> >> I don't know if I fully comprehended the scenario which in my patch >> might cause a ChromeOS crash, but this is what I understood: >> > > Chrome compositor only does an atomic test when the layout geometry > changes (ie: plane is added/removed/resized/moved). This does not take > into consideration the cursor. Once the atomic test has been validated > for a given layout geometry (set of planes/fbs along with their sizes > and locations), Chrome assumes this will continue to be valid. > > So the situation I'm envisioning is if the cursor is hidden, an > overlay-based strategy will pass in the kernel. If at any time the > cursor becomes visible, the kernel will start failing commits which > causes Chrome's GPU process to crash. > > In general I'm uneasy with using the cursor in the atomic check since > it feels like it could be racy independent of the issue above. I > haven't dove into the helper code enough to prove it, just a > spidey-sense. > Isn't the cursor supposed to be just another plane? If so, shouldn't adding/removing the cursor trigger an atomic test? Is Chrome's compositor doing cursor update through legacy IOCTLs or through the ATOMIC IOCTL? Thanks, Harry > Sean > >> There is a chance that we pass the atomic check >> (DRM_MODE_ATOMIC_TEST_ONLY - TEST) but fails in the atomic page flip >> because, after use TEST, the compositor might slightly change the commit >> config by adding the cursor. The reason behind that came from the >> assumption that adds the cursor in the atomic commit config after the >> test is harmless. Is my understand correct? Please, correct me if I'm >> wrong. >> >> If the above reasoning is correct, I think the compositor should not >> assume anything extra from what it got from the atomic check. >> >> Best Regards, >> Siqueira >> >>>>> >>>>> Harry >>>>> >>>>>> /* Perform the bounds check to ensure the overlay plane covers the primary */ >>>>>> if (primary_state->crtc_x < overlay_state->crtc_x || >>>>>> primary_state->crtc_y < overlay_state->crtc_y || >>>>>> >>>>> >> >> -- >> Rodrigo Siqueira >> https://siqueira.tech/> _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-06-07 18:37 ` Harry Wentland @ 2021-06-07 18:45 ` Sean Paul 2021-06-08 7:47 ` Michel Dänzer 0 siblings, 1 reply; 13+ messages in thread From: Sean Paul @ 2021-06-07 18:45 UTC (permalink / raw) To: Harry Wentland Cc: Mark Yacoub, Tianci . Yin, Rodrigo Siqueira, amd-gfx list, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas, Mark Yacoub [-- Attachment #1.1: Type: text/plain, Size: 6878 bytes --] On Mon, Jun 7, 2021 at 2:37 PM Harry Wentland <harry.wentland@amd.com> wrote: > On 2021-06-07 2:19 p.m., Sean Paul wrote: > > On Tue, May 18, 2021 at 2:58 PM Rodrigo Siqueira > > <Rodrigo.Siqueira@amd.com> wrote: > >> > >> On 05/14, Mark Yacoub wrote: > >>> On Fri, May 14, 2021 at 12:31 PM Mark Yacoub <markyacoub@google.com> > wrote: > >>>> > >>>> On Fri, May 14, 2021 at 11:28 AM Harry Wentland < > harry.wentland@amd.com> wrote: > >>>>> > >>>>> On 2021-05-14 7:47 a.m., Rodrigo Siqueira wrote: > >>>>>> A few weeks ago, we saw a two cursor issue in a ChromeOS system. We > >>>>>> fixed it in the commit: > >>>>>> > >>>>>> drm/amd/display: Fix two cursor duplication when using overlay > >>>>>> (read the commit message for more details) > >>>>>> > >>>>>> After this change, we noticed that some IGT subtests related to > >>>>>> kms_plane and kms_plane_scaling started to fail. After investigating > >>>>>> this issue, we noticed that all subtests that fail have a primary > plane > >>>>>> covering the overlay plane, which is currently rejected by amdgpu > dm. > >>>>>> Fail those IGT tests highlight that our verification was too broad > and > >>>>>> compromises the overlay usage in our drive. This patch fixes this > issue > >>> nit: s/drive/driver? > >>>>>> by ensuring that we only reject commits where the primary plane is > not > >>>>>> fully covered by the overlay when the cursor hardware is enabled. > With > >>>>>> this fix, all IGT tests start to pass again, which means our overlay > >>>>>> support works as expected. > >>>>>> > >>>>>> Cc: Tianci.Yin <tianci.yin@amd.com> > >>>>>> Cc: Harry Wentland <harry.wentland@amd.com> > >>>>>> Cc: Nicholas Choi <nicholas.choi@amd.com> > >>>>>> Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com> > >>>>>> Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com> > >>>>>> Cc: Mark Yacoub <markyacoub@google.com> > >>>>>> Cc: Daniel Wheeler <daniel.wheeler@amd.com> > >>>>>> > >>>>>> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- > >>>>>> 1 file changed, 9 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>>>>> index ccd67003b120..9c2537a17a7b 100644 > >>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>>>>> @@ -10067,7 +10067,7 @@ static int validate_overlay(struct > drm_atomic_state *state) > >>>>>> int i; > >>>>>> struct drm_plane *plane; > >>>>>> struct drm_plane_state *old_plane_state, *new_plane_state; > >>>>>> - struct drm_plane_state *primary_state, *overlay_state = NULL; > >>>>>> + struct drm_plane_state *primary_state, *cursor_state, > *overlay_state = NULL; > >>>>>> > >>>>>> /* Check if primary plane is contained inside overlay */ > >>>>>> for_each_oldnew_plane_in_state_reverse(state, plane, > old_plane_state, new_plane_state, i) { > >>>>>> @@ -10097,6 +10097,14 @@ static int validate_overlay(struct > drm_atomic_state *state) > >>>>>> if (!primary_state->crtc) > >>>>>> return 0; > >>>>>> > >>>>>> + /* check if cursor plane is enabled */ > >>>>>> + cursor_state = drm_atomic_get_plane_state(state, > overlay_state->crtc->cursor); > >>>>>> + if (IS_ERR(cursor_state)) > >>>>>> + return PTR_ERR(cursor_state); > >>>>>> + > >>>>>> + if (drm_atomic_plane_disabling(plane->state, cursor_state)) > >>>>>> + return 0; > >>>>>> + > >>>>> > >>>>> I thought this breaks Chrome's compositor since it can't handle an > atomic commit rejection > >>>>> based solely on whether a cursor is enabled or not. > >>> For context: To use overlays (the old/current async way), Chrome tests > >>> if an overlay strategy will work by doing an atomic commit with a TEST > >>> flag to check if the combination would work. If it works, it flags > >>> this planes configuration as valid. As it's valid, it performs an > >>> atomic page flip (which could also include the cursor). > >>> So to Harry's point, you would pass an atomic test but fail on an > >>> atomic page flip with the exact same configuration that's been flagged > >>> as valid. Failing a pageflip causes Chrome to crash (Reset the GPU > >>> process cause something went horribly wrong when it shouldn't). > >> > >> Hi Mark and Sean, > >> > >> I don't know if I fully comprehended the scenario which in my patch > >> might cause a ChromeOS crash, but this is what I understood: > >> > > > > Chrome compositor only does an atomic test when the layout geometry > > changes (ie: plane is added/removed/resized/moved). This does not take > > into consideration the cursor. Once the atomic test has been validated > > for a given layout geometry (set of planes/fbs along with their sizes > > and locations), Chrome assumes this will continue to be valid. > > > > So the situation I'm envisioning is if the cursor is hidden, an > > overlay-based strategy will pass in the kernel. If at any time the > > cursor becomes visible, the kernel will start failing commits which > > causes Chrome's GPU process to crash. > > > > In general I'm uneasy with using the cursor in the atomic check since > > it feels like it could be racy independent of the issue above. I > > haven't dove into the helper code enough to prove it, just a > > spidey-sense. > > > > Isn't the cursor supposed to be just another plane? If so, shouldn't > adding/removing the cursor trigger an atomic test? > > Chrome is using legacy cursor. Yes it will trigger an atomic test in the kernel, and that atomic test will fail. Unfortunately Chrome does not expect a failure so it will crash. Sean Is Chrome's compositor doing cursor update through legacy IOCTLs or > through the ATOMIC IOCTL? > > Thanks, > Harry > > > Sean > > > >> There is a chance that we pass the atomic check > >> (DRM_MODE_ATOMIC_TEST_ONLY - TEST) but fails in the atomic page flip > >> because, after use TEST, the compositor might slightly change the commit > >> config by adding the cursor. The reason behind that came from the > >> assumption that adds the cursor in the atomic commit config after the > >> test is harmless. Is my understand correct? Please, correct me if I'm > >> wrong. > >> > >> If the above reasoning is correct, I think the compositor should not > >> assume anything extra from what it got from the atomic check. > >> > >> Best Regards, > >> Siqueira > >> > >>>>> > >>>>> Harry > >>>>> > >>>>>> /* Perform the bounds check to ensure the overlay plane > covers the primary */ > >>>>>> if (primary_state->crtc_x < overlay_state->crtc_x || > >>>>>> primary_state->crtc_y < overlay_state->crtc_y || > >>>>>> > >>>>> > >> > >> -- > >> Rodrigo Siqueira > >> https://siqueira.tech/> > [-- Attachment #1.2: Type: text/html, Size: 10343 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-06-07 18:45 ` Sean Paul @ 2021-06-08 7:47 ` Michel Dänzer 2021-06-08 19:07 ` Harry Wentland 0 siblings, 1 reply; 13+ messages in thread From: Michel Dänzer @ 2021-06-08 7:47 UTC (permalink / raw) To: Sean Paul, Harry Wentland Cc: Mark Yacoub, Tianci . Yin, Rodrigo Siqueira, amd-gfx list, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas, Mark Yacoub On 2021-06-07 8:45 p.m., Sean Paul wrote: > > > On Mon, Jun 7, 2021 at 2:37 PM Harry Wentland <harry.wentland@amd.com <mailto:harry.wentland@amd.com>> wrote: > > On 2021-06-07 2:19 p.m., Sean Paul wrote: > > On Tue, May 18, 2021 at 2:58 PM Rodrigo Siqueira > > <Rodrigo.Siqueira@amd.com <mailto:Rodrigo.Siqueira@amd.com>> wrote: > >> > >> On 05/14, Mark Yacoub wrote: > >>> On Fri, May 14, 2021 at 12:31 PM Mark Yacoub <markyacoub@google.com <mailto:markyacoub@google.com>> wrote: > >>>> > >>>> On Fri, May 14, 2021 at 11:28 AM Harry Wentland <harry.wentland@amd.com <mailto:harry.wentland@amd.com>> wrote: > >>>>> > >>>>> On 2021-05-14 7:47 a.m., Rodrigo Siqueira wrote: > >>>>>> A few weeks ago, we saw a two cursor issue in a ChromeOS system. We > >>>>>> fixed it in the commit: > >>>>>> > >>>>>> drm/amd/display: Fix two cursor duplication when using overlay > >>>>>> (read the commit message for more details) > >>>>>> > >>>>>> After this change, we noticed that some IGT subtests related to > >>>>>> kms_plane and kms_plane_scaling started to fail. After investigating > >>>>>> this issue, we noticed that all subtests that fail have a primary plane > >>>>>> covering the overlay plane, which is currently rejected by amdgpu dm. > >>>>>> Fail those IGT tests highlight that our verification was too broad and > >>>>>> compromises the overlay usage in our drive. This patch fixes this issue > >>> nit: s/drive/driver? > >>>>>> by ensuring that we only reject commits where the primary plane is not > >>>>>> fully covered by the overlay when the cursor hardware is enabled. With > >>>>>> this fix, all IGT tests start to pass again, which means our overlay > >>>>>> support works as expected. > >>>>>> > >>>>>> Cc: Tianci.Yin <tianci.yin@amd.com <mailto:tianci.yin@amd.com>> > >>>>>> Cc: Harry Wentland <harry.wentland@amd.com <mailto:harry.wentland@amd.com>> > >>>>>> Cc: Nicholas Choi <nicholas.choi@amd.com <mailto:nicholas.choi@amd.com>> > >>>>>> Cc: Bhawanpreet Lakha <bhawanpreet.lakha@amd.com <mailto:bhawanpreet.lakha@amd.com>> > >>>>>> Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com <mailto:Nicholas.Kazlauskas@amd.com>> > >>>>>> Cc: Mark Yacoub <markyacoub@google.com <mailto:markyacoub@google.com>> > >>>>>> Cc: Daniel Wheeler <daniel.wheeler@amd.com <mailto:daniel.wheeler@amd.com>> > >>>>>> > >>>>>> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com <mailto:Rodrigo.Siqueira@amd.com>> > >>>>>> --- > >>>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++- > >>>>>> 1 file changed, 9 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>>>>> index ccd67003b120..9c2537a17a7b 100644 > >>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > >>>>>> @@ -10067,7 +10067,7 @@ static int validate_overlay(struct drm_atomic_state *state) > >>>>>> int i; > >>>>>> struct drm_plane *plane; > >>>>>> struct drm_plane_state *old_plane_state, *new_plane_state; > >>>>>> - struct drm_plane_state *primary_state, *overlay_state = NULL; > >>>>>> + struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL; > >>>>>> > >>>>>> /* Check if primary plane is contained inside overlay */ > >>>>>> for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) { > >>>>>> @@ -10097,6 +10097,14 @@ static int validate_overlay(struct drm_atomic_state *state) > >>>>>> if (!primary_state->crtc) > >>>>>> return 0; > >>>>>> > >>>>>> + /* check if cursor plane is enabled */ > >>>>>> + cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor); > >>>>>> + if (IS_ERR(cursor_state)) > >>>>>> + return PTR_ERR(cursor_state); > >>>>>> + > >>>>>> + if (drm_atomic_plane_disabling(plane->state, cursor_state)) > >>>>>> + return 0; > >>>>>> + > >>>>> > >>>>> I thought this breaks Chrome's compositor since it can't handle an atomic commit rejection > >>>>> based solely on whether a cursor is enabled or not. > >>> For context: To use overlays (the old/current async way), Chrome tests > >>> if an overlay strategy will work by doing an atomic commit with a TEST > >>> flag to check if the combination would work. If it works, it flags > >>> this planes configuration as valid. As it's valid, it performs an > >>> atomic page flip (which could also include the cursor). > >>> So to Harry's point, you would pass an atomic test but fail on an > >>> atomic page flip with the exact same configuration that's been flagged > >>> as valid. Failing a pageflip causes Chrome to crash (Reset the GPU > >>> process cause something went horribly wrong when it shouldn't). > >> > >> Hi Mark and Sean, > >> > >> I don't know if I fully comprehended the scenario which in my patch > >> might cause a ChromeOS crash, but this is what I understood: > >> > > > > Chrome compositor only does an atomic test when the layout geometry > > changes (ie: plane is added/removed/resized/moved). This does not take > > into consideration the cursor. Once the atomic test has been validated > > for a given layout geometry (set of planes/fbs along with their sizes > > and locations), Chrome assumes this will continue to be valid. > > > > So the situation I'm envisioning is if the cursor is hidden, an > > overlay-based strategy will pass in the kernel. If at any time the > > cursor becomes visible, the kernel will start failing commits which > > causes Chrome's GPU process to crash. > > > > In general I'm uneasy with using the cursor in the atomic check since > > it feels like it could be racy independent of the issue above. I > > haven't dove into the helper code enough to prove it, just a > > spidey-sense. > > > > Isn't the cursor supposed to be just another plane? If so, shouldn't > adding/removing the cursor trigger an atomic test? > > > Chrome is using legacy cursor. > > Yes it will trigger an atomic test in the kernel, and that atomic test will fail. Unfortunately Chrome does not expect a failure so it will crash. The solution is probably indeed for atomic check to reject state which couldn't work if the cursor was enabled, even if the cursor is currently disabled. Otherwise one can hit various surprising errors via legacy APIs, as described in b836a274b797 "drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is". -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-06-08 7:47 ` Michel Dänzer @ 2021-06-08 19:07 ` Harry Wentland 2021-06-08 19:50 ` Sean Paul 0 siblings, 1 reply; 13+ messages in thread From: Harry Wentland @ 2021-06-08 19:07 UTC (permalink / raw) To: Michel Dänzer, Sean Paul Cc: Mark Yacoub, Tianci . Yin, Rodrigo Siqueira, amd-gfx list, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas, Mark Yacoub On 2021-06-08 3:47 a.m., Michel Dänzer wrote: > On 2021-06-07 8:45 p.m., Sean Paul wrote: >> >> >> On Mon, Jun 7, 2021 at 2:37 PM Harry Wentland <harry.wentland@amd.com <mailto:harry.wentland@amd.com>> wrote: >> >> On 2021-06-07 2:19 p.m., Sean Paul wrote: >> > On Tue, May 18, 2021 at 2:58 PM Rodrigo Siqueira >> > <Rodrigo.Siqueira@amd.com <mailto:Rodrigo.Siqueira@amd.com>> wrote: >> >> <snip> >> >> Hi Mark and Sean, >> >> >> >> I don't know if I fully comprehended the scenario which in my patch >> >> might cause a ChromeOS crash, but this is what I understood: >> >> >> > >> > Chrome compositor only does an atomic test when the layout geometry >> > changes (ie: plane is added/removed/resized/moved). This does not take >> > into consideration the cursor. Once the atomic test has been validated >> > for a given layout geometry (set of planes/fbs along with their sizes >> > and locations), Chrome assumes this will continue to be valid. >> > >> > So the situation I'm envisioning is if the cursor is hidden, an >> > overlay-based strategy will pass in the kernel. If at any time the >> > cursor becomes visible, the kernel will start failing commits which >> > causes Chrome's GPU process to crash. >> > >> > In general I'm uneasy with using the cursor in the atomic check since >> > it feels like it could be racy independent of the issue above. I >> > haven't dove into the helper code enough to prove it, just a >> > spidey-sense. >> > >> >> Isn't the cursor supposed to be just another plane? If so, shouldn't >> adding/removing the cursor trigger an atomic test? >> >> >> Chrome is using legacy cursor. >> >> Yes it will trigger an atomic test in the kernel, and that atomic test will fail. Unfortunately Chrome does not expect a failure so it will crash. > > The solution is probably indeed for atomic check to reject state which couldn't work if the cursor was enabled, even if the cursor is currently disabled. Otherwise one can hit various surprising errors via legacy APIs, as described in b836a274b797 "drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is". > Agreed. It's a bit unfortunate but the only way to deal with this better is if we had some way for DRM master to tell us whether it will only use the atomic IOCTL or use legacy IOCTLs (including a combination of atomic and legacy). Harry > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors 2021-06-08 19:07 ` Harry Wentland @ 2021-06-08 19:50 ` Sean Paul 0 siblings, 0 replies; 13+ messages in thread From: Sean Paul @ 2021-06-08 19:50 UTC (permalink / raw) To: Harry Wentland Cc: Mark Yacoub, Michel Dänzer, Tianci . Yin, Rodrigo Siqueira, amd-gfx list, Daniel Wheeler, Nicholas Choi, Bhawanpreet Lakha, Nicholas Kazlauskas, Mark Yacoub On Tue, Jun 8, 2021 at 3:07 PM Harry Wentland <harry.wentland@amd.com> wrote: > > > > On 2021-06-08 3:47 a.m., Michel Dänzer wrote: > > On 2021-06-07 8:45 p.m., Sean Paul wrote: > >> > >> > >> On Mon, Jun 7, 2021 at 2:37 PM Harry Wentland <harry.wentland@amd.com <mailto:harry.wentland@amd.com>> wrote: > >> > >> On 2021-06-07 2:19 p.m., Sean Paul wrote: > >> > On Tue, May 18, 2021 at 2:58 PM Rodrigo Siqueira > >> > <Rodrigo.Siqueira@amd.com <mailto:Rodrigo.Siqueira@amd.com>> wrote: > >> >> > > <snip> > > >> >> Hi Mark and Sean, > >> >> > >> >> I don't know if I fully comprehended the scenario which in my patch > >> >> might cause a ChromeOS crash, but this is what I understood: > >> >> > >> > > >> > Chrome compositor only does an atomic test when the layout geometry > >> > changes (ie: plane is added/removed/resized/moved). This does not take > >> > into consideration the cursor. Once the atomic test has been validated > >> > for a given layout geometry (set of planes/fbs along with their sizes > >> > and locations), Chrome assumes this will continue to be valid. > >> > > >> > So the situation I'm envisioning is if the cursor is hidden, an > >> > overlay-based strategy will pass in the kernel. If at any time the > >> > cursor becomes visible, the kernel will start failing commits which > >> > causes Chrome's GPU process to crash. > >> > > >> > In general I'm uneasy with using the cursor in the atomic check since > >> > it feels like it could be racy independent of the issue above. I > >> > haven't dove into the helper code enough to prove it, just a > >> > spidey-sense. > >> > > >> > >> Isn't the cursor supposed to be just another plane? If so, shouldn't > >> adding/removing the cursor trigger an atomic test? > >> > >> > >> Chrome is using legacy cursor. > >> > >> Yes it will trigger an atomic test in the kernel, and that atomic test will fail. Unfortunately Chrome does not expect a failure so it will crash. > > > > The solution is probably indeed for atomic check to reject state which couldn't work if the cursor was enabled, even if the cursor is currently disabled. Otherwise one can hit various surprising errors via legacy APIs, as described in b836a274b797 "drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is". > > > > Agreed. > > It's a bit unfortunate but the only way to deal with this better is if we had some way for DRM master to tell us whether it will only use the atomic IOCTL or use legacy IOCTLs (including a combination of atomic and legacy). > I think even with this information we wouldn't want to depend on cursor for atomic test. IMO kernel should not back the compositor into re-rendering the scene based on cursor visibility. Given that cursor is usually handled independently and asynchronously from composition, it makes things extremely complex and most compositors would probably just enable cursor all the time to work around this. Sean > Harry > > > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-06-08 19:55 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-14 11:47 [PATCH] drm/amd/display: Fix overlay validation by considering cursors Rodrigo Siqueira 2021-05-14 14:54 ` Mark Yacoub 2021-05-14 15:18 ` Wheeler, Daniel 2021-05-14 15:27 ` Harry Wentland 2021-05-14 16:31 ` Mark Yacoub 2021-05-14 16:37 ` Mark Yacoub 2021-05-18 18:58 ` Rodrigo Siqueira 2021-06-07 18:19 ` Sean Paul 2021-06-07 18:37 ` Harry Wentland 2021-06-07 18:45 ` Sean Paul 2021-06-08 7:47 ` Michel Dänzer 2021-06-08 19:07 ` Harry Wentland 2021-06-08 19:50 ` Sean Paul
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.