Hi,
On 3/8/22 23:07, Harry Wentland wrote:
>
>
> On 2022-03-08 17:02, Hans de Goede wrote:
>> Hi,
>>
>> On 3/8/22 21:56, Sean Paul 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.
>>>
>>> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
>
> 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.
Since it happens first thing in the probe function and the GPU
has not been touched yet (and thus cannot have been put in
a "bad" state), this should be fine.
The kernel will just keep retrying each time some other drivers
have successfully probed, until the privacy screen is available
and then the probe will continue normally.
Regards,
Hans
>
> 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;
>>
>
