[PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[Alex Deucher]

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);
 
-- 
2.35.1

[PATCH 2/2] Revert "fbdev: fbmem: add a helper to determine if an aperture is used by a fw fb"

[Alex Deucher]

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(&registration_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(&registration_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);
-- 
2.35.1

Re: [PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[Javier Martinez Canillas]

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>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] Revert "fbdev: fbmem: add a helper to determine if an aperture is used by a fw fb"

[Javier Martinez Canillas]

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>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[Alex Deucher]

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
>

Re: [PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[Alex Deucher]

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
>

Re: [PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[Javier Martinez Canillas]

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.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[Javier Martinez Canillas]

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.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH 1/2] Revert "drm/amdgpu: disable runpm if we are the primary adapter"

[Alex Deucher]

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);
 
-- 
2.35.1