On 2022-03-08 17:02, Hans de Goede wrote:
> Hi,
>
> On 3/8/22 21:56, Sean Paul wrote:
>> From: Sean Paul <seanp...@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 <seanp...@chromium.org>
The amdgpu_dm portion looks fine to me with Hans' comment
addressed.
I don't know what the impact of the EPROBE_DEFER in amdgpu_pci_probe
is.
Harry
>> ---
>>
>> 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;
>
