Re: [RFC, 1/4] drm/i915: Convert requests to use struct fence

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@Intel.com:
>>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>>
>>>>> 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@Intel.com>
>>>>>
>>>>> ---
>>>>> 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx