AMD-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning
@ 2023-02-16  9:46 Bert Karwatzki
  0 siblings, 0 replies; 6+ messages in thread
From: Bert Karwatzki @ 2023-02-16  9:46 UTC (permalink / raw)
  To: amd-gfx

When drm_kms_helper_poll_disable is used in amdgpu_device_suspend
without drm_kms_helper_poll_init having been called it causes a warning
in __flush_work:
https://gitlab.freedesktop.org/drm/amd/-/issues/2411
To avoid this one can use drm_kms_helper_poll_fini instead:


From 51cba3ae1e9f557cca8e37eb43b9b9310d0d504d Mon Sep 17 00:00:00 2001
From: Bert Karwatzki <spasswolf@web.de>
Date: Thu, 16 Feb 2023 10:34:11 +0100
Subject: [PATCH] Use drm_kms_helper_poll_fini instead of
 drm_kms_helper_poll_disable in amdgpu_device.c to avoid a warning from
 __flush_work.

Signed-off-by: Bert Karwatzki <spasswolf@web.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
 drivers/gpu/drm/drm_probe_helper.c         | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index b325f7039e0e..dc9e9868a84b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4145,7 +4145,7 @@ int amdgpu_device_suspend(struct drm_device *dev,
bool fbcon)
 	if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
 		DRM_WARN("smart shift update failed\n");
 
-	drm_kms_helper_poll_disable(dev);
+	drm_kms_helper_poll_fini(dev);
 
 	if (fbcon)
 		drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)-
>fb_helper, true);
diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index 8127be134c39..105d00d5ebf3 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -842,9 +842,10 @@ EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
  *
  * This function disables the output polling work.
  *
- * Drivers can call this helper from their device suspend
implementation. It is
- * not an error to call this even when output polling isn't enabled or
already
- * disabled. Polling is re-enabled by calling
drm_kms_helper_poll_enable().
+ * Drivers can call this helper from their device suspend
implementation. If it
+ * is not known if drm_kms_helper_poll_init has been called before the
driver
+ * should use drm_kms_helper_poll_fini_instead.
+ * Polling is re-enabled by calling drm_kms_helper_poll_enable().
  *
  * Note that calls to enable and disable polling must be strictly
ordered, which
  * is automatically the case when they're only call from
suspend/resume
-- 
2.39.1



Bert Karwatzki


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning
@ 2023-02-23 20:51 Bert Karwatzki
  2023-02-24  2:26 ` Chen, Guchun
  0 siblings, 1 reply; 6+ messages in thread
From: Bert Karwatzki @ 2023-02-23 20:51 UTC (permalink / raw)
  To: amd-gfx

When drm_kms_helper_poll_disable is used in amdgpu_device_suspend
without drm_kms_helper_poll_init having been called it causes a warning
in __flush_work:
https://gitlab.freedesktop.org/drm/amd/-/issues/2411
To avoid this one can use drm_kms_helper_poll_fini instead:
Send a second time because Evolution seems to have garbled the first
patch. 

From 51cba3ae1e9f557cca8e37eb43b9b9310d0d504d Mon Sep 17 00:00:00 2001
From: Bert Karwatzki <spasswolf@web.de>
Date: Thu, 16 Feb 2023 10:34:11 +0100
Subject: [PATCH] Use drm_kms_helper_poll_fini instead of
 drm_kms_helper_poll_disable in amdgpu_device.c to avoid a warning from
 __flush_work.

Signed-off-by: Bert Karwatzki <spasswolf@web.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
 drivers/gpu/drm/drm_probe_helper.c         | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index b325f7039e0e..dc9e9868a84b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4145,7 +4145,7 @@ int amdgpu_device_suspend(struct drm_device *dev,
bool fbcon)
        if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
                DRM_WARN("smart shift update failed\n");
 
-       drm_kms_helper_poll_disable(dev);
+       drm_kms_helper_poll_fini(dev);
 
        if (fbcon)
                drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)-
> fb_helper, true);
diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index 8127be134c39..105d00d5ebf3 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -842,9 +842,10 @@ EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
  *
  * This function disables the output polling work.
  *
- * Drivers can call this helper from their device suspend
implementation. It is
- * not an error to call this even when output polling isn't enabled or
already
- * disabled. Polling is re-enabled by calling
drm_kms_helper_poll_enable().
+ * Drivers can call this helper from their device suspend
implementation. If it
+ * is not known if drm_kms_helper_poll_init has been called before the
driver
+ * should use drm_kms_helper_poll_fini_instead.
+ * Polling is re-enabled by calling drm_kms_helper_poll_enable().
  *
  * Note that calls to enable and disable polling must be strictly
ordered, which
  * is automatically the case when they're only call from
suspend/resume




^ permalink raw reply related	[flat|nested] 6+ messages in thread

* RE: [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning
  2023-02-23 20:51 [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning Bert Karwatzki
@ 2023-02-24  2:26 ` Chen, Guchun
  2023-02-24 10:46   ` Bert Karwatzki
  2023-02-24 15:57   ` Bert Karwatzki
  0 siblings, 2 replies; 6+ messages in thread
