Re: [PATCH v2] drm/amdgpu: Add support for drm_privacy_screen

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

 



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
>>>>
>>>
> 




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux