Hi, On 3/20/22 21:11, Rajat Jain wrote: > () Hello Hans, Sean, > > > > On Fri, Mar 11, 2022 at 4:12 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> >> 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) . > > 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: Ok, I've just send out a patch for this including the changes for the Chromebook entry in drivers/gpu/drm/drm_privacy_screen_x86.c . Note I've modified the changes to drivers/gpu/drm/i915/display/intel_display.c intel_modeset_probe_defer() a bit to walk over an array of internal-panel connector-names, to make it a bit more future proof. I expect / hope this new version to be better liked by the i915 maintainers. Regards, Hans > =================================================== > 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 >>>> >>> >
