[PATCH] drm/radeon: Always call radeon_suspend_kms() in radeon_pci_shutdown()

[PATCH] drm/radeon: Always call radeon_suspend_kms() in radeon_pci_shutdown()

[Tiezhu Yang]

radeon_suspend_kms() puts the hw in the suspend state (all asics),
it should always call radeon_suspend_kms() in radeon_pci_shutdown(),
this is a normal cleanup process to avoid more operations on radeon,
just remove #ifdef CONFIG_PPC64 and the related comments.

Co-developed-by: Jianmin Lv <lvjianmin@loongson.cn>
Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 drivers/gpu/drm/radeon/radeon_drv.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index efeb115..0b1f43d 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -386,16 +386,7 @@ radeon_pci_shutdown(struct pci_dev *pdev)
 	if (radeon_device_is_virtual())
 		radeon_pci_remove(pdev);
 
-#ifdef CONFIG_PPC64
-	/*
-	 * Some adapters need to be suspended before a
-	 * shutdown occurs in order to prevent an error
-	 * during kexec.
-	 * Make this power specific becauase it breaks
-	 * some non-power boards.
-	 */
 	radeon_suspend_kms(pci_get_drvdata(pdev), true, true, false);
-#endif
 }
 
 static int radeon_pmops_suspend(struct device *dev)
-- 
2.1.0

Re: [PATCH] drm/radeon: Always call radeon_suspend_kms() in radeon_pci_shutdown()

[Christian König]

Am 07.06.21 um 14:27 schrieb Tiezhu Yang:
> radeon_suspend_kms() puts the hw in the suspend state (all asics),
> it should always call radeon_suspend_kms() in radeon_pci_shutdown(),
> this is a normal cleanup process to avoid more operations on radeon,
> just remove #ifdef CONFIG_PPC64 and the related comments.

Well NAK.

Alex knows more about the details but suspending should not be part of 
the pci shotdown process at all.

We just add that here to enforce a GPU reset on PPC64 boards for some 
reason.

Regards,
Christian.

>
> Co-developed-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   drivers/gpu/drm/radeon/radeon_drv.c | 9 ---------
>   1 file changed, 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index efeb115..0b1f43d 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -386,16 +386,7 @@ radeon_pci_shutdown(struct pci_dev *pdev)
>   	if (radeon_device_is_virtual())
>   		radeon_pci_remove(pdev);
>   
> -#ifdef CONFIG_PPC64
> -	/*
> -	 * Some adapters need to be suspended before a
> -	 * shutdown occurs in order to prevent an error
> -	 * during kexec.
> -	 * Make this power specific becauase it breaks
> -	 * some non-power boards.
> -	 */
>   	radeon_suspend_kms(pci_get_drvdata(pdev), true, true, false);
> -#endif
>   }
>   
>   static int radeon_pmops_suspend(struct device *dev)

Re: [PATCH] drm/radeon: Always call radeon_suspend_kms() in radeon_pci_shutdown()

[Alex Deucher]

On Mon, Jun 7, 2021 at 8:30 AM Christian König <christian.koenig@amd.com> wrote:
>
> Am 07.06.21 um 14:27 schrieb Tiezhu Yang:
> > radeon_suspend_kms() puts the hw in the suspend state (all asics),
> > it should always call radeon_suspend_kms() in radeon_pci_shutdown(),
> > this is a normal cleanup process to avoid more operations on radeon,
> > just remove #ifdef CONFIG_PPC64 and the related comments.
>
> Well NAK.
>
> Alex knows more about the details but suspending should not be part of
> the pci shotdown process at all.
>
> We just add that here to enforce a GPU reset on PPC64 boards for some
> reason.

Everything in the comment still applies.

Alex

>
> Regards,
> Christian.
>
> >
> > Co-developed-by: Jianmin Lv <lvjianmin@loongson.cn>
> > Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> > ---
> >   drivers/gpu/drm/radeon/radeon_drv.c | 9 ---------
> >   1 file changed, 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> > index efeb115..0b1f43d 100644
> > --- a/drivers/gpu/drm/radeon/radeon_drv.c
> > +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> > @@ -386,16 +386,7 @@ radeon_pci_shutdown(struct pci_dev *pdev)
> >       if (radeon_device_is_virtual())
> >               radeon_pci_remove(pdev);
> >
> > -#ifdef CONFIG_PPC64
> > -     /*
> > -      * Some adapters need to be suspended before a
> > -      * shutdown occurs in order to prevent an error
> > -      * during kexec.
> > -      * Make this power specific becauase it breaks
> > -      * some non-power boards.
> > -      */
> >       radeon_suspend_kms(pci_get_drvdata(pdev), true, true, false);
> > -#endif
> >   }
> >
> >   static int radeon_pmops_suspend(struct device *dev)
>

Re: [PATCH] drm/radeon: Always call radeon_suspend_kms() in radeon_pci_shutdown()

[Tiezhu Yang]

On 06/07/2021 09:42 PM, Alex Deucher wrote:
> On Mon, Jun 7, 2021 at 8:30 AM Christian König <christian.koenig@amd.com> wrote:
>> Am 07.06.21 um 14:27 schrieb Tiezhu Yang:
>>> radeon_suspend_kms() puts the hw in the suspend state (all asics),
>>> it should always call radeon_suspend_kms() in radeon_pci_shutdown(),
>>> this is a normal cleanup process to avoid more operations on radeon,
>>> just remove #ifdef CONFIG_PPC64 and the related comments.
>> Well NAK.
>>
>> Alex knows more about the details but suspending should not be part of
>> the pci shotdown process at all.
>>
>> We just add that here to enforce a GPU reset on PPC64 boards for some
>> reason.
> Everything in the comment still applies.
>
> Alex

