Hi All,
On 3/9/22 18:53, Rajat Jain wrote:
> On Wed, Mar 9, 2022 at 7:06 AM Sean Paul <sean@xxxxxxxxxx> wrote:
>>
>> From: Sean Paul <seanpaul@xxxxxxxxxxxx>
>>
>> This patch adds the necessary hooks to make amdgpu aware of privacy
>> screens. On devices with privacy screen drivers (such as thinkpad-acpi),
>> the amdgpu driver will defer probe until it's ready and then sync the sw
>> and hw state on each commit the connector is involved and enabled.
>>
>> Changes in v2:
>> -Tweaked the drm_privacy_screen_get() error check to avoid logging
>> errors when privacy screen is absent (Hans)
>>
>> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
>> Link: https://patchwork.freedesktop.org/patch/477640/ #v1
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 +++++++++
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++
>> .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 16 +++++++++++++++-
>> 3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 2ab675123ae3..e2cfae56c020 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -26,6 +26,7 @@
>> #include <drm/drm_aperture.h>
>> #include <drm/drm_drv.h>
>> #include <drm/drm_gem.h>
>> +#include <drm/drm_privacy_screen_consumer.h>
>> #include <drm/drm_vblank.h>
>> #include <drm/drm_managed.h>
>> #include "amdgpu_drv.h"
>> @@ -1988,6 +1989,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>> {
>> struct drm_device *ddev;
>> struct amdgpu_device *adev;
>> + struct drm_privacy_screen *privacy_screen;
>> unsigned long flags = ent->driver_data;
>> int ret, retry = 0, i;
>> bool supports_atomic = false;
>> @@ -2063,6 +2065,13 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>> size = pci_resource_len(pdev, 0);
>> is_fw_fb = amdgpu_is_fw_framebuffer(base, size);
>>
>> + /* If the LCD panel has a privacy screen, defer probe until its ready */
>> + privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL);
>> + if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> +
>> + drm_privacy_screen_put(privacy_screen);
>> +
>> /* Get rid of things like offb */
>> ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &amdgpu_kms_driver);
>> if (ret)
>> 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 e1d3db3fe8de..9e2bb6523add 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -9781,6 +9781,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>> if (acrtc) {
>> new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
>> old_crtc_state = drm_atomic_get_old_crtc_state(state, &acrtc->base);
>> +
>> + /* Sync the privacy screen state between hw and sw */
>> + drm_connector_update_privacy_screen(new_con_state);
>> }
>>
>> /* Skip any modesets/resets */
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> index 740435ae3997..594a8002975a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> @@ -27,6 +27,7 @@
>> #include <drm/drm_atomic_helper.h>
>> #include <drm/dp/drm_dp_mst_helper.h>
>> #include <drm/dp/drm_dp_helper.h>
>> +#include <drm/drm_privacy_screen_consumer.h>
>> #include "dm_services.h"
>> #include "amdgpu.h"
>> #include "amdgpu_dm.h"
>> @@ -506,6 +507,7 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
>> struct amdgpu_dm_connector *aconnector,
>> int link_index)
>> {
>> + struct drm_device *dev = dm->ddev;
>> struct dc_link_settings max_link_enc_cap = {0};
>>
>> aconnector->dm_dp_aux.aux.name =
>> @@ -519,8 +521,20 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
>> drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
>> &aconnector->base);
>>
>> - if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
>> + if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP) {
>> + struct drm_privacy_screen *privacy_screen)
>> +
>> + /* Reference given up in drm_connector_cleanup() */
>> + privacy_screen = drm_privacy_screen_get(dev->dev, NULL);
>
> Can we try to be more specific when looking up for privacy screen, e.g.:
>
> privacy_screen = drm_privacy_screen_get(dev->dev, "eDP-1");
> (and then also making the corresponding change in arch_init_data[] in
> drm_privacy_screen_x86.c"
So I just checked and yes I think we can be more specific at least
for the thinkpad_acpi supported models. See the attached patch
which has been tested on a ThinkPad T14 gen 1 with a builtin privacy-screen.
Rajat, can you adjust the chrome code in drivers/gpu/drm/drm_privacy_screen_x86.c
to match and check that with the chrome code changes, my patch does not break
things on chromebooks? (Note your changes will need to be squashed into the
patch so that we change all of this in one go) .
Sean, same request to you, can you adjust your amdgpu patch to match
the i915 changes in the attached patch and then check if a kernel
with both changes still works ?
Regards,
Hans
>
> My comment applies to this driver as well as to i915. The reason being
> today there is only 1 internal display with privacy screen so we can
> just assume that there shall be only 1 privacy-screen and that shall
> apply to eDP-1/internal display. But dual display systems are not far
> enough out, and perhaps external monitors with inbuilt
> privacy-screens?
>
> Essentially the gap today is that today on Chromeos ACPI side, the
> device GOOG0010 represents the privacy-screen attached to the internal
> display/eDP-1, but there isn't a way to make it clear in the i915 and
> now amdgpu code.
>
> Thanks,
>
> Rajat
>
>
>> + if (!IS_ERR(privacy_screen)) {
>> + drm_connector_attach_privacy_screen_provider(&aconnector->base,
>> + privacy_screen);
>> + } else if (PTR_ERR(privacy_screen) != -ENODEV) {
>> + drm_err(dev, "Error getting privacy screen, ret=%d\n",
>> + PTR_ERR(privacy_screen));
>> + }
>> return;
>> + }
>>
>> dc_link_dp_get_max_link_enc_cap(aconnector->dc_link, &max_link_enc_cap);
>> aconnector->mst_mgr.cbs = &dm_mst_cbs;
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS
>>
> From 04320f93075eeb13a305ec703cc9e591ce334749 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Fri, 11 Mar 2022 12:28:47 +0100
Subject: [PATCH] drm/privacy-screen: Hardcode
ATM the drm privacy-screen code is not using connector names to lookup
drm privacy-screen providers, drm_privacy_screen_get() does support this,
but before this change the con_id is set to NULL everywhere which is
treated as a wildcard.
There are some worries that we may see devices with 2 displays with
a privacy-screen, be it 2 internal displays or 1 internal + 1 external.
All laptop-models which currently are supported by the drm_privacy_screen
code use an eDP display connected to eDP-1.
This commits enables the use of the con_id parameter, hardcoding this to
"eDP-1" in the lookup tables in drivers/gpu/drm/drm_privacy_screen_x86.c
and adjusting the i915 driver to match.
Using the con_id parameter paves the way for potentially having another
display (attached to a different connector) which also has a builtin
privacy-screen.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/gpu/drm/drm_privacy_screen_x86.c | 2 +-
drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
drivers/gpu/drm/i915/display/intel_display.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
index e7aa74ad0b24..06ea5c5346b7 100644
--- a/drivers/gpu/drm/drm_privacy_screen_x86.c
+++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
@@ -55,7 +55,7 @@ static const struct arch_init_data arch_init_data[] __initconst = {
{
.lookup = {
.dev_id = NULL,
- .con_id = NULL,
+ .con_id = "eDP-1",
.provider = "privacy_screen-thinkpad_acpi",
},
.detect = detect_thinkpad_privacy_screen,
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index cab505277595..e1930ab6ff11 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3951,7 +3951,7 @@ intel_ddi_init_dp_connector(struct intel_digital_port *dig_port)
struct drm_device *dev = dig_port->base.base.dev;
struct drm_privacy_screen *privacy_screen;
- privacy_screen = drm_privacy_screen_get(dev->dev, NULL);
+ privacy_screen = drm_privacy_screen_get(dev->dev, connector->base.name);
if (!IS_ERR(privacy_screen)) {
drm_connector_attach_privacy_screen_provider(&connector->base,
privacy_screen);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index fb455d3710c6..a5e12d36d2e3 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10835,7 +10835,7 @@ bool intel_modeset_probe_defer(struct pci_dev *pdev)
return true;
/* If the LCD panel has a privacy-screen, wait for it */
- privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL);
+ privacy_screen = drm_privacy_screen_get(&pdev->dev, "eDP-1");
if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER)
return true;
--
2.35.1
