| Message ID | 20220309150606.1877288-1-sean@poorly.run (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/amdgpu: Add support for drm_privacy_screen | expand |
Hi, On 3/9/22 16:06, Sean Paul wrote: > From: Sean Paul <seanpaul@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@chromium.org> > Link: https://patchwork.freedesktop.org/patch/477640/ #v1 Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > 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); > + 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;
On Wed, Mar 9, 2022 at 7:06 AM Sean Paul <sean@poorly.run> wrote: > > From: Sean Paul <seanpaul@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@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" 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 >
On Wed, Mar 9, 2022 at 12:54 PM Rajat Jain <rajatja@google.com> wrote: > > On Wed, Mar 9, 2022 at 7:06 AM Sean Paul <sean@poorly.run> wrote: > > > > From: Sean Paul <seanpaul@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@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" > > 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 > >
Hi Sean,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-intel/for-linux-next drm-tip/drm-tip drm-exynos/exynos-drm-next next-20220309]
[cannot apply to tegra-drm/drm/tegra/for-next airlied/drm-next v5.17-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sean-Paul/drm-amdgpu-Add-support-for-drm_privacy_screen/20220309-230712
base: git://anongit.freedesktop.org/drm/drm drm-next
config: arc-randconfig-r043-20220309 (https://download.01.org/0day-ci/archive/20220310/202203100441.aipzrKHU-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6fedd7f8ea75c68634df4fa3cbacb0ee5f2fc984
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sean-Paul/drm-amdgpu-Add-support-for-drm_privacy_screen/20220309-230712
git checkout 6fedd7f8ea75c68634df4fa3cbacb0ee5f2fc984
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/gpu/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_mst_types.c:44:
drivers/gpu/drm/amd/amdgpu/../display/dmub/inc/dmub_cmd.h: In function 'dmub_rb_flush_pending':
drivers/gpu/drm/amd/amdgpu/../display/dmub/inc/dmub_cmd.h:2961:26: warning: variable 'temp' set but not used [-Wunused-but-set-variable]
2961 | uint64_t temp;
| ^~~~
In file included from include/linux/device.h:15,
from include/linux/acpi.h:15,
from include/linux/i2c.h:13,
from include/drm/drm_crtc.h:28,
from include/drm/drm_atomic.h:31,
from drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_mst_types.c:26:
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_mst_types.c: In function 'amdgpu_dm_initialize_dp_connector':
>> drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu.h:41:22: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
41 | #define dev_fmt(fmt) "amdgpu: " fmt
| ^~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
include/drm/drm_print.h:425:9: note: in expansion of macro 'dev_err'
425 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
| ^~~~
include/drm/drm_print.h:438:9: note: in expansion of macro '__drm_printk'
438 | __drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_mst_types.c:533:25: note: in expansion of macro 'drm_err'
533 | drm_err(dev, "Error getting privacy screen, ret=%d\n",
| ^~~~~~~
In file included from drivers/gpu/drm/amd/amdgpu/../display/dc/inc/dc_link_ddc.h:29,
from drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_mst_types.c:39:
At top level:
drivers/gpu/drm/amd/amdgpu/../display/include/ddc_service_types.h:128:22: warning: 'SYNAPTICS_DEVICE_ID' defined but not used [-Wunused-const-variable=]
128 | static const uint8_t SYNAPTICS_DEVICE_ID[] = "SYNA";
| ^~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../display/include/ddc_service_types.h:125:22: warning: 'DP_SINK_DEVICE_STR_ID_2' defined but not used [-Wunused-const-variable=]
125 | static const uint8_t DP_SINK_DEVICE_STR_ID_2[] = {7, 1, 8, 7, 5, 0};
| ^~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../display/include/ddc_service_types.h:124:22: warning: 'DP_SINK_DEVICE_STR_ID_1' defined but not used [-Wunused-const-variable=]
124 | static const uint8_t DP_SINK_DEVICE_STR_ID_1[] = {7, 1, 8, 7, 3, 0};
| ^~~~~~~~~~~~~~~~~~~~~~~
vim +41 drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu.h
539489fc91ea77 Aurabindo Pillai 2020-04-08 40
539489fc91ea77 Aurabindo Pillai 2020-04-08 @41 #define dev_fmt(fmt) "amdgpu: " fmt
539489fc91ea77 Aurabindo Pillai 2020-04-08 42
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi All, On 3/9/22 18:53, Rajat Jain wrote: > On Wed, Mar 9, 2022 at 7:06 AM Sean Paul <sean@poorly.run> wrote: >> >> From: Sean Paul <seanpaul@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@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) . 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 ? 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 >> >
() Hello Hans, Sean, On Fri, Mar 11, 2022 at 4:12 AM Hans de Goede <hdegoede@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@poorly.run> wrote: > >> > >> From: Sean Paul <seanpaul@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@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 > >> > >
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@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@poorly.run> wrote: >>>> >>>> From: Sean Paul <seanpaul@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@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: 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 >>>> >>> >
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); + 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;