Hi Alex and Christian,

Thanks for your quick reply. What do you think of the following changes?
If it is OK, I will send v2. If no, please ignore it.

Any comments will be much appreciated.

Thanks,
Tiezhu


Subject: [PATCH] drm/radeon: Call radeon_suspend_kms() in
  radeon_pci_shutdown() for Loongson64

On the Loongson64 platform used with Radeon GPU, shutdown or reboot failed
when console=tty is in the boot cmdline.

radeon_suspend_kms() puts the hw in the suspend state, especially set fb
state as FBINFO_STATE_SUSPENDED:

     if (fbcon) {
         console_lock();
         radeon_fbdev_set_suspend(rdev, 1);
         console_unlock();
     }

Then avoid to do any more fb operations in the related functions:

     if (p->state != FBINFO_STATE_RUNNING)
         return;

So call radeon_suspend_kms() in radeon_pci_shutdown() for Loongson64 to fix
this issue, it looks like some kind of workaround like powerpc.

Co-developed-by: Jianmin Lv <lvjianmin@loongson.cn>
Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
  drivers/gpu/drm/radeon/radeon_drv.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index efeb115..daabbf5 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -386,13 +386,13 @@ radeon_pci_shutdown(struct pci_dev *pdev)
      if (radeon_device_is_virtual())
          radeon_pci_remove(pdev);

-#ifdef CONFIG_PPC64
+#if defined(CONFIG_PPC64) || defined(CONFIG_MACH_LOONGSON64)
      /*
       * Some adapters need to be suspended before a
       * shutdown occurs in order to prevent an error
-     * during kexec.
-     * Make this power specific becauase it breaks
-     * some non-power boards.
+     * during kexec, shutdown or reboot.
+     * Make this power and Loongson specific becauase
+     * it breaks some other boards.
       */
      radeon_suspend_kms(pci_get_drvdata(pdev), true, true, false);
  #endif
-- 
2.1.0

Re: [PATCH] drm/radeon: Always call radeon_suspend_kms() in radeon_pci_shutdown()

[Alex Deucher]

On Mon, Jun 7, 2021 at 10:26 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> On 06/07/2021 09:42 PM, Alex Deucher wrote:
> > On Mon, Jun 7, 2021 at 8:30 AM Christian König <christian.koenig@amd.com> wrote:
> >> Am 07.06.21 um 14:27 schrieb Tiezhu Yang:
> >>> radeon_suspend_kms() puts the hw in the suspend state (all asics),
> >>> it should always call radeon_suspend_kms() in radeon_pci_shutdown(),
> >>> this is a normal cleanup process to avoid more operations on radeon,
> >>> just remove #ifdef CONFIG_PPC64 and the related comments.
> >> Well NAK.
> >>
> >> Alex knows more about the details but suspending should not be part of
> >> the pci shotdown process at all.
> >>
> >> We just add that here to enforce a GPU reset on PPC64 boards for some
> >> reason.
> > Everything in the comment still applies.
> >
> > Alex
>
> Hi Alex and Christian,
>
> Thanks for your quick reply. What do you think of the following changes?
> If it is OK, I will send v2. If no, please ignore it.
>
> Any comments will be much appreciated.

Looks fine.  Please send it out.

Alex

>
> Thanks,
> Tiezhu
>
>
> Subject: [PATCH] drm/radeon: Call radeon_suspend_kms() in
>   radeon_pci_shutdown() for Loongson64
>
> On the Loongson64 platform used with Radeon GPU, shutdown or reboot failed
> when console=tty is in the boot cmdline.
>
> radeon_suspend_kms() puts the hw in the suspend state, especially set fb
> state as FBINFO_STATE_SUSPENDED:
>
>      if (fbcon) {
>          console_lock();
>          radeon_fbdev_set_suspend(rdev, 1);
>          console_unlock();
>      }
>
> Then avoid to do any more fb operations in the related functions:
>
>      if (p->state != FBINFO_STATE_RUNNING)
>          return;
>
> So call radeon_suspend_kms() in radeon_pci_shutdown() for Loongson64 to fix
> this issue, it looks like some kind of workaround like powerpc.
>
> Co-developed-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   drivers/gpu/drm/radeon/radeon_drv.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
> b/drivers/gpu/drm/radeon/radeon_drv.c
> index efeb115..daabbf5 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -386,13 +386,13 @@ radeon_pci_shutdown(struct pci_dev *pdev)
>       if (radeon_device_is_virtual())
>           radeon_pci_remove(pdev);
>
> -#ifdef CONFIG_PPC64
> +#if defined(CONFIG_PPC64) || defined(CONFIG_MACH_LOONGSON64)
>       /*
>        * Some adapters need to be suspended before a
>        * shutdown occurs in order to prevent an error
> -     * during kexec.
> -     * Make this power specific becauase it breaks
> -     * some non-power boards.
> +     * during kexec, shutdown or reboot.
> +     * Make this power and Loongson specific becauase
> +     * it breaks some other boards.
>        */
>       radeon_suspend_kms(pci_get_drvdata(pdev), true, true, false);
>   #endif
> --
> 2.1.0
>