| Message ID | 20220504134833.1672728-1-alexander.deucher@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter" | expand |
Hello Alex, On 5/4/22 15:48, 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 would write this line instead as: in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are displays attached (v3)"). > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > --- The patch looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
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@ffwll.ch> > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > --- > 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 >
On Wed, May 4, 2022 at 12:46 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > > Hello Alex, > > On 5/4/22 15:48, 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 would write this line instead as: > > in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are > displays attached (v3)"). When you do it that way checkpatch and some maintainers complain about splitting up a commit line across multiple lines. Alex > > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > > --- > > The patch looks good to me. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > -- > Best regards, > > Javier Martinez Canillas > Linux Engineering > Red Hat >
On 5/4/22 18:50, Alex Deucher wrote: > On Wed, May 4, 2022 at 12:46 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> >> Hello Alex, >> >> On 5/4/22 15:48, 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 would write this line instead as: >> >> in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are >> displays attached (v3)"). > > When you do it that way checkpatch and some maintainers complain about > splitting up a commit line across multiple lines. > It does indeed, how silly. Scratch my comment then.
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);
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)"). Signed-off-by: Alex Deucher <alexander.deucher@amd.com> --- 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(-)