[PATCH] amd/display: remove ChromeOS workaround

[PATCH] amd/display: remove ChromeOS workaround

[Simon Ser]

This reverts commits ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication
when using overlay") and e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay
validation by considering cursors"").

tl;dr ChromeOS uses the atomic interface for everything except the cursor. This
is incorrect and forces amdgpu to disable some hardware features. Let's revert
the ChromeOS-specific workaround in mainline and allow the Chrome team to keep
it internally in their own tree.

See [1] for more details. This patch is an alternative to [2], which added
ChromeOS detection.

[1]: https://lore.kernel.org/amd-gfx/JIQ_93_cHcshiIDsrMU1huBzx9P9LVQxucx8hQArpQu7Wk5DrCl_vTXj_Q20m_L-8C8A5dSpNcSJ8ehfcCrsQpfB5QG_Spn14EYkH9chtg0=@emersion.fr/
[2]: https://lore.kernel.org/amd-gfx/20211011151609.452132-1-contact@emersion.fr/

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Cc: Sean Paul <seanpaul@chromium.org>
Fixes: ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication when using overlay")
Fixes: e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by considering cursors"")
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 -------------------
 1 file changed, 51 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 20065a145851..014c5a9fe461 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10628,53 +10628,6 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm
 }
 #endif
 
