[PATCH 2/3] drm: Warn if vblank state has become inconsistent.

[PATCH 2/3] drm: Warn if vblank state has become inconsistent.

[christopher.halse.rogers]

From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>

After emitting all the waiting vblank events no-one should hold
a vblank reference.  Emit a warning if this is not the case.

Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
---
 drivers/gpu/drm/drm_irq.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index a1f12cb..72407fa 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 						 e->event.sequence);
 	}
 
+	WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0);
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 }
 EXPORT_SYMBOL(drm_vblank_off);
-- 
1.7.4.1

Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent.

[Michel Dänzer]

On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
> From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
> 
> After emitting all the waiting vblank events no-one should hold
> a vblank reference.  Emit a warning if this is not the case.
> 
> Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
> ---
>  drivers/gpu/drm/drm_irq.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index a1f12cb..72407fa 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>  						 e->event.sequence);
>  	}
>  
> +	WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0);
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  }
>  EXPORT_SYMBOL(drm_vblank_off);

Reviewed-by: Michel Dänzer <michel@daenzer.net>


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent.

[Mario Kleiner]

On Apr 27, 2011, at 10:58 AM, dri-devel-request@lists.freedesktop.org  
wrote:
> Message: 5
> Date: Wed, 27 Apr 2011 10:38:14 +0200
> From: Michel D?nzer <michel@daenzer.net>
> Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become
> 	inconsistent.
> To: christopher.halse.rogers@canonical.com
> Cc: dri-devel@lists.freedesktop.org
> Message-ID: <1303893494.5633.129.camel@thor.local>
> Content-Type: text/plain; charset="UTF-8"
>
> On Mit, 2011-04-27 at 16:10 +1000,  
> christopher.halse.rogers@canonical.com wrote:
>> From: Christopher James Halse Rogers  
>> <christopher.halse.rogers@canonical.com>
>>
>> After emitting all the waiting vblank events no-one should hold
>> a vblank reference.  Emit a warning if this is not the case.
>>
>> Signed-off-by: Christopher James Halse Rogers  
>> <christopher.halse.rogers@canonical.com>
>> ---
>>  drivers/gpu/drm/drm_irq.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index a1f12cb..72407fa 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev,  
>> int crtc)
>>  						 e->event.sequence);
>>  	}
>>
>> +	WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0);
>>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>>  }
>>  EXPORT_SYMBOL(drm_vblank_off);
>
> Reviewed-by: Michel D?nzer <michel@daenzer.net>
>


Any pending kms pageflip will also hold a reference on the vblank of  
a crtc, so having the refcount non-zero there is not really a sign of  
inconsistency, so i'm not sure if a warning is appropriate there.

-mario

*********************************************************************
Mario Kleiner
Max Planck Institute for Biological Cybernetics
Spemannstr. 38
72076 Tuebingen
Germany

e-mail: mario.kleiner@tuebingen.mpg.de
office: +49 (0)7071/601-1623
fax:    +49 (0)7071/601-616
www:    http://www.kyb.tuebingen.mpg.de/~kleinerm
*********************************************************************
"For a successful technology, reality must take precedence
over public relations, for Nature cannot be fooled."
(Richard Feynman)

Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent.

[Christopher James Halse Rogers]

[-- Attachment #1.1: Type: text/plain, Size: 2153 bytes --]

On Wed, 2011-04-27 at 17:50 +0200, Mario Kleiner wrote:
> On Apr 27, 2011, at 10:58 AM, dri-devel-request@lists.freedesktop.org  
> wrote:
> > Message: 5
> > Date: Wed, 27 Apr 2011 10:38:14 +0200
> > From: Michel D?nzer <michel@daenzer.net>
> > Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become
> > 	inconsistent.
> > To: christopher.halse.rogers@canonical.com
> > Cc: dri-devel@lists.freedesktop.org
> > Message-ID: <1303893494.5633.129.camel@thor.local>
> > Content-Type: text/plain; charset="UTF-8"
> >
> > On Mit, 2011-04-27 at 16:10 +1000,  
> > christopher.halse.rogers@canonical.com wrote:
> >> From: Christopher James Halse Rogers  
> >> <christopher.halse.rogers@canonical.com>
> >>
> >> After emitting all the waiting vblank events no-one should hold
> >> a vblank reference.  Emit a warning if this is not the case.
> >>
> >> Signed-off-by: Christopher James Halse Rogers  
> >> <christopher.halse.rogers@canonical.com>
> >> ---
> >>  drivers/gpu/drm/drm_irq.c |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> >> index a1f12cb..72407fa 100644
> >> --- a/drivers/gpu/drm/drm_irq.c
> >> +++ b/drivers/gpu/drm/drm_irq.c
> >> @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev,  
> >> int crtc)
> >>  						 e->event.sequence);
> >>  	}
> >>
> >> +	WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0);
> >>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> >>  }
> >>  EXPORT_SYMBOL(drm_vblank_off);
> >
> > Reviewed-by: Michel D?nzer <michel@daenzer.net>
> >
> 
> 
> Any pending kms pageflip will also hold a reference on the vblank of  
> a crtc, so having the refcount non-zero there is not really a sign of  
> inconsistency, so i'm not sure if a warning is appropriate there.

