Re: [PATCH] drm/nouveau: don't attempt to schedule hpd_work on headless cards

[Vasily Khoruzhick <anarsoul@xxxxxxxxx>]

On Thu, Jun 6, 2024 at 6:37 PM Dave Airlie <airlied@xxxxxxxxx> 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@xxxxxxxxxx> 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