| Message ID | 20240528215344.2442-1-anarsoul@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/nouveau: don't attempt to schedule hpd_work on headless cards | expand |
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;
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;
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
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;
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(-)