From: Chen, Guchun @ 2023-02-24  2:26 UTC (permalink / raw)
  To: Bert Karwatzki, amd-gfx@lists.freedesktop.org

Hi Bert,

Thanks for your patch. As we will can drm_kms_helper_poll_enable in resume, so it may not make sense using drm_kms_helper_poll_fini in suspend, from code pairing perspective.

For your case, is it possible to fix the problem by limiting the access of drm_kms_helper_poll_disable with checking mode_config_initialized in adev structure? We can get rid of the code change in drm core in this way.

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Bert Karwatzki
Sent: Friday, February 24, 2023 4:52 AM
To: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning

When drm_kms_helper_poll_disable is used in amdgpu_device_suspend without drm_kms_helper_poll_init having been called it causes a warning in __flush_work:
https://gitlab.freedesktop.org/drm/amd/-/issues/2411
To avoid this one can use drm_kms_helper_poll_fini instead:
Send a second time because Evolution seems to have garbled the first patch. 

From 51cba3ae1e9f557cca8e37eb43b9b9310d0d504d Mon Sep 17 00:00:00 2001
From: Bert Karwatzki <spasswolf@web.de>
Date: Thu, 16 Feb 2023 10:34:11 +0100
Subject: [PATCH] Use drm_kms_helper_poll_fini instead of
 drm_kms_helper_poll_disable in amdgpu_device.c to avoid a warning from
 __flush_work.

Signed-off-by: Bert Karwatzki <spasswolf@web.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
 drivers/gpu/drm/drm_probe_helper.c         | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index b325f7039e0e..dc9e9868a84b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4145,7 +4145,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
        if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
                DRM_WARN("smart shift update failed\n");
 
-       drm_kms_helper_poll_disable(dev);
+       drm_kms_helper_poll_fini(dev);
 
        if (fbcon)
                drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)-
> fb_helper, true);
diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index 8127be134c39..105d00d5ebf3 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -842,9 +842,10 @@ EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
  *
  * This function disables the output polling work.
  *
- * Drivers can call this helper from their device suspend implementation. It is
- * not an error to call this even when output polling isn't enabled or already
- * disabled. Polling is re-enabled by calling drm_kms_helper_poll_enable().
+ * Drivers can call this helper from their device suspend
implementation. If it
+ * is not known if drm_kms_helper_poll_init has been called before the
driver
+ * should use drm_kms_helper_poll_fini_instead.
+ * Polling is re-enabled by calling drm_kms_helper_poll_enable().
  *
  * Note that calls to enable and disable polling must be strictly ordered, which
  * is automatically the case when they're only call from suspend/resume




