[PATCH v5] amd/display: only require overlay plane to cover whole CRTC on ChromeOS

[Paul Menzel]

Dear Simon,


Am 12.10.21 um 11:15 schrieb Simon Ser:
> On Tuesday, October 12th, 2021 at 11:08, Paul Menzel <pmenzel at molgen.mpg.de> wrote:
> 
>>> the cursor plane (which uses the legacy API). Thus amdgpu must always
>>> be prepared to enable/disable/move the cursor plane at any time without
>>> failing (or else ChromeOS will trip over).
>>
>> What ChromeOS version did you test with? Are there plans to improve
>> ChromeOS?
> 
> No idea, I haven't received feedback from the ChromeOS folks.
> 
>>> As discussed in [1], there's no reason why the ChromeOS limitation
>>> should prevent other fully atomic users from taking advantage of the
>>> overlay plane. Let's limit the check to ChromeOS.
>>
>> How do we know, no other userspace programs are affected, breaking
>> Linux’ no-regression in userspace policy?
> 
> Actually this is the other way around: the ChromeOS fix which landed
> has broken my user-space. This patch tries to fix the situation for
> both ChromeOS and gamescope.

Thank you.

> That said, it seems like amdgpu maintainers are open to just revert the
> ChromeOS fix, thus fixing gamescope. ChromeOS can carry the fix in their
> kernel tree. More on that soon.
> 
>>> v4: fix ChromeOS detection (Harry)
>>>
>>> v5: fix conflict with linux-next
>>>
>>> [1]: https://lore.kernel.org/amd-gfx/JIQ_93_cHcshiIDsrMU1huBzx9P9LVQxucx8hQArpQu7Wk5DrCl_vTXj_Q20m_L-8C8A5dSpNcSJ8ehfcCrsQpfB5QG_Spn14EYkH9chtg0=@emersion.fr/
>>>
>>> Signed-off-by: Simon Ser <contact at emersion.fr>
>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>> Cc: Harry Wentland <hwentlan at amd.com>
>>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>>> Cc: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
>>> Cc: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
>>> Cc: Sean Paul <seanpaul at chromium.org>
>>> Fixes: ddab8bd788f5 ("drm/amd/display: Fix two cursor duplication when using overlay")
>>> ---
>>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 29 +++++++++++++++++++
>>>    1 file changed, 29 insertions(+)
>>>
>>> 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 f35561b5a465..2eeda1fec506 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -10594,6 +10594,31 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm
>>>    }
>>>    #endif
>>>
>>> +static bool is_chromeos(void)
>>> +{
>>> +	struct mm_struct *mm = current->mm;
>>> +	struct file *exe_file;
>>> +	bool ret;
>>> +
>>> +	/* ChromeOS renames its thread to DrmThread. Also check the executable
>>> +	 * name. */
>>> +	if (strcmp(current->comm, "DrmThread") != 0 || !mm)
>>> +		return false;
>>> +
>>> +	rcu_read_lock();
>>> +	exe_file = rcu_dereference(mm->exe_file);
>>> +	if (exe_file && !get_file_rcu(exe_file))
>>> +		exe_file = NULL;
>>> +	rcu_read_unlock();
>>> +
>>> +	if (!exe_file)
>>> +		return false;
>>> +	ret = strcmp(exe_file->f_path.dentry->d_name.name, "chrome") == 0;
>>> +	fput(exe_file);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>    static int validate_overlay(struct drm_atomic_state *state)
>>>    {
>>>    	int i;
>>> @@ -10601,6 +10626,10 @@ static int validate_overlay(struct drm_atomic_state *state)
>>>    	struct drm_plane_state *new_plane_state;
>>>    	struct drm_plane_state *primary_state, *overlay_state = NULL;
>>>
>>> +	/* This is a workaround for ChromeOS only */
>>> +	if (!is_chromeos())
>>> +		return 0;
>>
>> I would have expected the check to be the other way around, as no the
>> behavior on non-Chrome OS is changed?
> 
> This function performs a check which is only necessary on ChromeOS. On
> non-ChromeOS, this function prevents user-space from using some hardware
> features. The early return ensures non-ChromeOS user-space can use these
> features.

Thank you for the explanation. Then I misunderstood commit ddab8bd7 
(drm/amd/display: Fix two cursor duplication when using overlay) from 
the Fixes tag, as commit ddab8bd7 does not mention Chrome OS, and also 
does not carry a fixes tag.

With that background, I guess the workaround it fine.


Kind regards,

Paul