[PATCH] drm/i915: Check locking in i915_gem_request_unreference

[PATCH] drm/i915: Check locking in i915_gem_request_unreference

[Daniel Vetter]

With refcounting it looks like you can just drop that refcount, but
that's not really the case. So make sure no one forgets.

Motivated by the unlocked call in the mmio flip code.

Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Thomas Daniel <Thomas.Daniel@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a2a1569dcee..e52c3d8d120c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2046,6 +2046,7 @@ i915_gem_request_reference(struct drm_i915_gem_request *req)
 static inline void
 i915_gem_request_unreference(struct drm_i915_gem_request *req)
 {
+	WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
 	kref_put(&req->ref, i915_gem_request_free);
 }
 
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check locking in i915_gem_request_unreference

[Chris Wilson]

On Wed, Nov 26, 2014 at 10:28:13AM +0100, Daniel Vetter wrote:
> With refcounting it looks like you can just drop that refcount, but
> that's not really the case. So make sure no one forgets.
> 
> Motivated by the unlocked call in the mmio flip code.

I had an unlocked variant for exactly this purpose (and a few others
where we do not want to take the lock again) and so also had the WARN
inside i915_request_free.

Drop the _gem_, the requests are lower level than GEM itself.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check locking in i915_gem_request_unreference

[John Harrison]

On 26/11/2014 10:39, Chris Wilson wrote:
> On Wed, Nov 26, 2014 at 10:28:13AM +0100, Daniel Vetter wrote:
>> With refcounting it looks like you can just drop that refcount, but
>> that's not really the case. So make sure no one forgets.
>>
>> Motivated by the unlocked call in the mmio flip code.
> I had an unlocked variant for exactly this purpose (and a few others
> where we do not want to take the lock again) and so also had the WARN
> inside i915_request_free.
>
> Drop the _gem_, the requests are lower level than GEM itself.
> -Chris
>

Daniel: Should I fold the WARN_ON patch into my patch series and repost?

Chris: Are you saying that you want an extra patch to rename 
'drm_i915_gem_request' to 'drm_i915_request' throughout the entire driver?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check locking in i915_gem_request_unreference

[Chris Wilson]

On Wed, Nov 26, 2014 at 12:41:43PM +0000, John Harrison wrote:
> On 26/11/2014 10:39, Chris Wilson wrote:
> >On Wed, Nov 26, 2014 at 10:28:13AM +0100, Daniel Vetter wrote:
> >>With refcounting it looks like you can just drop that refcount, but
> >>that's not really the case. So make sure no one forgets.
> >>
> >>Motivated by the unlocked call in the mmio flip code.
> >I had an unlocked variant for exactly this purpose (and a few others
> >where we do not want to take the lock again) and so also had the WARN
> >inside i915_request_free.
> >
> >Drop the _gem_, the requests are lower level than GEM itself.
> >-Chris
> >
> 
> Daniel: Should I fold the WARN_ON patch into my patch series and repost?
> 
> Chris: Are you saying that you want an extra patch to rename
> 'drm_i915_gem_request' to 'drm_i915_request' throughout the entire
> driver?

Not even struct drm_i915_request, just struct i915_request and friends.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check locking in i915_gem_request_unreference

[Daniel Vetter]

On Wed, Nov 26, 2014 at 1:46 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> Daniel: Should I fold the WARN_ON patch into my patch series and repost?

Nope I'll wrestle while applying.

>> Chris: Are you saying that you want an extra patch to rename
>> 'drm_i915_gem_request' to 'drm_i915_request' throughout the entire
>> driver?
>
> Not even struct drm_i915_request, just struct i915_request and friends.

Imo let's settle things first a bit, rename fests can be easily done
with cocci. But yeah dropping the drm_ and gem_ prefixes sounds like a
good idea for all the request handling. We want to reuse requests for
SVM direct submission, which is totally not going to look like gem at
all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check locking in i915_gem_request_unreference

[Daniel Vetter]

On Wed, Nov 26, 2014 at 11:39 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Nov 26, 2014 at 10:28:13AM +0100, Daniel Vetter wrote:
>> With refcounting it looks like you can just drop that refcount, but
>> that's not really the case. So make sure no one forgets.
>>
>> Motivated by the unlocked call in the mmio flip code.
>
> I had an unlocked variant for exactly this purpose (and a few others
> where we do not want to take the lock again) and so also had the WARN
> inside i915_request_free.
>
> Drop the _gem_, the requests are lower level than GEM itself.

Yeah there's definitely more to do. The one I actually expect you to
scream about is that it keeps an explicit refcount for all the
obj->las_* fences, instead of relying upon the implicit fence that a
correct retire sequence allows us.

This here is just about enforcing correctness.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx