On Wed, May 04, 2022 at 09:48:32AM -0400, Alex Deucher wrote:
> This reverts commit b95dc06af3e683d6b7ddbbae178b2b2a21ee8b2b.
>
> This workaround is no longer necessary. We have a better workaround
> in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are displays attached (v3)").
I looked at this patch here quickly, and you still have a bit a design
issue. The trouble is that this is a pretty nasty locking inversion
compared to any other drivers, because you check modeset locks within
runtime pm callbacks.
The way this is meant to work with atomic is that in your atomic commit
you grab/drop runtime pm references as needed (simple for pci devices, but
the arm-soc have a rpm domain pretty much per plane/crtc/encoder
sometimes), in conjunction with drm_atomic_helper_commit_tail_rpm - if
you're using the default commit functions at least, so that ordering is
correct. Which doesn't apply to amdgpu.
I think in general it's a antipattern to check whether you're in use in
your suspend callback - it's gone boom wrt locking in a few places and
also once you reject I think there's nothing really that tries again. The
autosuspend (if enabled) only kicks in when the refcount drops to zero.
Anyway nothing terrible, just more work to do here I guess, it's good to
drop the earlier approaches still.
On the series:
Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 -
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 28 -------------------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 ------
> 3 files changed, 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index d557f4db2565..682ec660f2c4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -981,7 +981,6 @@ struct amdgpu_device {
> bool runpm;
> bool in_runpm;
> bool has_pr3;
> - bool is_fw_fb;
>
> bool pm_sysfs_en;
> bool ucode_sysfs_en;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index ebd37fb19cdb..3c198b2a86db 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -38,7 +38,6 @@
> #include <linux/mmu_notifier.h>
> #include <linux/suspend.h>
> #include <linux/cc_platform.h>
> -#include <linux/fb.h>
>
> #include "amdgpu.h"
> #include "amdgpu_irq.h"
> @@ -1950,26 +1949,6 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>
> static const struct drm_driver amdgpu_kms_driver;
>
> -static bool amdgpu_is_fw_framebuffer(resource_size_t base,
> - resource_size_t size)
> -{
> - bool found = false;
> -#if IS_REACHABLE(CONFIG_FB)
> - struct apertures_struct *a;
> -
> - a = alloc_apertures(1);
> - if (!a)
> - return false;
> -
> - a->ranges[0].base = base;
> - a->ranges[0].size = size;
> -
> - found = is_firmware_framebuffer(a);
> - kfree(a);
> -#endif
> - return found;
> -}
> -
> static void amdgpu_get_secondary_funcs(struct amdgpu_device *adev)
> {
> struct pci_dev *p = NULL;
> @@ -2000,8 +1979,6 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> unsigned long flags = ent->driver_data;
> int ret, retry = 0, i;
> bool supports_atomic = false;
> - bool is_fw_fb;
> - resource_size_t base, size;
>
> /* skip devices which are owned by radeon */
> for (i = 0; i < ARRAY_SIZE(amdgpu_unsupported_pciidlist); i++) {
> @@ -2068,10 +2045,6 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> }
> #endif
>
> - base = pci_resource_start(pdev, 0);
> - size = pci_resource_len(pdev, 0);
> - is_fw_fb = amdgpu_is_fw_framebuffer(base, size);
> -
> /* Get rid of things like offb */
> ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &amdgpu_kms_driver);
> if (ret)
> @@ -2084,7 +2057,6 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> adev->dev = &pdev->dev;
> adev->pdev = pdev;
> ddev = adev_to_drm(adev);
> - adev->is_fw_fb = is_fw_fb;
>
> if (!supports_atomic)
> ddev->driver_features &= ~DRIVER_ATOMIC;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 51bb977154eb..497478f8a5d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -185,12 +185,6 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
> adev->runpm = true;
> break;
> }
> - /* XXX: disable runtime pm if we are the primary adapter
> - * to avoid displays being re-enabled after DPMS.
> - * This needs to be sorted out and fixed properly.
> - */
> - if (adev->is_fw_fb)
> - adev->runpm = false;
>
> amdgpu_runtime_pm_quirk(adev);
>
> --
> 2.35.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
