diff mbox

drm/nouveau: set irq_enabled manually

Message ID 1391043180-27875-1-git-send-email-imirkin@alum.mit.edu (mailing list archive)
State New, archived
Headers show

Commit Message

Ilia Mirkin Jan. 30, 2014, 12:53 a.m. UTC
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(+)

Comments

Jan Jan. 30, 2014, 1:50 a.m. UTC | #1
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
>
>
Daniel Vetter Jan. 30, 2014, 8:33 a.m. UTC | #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
Ilia Mirkin Jan. 30, 2014, 5:11 p.m. UTC | #3
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
Jan Jan. 30, 2014, 6 p.m. UTC | #4
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.
Daniel Vetter Feb. 4, 2014, 9 a.m. UTC | #5
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 mbox

Patch

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