-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, *overlay_state = NULL;
-
-	/* Check if primary plane is contained inside overlay */
-	for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i) {
-		if (plane->type == DRM_PLANE_TYPE_OVERLAY) {
-			if (drm_atomic_plane_disabling(plane->state, new_plane_state))
-				return 0;
-
-			overlay_state = new_plane_state;
-			continue;
-		}
-	}
-
-	/* check if we're making changes to the overlay plane */
-	if (!overlay_state)
-		return 0;
-
-	/* check if overlay plane is enabled */
-	if (!overlay_state->crtc)
-		return 0;
-
-	/* find the primary plane for the CRTC that the overlay is enabled on */
-	primary_state = drm_atomic_get_plane_state(state, overlay_state->crtc->primary);
-	if (IS_ERR(primary_state))
-		return PTR_ERR(primary_state);
-
-	/* check if primary plane is enabled */
-	if (!primary_state->crtc)
-		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 ||
-	    primary_state->crtc_x + primary_state->crtc_w > overlay_state->crtc_x + overlay_state->crtc_w ||
-	    primary_state->crtc_y + primary_state->crtc_h > overlay_state->crtc_y + overlay_state->crtc_h) {
-		DRM_DEBUG_ATOMIC("Overlay plane is enabled with hardware cursor but does not fully cover primary plane\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 /**
  * amdgpu_dm_atomic_check() - Atomic check implementation for AMDgpu DM.
  * @dev: The DRM device
@@ -10856,10 +10809,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
 			goto fail;
 	}
 
-	ret = validate_overlay(state);
-	if (ret)
-		goto fail;
-
 	/* Add new/modified planes */
 	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
 		ret = dm_update_plane_state(dc, state, plane,
-- 
2.33.0

Re: [PATCH] amd/display: remove ChromeOS workaround

[Paul Menzel]

Dear Simon,


Am 14.10.21 um 17:35 schrieb Simon Ser:
> This reverts commits ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication
> when using overlay") and e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay
> validation by considering cursors"").
> 
> tl;dr ChromeOS uses the atomic interface for everything except the cursor. This
> is incorrect and forces amdgpu to disable some hardware features. Let's revert
> the ChromeOS-specific workaround in mainline and allow the Chrome team to keep
> it internally in their own tree.
> 
> See [1] for more details. This patch is an alternative to [2], which added
> ChromeOS detection.

Excuse my ignorance. Reading the commit message, there was a Linux 
kernel change, that broke Chrome OS userspace, right? If so, and we do 
not know if there is other userspace using the API incorrectly, 
shouldn’t the patch breaking Chrome OS userspace be reverted to adhere 
to Linux’ no-regression rule?


Kind regards,

Paul


> [1]: https://lore.kernel.org/amd-gfx/JIQ_93_cHcshiIDsrMU1huBzx9P9LVQxucx8hQArpQu7Wk5DrCl_vTXj_Q20m_L-8C8A5dSpNcSJ8ehfcCrsQpfB5QG_Spn14EYkH9chtg0=@emersion.fr/
> [2]: https://lore.kernel.org/amd-gfx/20211011151609.452132-1-contact@emersion.fr/
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Fixes: ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication when using overlay")
> Fixes: e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by considering cursors"")
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 -------------------
>   1 file changed, 51 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 20065a145851..014c5a9fe461 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10628,53 +10628,6 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm
>   }
>   #endif
>   
> -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, *overlay_state = NULL;
> -
> -	/* Check if primary plane is contained inside overlay */
> -	for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i) {
> -		if (plane->type == DRM_PLANE_TYPE_OVERLAY) {
> -			if (drm_atomic_plane_disabling(plane->state, new_plane_state))
> -				return 0;
> -
> -			overlay_state = new_plane_state;
> -			continue;
> -		}
> -	}
> -
> -	/* check if we're making changes to the overlay plane */
> -	if (!overlay_state)
> -		return 0;
> -
> -	/* check if overlay plane is enabled */
> -	if (!overlay_state->crtc)
> -		return 0;
> -
> -	/* find the primary plane for the CRTC that the overlay is enabled on */
> -	primary_state = drm_atomic_get_plane_state(state, overlay_state->crtc->primary);
> -	if (IS_ERR(primary_state))
> -		return PTR_ERR(primary_state);
> -
> -	/* check if primary plane is enabled */
> -	if (!primary_state->crtc)
> -		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 ||
> -	    primary_state->crtc_x + primary_state->crtc_w > overlay_state->crtc_x + overlay_state->crtc_w ||
> -	    primary_state->crtc_y + primary_state->crtc_h > overlay_state->crtc_y + overlay_state->crtc_h) {
> -		DRM_DEBUG_ATOMIC("Overlay plane is enabled with hardware cursor but does not fully cover primary plane\n");
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
>   /**
>    * amdgpu_dm_atomic_check() - Atomic check implementation for AMDgpu DM.
>    * @dev: The DRM device
> @@ -10856,10 +10809,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   			goto fail;
>   	}
>   
> -	ret = validate_overlay(state);
> -	if (ret)
> -		goto fail;
> -
>   	/* Add new/modified planes */
>   	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
>   		ret = dm_update_plane_state(dc, state, plane,
> 

Re: [PATCH] amd/display: remove ChromeOS workaround

[Simon Ser]

On Tuesday, October 19th, 2021 at 01:03, Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> Excuse my ignorance. Reading the commit message, there was a Linux
> kernel change, that broke Chrome OS userspace, right? If so, and we do
> not know if there is other userspace using the API incorrectly,
> shouldn’t the patch breaking Chrome OS userspace be reverted to adhere
> to Linux’ no-regression rule?

No. There was a ChromeOS bug which has been thought to be an amdgpu bug. But
fixing that "bug" breaks other user-space.

Re: [PATCH] amd/display: remove ChromeOS workaround

[Paul Menzel]

Dear Simon,


Am 19.10.21 um 01:06 schrieb Simon Ser:
> On Tuesday, October 19th, 2021 at 01:03, Paul Menzel wrote:
> 
>> Excuse my ignorance. Reading the commit message, there was a Linux
>> kernel change, that broke Chrome OS userspace, right? If so, and we do
>> not know if there is other userspace using the API incorrectly,
>> shouldn’t the patch breaking Chrome OS userspace be reverted to adhere
>> to Linux’ no-regression rule?
> 
> No. There was a ChromeOS bug which has been thought to be an amdgpu bug. But
> fixing that "bug" breaks other user-space.

Thank you for the explanation. I guess the bug was only surfacing 
because Chrome OS device, like Chromebooks, are only using AMD hardware 
since a short while (maybe last year).

Reading your message *amdgpu: atomic API and cursor/overlay planes* [1] 
again, it says:

> Up until now we were using cursor and overlay planes in gamescope [3],
> but some changes in the amdgpu driver [1] makes us unable to use planes

So this statement was incorrect? Which changes are that? Or did Chrome 
OS ever work correctly with an older Linux kernel or not?


Kind regards,

Paul


[1]: 
https://lore.kernel.org/amd-gfx/JIQ_93_cHcshiIDsrMU1huBzx9P9LVQxucx8hQArpQu7Wk5DrCl_vTXj_Q20m_L-8C8A5dSpNcSJ8ehfcCrsQpfB5QG_Spn14EYkH9chtg0=@emersion.fr/

Re: [PATCH] amd/display: remove ChromeOS workaround

[Simon Ser]

On Tuesday, October 19th, 2021 at 01:21, Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> Am 19.10.21 um 01:06 schrieb Simon Ser:
> > On Tuesday, October 19th, 2021 at 01:03, Paul Menzel wrote:
> >
> >> Excuse my ignorance. Reading the commit message, there was a Linux
> >> kernel change, that broke Chrome OS userspace, right? If so, and we do
> >> not know if there is other userspace using the API incorrectly,
> >> shouldn’t the patch breaking Chrome OS userspace be reverted to adhere
> >> to Linux’ no-regression rule?
> >
> > No. There was a ChromeOS bug which has been thought to be an amdgpu bug. But
> > fixing that "bug" breaks other user-space.
>
> Thank you for the explanation. I guess the bug was only surfacing
> because Chrome OS device, like Chromebooks, are only using AMD hardware
> since a short while (maybe last year).
>
> Reading your message *amdgpu: atomic API and cursor/overlay planes* [1]
> again, it says:
>
> > Up until now we were using cursor and overlay planes in gamescope [3],
> > but some changes in the amdgpu driver [1] makes us unable to use planes
>
> So this statement was incorrect? Which changes are that? Or did Chrome
> OS ever work correctly with an older Linux kernel or not?

The sequence of events is as follows:

- gamescope can use cursor and overlay planes.
- ChromeOS-specific commit lands, fixing some ChromeOS issues related to video
  playback. This breaks gamescope overlays.
- Discussion to restrict the ChromeOS-specific logic to ChromeOS, or to revert
  it, either of these fix gamescope.

Given this, I don't see how the quoted statement is incorrect? Maybe I'm
missing something?

Hope that helps,

Simon

Re: [PATCH] amd/display: remove ChromeOS workaround

[Paul Menzel]

Dear Simon,


Am 19.10.21 um 10:10 schrieb Simon Ser:
> On Tuesday, October 19th, 2021 at 01:21, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> 
>> Am 19.10.21 um 01:06 schrieb Simon Ser:
>>> On Tuesday, October 19th, 2021 at 01:03, Paul Menzel wrote:
>>>
>>>> Excuse my ignorance. Reading the commit message, there was a Linux
>>>> kernel change, that broke Chrome OS userspace, right? If so, and we do
>>>> not know if there is other userspace using the API incorrectly,
>>>> shouldn’t the patch breaking Chrome OS userspace be reverted to adhere
>>>> to Linux’ no-regression rule?
>>>
>>> No. There was a ChromeOS bug which has been thought to be an amdgpu bug. But
>>> fixing that "bug" breaks other user-space.
>>
>> Thank you for the explanation. I guess the bug was only surfacing
>> because Chrome OS device, like Chromebooks, are only using AMD hardware
>> since a short while (maybe last year).
>>
>> Reading your message *amdgpu: atomic API and cursor/overlay planes* [1]
>> again, it says:
>>
>>> Up until now we were using cursor and overlay planes in gamescope [3],
>>> but some changes in the amdgpu driver [1] makes us unable to use planes
>>
>> So this statement was incorrect? Which changes are that? Or did Chrome
>> OS ever work correctly with an older Linux kernel or not?
> 
> The sequence of events is as follows:
> 
> - gamescope can use cursor and overlay planes.
> - ChromeOS-specific commit lands, fixing some ChromeOS issues related to video
>    playback. This breaks gamescope overlays.

I guess, I am confused, which Chrome OS specific commit that is. Is it 
one of the reverted commits below? Which one?

1.  ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication
when using overlay")
2.  e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by 
considering cursors"")

> - Discussion to restrict the ChromeOS-specific logic to ChromeOS, or to revert
>    it, either of these fix gamescope.
> 
> Given this, I don't see how the quoted statement is incorrect? Maybe I'm
> missing something?

Your reply from August 2021 to commit ddab8bd788f5 (drm/amd/display: Fix 
two cursor duplication when using overlay) from April 2021 [2]:

> Hm. This patch causes a regression for me. I was using primary + overlay
> not covering the whole primary plane + cursor before. This patch breaks it.
> 
> This patch makes the overlay plane very useless for me, because the primary
> plane is always under the overlay plane.

So, I would have thought, everything worked fine before some Linux 
kernel commit changed behavior, and regressed userspace.


Kind regards,

Paul


[2]: 
https://lore.kernel.org/amd-gfx/SrcUnUUGJquVgjp9P79uV8sv6s-kMHG4wp0S3b4Nh9ksi29EIOye5edofuXkDLRvGfvkkRpQZ9JM7MNqew2B3kFUhaxsonDRXprkAYXaQUo=@emersion.fr/

Re: [PATCH] amd/display: remove ChromeOS workaround

[Simon Ser]

Hi again,

On Tuesday, October 19th, 2021 at 10:25, Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> Dear Simon,
>
>
> Am 19.10.21 um 10:10 schrieb Simon Ser:
> > On Tuesday, October 19th, 2021 at 01:21, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> >
> >> Am 19.10.21 um 01:06 schrieb Simon Ser:
> >>> On Tuesday, October 19th, 2021 at 01:03, Paul Menzel wrote:
> >>>
> >>>> Excuse my ignorance. Reading the commit message, there was a Linux
> >>>> kernel change, that broke Chrome OS userspace, right? If so, and we do
> >>>> not know if there is other userspace using the API incorrectly,
> >>>> shouldn’t the patch breaking Chrome OS userspace be reverted to adhere
> >>>> to Linux’ no-regression rule?
> >>>
> >>> No. There was a ChromeOS bug which has been thought to be an amdgpu bug. But
> >>> fixing that "bug" breaks other user-space.
> >>
> >> Thank you for the explanation. I guess the bug was only surfacing
> >> because Chrome OS device, like Chromebooks, are only using AMD hardware
> >> since a short while (maybe last year).
> >>
> >> Reading your message *amdgpu: atomic API and cursor/overlay planes* [1]
> >> again, it says:
> >>
> >>> Up until now we were using cursor and overlay planes in gamescope [3],
> >>> but some changes in the amdgpu driver [1] makes us unable to use planes
> >>
> >> So this statement was incorrect? Which changes are that? Or did Chrome
> >> OS ever work correctly with an older Linux kernel or not?
> >
> > The sequence of events is as follows:
> >
> > - gamescope can use cursor and overlay planes.
> > - ChromeOS-specific commit lands, fixing some ChromeOS issues related to video
> >    playback. This breaks gamescope overlays.
>
> I guess, I am confused, which Chrome OS specific commit that is. Is it
> one of the reverted commits below? Which one?
>
> 1.  ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication
> when using overlay")
> 2.  e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by
> considering cursors"")

ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication when using overlay")
is the commit which introduced the validate_overlay logic fixing ChromeOS and
breaking gamescope.

