| Message ID | 1391043180-27875-1-git-send-email-imirkin@alum.mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I can confirm that this patch fixes the problem. (had to change spaces to tabs, but that was probably just screwed up by mail) 2014-01-30, Ilia Mirkin <imirkin@alum.mit.edu>: > Since commit 0fa9061ae8c ("drm/nouveau/mc: handle irq-related setup > ourselves"), drm_device->irq_enabled remained unset. This is needed in > order to properly wait for a vblank event in the generic drm code. > > See https://bugs.freedesktop.org/show_bug.cgi?id=74195 > > Reported-by: Jan Janecek <janjanjanx@gmail.com> > Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> > Cc: stable@vger.kernel.org # 3.10+ > --- > > TBH, not sure why this fixes things, as irq_enabled == false should have > caused the vblank wait to not wait, since the condition would be > immediately true. > > Jan, mind double-checking that this version of the patch fixes things > for you? Not 100% sure where you stuck the irq_enabled=true line when you > tried it out. > > drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c > b/drivers/gpu/drm/nouveau/nouveau_drm.c > index bfd02410..3ba7b62 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c > @@ -376,6 +376,8 @@ nouveau_drm_load(struct drm_device *dev, unsigned long > flags) > if (ret) > goto fail_device; > > + dev->irq_enabled = true; > + > /* workaround an odd issue on nvc1 by disabling the device's > * nosnoop capability. hopefully won't cause issues until a > * better fix is found - assuming there is one... > @@ -475,6 +477,7 @@ nouveau_drm_remove(struct pci_dev *pdev) > struct nouveau_drm *drm = nouveau_drm(dev); > struct nouveau_object *device; > > + dev->irq_enabled = false; > device = drm->client.base.device; > drm_put_dev(dev); > > -- > 1.8.3.2 > >
On Thu, Jan 30, 2014 at 1:53 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote: > Since commit 0fa9061ae8c ("drm/nouveau/mc: handle irq-related setup > ourselves"), drm_device->irq_enabled remained unset. This is needed in > order to properly wait for a vblank event in the generic drm code. > > See https://bugs.freedesktop.org/show_bug.cgi?id=74195 > > Reported-by: Jan Janecek <janjanjanx@gmail.com> > Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> > Cc: stable@vger.kernel.org # 3.10+ > --- > > TBH, not sure why this fixes things, as irq_enabled == false should have > caused the vblank wait to not wait, since the condition would be > immediately true. > > Jan, mind double-checking that this version of the patch fixes things > for you? Not 100% sure where you stuck the irq_enabled=true line when you > tried it out. The core drm vblank code bails out if dev->irq_enabled isn't set. So if you opt to not use the drm irq helpers and instead roll your own you still need to set this to allow vblank wait ioctls and related stuff. It's even documented in the drm DocBook ;-) So Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> if you will. Cheers, Daniel
On Thu, Jan 30, 2014 at 3:33 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Jan 30, 2014 at 1:53 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote: >> Since commit 0fa9061ae8c ("drm/nouveau/mc: handle irq-related setup >> ourselves"), drm_device->irq_enabled remained unset. This is needed in >> order to properly wait for a vblank event in the generic drm code. >> >> See https://bugs.freedesktop.org/show_bug.cgi?id=74195 >> >> Reported-by: Jan Janecek <janjanjanx@gmail.com> >> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> >> Cc: stable@vger.kernel.org # 3.10+ >> --- >> >> TBH, not sure why this fixes things, as irq_enabled == false should have >> caused the vblank wait to not wait, since the condition would be >> immediately true. >> >> Jan, mind double-checking that this version of the patch fixes things >> for you? Not 100% sure where you stuck the irq_enabled=true line when you >> tried it out. > > The core drm vblank code bails out if dev->irq_enabled isn't set. So Right. And what I'm unclear on is how does bailing out on vblank wait cause the originally reported issue -- sluggishness. That seems to imply that one is waiting too long rather than not waiting enough. > if you opt to not use the drm irq helpers and instead roll your own > you still need to set this to allow vblank wait ioctls and related > stuff. It's even documented in the drm DocBook ;-) So > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > if you will. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
2014-01-30, Ilia Mirkin <imirkin@alum.mit.edu>: > On Thu, Jan 30, 2014 at 3:33 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Thu, Jan 30, 2014 at 1:53 AM, Ilia Mirkin <imirkin@alum.mit.edu> >> wrote: >>> Since commit 0fa9061ae8c ("drm/nouveau/mc: handle irq-related setup >>> ourselves"), drm_device->irq_enabled remained unset. This is needed in >>> order to properly wait for a vblank event in the generic drm code. >>> >>> See https://bugs.freedesktop.org/show_bug.cgi?id=74195 >>> >>> Reported-by: Jan Janecek <janjanjanx@gmail.com> >>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> >>> Cc: stable@vger.kernel.org # 3.10+ >>> --- >>> >>> TBH, not sure why this fixes things, as irq_enabled == false should have >>> caused the vblank wait to not wait, since the condition would be >>> immediately true. >>> >>> Jan, mind double-checking that this version of the patch fixes things >>> for you? Not 100% sure where you stuck the irq_enabled=true line when >>> you >>> tried it out. >> >> The core drm vblank code bails out if dev->irq_enabled isn't set. So > > Right. And what I'm unclear on is how does bailing out on vblank wait > cause the originally reported issue -- sluggishness. That seems to > imply that one is waiting too long rather than not waiting enough. > >> if you opt to not use the drm irq helpers and instead roll your own >> you still need to set this to allow vblank wait ioctls and related >> stuff. It's even documented in the drm DocBook ;-) So >> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> if you will. >> >> Cheers, Daniel >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch > Now I have noticed one more thing: If you disable GLXVblank globally in xorg.conf in the non-fixed version, the compton is not able to vsync at all. If you enable it globally, compton probably still can't use DRM_IOCTL_WAIT_VBLANK properly, but is forced to vsync using some other method (i guess?) resulting in "slugishness". With the fixed version the compton achieves vsync with great responsiveness using DRM_IOCTL_WAIT_VBLANK, regardless of the GLXVblank setting in xorg.conf.
On Thu, Jan 30, 2014 at 12:11:30PM -0500, Ilia Mirkin wrote: > On Thu, Jan 30, 2014 at 3:33 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Jan 30, 2014 at 1:53 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote: > >> Since commit 0fa9061ae8c ("drm/nouveau/mc: handle irq-related setup > >> ourselves"), drm_device->irq_enabled remained unset. This is needed in > >> order to properly wait for a vblank event in the generic drm code. > >> > >> See https://bugs.freedesktop.org/show_bug.cgi?id=74195 > >> > >> Reported-by: Jan Janecek <janjanjanx@gmail.com> > >> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> > >> Cc: stable@vger.kernel.org # 3.10+ > >> --- > >> > >> TBH, not sure why this fixes things, as irq_enabled == false should have > >> caused the vblank wait to not wait, since the condition would be > >> immediately true. > >> > >> Jan, mind double-checking that this version of the patch fixes things > >> for you? Not 100% sure where you stuck the irq_enabled=true line when you > >> tried it out. > > > > The core drm vblank code bails out if dev->irq_enabled isn't set. So > > Right. And what I'm unclear on is how does bailing out on vblank wait > cause the originally reported issue -- sluggishness. That seems to > imply that one is waiting too long rather than not waiting enough. Hm, that's indeed fairly strange. No idea how this can come about tbh. -Daniel
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index bfd02410..3ba7b62 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -376,6 +376,8 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags) if (ret) goto fail_device; + dev->irq_enabled = true; + /* workaround an odd issue on nvc1 by disabling the device's * nosnoop capability. hopefully won't cause issues until a * better fix is found - assuming there is one... @@ -475,6 +477,7 @@ nouveau_drm_remove(struct pci_dev *pdev) struct nouveau_drm *drm = nouveau_drm(dev); struct nouveau_object *device; + dev->irq_enabled = false; device = drm->client.base.device; drm_put_dev(dev);
Since commit 0fa9061ae8c ("drm/nouveau/mc: handle irq-related setup ourselves"), drm_device->irq_enabled remained unset. This is needed in order to properly wait for a vblank event in the generic drm code. See https://bugs.freedesktop.org/show_bug.cgi?id=74195 Reported-by: Jan Janecek <janjanjanx@gmail.com> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> Cc: stable@vger.kernel.org # 3.10+ --- TBH, not sure why this fixes things, as irq_enabled == false should have caused the vblank wait to not wait, since the condition would be immediately true. Jan, mind double-checking that this version of the patch fixes things for you? Not 100% sure where you stuck the irq_enabled=true line when you tried it out. drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +++ 1 file changed, 3 insertions(+)