^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning
  2023-02-24  2:26 ` Chen, Guchun
@ 2023-02-24 10:46   ` Bert Karwatzki
  2023-02-24 15:57   ` Bert Karwatzki
  1 sibling, 0 replies; 6+ messages in thread
From: Bert Karwatzki @ 2023-02-24 10:46 UTC (permalink / raw)
  To: Chen, Guchun, amd-gfx@lists.freedesktop.org

Am Freitag, dem 24.02.2023 um 02:26 +0000 schrieb Chen, Guchun:
> > Hi Bert,
> > 
> > Thanks for your patch. As we will can drm_kms_helper_poll_enable in
> > resume, so it may not make sense using drm_kms_helper_poll_fini in
> > suspend, from code pairing perspective.
> > 
> > For your case, is it possible to fix the problem by limiting the
> > access of drm_kms_helper_poll_disable with checking
> > mode_config_initialized in adev structure? We can get rid of the
> > code
> > change in drm core in this way.
> > 
> > Regards,
> > Guchun
> > 
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Bert Karwatzki
> > Sent: Friday, February 24, 2023 4:52 AM
> > To: amd-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH] drm/amd: use drm_kms_helper_poll_fini in
> > amdgpu_device_suspend to avoid warning
> > 
> > When drm_kms_helper_poll_disable is used in amdgpu_device_suspend
> > without drm_kms_helper_poll_init having been called it causes a
> > warning in __flush_work:
> > https://gitlab.freedesktop.org/drm/amd/-/issues/2411
> > To avoid this one can use drm_kms_helper_poll_fini instead:
> > Send a second time because Evolution seems to have garbled the
> > first
> > patch. 
> > 
> > From 51cba3ae1e9f557cca8e37eb43b9b9310d0d504d Mon Sep 17 00:00:00
> > 2001
> > From: Bert Karwatzki <spasswolf@web.de>
> > Date: Thu, 16 Feb 2023 10:34:11 +0100
> > Subject: [PATCH] Use drm_kms_helper_poll_fini instead of
> >  drm_kms_helper_poll_disable in amdgpu_device.c to avoid a warning
> > from
> >  __flush_work.
> > 
> > Signed-off-by: Bert Karwatzki <spasswolf@web.de>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> >  drivers/gpu/drm/drm_probe_helper.c         | 7 ++++---
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index b325f7039e0e..dc9e9868a84b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4145,7 +4145,7 @@ int amdgpu_device_suspend(struct drm_device
> > *dev, bool fbcon)
> >         if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
> >                 DRM_WARN("smart shift update failed\n");
> >  
> > -       drm_kms_helper_poll_disable(dev);
> > +       drm_kms_helper_poll_fini(dev);
> >  
> >         if (fbcon)
> >                 drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev
> > )-
> > > > fb_helper, true);
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index 8127be134c39..105d00d5ebf3 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -842,9 +842,10 @@ EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
> >   *
> >   * This function disables the output polling work.
> >   *
> > - * Drivers can call this helper from their device suspend
> > implementation. It is
> > - * not an error to call this even when output polling isn't
> > enabled
> > or already
> > - * disabled. Polling is re-enabled by calling
> > drm_kms_helper_poll_enable().
> > + * Drivers can call this helper from their device suspend
> > implementation. If it
> > + * is not known if drm_kms_helper_poll_init has been called before
> > the
> > driver
> > + * should use drm_kms_helper_poll_fini_instead.
> > + * Polling is re-enabled by calling drm_kms_helper_poll_enable().
> >   *
> >   * Note that calls to enable and disable polling must be strictly
> > ordered, which
> >   * is automatically the case when they're only call from
> > suspend/resume
> > 
No, that does not work for me. I tried (in linux-next-20230224):

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c4a4e2fe6681..27fb42b1bde4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4145,7 +4145,13 @@ int amdgpu_device_suspend(struct drm_device
*dev, bool fbcon)
        if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
                DRM_WARN("smart shift update failed\n");
 
-       drm_kms_helper_poll_disable(dev);
+       if (adev->mode_info.mode_config_initialized) {
+               printk(KERN_INFO "adev-
>mode_info.mode_config_initialized = %d\n",
+                               adev-
>mode_info.mode_config_initialized);
+               printk(KERN_INFO "dev->mode_config.poll_enabled =
%d\n",
+                               dev->mode_config.poll_enabled);
+               drm_kms_helper_poll_disable(dev);
+       }
 
        if (fbcon)
                drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)-
>fb_helper, true);

and found that mode_config_initialized=1 but dev-
>mode_config.poll_enabled=0 with the usual warning:
[   23.287058] adev->mode_info.mode_config_initialized = 1
[   23.287061] dev->mode_config.poll_enabled = 0
[   23.287063] ------------[ cut here ]------------
[   23.287064] WARNING: CPU: 0 PID: 16 at kernel/workqueue.c:3167
__flush_work.isra.0+0x261/0x270

The flag that needs to be checked to avoid the warning is dev-
>mode_config.poll_enabled which gets set by drm_kms_helper_poll_init
and drm_kms_helper_poll_fini does just that. Changing the comment of
drms_kms_helper_poll_disable is technically not necessary though.
Bert Karwatzki


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning
  2023-02-24  2:26 ` Chen, Guchun
  2023-02-24 10:46   ` Bert Karwatzki
@ 2023-02-24 15:57   ` Bert Karwatzki
  2023-02-27  5:51     ` Chen, Guchun
  1 sibling, 1 reply; 6+ messages in thread
From: Bert Karwatzki @ 2023-02-24 15:57 UTC (permalink / raw)
  To: Chen, Guchun, amd-gfx

Am Freitag, dem 24.02.2023 um 02:26 +0000 schrieb Chen, Guchun:
> > Hi Bert,
> > 
> > Thanks for your patch. As we will can drm_kms_helper_poll_enable in
> > resume, so it may not make sense using drm_kms_helper_poll_fini in
> > suspend, from code pairing perspective.
> > 
> > For your case, is it possible to fix the problem by limiting the
> > access of drm_kms_helper_poll_disable with checking
> > mode_config_initialized in adev structure? We can get rid of the
> > code
> > change in drm core in this way.
> > 
> > Regards,
> > Guchun
> > 
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Bert Karwatzki
> > Sent: Friday, February 24, 2023 4:52 AM
> > To: amd-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH] drm/amd: use drm_kms_helper_poll_fini in
> > amdgpu_device_suspend to avoid warning
> > 
> > When drm_kms_helper_poll_disable is used in amdgpu_device_suspend
> > without drm_kms_helper_poll_init having been called it causes a
> > warning in __flush_work:
> > https://gitlab.freedesktop.org/drm/amd/-/issues/2411
> > To avoid this one can use drm_kms_helper_poll_fini instead:
> > Send a second time because Evolution seems to have garbled the
> > first
> > patch. 
> > 
> > From 51cba3ae1e9f557cca8e37eb43b9b9310d0d504d Mon Sep 17 00:00:00
> > 2001
> > From: Bert Karwatzki <spasswolf@web.de>
> > Date: Thu, 16 Feb 2023 10:34:11 +0100
> > Subject: [PATCH] Use drm_kms_helper_poll_fini instead of
> >  drm_kms_helper_poll_disable in amdgpu_device.c to avoid a warning
> > from
> >  __flush_work.
> > 
> > Signed-off-by: Bert Karwatzki <spasswolf@web.de>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> >  drivers/gpu/drm/drm_probe_helper.c         | 7 ++++---
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index b325f7039e0e..dc9e9868a84b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4145,7 +4145,7 @@ int amdgpu_device_suspend(struct drm_device
> > *dev, bool fbcon)
> >         if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
> >                 DRM_WARN("smart shift update failed\n");
> >  
> > -       drm_kms_helper_poll_disable(dev);
> > +       drm_kms_helper_poll_fini(dev);
> >  
> >         if (fbcon)
> >                 drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev
> > )-
> > > > fb_helper, true);
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index 8127be134c39..105d00d5ebf3 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -842,9 +842,10 @@ EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
> >   *
> >   * This function disables the output polling work.
> >   *
> > - * Drivers can call this helper from their device suspend
> > implementation. It is
> > - * not an error to call this even when output polling isn't
> > enabled
> > or already
> > - * disabled. Polling is re-enabled by calling
> > drm_kms_helper_poll_enable().
> > + * Drivers can call this helper from their device suspend
> > implementation. If it
> > + * is not known if drm_kms_helper_poll_init has been called before
> > the
> > driver
> > + * should use drm_kms_helper_poll_fini_instead.
> > + * Polling is re-enabled by calling drm_kms_helper_poll_enable().
> >   *
> >   * Note that calls to enable and disable polling must be strictly
> > ordered, which
> >   * is automatically the case when they're only call from
> > suspend/resume
> > 
No, that does not work for me. I tried (in linux-next-20230224):

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c4a4e2fe6681..27fb42b1bde4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4145,7 +4145,13 @@ int amdgpu_device_suspend(struct drm_device
*dev, bool fbcon)
        if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
                DRM_WARN("smart shift update failed\n");
 
-       drm_kms_helper_poll_disable(dev);
+       if (adev->mode_info.mode_config_initialized) {
+               printk(KERN_INFO "adev-
> mode_info.mode_config_initialized = %d\n",
+                               adev-
> mode_info.mode_config_initialized);
+               printk(KERN_INFO "dev->mode_config.poll_enabled =
%d\n",
+                               dev->mode_config.poll_enabled);
+               drm_kms_helper_poll_disable(dev);
+       }
 
        if (fbcon)
                drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)-
> fb_helper, true);

and found that mode_config_initialized=1 but dev-
> mode_config.poll_enabled=0 with the usual warning:
[   23.287058] adev->mode_info.mode_config_initialized = 1
[   23.287061] dev->mode_config.poll_enabled = 0
[   23.287063] ------------[ cut here ]------------
[   23.287064] WARNING: CPU: 0 PID: 16 at kernel/workqueue.c:3167
__flush_work.isra.0+0x261/0x270

The flag that needs to be checked to avoid the warning is dev-
> mode_config.poll_enabled which gets set by drm_kms_helper_poll_init
and drm_kms_helper_poll_fini does just that. Changing the comment of
drms_kms_helper_poll_disable is technically not necessary though.
(resent because this mail didn't appear in the archive of amd-gfx
so I thought it might have been lost)
Bert Karwatzki



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* RE: [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning
  2023-02-24 15:57   ` Bert Karwatzki
