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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dear Simon,


Am 12.10.21 um 11:15 schrieb Simon Ser:
On Tuesday, October 12th, 2021 at 11:08, Paul Menzel <pmenzel@xxxxxxxxxxxxx> 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@xxxxxxxxxxx>
Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Cc: Harry Wentland <hwentlan@xxxxxxx>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
Cc: Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx>
Cc: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>
Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
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



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux