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

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

 



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



[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