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

Dave Gordon
2015-04-17T19:22:14Z

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);

.Dave.