Nouveau - May 2024 - [PATCH] drm/nouveau: don't attempt to schedule hpd_work on headless cards

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 at 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 at 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 at 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;