[PATCH] drm/nouveau: set irq_enabled manually

[PATCH] drm/nouveau: set irq_enabled manually

[Ilia Mirkin]

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

Re: [PATCH] drm/nouveau: set irq_enabled manually

[Jan]

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

Re: [PATCH] drm/nouveau: set irq_enabled manually

[Daniel Vetter]

On Thu, Jan 30, 2014 at 1:53 AM, Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-/w4YWyX8dFk@public.gmane.org>

if you will.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/nouveau: set irq_enabled manually

[Ilia Mirkin]

On Thu, Jan 30, 2014 at 3:33 AM, Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org> wrote:
> On Thu, Jan 30, 2014 at 1:53 AM, Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org>
>> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-/w4YWyX8dFk@public.gmane.org>
>
> if you will.
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/nouveau: set irq_enabled manually

[Jan Janecek]

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.

Re: [PATCH] drm/nouveau: set irq_enabled manually

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch