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(-)
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 9a45ac2320d0a6ae01880a30d4b86025fce4061b.
This was added a helper for amdgpu to workaround a runtime pm regression caused by a runtime pm fix in efifb. We now have a better workarouund in amdgpu in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are displays attached (v3)") so this workaround is no longer necessary. Since amdgpu was the only user of this interface, we can remove it.
Signed-off-by: Alex Deucher alexander.deucher@amd.com --- drivers/video/fbdev/core/fbmem.c | 47 -------------------------------- include/linux/fb.h | 1 - 2 files changed, 48 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index ad9aac06427a..700ac4a83329 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1783,53 +1783,6 @@ int remove_conflicting_framebuffers(struct apertures_struct *a, } EXPORT_SYMBOL(remove_conflicting_framebuffers);
-/** - * is_firmware_framebuffer - detect if firmware-configured framebuffer matches - * @a: memory range, users of which are to be checked - * - * This function checks framebuffer devices (initialized by firmware/bootloader) - * which use memory range described by @a. If @a matchesm the function returns - * true, otherwise false. - */ -bool is_firmware_framebuffer(struct apertures_struct *a) -{ - bool do_free = false; - bool found = false; - int i; - - if (!a) { - a = alloc_apertures(1); - if (!a) - return false; - - a->ranges[0].base = 0; - a->ranges[0].size = ~0; - do_free = true; - } - - mutex_lock(®istration_lock); - /* check all firmware fbs and kick off if the base addr overlaps */ - for_each_registered_fb(i) { - struct apertures_struct *gen_aper; - - if (!(registered_fb[i]->flags & FBINFO_MISC_FIRMWARE)) - continue; - - gen_aper = registered_fb[i]->apertures; - if (fb_do_apertures_overlap(gen_aper, a)) { - found = true; - break; - } - } - mutex_unlock(®istration_lock); - - if (do_free) - kfree(a); - - return found; -} -EXPORT_SYMBOL(is_firmware_framebuffer); - /** * remove_conflicting_pci_framebuffers - remove firmware-configured framebuffers for PCI devices * @pdev: PCI device diff --git a/include/linux/fb.h b/include/linux/fb.h index 9a77ab615c36..147d582dab41 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -612,7 +612,6 @@ extern int remove_conflicting_pci_framebuffers(struct pci_dev *pdev, const char *name); extern int remove_conflicting_framebuffers(struct apertures_struct *a, const char *name, bool primary); -extern bool is_firmware_framebuffer(struct apertures_struct *a); extern int fb_prepare_logo(struct fb_info *fb_info, int rotate); extern int fb_show_logo(struct fb_info *fb_info, int rotate); extern char* fb_get_buffer_offset(struct fb_info *info, struct fb_pixmap *buf, u32 size);
On 5/4/22 15:48, Alex Deucher wrote:
This reverts commit 9a45ac2320d0a6ae01880a30d4b86025fce4061b.
This was added a helper for amdgpu to workaround a runtime pm regression caused by a runtime pm fix in efifb. We now have a better workarouund
s/workarouund/workaround
in amdgpu in commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are displays attached (v3)")
Again I would write it as:
commit f95af4a9236695 ("drm/amdgpu: don't runtime suspend if there are displays attached (v3)")
so this workaround is no longer necessary. Since amdgpu was the only user of this interface, we can remove it.
Signed-off-by: Alex Deucher alexander.deucher@amd.com
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
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 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.
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
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Alex Deucher -
Daniel Vetter -
Javier Martinez Canillas