But at this point the vblank interrupts have been disabled, or at least
the driver's disable function has been called.  Will that not mean that
any pending pageflips will wait indefinitely for a vblank interrupt
that's not going to come - ie: exactly the state we're warning against?


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent.

[Mario Kleiner]

On Apr 28, 2011, at 9:47 AM, Christopher James Halse Rogers wrote:

> On Wed, 2011-04-27 at 17:50 +0200, Mario Kleiner wrote:
>> On Apr 27, 2011, at 10:58 AM, dri-devel-request@lists.freedesktop.org
>> wrote:
>>> Message: 5
>>> Date: Wed, 27 Apr 2011 10:38:14 +0200
>>> From: Michel D?nzer <michel@daenzer.net>
>>> Subject: Re: [PATCH 2/3] drm: Warn if vblank state has become
>>> 	inconsistent.
>>> To: christopher.halse.rogers@canonical.com
>>> Cc: dri-devel@lists.freedesktop.org
>>> Message-ID: <1303893494.5633.129.camel@thor.local>
>>> Content-Type: text/plain; charset="UTF-8"
>>>
>>> On Mit, 2011-04-27 at 16:10 +1000,
>>> christopher.halse.rogers@canonical.com wrote:
>>>> From: Christopher James Halse Rogers
>>>> <christopher.halse.rogers@canonical.com>
>>>>
>>>> After emitting all the waiting vblank events no-one should hold
>>>> a vblank reference.  Emit a warning if this is not the case.
>>>>
>>>> Signed-off-by: Christopher James Halse Rogers
>>>> <christopher.halse.rogers@canonical.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_irq.c |    1 +
>>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>>>> index a1f12cb..72407fa 100644
>>>> --- a/drivers/gpu/drm/drm_irq.c
>>>> +++ b/drivers/gpu/drm/drm_irq.c
>>>> @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev,
>>>> int crtc)
>>>>  						 e->event.sequence);
>>>>  	}
>>>>
>>>> +	WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0);
>>>>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>>>>  }
>>>>  EXPORT_SYMBOL(drm_vblank_off);
>>>
>>> Reviewed-by: Michel D?nzer <michel@daenzer.net>
>>>
>>
>>
>> Any pending kms pageflip will also hold a reference on the vblank of
>> a crtc, so having the refcount non-zero there is not really a sign of
>> inconsistency, so i'm not sure if a warning is appropriate there.
>
> But at this point the vblank interrupts have been disabled, or at  
> least
> the driver's disable function has been called.  Will that not mean  
> that
> any pending pageflips will wait indefinitely for a vblank interrupt
> that's not going to come - ie: exactly the state we're warning  
> against?
>


Ok, if that is the state we're warning against, then the warning  
makes sense. But it would make more sense to somehow handle this more  
gracefully. The pageflip completion path does depend on vblank irq's  
to unpin buffers, release fences and notify user apps of swap  
completion. Hanging there due to disabled vblank irq sounds bad.

Otoh the only caller of drm_vblank_off() i can find in 2.6.38 is the  
crtc disable code in intel_display.c and those routines protect  
against pending pageflips - they wait for all pageflips to finish  
before calling drm_vblank_off().

If this warning is just a precaution against possible future bugs  
then i happily rest my case.
-mario