Later, 33f409e60eb0 ("drm/amd/display: Fix overlay validation by considering
cursors") relaxed validate_overlay. This breaks ChromeOS and partially fixes
gamescope (when the overlay is used and the cursor plane is unused).

Finally, e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by
considering cursors"") has reverted that change, fixing ChromeOS (again) and
breaking gamescope completely again.

> > - Discussion to restrict the ChromeOS-specific logic to ChromeOS, or to revert
> >    it, either of these fix gamescope.
> >
> > Given this, I don't see how the quoted statement is incorrect? Maybe I'm
> > missing something?
>
> Your reply from August 2021 to commit ddab8bd788f5 (drm/amd/display: Fix
> two cursor duplication when using overlay) from April 2021 [2]:
>
> > Hm. This patch causes a regression for me. I was using primary + overlay
> > not covering the whole primary plane + cursor before. This patch breaks it.
> >
> > This patch makes the overlay plane very useless for me, because the primary
> > plane is always under the overlay plane.
>
> So, I would have thought, everything worked fine before some Linux
> kernel commit changed behavior, and regressed userspace.

I've tried to explain the full story above. My user-space went from working to
broken to partially broken to broken. The quoted reply is a complaint that the
commit flipped gamescope from partially broken to completely broken. At the
time I didn't realize that ddab8bd788f5 caused some pain too.

