[PATCH] drm/amdgpu: Add support for drm_privacy_screen

[Hans de Goede]

Hi,

On 3/8/22 21:56, Sean Paul wrote:
> From: Sean Paul <seanpaul at 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.
> 
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
> ---
> 
> I tested this locally, but am not super confident everything is in the
> correct place. Hopefully the intent of the patch is clear and we can
> tweak positioning if needed.

Thank you for working on this, from a drm-privacy screen
pov this looks good, one small remark below.

>  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..e369fc95585e 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 {
> +			drm_err(dev, "Error getting privacy screen, ret=%d\n",
> +				PTR_ERR(privacy_screen));

This will now log a warning on all devices without a privacy-screen. The i915
code uses this instead:

                } else if (PTR_ERR(privacy_screen) != -ENODEV) {
			drm_err(dev, "Error getting privacy screen, ret=%d\n",
				PTR_ERR(privacy_screen));
                }

Regards,

Hans





> +		}
>  		return;
> +	}
>  
>  	dc_link_dp_get_max_link_enc_cap(aconnector->dc_link, &max_link_enc_cap);
>  	aconnector->mst_mgr.cbs = &dm_mst_cbs;