[PATCH v2] drm/amdgpu: Add support for drm_privacy_screen
Rajat Jain
rajatja at google.com
Sun Mar 20 20:11:24 UTC 2022
() Hello Hans, Sean,
On Fri, Mar 11, 2022 at 4:12 AM Hans de Goede <hdegoede at redhat.com> wrote:
>
> Hi All,
>
> On 3/9/22 18:53, Rajat Jain wrote:
> > On Wed, Mar 9, 2022 at 7:06 AM Sean Paul <sean at poorly.run> wrote:
> >>
> >> From: Sean Paul <seanpaul at chromium.org>
> >>
> >> 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 at chromium.org>
> >> 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) .
Thanks, I just confirmed that with a change similar to yours (use
"eDP-1"), it works fine on the Intel chromebooks at my end, so feel
free to do it:
===================================================
diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c
b/drivers/gpu/drm/drm_privacy_screen_x86.c
index 88802cd7a1ee..894beefb6628 100644
--- a/drivers/gpu/drm/drm_privacy_screen_x86.c
+++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
@@ -69,7 +69,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-GOOG0010:00",
},
.detect = detect_chromeos_privacy_screen,
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
b/drivers/gpu/drm/i915/display/intel_ddi.c
index 1682ace5cd53..2666ba7b5a28 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -4250,7 +4250,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 89be498127e4..b2903a55f910 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13360,7 +13360,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;
=================================================
I found it a little surprising though. From what I remembered from my
early venture, was that connector->base.name did not get filled in at
the time intel_ddi_init_dp_connector() was called, but I guess I was
remembering it wrong.
>
> 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 ?
Defer to Sean since I do not have the AMD chromebook to test.
Thanks & Best Regards,
Rajat
>
> 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
> >>
> >
More information about the dri-devel
mailing list