Does that clear things up?

Re: [PATCH] amd/display: remove ChromeOS workaround

[Paul Menzel]

Dear Simon,


Am 21.10.21 um 18:08 schrieb Simon Ser:

> On Tuesday, October 19th, 2021 at 10:25, Paul Menzel wrote:

>> Am 19.10.21 um 10:10 schrieb Simon Ser:
>>> On Tuesday, October 19th, 2021 at 01:21, Paul Menzel wrote:
>>>
>>>> Am 19.10.21 um 01:06 schrieb Simon Ser:
>>>>> On Tuesday, October 19th, 2021 at 01:03, Paul Menzel wrote:
>>>>>
>>>>>> Excuse my ignorance. Reading the commit message, there was a Linux
>>>>>> kernel change, that broke Chrome OS userspace, right? If so, and we do
>>>>>> not know if there is other userspace using the API incorrectly,
>>>>>> shouldn’t the patch breaking Chrome OS userspace be reverted to adhere
>>>>>> to Linux’ no-regression rule?
>>>>>
>>>>> No. There was a ChromeOS bug which has been thought to be an amdgpu bug. But
>>>>> fixing that "bug" breaks other user-space.
>>>>
>>>> Thank you for the explanation. I guess the bug was only surfacing
>>>> because Chrome OS device, like Chromebooks, are only using AMD hardware
>>>> since a short while (maybe last year).
>>>>
>>>> Reading your message *amdgpu: atomic API and cursor/overlay planes* [1]
>>>> again, it says:
>>>>
>>>>> Up until now we were using cursor and overlay planes in gamescope [3],
>>>>> but some changes in the amdgpu driver [1] makes us unable to use planes
>>>>
>>>> So this statement was incorrect? Which changes are that? Or did Chrome
>>>> OS ever work correctly with an older Linux kernel or not?
>>>
>>> The sequence of events is as follows:
>>>
>>> - gamescope can use cursor and overlay planes.
>>> - ChromeOS-specific commit lands, fixing some ChromeOS issues related to video
>>>     playback. This breaks gamescope overlays.
>>
>> I guess, I am confused, which Chrome OS specific commit that is. Is it
>> one of the reverted commits below? Which one?
>>
>> 1.  ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication
>> when using overlay")
>> 2.  e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by
>> considering cursors"")
> 
> ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication when using overlay")
> is the commit which introduced the validate_overlay logic fixing ChromeOS and
> breaking gamescope.