@ 2023-02-27  5:51     ` Chen, Guchun
  0 siblings, 0 replies; 6+ messages in thread
From: Chen, Guchun @ 2023-02-27  5:51 UTC (permalink / raw)
  To: Bert Karwatzki, amd-gfx@lists.freedesktop.org

Hi Bert,

I mischecked the code base. After checking the code on linux-next branch after commit " drm/probe_helper: sort out poll_running vs poll_enabled ", I guess below change in drm_probe_helper.c may help:

drm_kms_helper_poll_disable() {
......
+if (dev->mode_config.poll_enabled)
+	cancel_delayed_work_sync(&dev->mode_config.output_poll_work);
....

This can tie workqueue output_poll_work to flag 'poll_enabled', as it's set unconditionally after initing wq output_poll_work in drm_kms_helper_poll_init.

Regards,
Guchun

-----Original Message-----
From: Bert Karwatzki <spasswolf@web.de> 
Sent: Friday, February 24, 2023 11:58 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning

Am Freitag, dem 24.02.2023 um 02:26 +0000 schrieb Chen, Guchun:
> > Hi Bert,
> > 
> > Thanks for your patch. As we will can drm_kms_helper_poll_enable in 
> > resume, so it may not make sense using drm_kms_helper_poll_fini in 
> > suspend, from code pairing perspective.
> > 
> > For your case, is it possible to fix the problem by limiting the 
> > access of drm_kms_helper_poll_disable with checking 
> > mode_config_initialized in adev structure? We can get rid of the 
> > code change in drm core in this way.
> > 
> > Regards,
> > Guchun
> > 
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of 
> > Bert Karwatzki
> > Sent: Friday, February 24, 2023 4:52 AM
> > To: amd-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH] drm/amd: use drm_kms_helper_poll_fini in 
> > amdgpu_device_suspend to avoid warning
> > 
> > When drm_kms_helper_poll_disable is used in amdgpu_device_suspend 
> > without drm_kms_helper_poll_init having been called it causes a 
> > warning in __flush_work:
> > https://gitlab.freedesktop.org/drm/amd/-/issues/2411
> > To avoid this one can use drm_kms_helper_poll_fini instead:
> > Send a second time because Evolution seems to have garbled the first 
> > patch.
> > 
> > From 51cba3ae1e9f557cca8e37eb43b9b9310d0d504d Mon Sep 17 00:00:00
> > 2001
> > From: Bert Karwatzki <spasswolf@web.de>
> > Date: Thu, 16 Feb 2023 10:34:11 +0100
> > Subject: [PATCH] Use drm_kms_helper_poll_fini instead of
> >  drm_kms_helper_poll_disable in amdgpu_device.c to avoid a warning 
> > from
> >  __flush_work.
> > 
> > Signed-off-by: Bert Karwatzki <spasswolf@web.de>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> >  drivers/gpu/drm/drm_probe_helper.c         | 7 ++++---
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index b325f7039e0e..dc9e9868a84b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4145,7 +4145,7 @@ int amdgpu_device_suspend(struct drm_device 
> > *dev, bool fbcon)
> >         if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
> >                 DRM_WARN("smart shift update failed\n");
> >  
> > -       drm_kms_helper_poll_disable(dev);
> > +       drm_kms_helper_poll_fini(dev);
> >  
> >         if (fbcon)
> >                 drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev
> > )-
> > > > fb_helper, true);
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index 8127be134c39..105d00d5ebf3 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -842,9 +842,10 @@ EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
> >   *
> >   * This function disables the output polling work.
> >   *
> > - * Drivers can call this helper from their device suspend 
> > implementation. It is
> > - * not an error to call this even when output polling isn't enabled 
> > or already
> > - * disabled. Polling is re-enabled by calling 
> > drm_kms_helper_poll_enable().
> > + * Drivers can call this helper from their device suspend
> > implementation. If it
> > + * is not known if drm_kms_helper_poll_init has been called before
> > the
> > driver
> > + * should use drm_kms_helper_poll_fini_instead.
> > + * Polling is re-enabled by calling drm_kms_helper_poll_enable().
> >   *
> >   * Note that calls to enable and disable polling must be strictly 
> > ordered, which
> >   * is automatically the case when they're only call from 
> > suspend/resume
> > 
No, that does not work for me. I tried (in linux-next-20230224):

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c4a4e2fe6681..27fb42b1bde4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4145,7 +4145,13 @@ int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
        if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
                DRM_WARN("smart shift update failed\n");
 
-       drm_kms_helper_poll_disable(dev);
+       if (adev->mode_info.mode_config_initialized) {
+               printk(KERN_INFO "adev-
> mode_info.mode_config_initialized = %d\n",
+                               adev-
> mode_info.mode_config_initialized);
+               printk(KERN_INFO "dev->mode_config.poll_enabled =
%d\n",
+                               dev->mode_config.poll_enabled);
+               drm_kms_helper_poll_disable(dev);
+       }
 
        if (fbcon)
                drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)-
> fb_helper, true);

and found that mode_config_initialized=1 but dev-
> mode_config.poll_enabled=0 with the usual warning:
[   23.287058] adev->mode_info.mode_config_initialized = 1 [   23.287061] dev->mode_config.poll_enabled = 0 [   23.287063] ------------[ cut here ]------------ [   23.287064] WARNING: CPU: 0 PID: 16 at kernel/workqueue.c:3167
__flush_work.isra.0+0x261/0x270

The flag that needs to be checked to avoid the warning is dev-
> mode_config.poll_enabled which gets set by drm_kms_helper_poll_init
and drm_kms_helper_poll_fini does just that. Changing the comment of drms_kms_helper_poll_disable is technically not necessary though.
(resent because this mail didn't appear in the archive of amd-gfx so I thought it might have been lost) Bert Karwatzki



^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-02-27  5:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-23 20:51 [PATCH] drm/amd: use drm_kms_helper_poll_fini in amdgpu_device_suspend to avoid warning Bert Karwatzki
2023-02-24  2:26 ` Chen, Guchun
2023-02-24 10:46   ` Bert Karwatzki
2023-02-24 15:57   ` Bert Karwatzki
2023-02-27  5:51     ` Chen, Guchun
  -- strict thread matches above, loose matches on Subject: below --
2023-02-16  9:46 Bert Karwatzki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox