The comment says that the caller must hold the dev->event_lock spinlock, so let's enforce this.
A quick audit over all driver shows that except for the one place in i915 which motivated this all callers fullfill this requirement already.
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: dri-devel@lists.freedesktop.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_irq.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 80ff94ada75e..bf248eb9ffb2 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -907,6 +907,9 @@ void drm_send_vblank_event(struct drm_device *dev, int crtc, { struct timeval now; unsigned int seq; + + assert_spin_locked(&dev->event_lock); + if (crtc >= 0) { seq = drm_vblank_count_and_time(dev, crtc, &now); } else {
On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote:
The comment says that the caller must hold the dev->event_lock spinlock, so let's enforce this.
A quick audit over all driver shows that except for the one place in i915 which motivated this all callers fullfill this requirement already.
Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in send_vblank_event() as well then. -Chris
On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote:
On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote:
The comment says that the caller must hold the dev->event_lock spinlock, so let's enforce this.
A quick audit over all driver shows that except for the one place in i915 which motivated this all callers fullfill this requirement already.
Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in send_vblank_event() as well then.
Meh, I've missed that one, that's actually better I think. I'll drop my patch here. -Daniel
On Fri, Sep 12, 2014 at 05:34:56PM +0200, Daniel Vetter wrote:
On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote:
On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote:
The comment says that the caller must hold the dev->event_lock spinlock, so let's enforce this.
A quick audit over all driver shows that except for the one place in i915 which motivated this all callers fullfill this requirement already.
Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in send_vblank_event() as well then.
Meh, I've missed that one, that's actually better I think. I'll drop my patch here.
I thought assert_spin_lock was the preferred form? -Chris
On 09/12/2014 12:04 PM, Chris Wilson wrote:
On Fri, Sep 12, 2014 at 05:34:56PM +0200, Daniel Vetter wrote:
On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote:
On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote:
The comment says that the caller must hold the dev->event_lock spinlock, so let's enforce this.
A quick audit over all driver shows that except for the one place in i915 which motivated this all callers fullfill this requirement already.
Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in send_vblank_event() as well then.
Meh, I've missed that one, that's actually better I think. I'll drop my patch here.
I thought assert_spin_lock was the preferred form?
Actually, lockdep_assert_held() is the preferred form.
See https://lkml.org/lkml/2014/9/3/171
Regards, Peter Hurley
On Fri, Sep 12, 2014 at 01:03:51PM -0400, Peter Hurley wrote:
On 09/12/2014 12:04 PM, Chris Wilson wrote:
On Fri, Sep 12, 2014 at 05:34:56PM +0200, Daniel Vetter wrote:
On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote:
On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote:
The comment says that the caller must hold the dev->event_lock spinlock, so let's enforce this.
A quick audit over all driver shows that except for the one place in i915 which motivated this all callers fullfill this requirement already.
Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in send_vblank_event() as well then.
Meh, I've missed that one, that's actually better I think. I'll drop my patch here.
I thought assert_spin_lock was the preferred form?
Actually, lockdep_assert_held() is the preferred form.
Which unfortunately doesn't warn for all the normal users which are not insane enough to enable lockdep and so is totally useless to validate a driver that runs on metric piles of different chips (with a resulting combinatorial explosion of code-paths because hw designers are creative). And we rely a lot on random drive-by testers to report such issues. -Daniel
On 09/12/2014 01:25 PM, Daniel Vetter wrote:
On Fri, Sep 12, 2014 at 01:03:51PM -0400, Peter Hurley wrote:
On 09/12/2014 12:04 PM, Chris Wilson wrote:
On Fri, Sep 12, 2014 at 05:34:56PM +0200, Daniel Vetter wrote:
On Fri, Sep 12, 2014 at 04:23:29PM +0100, Chris Wilson wrote:
On Fri, Sep 12, 2014 at 03:40:56PM +0200, Daniel Vetter wrote:
The comment says that the caller must hold the dev->event_lock spinlock, so let's enforce this.
A quick audit over all driver shows that except for the one place in i915 which motivated this all callers fullfill this requirement already.
Replace the rogue WARN_ON_SMP(!spin_is_locked(&dev->event_lock)) in send_vblank_event() as well then.
Meh, I've missed that one, that's actually better I think. I'll drop my patch here.
I thought assert_spin_lock was the preferred form?
Actually, lockdep_assert_held() is the preferred form.
Which unfortunately doesn't warn for all the normal users which are not insane enough to enable lockdep and so is totally useless to validate a driver that runs on metric piles of different chips (with a resulting combinatorial explosion of code-paths because hw designers are creative). And we rely a lot on random drive-by testers to report such issues.
I know. When I wrote [in that thread linked above]:
On Wed, Sep 03, 2014 at 10:50:01AM -0400, Peter Hurley wrote:
So a lockdep-only assert is unlikely to draw attention to existing bugs, especially in established drivers.
here's the replies I got:
Peter Zijlstra peterz@infradead.org wrote:
By the same logic lockdep will not find locking errors in established drivers.
and
On 09/04/2014 01:14 AM, Ingo Molnar wrote:
Indeed, this patch is ill-advised in several ways:
it extends an API variant that we want to phase
emits a warning even if say lockdep has already emitted a warning and locking state is not guaranteed to be consistent.
makes the kernel more expensive once fully debugged, in that non-fatal checks are unconditional.
:/
Regards, Peter Hurley
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Daniel Vetter -
Peter Hurley