Thank you for elaborating on this. I guess I mixed up Chrome OS and 
gamescope, and was especially confused, the commit message of commit 
ddab8bd788f5 not explicitly listing the problematic userspace. Despite 
the commit message being well written, this crucial information is missing.

> Later, 33f409e60eb0 ("drm/amd/display: Fix overlay validation by considering
> cursors") relaxed validate_overlay. This breaks ChromeOS and partially fixes
> gamescope (when the overlay is used and the cursor plane is unused).
> 
> Finally, e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by
> considering cursors"") has reverted that change, fixing ChromeOS (again) and
> breaking gamescope completely again.
> 
>>> - Discussion to restrict the ChromeOS-specific logic to ChromeOS, or to revert
>>>     it, either of these fix gamescope.
>>>
>>> Given this, I don't see how the quoted statement is incorrect? Maybe I'm
>>> missing something?
>>
>> Your reply from August 2021 to commit ddab8bd788f5 (drm/amd/display: Fix
>> two cursor duplication when using overlay) from April 2021 [2]:
>>
>>> Hm. This patch causes a regression for me. I was using primary + overlay
>>> not covering the whole primary plane + cursor before. This patch breaks it.
>>>
>>> This patch makes the overlay plane very useless for me, because the primary
>>> plane is always under the overlay plane.
>>
>> So, I would have thought, everything worked fine before some Linux
>> kernel commit changed behavior, and regressed userspace.
> 
> I've tried to explain the full story above. My user-space went from working to
> broken to partially broken to broken. The quoted reply is a complaint that the
> commit flipped gamescope from partially broken to completely broken. At the
> time I didn't realize that ddab8bd788f5 caused some pain too.
> 
> Does that clear things up?

Yes, it does. Thank you very much for taking the time for walking me 
through this.


Kind regards,

Paul

Re: [PATCH] amd/display: remove ChromeOS workaround

[Simon Ser]

On Thursday, October 14th, 2021 at 17:35, Simon Ser <contact@emersion.fr> wrote:

> This reverts commits ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication
> when using overlay") and e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay
> validation by considering cursors"").
>
> tl;dr ChromeOS uses the atomic interface for everything except the cursor. This
> is incorrect and forces amdgpu to disable some hardware features. Let's revert
> the ChromeOS-specific workaround in mainline and allow the Chrome team to keep
> it internally in their own tree.
>
> See [1] for more details. This patch is an alternative to [2], which added
> ChromeOS detection.
>
> [1]: https://lore.kernel.org/amd-gfx/JIQ_93_cHcshiIDsrMU1huBzx9P9LVQxucx8hQArpQu7Wk5DrCl_vTXj_Q20m_L-8C8A5dSpNcSJ8ehfcCrsQpfB5QG_Spn14EYkH9chtg0=@emersion.fr/
> [2]: https://lore.kernel.org/amd-gfx/20211011151609.452132-1-contact@emersion.fr/

Alex, are you okay with moving forward with this patch, or do you prefer the
other approach?

Re: [PATCH] amd/display: remove ChromeOS workaround

[Rodrigo Siqueira Jordao]

