Re: [PATCH 045/190] drm/i915: Move releasing of the GEM request from free to retire/cancel

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 08/03/16 13:15, Tvrtko Ursulin wrote:
> On 11/01/16 09:16, Chris Wilson wrote:
> > If we move the release of the GEM request (i.e. decoupling it from the
> > various lists used for client and context tracking) after it is complete
> > (either by the GPU retiring the request, or by the caller cancelling the
> > request), we can remove the requirement that the final unreference of
> > the GEM request need to be under the struct_mutex.
> >
> > v2: Execlists as always is badly asymetric and year old patches still
> > haven't landed to fix it up.
>
> Looks good and pretty standalone to me (depends on i915_gem_request code
> movement only I think). Just one question below.
>
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c          |  4 +--
> >   drivers/gpu/drm/i915/i915_gem_request.c  | 50
> > ++++++++++++++------------------
> >   drivers/gpu/drm/i915/i915_gem_request.h  | 14 ---------
> >   drivers/gpu/drm/i915/intel_breadcrumbs.c |  2 +-
> >   drivers/gpu/drm/i915/intel_display.c     |  2 +-
> >   drivers/gpu/drm/i915/intel_lrc.c         |  6 ++--
> >   drivers/gpu/drm/i915/intel_pm.c          |  2 +-
> >   7 files changed, 30 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 68a25617ca7a..6d8d65304abf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2502,7 +2502,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void
> > *data, struct drm_file *file)
> >               ret = __i915_wait_request(req[i], true,
> >                             args->timeout_ns > 0 ? &args->timeout_ns :
> > NULL,
> >                             to_rps_client(file));
> > -        i915_gem_request_unreference__unlocked(req[i]);
> > +        i915_gem_request_unreference(req[i]);
> >       }
> >       return ret;
> >
> > @@ -3505,7 +3505,7 @@ i915_gem_ring_throttle(struct drm_device *dev,
> > struct drm_file *file)
> >           return 0;
> >
> >       ret = __i915_wait_request(target, true, NULL, NULL);
> > -    i915_gem_request_unreference__unlocked(target);
> > +    i915_gem_request_unreference(target);
> >
> >       return ret;
> >   }
> > diff --git a/drivers/gpu/drm/i915/i915_gem_request.c
> > b/drivers/gpu/drm/i915/i915_gem_request.c
> > index b4ede6dd7b20..1c4f4d83a3c2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_request.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> > @@ -184,13 +184,6 @@ err:
> >       return ret;
> >   }
> >
> > -void i915_gem_request_cancel(struct drm_i915_gem_request *req)
> > -{
> > -    intel_ring_reserved_space_cancel(req->ringbuf);
> > -
> > -    i915_gem_request_unreference(req);
> > -}
> > -
> >   int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> >                      struct drm_file *file)
> >   {
> > @@ -235,9 +228,28 @@ i915_gem_request_remove_from_client(struct
> > drm_i915_gem_request *request)
> >       request->pid = NULL;
> >   }
> >
> > +static void __i915_gem_request_release(struct drm_i915_gem_request
> > *request)
> > +{
> > +    i915_gem_request_remove_from_client(request);
> > +
> > +    i915_gem_context_unreference(request->ctx);
> > +    i915_gem_request_unreference(request);
> > +}
> > +
> > +void i915_gem_request_cancel(struct drm_i915_gem_request *req)
> > +{
> > +    intel_ring_reserved_space_cancel(req->ringbuf);
> > +    if (i915.enable_execlists) {
> > +        if (req->ctx != req->ring->default_context)
> > +            intel_lr_context_unpin(req);
> > +    }
> > +    __i915_gem_request_release(req);
> > +}
> > +
> >   static void i915_gem_request_retire(struct drm_i915_gem_request
> > *request)
> >   {
> >       trace_i915_gem_request_retire(request);
> > +    list_del_init(&request->list);
> >
> >       /* We know the GPU must have read the request to have
> >        * sent us the seqno + interrupt, so use the position
> > @@ -248,11 +260,7 @@ static void i915_gem_request_retire(struct
> > drm_i915_gem_request *request)
> >        * completion order.
> >        */
> >       request->ringbuf->last_retired_head = request->postfix;
> > -
> > -    list_del_init(&request->list);
> > -    i915_gem_request_remove_from_client(request);
> > -
> > -    i915_gem_request_unreference(request);
> > +    __i915_gem_request_release(request);
> >   }
> >
> >   void
> > @@ -639,21 +647,7 @@ i915_wait_request(struct drm_i915_gem_request *req)
> >
> >   void i915_gem_request_free(struct kref *req_ref)
> >   {
> > -    struct drm_i915_gem_request *req = container_of(req_ref,
> > -                         typeof(*req), ref);
> > -    struct intel_context *ctx = req->ctx;
> > -
> > -    if (req->file_priv)
> > -        i915_gem_request_remove_from_client(req);
> > -
> > -    if (ctx) {
> > -        if (i915.enable_execlists) {
> > -            if (ctx != req->ring->default_context)
> > -                intel_lr_context_unpin(req);
> > -        }
> > -
> > -        i915_gem_context_unreference(ctx);
> > -    }
> > -
> > +    struct drm_i915_gem_request *req =
> > +        container_of(req_ref, typeof(*req), ref);
> >       kmem_cache_free(req->i915->requests, req);
> >   }
> > diff --git a/drivers/gpu/drm/i915/i915_gem_request.h
> > b/drivers/gpu/drm/i915/i915_gem_request.h
> > index d46f22f30b0a..af1b825fce50 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_request.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> > @@ -154,23 +154,9 @@ 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);
> >   }
> >
> > -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);
> > -}
> > -
> >   static inline void i915_gem_request_assign(struct
> > drm_i915_gem_request **pdst,
> >                          struct drm_i915_gem_request *src)
> >   {
> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > index 0ea01bd6811c..f6731aac7fcf 100644
> > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > @@ -390,7 +390,7 @@ static int intel_breadcrumbs_signaler(void *arg)
> >                */
> >               intel_engine_remove_wait(engine, &signal->wait);
> >
> > -            i915_gem_request_unreference__unlocked(signal->request);
> > +            i915_gem_request_unreference(signal->request);
> >
> >               /* Find the next oldest signal. Note that as we have
> >                * not been holding the lock, another client may
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 57c54c9bc82b..32885b8d5c02 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11431,7 +11431,7 @@ static void intel_mmio_flip_work_func(struct
> > work_struct *work)
> >           WARN_ON(__i915_wait_request(mmio_flip->req,
> >                           false, NULL,
> >                           &mmio_flip->i915->rps.mmioflips));
> > -        i915_gem_request_unreference__unlocked(mmio_flip->req);
> > +        i915_gem_request_unreference(mmio_flip->req);
> >       }
> >
> >       /* For framebuffer backed by dmabuf, wait for fence */
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index b634e7d7a92b..7a3069a2beb2 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -587,9 +587,6 @@ static int execlists_context_queue(struct
> > drm_i915_gem_request *request)
> >       struct drm_i915_gem_request *cursor;
> >       int num_elements = 0;
> >
> > -    if (request->ctx != ring->default_context)
> > -        intel_lr_context_pin(request);
> > -
>
> Since you remove LRC pin from queue, the lifetime is now either:
>
> 1. From request create to cancel.
> 2. From request create to execlist retirement.
>
> Would it be more logical to leave the LRC pin in queue, but remove it
> from request creation instead? That would make the LRC pin lifetime only
> a single possibility, from queue to execlist retire.

I felt was so close in getting rid of execlist_retired_req_queue, using 
this patch as a starting point, when I realised this patch does not play 
nicely with the GuC. Back to the drawing board. :(

Regards,

Tvrtko

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