Op 17-04-15 om 21:22 schreef Dave Gordon:
> On 07/04/15 12:18, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 07-04-15 om 12:59 schreef John Harrison:
>>> On 07/04/2015 10:18, Maarten Lankhorst wrote:
>>>> Hey,
>>>>
>>>> Op 20-03-15 om 18:48 schreef John.C.Harrison@xxxxxxxxx:
>>>>> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>>>>>
>>>>> There is a construct in the linux kernel called 'struct fence' that is intended
>>>>> to keep track of work that is executed on hardware. I.e. it solves the basic
>>>>> problem that the drivers 'struct drm_i915_gem_request' is trying to address. The
>>>>> request structure does quite a lot more than simply track the execution progress
>>>>> so is very definitely still required. However, the basic completion status side
>>>>> could be updated to use the ready made fence implementation and gain all the
>>>>> advantages that provides.
>>>>>
>>>>> This patch makes the first step of integrating a struct fence into the request.
>>>>> It replaces the explicit reference count with that of the fence. It also
>>>>> replaces the 'is completed' test with the fence's equivalent. Currently, that
>>>>> simply chains on to the original request implementation. A future patch will
>>>>> improve this.
>>>>>
>>>>> For: VIZ-5190
>>>>> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
>>>>>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_drv.h | 37 +++++++++------------
>>>>> drivers/gpu/drm/i915/i915_gem.c | 55 ++++++++++++++++++++++++++++---
>>>>> drivers/gpu/drm/i915/intel_lrc.c | 1 +
>>>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
>>>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++
>>>>> 5 files changed, 70 insertions(+), 27 deletions(-)
> Since Maarten provided i915_gem_request_unreference__unlocked() in the
> interval since this was first posted, you'll need to convert that too:
>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 4213dfb..97073fe 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2218,14 +2218,8 @@ void i915_gem_request_unreference_irq(struct drm_i915_gem_request *req);
>> static inline void
>> i915_gem_request_unreference__unlocked(struct drm_i915_gem_request *req)
>> {
>> - struct drm_device *dev;
>> -
>> - if (!req)
>> - return;
>> -
>> - dev = req->ring->dev;
>> - if (kref_put_mutex(&req->ref, i915_gem_request_free, &dev->struct_mutex))
>> - mutex_unlock(&dev->struct_mutex);
>> + if (req)
>> + fence_put_mutex(&req->fence, &req->ring->dev->struct_mutex);
>> }
>> static inline void i915_gem_request_assign(struct drm_i915_gem_request **pdst,
> ... and here's the implementation of fence_put_mutex() ...
>
>> diff --git a/include/linux/fence.h b/include/linux/fence.h
>> index 39efee1..c4fbb25 100644
>> --- a/include/linux/fence.h
>> +++ b/include/linux/fence.h
>> @@ -218,6 +218,24 @@ static inline void fence_put(struct fence *fence)
>> kref_put(&fence->refcount, fence_release);
>> }
>>
>> +/**
>> + * fence_put_mutex - decrement refcount for object.
>> + * @fence: object.
>> + * @lock: lock to take in release case
>> + *
>> + * A version of fence_put() that doesn't require the caller to hold the
>> + * associated lock already. If the refcount doesn't go to zero, the lock
>> + * is not needed; if it does, the lock will automatically be acquired and
>> + * released around the call to fence_release().
>> + */
>> +static inline void fence_put_mutex(struct fence *fence,
>> + struct mutex *lock)
>> +{
>> + if (fence)
>> + if (kref_put_mutex(&fence->refcount, fence_release, lock))
>> + mutex_unlock(lock);
>> +}
>> +
>> int fence_signal(struct fence *fence);
>> int fence_signal_locked(struct fence *fence);
>> signed long fence_default_wait(struct fence *fence, bool intr, signed long timeout);
>
I think this is wrong, fence can't assume a lock is held by the caller of fence_put, because the caller might call it from a different driver
without knowing what lock to take.
It's fine to have something like this internally for now with a big TODO that it has to go for cross-device sync, but fence_put_mutex
is broken.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
