| Message ID | 51EE8888.4030302@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 23, 2013 at 11:43 PM, Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote: > Sort of fixes mmiotrace for me again, I could sear I sent a similar patch before > the rework to event interface, so I guess it got reintroduced. I don't know how/why you think this fixes anything. The interrupt handler can't possibly be called until after priv->base.vblank has been initialised... Seriously, go look, the subdev interrupt handler pointer isn't filled in (and can't be) until after nouveau_disp_create() has been called, and nouveau_disp_create() initialises priv->base.vblank before it returns... Ben. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > --- > diff --git a/drivers/gpu/drm/nouveau/core/engine/disp/nv50.c b/drivers/gpu/drm/nouveau/core/engine/disp/nv50.c > index 7e3875d..35e526b 100644 > --- a/drivers/gpu/drm/nouveau/core/engine/disp/nv50.c > +++ b/drivers/gpu/drm/nouveau/core/engine/disp/nv50.c > @@ -1266,13 +1266,15 @@ nv50_disp_intr(struct nouveau_subdev *subdev) > } > > if (intr1 & 0x00000004) { > - nouveau_event_trigger(priv->base.vblank, 0); > + if (priv->base.vblank) > + nouveau_event_trigger(priv->base.vblank, 0); > nv_wr32(priv, 0x610024, 0x00000004); > intr1 &= ~0x00000004; > } > > if (intr1 & 0x00000008) { > - nouveau_event_trigger(priv->base.vblank, 1); > + if (priv->base.vblank) > + nouveau_event_trigger(priv->base.vblank, 1); > nv_wr32(priv, 0x610024, 0x00000008); > intr1 &= ~0x00000008; > } > diff --git a/drivers/gpu/drm/nouveau/core/engine/disp/nvd0.c b/drivers/gpu/drm/nouveau/core/engine/disp/nvd0.c > index 52dd7a1..4095f65 100644 > --- a/drivers/gpu/drm/nouveau/core/engine/disp/nvd0.c > +++ b/drivers/gpu/drm/nouveau/core/engine/disp/nvd0.c > @@ -941,7 +941,7 @@ nvd0_disp_intr(struct nouveau_subdev *subdev) > u32 mask = 0x01000000 << i; > if (mask & intr) { > u32 stat = nv_rd32(priv, 0x6100bc + (i * 0x800)); > - if (stat & 0x00000001) > + if ((stat & 0x00000001) && priv->base.vblank) > nouveau_event_trigger(priv->base.vblank, i); > nv_mask(priv, 0x6100bc + (i * 0x800), 0, 0); > nv_rd32(priv, 0x6100c0 + (i * 0x800)); > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 30-07-13 04:42, Ben Skeggs schreef: > On Tue, Jul 23, 2013 at 11:43 PM, Maarten Lankhorst > <maarten.lankhorst@canonical.com> wrote: >> Sort of fixes mmiotrace for me again, I could sear I sent a similar patch before >> the rework to event interface, so I guess it got reintroduced. > I don't know how/why you think this fixes anything. The interrupt > handler can't possibly be called until after priv->base.vblank has > been initialised... > > Seriously, go look, the subdev interrupt handler pointer isn't filled > in (and can't be) until after nouveau_disp_create() has been called, > and nouveau_disp_create() initialises priv->base.vblank before it > returns... > There's also a disp_dtor function right above it and without a smp_wmb the writes could still be reordered in the constructor. O:-) A spinlock would be needed to make sure that no interrupt is in process if you set intr to null before destroying vblank event, though. I can probably try to get the oops again indicating that priv->base.vblank was null in the interrupt handler if you want, iirc it happened reliably during mmiotracing. ~Maarten
On Tue, Jul 30, 2013 at 4:10 PM, Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote: > Op 30-07-13 04:42, Ben Skeggs schreef: >> On Tue, Jul 23, 2013 at 11:43 PM, Maarten Lankhorst >> <maarten.lankhorst@canonical.com> wrote: >>> Sort of fixes mmiotrace for me again, I could sear I sent a similar patch before >>> the rework to event interface, so I guess it got reintroduced. >> I don't know how/why you think this fixes anything. The interrupt >> handler can't possibly be called until after priv->base.vblank has >> been initialised... >> >> Seriously, go look, the subdev interrupt handler pointer isn't filled >> in (and can't be) until after nouveau_disp_create() has been called, >> and nouveau_disp_create() initialises priv->base.vblank before it >> returns... >> > There's also a disp_dtor function right above it and without a smp_wmb > the writes could still be reordered in the constructor. O:-) A spinlock would > be needed to make sure that no interrupt is in process if you set intr to null > before destroying vblank event, though. > > I can probably try to get the oops again indicating that priv->base.vblank was null in the interrupt handler if you want, > iirc it happened reliably during mmiotracing. It would've had to have happened during a module unload in that case, I can't see how else it could've possibly happened. The destructor case is valid though, but not really a critical issue, so I'd rather find a better solution than these hacked checks :) I'll add it to the list... Ben. > > ~Maarten
diff --git a/drivers/gpu/drm/nouveau/core/engine/disp/nv50.c b/drivers/gpu/drm/nouveau/core/engine/disp/nv50.c index 7e3875d..35e526b 100644 --- a/drivers/gpu/drm/nouveau/core/engine/disp/nv50.c +++ b/drivers/gpu/drm/nouveau/core/engine/disp/nv50.c @@ -1266,13 +1266,15 @@ nv50_disp_intr(struct nouveau_subdev *subdev) } if (intr1 & 0x00000004) { - nouveau_event_trigger(priv->base.vblank, 0); + if (priv->base.vblank) + nouveau_event_trigger(priv->base.vblank, 0); nv_wr32(priv, 0x610024, 0x00000004); intr1 &= ~0x00000004; } if (intr1 & 0x00000008) { - nouveau_event_trigger(priv->base.vblank, 1); + if (priv->base.vblank) + nouveau_event_trigger(priv->base.vblank, 1); nv_wr32(priv, 0x610024, 0x00000008); intr1 &= ~0x00000008; } diff --git a/drivers/gpu/drm/nouveau/core/engine/disp/nvd0.c b/drivers/gpu/drm/nouveau/core/engine/disp/nvd0.c index 52dd7a1..4095f65 100644 --- a/drivers/gpu/drm/nouveau/core/engine/disp/nvd0.c +++ b/drivers/gpu/drm/nouveau/core/engine/disp/nvd0.c @@ -941,7 +941,7 @@ nvd0_disp_intr(struct nouveau_subdev *subdev) u32 mask = 0x01000000 << i; if (mask & intr) { u32 stat = nv_rd32(priv, 0x6100bc + (i * 0x800)); - if (stat & 0x00000001) + if ((stat & 0x00000001) && priv->base.vblank) nouveau_event_trigger(priv->base.vblank, i); nv_mask(priv, 0x6100bc + (i * 0x800), 0, 0); nv_rd32(priv, 0x6100c0 + (i * 0x800));
Sort of fixes mmiotrace for me again, I could sear I sent a similar patch before the rework to event interface, so I guess it got reintroduced. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> ---