* [PATCH] drm/nouveau: don't attempt to schedule hpd_work on headless cards
@ 2024-05-28 21:52 Vasily Khoruzhick
2024-05-28 12:15 ` Ben Skeggs
0 siblings, 1 reply; 4+ messages in thread
From: Vasily Khoruzhick @ 2024-05-28 21:52 UTC (permalink / raw)
To: Karol Herbst, Lyude Paul, Danilo Krummrich, David Airlie,
Daniel Vetter, Ben Skeggs, Jani Nikula, Wayne Lin,
Thomas Zimmermann, dri-devel, nouveau
Cc: Vasily Khoruzhick
If the card doesn't have display hardware, hpd_work and hpd_lock are
left uninitialized which causes BUG when attempting to schedule hpd_work
on runtime PM resume.
Fix it by adding headless flag to DRM and skip any hpd if it's set.
Fixes: ae1aadb1eb8d ("nouveau: don't fail driver load if no display hw present.")
Link: https://gitlab.freedesktop.org/drm/nouveau/-/issues/337
Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
drivers/gpu/drm/nouveau/dispnv04/disp.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_connector.c | 3 +++
drivers/gpu/drm/nouveau/nouveau_display.c | 11 ++++++++++-
drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
5 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c b/drivers/gpu/drm/nouveau/dispnv04/disp.c
index 13705c5f1497..4b7497a8755c 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c
@@ -68,7 +68,7 @@ nv04_display_fini(struct drm_device *dev, bool runtime, bool suspend)
if (nv_two_heads(dev))
NVWriteCRTC(dev, 1, NV_PCRTC_INTR_EN_0, 0);
- if (!runtime)
+ if (!runtime && !drm->headless)
cancel_work_sync(&drm->hpd_work);
if (!suspend)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 88728a0b2c25..674dc567e179 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -2680,7 +2680,7 @@ nv50_display_fini(struct drm_device *dev, bool runtime, bool suspend)
nv50_mstm_fini(nouveau_encoder(encoder));
}
- if (!runtime)
+ if (!runtime && !drm->headless)
cancel_work_sync(&drm->hpd_work);
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 856b3ef5edb8..b315a2ef5b28 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -1190,6 +1190,9 @@ nouveau_connector_hpd(struct nouveau_connector *nv_connector, u64 bits)
u32 mask = drm_connector_mask(&nv_connector->base);
unsigned long flags;
+ if (drm->headless)
+ return;
+
spin_lock_irqsave(&drm->hpd_lock, flags);
if (!(drm->hpd_pending & mask)) {
nv_connector->hpd_pending |= bits;
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index aed5d5b51b43..1961ef665e97 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -450,6 +450,9 @@ nouveau_display_hpd_resume(struct drm_device *dev)
{
struct nouveau_drm *drm = nouveau_drm(dev);
+ if (drm->headless)
+ return;
+
spin_lock_irq(&drm->hpd_lock);
drm->hpd_pending = ~0;
spin_unlock_irq(&drm->hpd_lock);
@@ -468,6 +471,11 @@ nouveau_display_hpd_work(struct work_struct *work)
int changed = 0;
struct drm_connector *first_changed_connector = NULL;
+ WARN_ON_ONCE(drm->headless);
+
+ if (drm->headless)
+ return;
+
pm_runtime_get_sync(dev->dev);
spin_lock_irq(&drm->hpd_lock);
@@ -635,7 +643,7 @@ nouveau_display_fini(struct drm_device *dev, bool suspend, bool runtime)
}
drm_connector_list_iter_end(&conn_iter);
- if (!runtime)
+ if (!runtime && !drm->headless)
cancel_work_sync(&drm->hpd_work);
drm_kms_helper_poll_disable(dev);
@@ -729,6 +737,7 @@ nouveau_display_create(struct drm_device *dev)
/* no display hw */
if (ret == -ENODEV) {
ret = 0;
+ drm->headless = true;
goto disp_create_err;
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index e239c6bf4afa..25fca98a20bc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -276,6 +276,7 @@ struct nouveau_drm {
/* modesetting */
struct nvbios vbios;
struct nouveau_display *display;
+ bool headless;
struct work_struct hpd_work;
spinlock_t hpd_lock;
u32 hpd_pending;
--
2.45.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/nouveau: don't attempt to schedule hpd_work on headless cards
2024-05-28 21:52 [PATCH] drm/nouveau: don't attempt to schedule hpd_work on headless cards Vasily Khoruzhick
@ 2024-05-28 12:15 ` Ben Skeggs
2024-06-07 1:37 ` Dave Airlie
0 siblings, 1 reply; 4+ messages in thread
From: Ben Skeggs @ 2024-05-28 12:15 UTC (permalink / raw)
To: dri-devel, nouveau
On 29/5/24 07:52, Vasily Khoruzhick wrote:
> If the card doesn't have display hardware, hpd_work and hpd_lock are
> left uninitialized which causes BUG when attempting to schedule hpd_work
> on runtime PM resume.
Hi,
Good catch, thank you for looking at this. A couple of initial comments
below:
Ben.
>
> Fix it by adding headless flag to DRM and skip any hpd if it's set.
>
> Fixes: ae1aadb1eb8d ("nouveau: don't fail driver load if no display hw present.")
> Link: https://gitlab.freedesktop.org/drm/nouveau/-/issues/337
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> drivers/gpu/drm/nouveau/dispnv04/disp.c | 2 +-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_connector.c | 3 +++
> drivers/gpu/drm/nouveau/nouveau_display.c | 11 ++++++++++-
> drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
> 5 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c b/drivers/gpu/drm/nouveau/dispnv04/disp.c
> index 13705c5f1497..4b7497a8755c 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c
> @@ -68,7 +68,7 @@ nv04_display_fini(struct drm_device *dev, bool runtime, bool suspend)
> if (nv_two_heads(dev))
> NVWriteCRTC(dev, 1, NV_PCRTC_INTR_EN_0, 0);
>
> - if (!runtime)
> + if (!runtime && !drm->headless)
> cancel_work_sync(&drm->hpd_work);
>
> if (!suspend)
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 88728a0b2c25..674dc567e179 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -2680,7 +2680,7 @@ nv50_display_fini(struct drm_device *dev, bool runtime, bool suspend)
> nv50_mstm_fini(nouveau_encoder(encoder));
> }
>
> - if (!runtime)
> + if (!runtime && !drm->headless)
> cancel_work_sync(&drm->hpd_work);
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 856b3ef5edb8..b315a2ef5b28 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -1190,6 +1190,9 @@ nouveau_connector_hpd(struct nouveau_connector *nv_connector, u64 bits)
> u32 mask = drm_connector_mask(&nv_connector->base);
> unsigned long flags;
>
> + if (drm->headless)
> + return;
> +
You shouldn't need this change, the function can't be called if there's
no display.
> spin_lock_irqsave(&drm->hpd_lock, flags);
> if (!(drm->hpd_pending & mask)) {
> nv_connector->hpd_pending |= bits;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index aed5d5b51b43..1961ef665e97 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -450,6 +450,9 @@ nouveau_display_hpd_resume(struct drm_device *dev)
> {
> struct nouveau_drm *drm = nouveau_drm(dev);
>
> + if (drm->headless)
> + return;
> +
> spin_lock_irq(&drm->hpd_lock);
> drm->hpd_pending = ~0;
> spin_unlock_irq(&drm->hpd_lock);
> @@ -468,6 +471,11 @@ nouveau_display_hpd_work(struct work_struct *work)
> int changed = 0;
> struct drm_connector *first_changed_connector = NULL;
>
> + WARN_ON_ONCE(drm->headless);
> +
> + if (drm->headless)
> + return;
> +
Same here.
> pm_runtime_get_sync(dev->dev);
>
> spin_lock_irq(&drm->hpd_lock);
> @@ -635,7 +643,7 @@ nouveau_display_fini(struct drm_device *dev, bool suspend, bool runtime)
> }
> drm_connector_list_iter_end(&conn_iter);
>
> - if (!runtime)
> + if (!runtime && !drm->headless)
> cancel_work_sync(&drm->hpd_work);
>
> drm_kms_helper_poll_disable(dev);
> @@ -729,6 +737,7 @@ nouveau_display_create(struct drm_device *dev)
> /* no display hw */
> if (ret == -ENODEV) {
> ret = 0;
> + drm->headless = true;
> goto disp_create_err;
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index e239c6bf4afa..25fca98a20bc 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -276,6 +276,7 @@ struct nouveau_drm {
> /* modesetting */
> struct nvbios vbios;
> struct nouveau_display *display;
> + bool headless;
> struct work_struct hpd_work;
> spinlock_t hpd_lock;
> u32 hpd_pending;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/nouveau: don't attempt to schedule hpd_work on headless cards
2024-05-28 12:15 ` Ben Skeggs
@ 2024-06-07 1:37 ` Dave Airlie
2024-06-07 16:52 ` Vasily Khoruzhick
0 siblings, 1 reply; 4+ messages in thread
From: Dave Airlie @ 2024-06-07 1:37 UTC (permalink / raw)
To: Ben Skeggs, Vasily Khoruzhick; +Cc: dri-devel, nouveau
readding original poster
On Wed, 29 May 2024 at 09:57, Ben Skeggs <bskeggs@nvidia.com> wrote:
>
> On 29/5/24 07:52, Vasily Khoruzhick wrote:
>
> > If the card doesn't have display hardware, hpd_work and hpd_lock are
> > left uninitialized which causes BUG when attempting to schedule hpd_work
> > on runtime PM resume.
>
> Hi,
>
> Good catch, thank you for looking at this. A couple of initial comments
> below:
>
> Ben.
>
> >
> > Fix it by adding headless flag to DRM and skip any hpd if it's set.
> >
> > Fixes: ae1aadb1eb8d ("nouveau: don't fail driver load if no display hw present.")
> > Link: https://gitlab.freedesktop.org/drm/nouveau/-/issues/337
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> > drivers/gpu/drm/nouveau/dispnv04/disp.c | 2 +-
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
> > drivers/gpu/drm/nouveau/nouveau_connector.c | 3 +++
> > drivers/gpu/drm/nouveau/nouveau_display.c | 11 ++++++++++-
> > drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
> > 5 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c b/drivers/gpu/drm/nouveau/dispnv04/disp.c
> > index 13705c5f1497..4b7497a8755c 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c
> > @@ -68,7 +68,7 @@ nv04_display_fini(struct drm_device *dev, bool runtime, bool suspend)
> > if (nv_two_heads(dev))
> > NVWriteCRTC(dev, 1, NV_PCRTC_INTR_EN_0, 0);
> >
> > - if (!runtime)
> > + if (!runtime && !drm->headless)
> > cancel_work_sync(&drm->hpd_work);
> >
> > if (!suspend)
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index 88728a0b2c25..674dc567e179 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -2680,7 +2680,7 @@ nv50_display_fini(struct drm_device *dev, bool runtime, bool suspend)
> > nv50_mstm_fini(nouveau_encoder(encoder));
> > }
> >
> > - if (!runtime)
> > + if (!runtime && !drm->headless)
> > cancel_work_sync(&drm->hpd_work);
> > }
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > index 856b3ef5edb8..b315a2ef5b28 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > @@ -1190,6 +1190,9 @@ nouveau_connector_hpd(struct nouveau_connector *nv_connector, u64 bits)
> > u32 mask = drm_connector_mask(&nv_connector->base);
> > unsigned long flags;
> >
> > + if (drm->headless)
> > + return;
> > +
>
> You shouldn't need this change, the function can't be called if there's
> no display.
>
>
> > spin_lock_irqsave(&drm->hpd_lock, flags);
> > if (!(drm->hpd_pending & mask)) {
> > nv_connector->hpd_pending |= bits;
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> > index aed5d5b51b43..1961ef665e97 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> > @@ -450,6 +450,9 @@ nouveau_display_hpd_resume(struct drm_device *dev)
> > {
> > struct nouveau_drm *drm = nouveau_drm(dev);
> >
> > + if (drm->headless)
> > + return;
> > +
> > spin_lock_irq(&drm->hpd_lock);
> > drm->hpd_pending = ~0;
> > spin_unlock_irq(&drm->hpd_lock);
> > @@ -468,6 +471,11 @@ nouveau_display_hpd_work(struct work_struct *work)
> > int changed = 0;
> > struct drm_connector *first_changed_connector = NULL;
> >
> > + WARN_ON_ONCE(drm->headless);
> > +
> > + if (drm->headless)
> > + return;
> > +
>
> Same here.
>
>
> > pm_runtime_get_sync(dev->dev);
> >
> > spin_lock_irq(&drm->hpd_lock);
> > @@ -635,7 +643,7 @@ nouveau_display_fini(struct drm_device *dev, bool suspend, bool runtime)
> > }
> > drm_connector_list_iter_end(&conn_iter);
> >
> > - if (!runtime)
> > + if (!runtime && !drm->headless)
> > cancel_work_sync(&drm->hpd_work);
> >
> > drm_kms_helper_poll_disable(dev);
> > @@ -729,6 +737,7 @@ nouveau_display_create(struct drm_device *dev)
> > /* no display hw */
> > if (ret == -ENODEV) {
> > ret = 0;
> > + drm->headless = true;
> > goto disp_create_err;
> > }
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > index e239c6bf4afa..25fca98a20bc 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > @@ -276,6 +276,7 @@ struct nouveau_drm {
> > /* modesetting */
> > struct nvbios vbios;
> > struct nouveau_display *display;
> > + bool headless;
> > struct work_struct hpd_work;
> > spinlock_t hpd_lock;
> > u32 hpd_pending;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/nouveau: don't attempt to schedule hpd_work on headless cards
2024-06-07 1:37 ` Dave Airlie
@ 2024-06-07 16:52 ` Vasily Khoruzhick
0 siblings, 0 replies; 4+ messages in thread
From: Vasily Khoruzhick @ 2024-06-07 16:52 UTC (permalink / raw)
To: Dave Airlie; +Cc: Ben Skeggs, dri-devel, nouveau
On Thu, Jun 6, 2024 at 6:37 PM Dave Airlie <airlied@gmail.com> wrote:
>
> readding original poster
Thanks, Dave!
Ben, please keep me on CC, since I'm not subscribed to either nouveau
or dri-devel mailing lists.
> On Wed, 29 May 2024 at 09:57, Ben Skeggs <bskeggs@nvidia.com> wrote:
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > > index 856b3ef5edb8..b315a2ef5b28 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > > @@ -1190,6 +1190,9 @@ nouveau_connector_hpd(struct nouveau_connector *nv_connector, u64 bits)
> > > u32 mask = drm_connector_mask(&nv_connector->base);
> > > unsigned long flags;
> > >
> > > + if (drm->headless)
> > > + return;
> > > +
> >
> > You shouldn't need this change, the function can't be called if there's
> > no display.
You are right. I added it for extra safety in case if the code changes
in future and it somehow gets called, but I don't have a strong
preference, so I can drop it in v2.
> >
> > > spin_lock_irqsave(&drm->hpd_lock, flags);
> > > if (!(drm->hpd_pending & mask)) {
> > > nv_connector->hpd_pending |= bits;
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> > > index aed5d5b51b43..1961ef665e97 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> > > @@ -450,6 +450,9 @@ nouveau_display_hpd_resume(struct drm_device *dev)
> > > {
> > > struct nouveau_drm *drm = nouveau_drm(dev);
> > >
> > > + if (drm->headless)
> > > + return;
> > > +
> > > spin_lock_irq(&drm->hpd_lock);
> > > drm->hpd_pending = ~0;
> > > spin_unlock_irq(&drm->hpd_lock);
> > > @@ -468,6 +471,11 @@ nouveau_display_hpd_work(struct work_struct *work)
> > > int changed = 0;
> > > struct drm_connector *first_changed_connector = NULL;
> > >
> > > + WARN_ON_ONCE(drm->headless);
> > > +
> > > + if (drm->headless)
> > > + return;
> > > +
> >
> > Same here.
Same comment, I'll drop it for v2.
Regards,
Vasily
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-02-07 1:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-28 21:52 [PATCH] drm/nouveau: don't attempt to schedule hpd_work on headless cards Vasily Khoruzhick
2024-05-28 12:15 ` Ben Skeggs
2024-06-07 1:37 ` Dave Airlie
2024-06-07 16:52 ` Vasily Khoruzhick
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.