Hi Simon,

I tested this patch and it lgtm. I also agree to revert it.

Btw, did you send the revert patch for "amd/display: only require 
overlay plane to cover whole CRTC on ChromeOS"? I think we need to 
revert it as well.

Sean, Mark

For ChromeOS, we should ignore this patch. Do we need to take any action 
to avoid landing this patch on ChromeOS tree?

Thanks
Siqueira

On 2021-10-14 11:35 a.m., Simon Ser wrote:
> This reverts commits ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication
> when using overlay") and e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay
> validation by considering cursors"").
> 
> tl;dr ChromeOS uses the atomic interface for everything except the cursor. This
> is incorrect and forces amdgpu to disable some hardware features. Let's revert
> the ChromeOS-specific workaround in mainline and allow the Chrome team to keep
> it internally in their own tree.
> 
> See [1] for more details. This patch is an alternative to [2], which added
> ChromeOS detection.
> 
> [1]: https://lore.kernel.org/amd-gfx/JIQ_93_cHcshiIDsrMU1huBzx9P9LVQxucx8hQArpQu7Wk5DrCl_vTXj_Q20m_L-8C8A5dSpNcSJ8ehfcCrsQpfB5QG_Spn14EYkH9chtg0=@emersion.fr/>> [2]: https://lore.kernel.org/amd-gfx/20211011151609.452132-1-contact@emersion.fr/>> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Fixes: ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication when using overlay")
> Fixes: e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by considering cursors"")
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 -------------------
>   1 file changed, 51 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 20065a145851..014c5a9fe461 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10628,53 +10628,6 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm
>   }
>   #endif
>   
> -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, *overlay_state = NULL;
> -
> -	/* Check if primary plane is contained inside overlay */
> -	for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i) {
> -		if (plane->type == DRM_PLANE_TYPE_OVERLAY) {
> -			if (drm_atomic_plane_disabling(plane->state, new_plane_state))
> -				return 0;
> -
> -			overlay_state = new_plane_state;
> -			continue;
> -		}
> -	}
> -
> -	/* check if we're making changes to the overlay plane */
> -	if (!overlay_state)
> -		return 0;
> -
> -	/* check if overlay plane is enabled */
> -	if (!overlay_state->crtc)
> -		return 0;
> -
> -	/* find the primary plane for the CRTC that the overlay is enabled on */
> -	primary_state = drm_atomic_get_plane_state(state, overlay_state->crtc->primary);
> -	if (IS_ERR(primary_state))
> -		return PTR_ERR(primary_state);
> -
> -	/* check if primary plane is enabled */
> -	if (!primary_state->crtc)
> -		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 ||
> -	    primary_state->crtc_x + primary_state->crtc_w > overlay_state->crtc_x + overlay_state->crtc_w ||
> -	    primary_state->crtc_y + primary_state->crtc_h > overlay_state->crtc_y + overlay_state->crtc_h) {
> -		DRM_DEBUG_ATOMIC("Overlay plane is enabled with hardware cursor but does not fully cover primary plane\n");
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
>   /**
>    * amdgpu_dm_atomic_check() - Atomic check implementation for AMDgpu DM.
>    * @dev: The DRM device
> @@ -10856,10 +10809,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   			goto fail;
>   	}
>   
> -	ret = validate_overlay(state);
> -	if (ret)
> -		goto fail;
> -
>   	/* Add new/modified planes */
>   	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
>   		ret = dm_update_plane_state(dc, state, plane,
> 

Re: [PATCH] amd/display: remove ChromeOS workaround

[Harry Wentland]

On 2021-10-21 13:55, Rodrigo Siqueira Jordao wrote:
> Hi Simon,
> 
> I tested this patch and it lgtm. I also agree to revert it.
> 
> Btw, did you send the revert patch for "amd/display: only require overlay plane to cover whole CRTC on ChromeOS"? I think we need to revert it as well.
> 

Agreed that this patch is good but we'll need to also revert the is_chromeos w/a.

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

Harry

> Sean, Mark
> 
> For ChromeOS, we should ignore this patch. Do we need to take any action to avoid landing this patch on ChromeOS tree?
> 
> Thanks
> Siqueira
> 
> On 2021-10-14 11:35 a.m., Simon Ser wrote:
>> This reverts commits ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication
>> when using overlay") and e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay
>> validation by considering cursors"").
>>
>> tl;dr ChromeOS uses the atomic interface for everything except the cursor. This
>> is incorrect and forces amdgpu to disable some hardware features. Let's revert
>> the ChromeOS-specific workaround in mainline and allow the Chrome team to keep
>> it internally in their own tree.
>>
>> See [1] for more details. This patch is an alternative to [2], which added
>> ChromeOS detection.
>>
>> [1]: https://lore.kernel.org/amd-gfx/JIQ_93_cHcshiIDsrMU1huBzx9P9LVQxucx8hQArpQu7Wk5DrCl_vTXj_Q20m_L-8C8A5dSpNcSJ8ehfcCrsQpfB5QG_Spn14EYkH9chtg0=@emersion.fr/>> [2]: https://lore.kernel.org/amd-gfx/20211011151609.452132-1-contact@emersion.fr/>> Signed-off-by: Simon Ser <contact@emersion.fr>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Harry Wentland <hwentlan@amd.com>
>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
>> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Fixes: ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication when using overlay")
>> Fixes: e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by considering cursors"")
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 -------------------
>>   1 file changed, 51 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 20065a145851..014c5a9fe461 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -10628,53 +10628,6 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm
>>   }
>>   #endif
>>   -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, *overlay_state = NULL;
>> -
>> -    /* Check if primary plane is contained inside overlay */
>> -    for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i) {
>> -        if (plane->type == DRM_PLANE_TYPE_OVERLAY) {
>> -            if (drm_atomic_plane_disabling(plane->state, new_plane_state))
>> -                return 0;
>> -
>> -            overlay_state = new_plane_state;
>> -            continue;
>> -        }
>> -    }
>> -
>> -    /* check if we're making changes to the overlay plane */
>> -    if (!overlay_state)
>> -        return 0;
>> -
>> -    /* check if overlay plane is enabled */
>> -    if (!overlay_state->crtc)
>> -        return 0;
>> -
>> -    /* find the primary plane for the CRTC that the overlay is enabled on */
>> -    primary_state = drm_atomic_get_plane_state(state, overlay_state->crtc->primary);
>> -    if (IS_ERR(primary_state))
>> -        return PTR_ERR(primary_state);
>> -
>> -    /* check if primary plane is enabled */
>> -    if (!primary_state->crtc)
>> -        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 ||
>> -        primary_state->crtc_x + primary_state->crtc_w > overlay_state->crtc_x + overlay_state->crtc_w ||
>> -        primary_state->crtc_y + primary_state->crtc_h > overlay_state->crtc_y + overlay_state->crtc_h) {
>> -        DRM_DEBUG_ATOMIC("Overlay plane is enabled with hardware cursor but does not fully cover primary plane\n");
>> -        return -EINVAL;
>> -    }
>> -
>> -    return 0;
>> -}
>> -
>>   /**
>>    * amdgpu_dm_atomic_check() - Atomic check implementation for AMDgpu DM.
>>    * @dev: The DRM device
>> @@ -10856,10 +10809,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>               goto fail;
>>       }
>>   -    ret = validate_overlay(state);
>> -    if (ret)
>> -        goto fail;
>> -
>>       /* Add new/modified planes */
>>       for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
>>           ret = dm_update_plane_state(dc, state, plane,
>>
> 

Re: [PATCH] amd/display: remove ChromeOS workaround

[Alex Deucher]

On Thu, Oct 21, 2021 at 2:19 PM Harry Wentland <harry.wentland@amd.com> wrote:
>
>
>
> On 2021-10-21 13:55, Rodrigo Siqueira Jordao wrote:
> > Hi Simon,
> >
> > I tested this patch and it lgtm. I also agree to revert it.
> >
> > Btw, did you send the revert patch for "amd/display: only require overlay plane to cover whole CRTC on ChromeOS"? I think we need to revert it as well.
> >
>
> Agreed that this patch is good but we'll need to also revert the is_chromeos w/a.

I've reverted that and applied this one.  Thanks!

Alex

>
> This patch is
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>
> Harry
>
> > Sean, Mark
> >
> > For ChromeOS, we should ignore this patch. Do we need to take any action to avoid landing this patch on ChromeOS tree?
> >
> > Thanks
> > Siqueira
> >
> > On 2021-10-14 11:35 a.m., Simon Ser wrote:
> >> This reverts commits ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication
> >> when using overlay") and e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay
> >> validation by considering cursors"").
> >>
> >> tl;dr ChromeOS uses the atomic interface for everything except the cursor. This
> >> is incorrect and forces amdgpu to disable some hardware features. Let's revert
> >> the ChromeOS-specific workaround in mainline and allow the Chrome team to keep
> >> it internally in their own tree.
> >>
> >> See [1] for more details. This patch is an alternative to [2], which added
> >> ChromeOS detection.
> >>
> >> [1]: https://lore.kernel.org/amd-gfx/JIQ_93_cHcshiIDsrMU1huBzx9P9LVQxucx8hQArpQu7Wk5DrCl_vTXj_Q20m_L-8C8A5dSpNcSJ8ehfcCrsQpfB5QG_Spn14EYkH9chtg0=@emersion.fr/>> [2]: https://lore.kernel.org/amd-gfx/20211011151609.452132-1-contact@emersion.fr/>> Signed-off-by: Simon Ser <contact@emersion.fr>
> >> Cc: Alex Deucher <alexander.deucher@amd.com>
> >> Cc: Harry Wentland <hwentlan@amd.com>
> >> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> >> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> >> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> >> Cc: Sean Paul <seanpaul@chromium.org>
> >> Fixes: ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication when using overlay")
> >> Fixes: e7d9560aeae5 ("Revert "drm/amd/display: Fix overlay validation by considering cursors"")
> >> ---
> >>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 -------------------
> >>   1 file changed, 51 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 20065a145851..014c5a9fe461 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -10628,53 +10628,6 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm
> >>   }
> >>   #endif
> >>   -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, *overlay_state = NULL;
> >> -
> >> -    /* Check if primary plane is contained inside overlay */
> >> -    for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i) {
> >> -        if (plane->type == DRM_PLANE_TYPE_OVERLAY) {
> >> -            if (drm_atomic_plane_disabling(plane->state, new_plane_state))
> >> -                return 0;
> >> -
> >> -            overlay_state = new_plane_state;
> >> -            continue;
> >> -        }
> >> -    }
> >> -
> >> -    /* check if we're making changes to the overlay plane */
> >> -    if (!overlay_state)
> >> -        return 0;
> >> -
> >> -    /* check if overlay plane is enabled */
> >> -    if (!overlay_state->crtc)
> >> -        return 0;
> >> -
> >> -    /* find the primary plane for the CRTC that the overlay is enabled on */
> >> -    primary_state = drm_atomic_get_plane_state(state, overlay_state->crtc->primary);
> >> -    if (IS_ERR(primary_state))
> >> -        return PTR_ERR(primary_state);
> >> -
> >> -    /* check if primary plane is enabled */
> >> -    if (!primary_state->crtc)
> >> -        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 ||
> >> -        primary_state->crtc_x + primary_state->crtc_w > overlay_state->crtc_x + overlay_state->crtc_w ||
> >> -        primary_state->crtc_y + primary_state->crtc_h > overlay_state->crtc_y + overlay_state->crtc_h) {
> >> -        DRM_DEBUG_ATOMIC("Overlay plane is enabled with hardware cursor but does not fully cover primary plane\n");
> >> -        return -EINVAL;
> >> -    }
> >> -
> >> -    return 0;
> >> -}
> >> -
> >>   /**
> >>    * amdgpu_dm_atomic_check() - Atomic check implementation for AMDgpu DM.
> >>    * @dev: The DRM device
> >> @@ -10856,10 +10809,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> >>               goto fail;
> >>       }
> >>   -    ret = validate_overlay(state);
> >> -    if (ret)
> >> -        goto fail;
> >> -
> >>       /* Add new/modified planes */
> >>       for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> >>           ret = dm_update_plane_state(dc, state, plane,
> >>
> >
>

Re: [PATCH] amd/display: remove ChromeOS workaround

[Simon Ser]

On Friday, October 22nd, 2021 at 15:58, Alex Deucher <alexdeucher@gmail.com> wrote:

> > Agreed that this patch is good but we'll need to also revert the is_chromeos w/a.
>
> I've reverted that and applied this one. Thanks!

Ah, didn't realize I needed to revert that one too. Thank you!

Re: [PATCH] amd/display: remove ChromeOS workaround

[Alex Deucher]

On Fri, Oct 22, 2021 at 11:02 AM Simon Ser <contact@emersion.fr> wrote:
>
> On Friday, October 22nd, 2021 at 15:58, Alex Deucher <alexdeucher@gmail.com> wrote:
>
> > > Agreed that this patch is good but we'll need to also revert the is_chromeos w/a.
> >
> > I've reverted that and applied this one. Thanks!
>
> Ah, didn't realize I needed to revert that one too. Thank you!

Well it was only applied to our amd-staging-drm-next branch, not to upstream.

Alex