[PATCH] Revert "drm/amd/display: Fix overlay validation by considering cursors"

[PATCH] Revert "drm/amd/display: Fix overlay validation by considering cursors"

[Rodrigo Siqueira]

This reverts commit 04cc17a951f73f9a9092ca572b063e6292aeb085.

The patch that we are reverting here was originally applied because it
fixes multiple IGT issues and flickering in Android. However, after a
discussion with Sean Paul and Mark, it looks like that this patch might
cause problems on ChromeOS. For this reason, we decided to revert this
patch.

Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
Cc: Harry Wentland <Harry.Wentland@amd.com>
Cc: Hersen Wu <hersenxs.wu@amd.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Mark Yacoub <markyacoub@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

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 8358112b5822..3fd41e098c90 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10200,8 +10200,8 @@ static int validate_overlay(struct drm_atomic_state *state)
 {
 	int i;
 	struct drm_plane *plane;
-	struct drm_plane_state *new_plane_state;
-	struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
+	struct drm_plane_state *primary_state, *overlay_state = NULL;
 
 	/* Check if primary plane is contained inside overlay */
 	for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i) {
@@ -10231,14 +10231,6 @@ 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

Re: [PATCH] Revert "drm/amd/display: Fix overlay validation by considering cursors"

[Sean Paul]

On Wed, Jun 16, 2021 at 12:21 PM Rodrigo Siqueira
<Rodrigo.Siqueira@amd.com> wrote:
>
> This reverts commit 04cc17a951f73f9a9092ca572b063e6292aeb085.
>
> The patch that we are reverting here was originally applied because it
> fixes multiple IGT issues and flickering in Android. However, after a
> discussion with Sean Paul and Mark, it looks like that this patch might
> cause problems on ChromeOS. For this reason, we decided to revert this
> patch.

Thanks for sending this, Siqueira!

To be clear for those unfamiliar, the issue extends beyond ChromeOS
(we're not just pushing our compositor problems on the rest of the
community).

Relying on cursor enable/disable for atomic creates non-deterministic
behavior which would be very hard for any compositor to reason out
without knowing the hardware-specific limitations. The case I'm
worried about is that the compositor has an overlay active without the
cursor and at some point the compositor enables the cursor which will
fail because of the overlay.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

>
> Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
> Cc: Harry Wentland <Harry.Wentland@amd.com>
> Cc: Hersen Wu <hersenxs.wu@amd.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Mark Yacoub <markyacoub@chromium.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
>
> 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 8358112b5822..3fd41e098c90 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10200,8 +10200,8 @@ static int validate_overlay(struct drm_atomic_state *state)
>  {
>         int i;
>         struct drm_plane *plane;
> -       struct drm_plane_state *new_plane_state;
> -       struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL;
> +       struct drm_plane_state *old_plane_state, *new_plane_state;
> +       struct drm_plane_state *primary_state, *overlay_state = NULL;
>
>         /* Check if primary plane is contained inside overlay */
>         for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i) {
> @@ -10231,14 +10231,6 @@ 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

Re: [PATCH] Revert "drm/amd/display: Fix overlay validation by considering cursors"

[Harry Wentland]

On 2021-06-16 1:14 p.m., Sean Paul wrote:
> On Wed, Jun 16, 2021 at 12:21 PM Rodrigo Siqueira
> <Rodrigo.Siqueira@amd.com> wrote:
>>
>> This reverts commit 04cc17a951f73f9a9092ca572b063e6292aeb085.
>>
>> The patch that we are reverting here was originally applied because it
>> fixes multiple IGT issues and flickering in Android. However, after a
>> discussion with Sean Paul and Mark, it looks like that this patch might
>> cause problems on ChromeOS. For this reason, we decided to revert this
>> patch.
> 
> Thanks for sending this, Siqueira!
> 
> To be clear for those unfamiliar, the issue extends beyond ChromeOS
> (we're not just pushing our compositor problems on the rest of the
> community).
> 
> Relying on cursor enable/disable for atomic creates non-deterministic
> behavior which would be very hard for any compositor to reason out
> without knowing the hardware-specific limitations. The case I'm
> worried about is that the compositor has an overlay active without the
> cursor and at some point the compositor enables the cursor which will
> fail because of the overlay.
> 

Previous discussion highlighted that the cursor IOCTL should never
fail and that userspace generally is not well equipped to deal with
it if it does.

https://patchwork.freedesktop.org/patch/387230/
https://patchwork.freedesktop.org/patch/389084/

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> 
>>
>> Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
>> Cc: Harry Wentland <Harry.Wentland@amd.com>
>> Cc: Hersen Wu <hersenxs.wu@amd.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Mark Yacoub <markyacoub@chromium.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>> ---
>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 ++----------
>>  1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> 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 8358112b5822..3fd41e098c90 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -10200,8 +10200,8 @@ static int validate_overlay(struct drm_atomic_state *state)
>>  {
>>         int i;
>>         struct drm_plane *plane;
>> -       struct drm_plane_state *new_plane_state;
>> -       struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL;
>> +       struct drm_plane_state *old_plane_state, *new_plane_state;
>> +       struct drm_plane_state *primary_state, *overlay_state = NULL;
>>
>>         /* Check if primary plane is contained inside overlay */
>>         for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i) {
>> @@ -10231,14 +10231,6 @@ 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