On Wed, Mar 9, 2022 at 12:54 PM Rajat Jain <rajatja@xxxxxxxxxx> 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" > > 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. I was wondering the same thing. There are devices out there today that have multiple eDP panels already. I don't know that there are any platforms today with privacy screens and multiple panels, but as you say, I suppose it is only a matter of time. Additionally what if you have several eDP panels and only one has a privacy screen? How do you map to the appropriate display? Alex > > 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 > >
