Re: [PATCH 09/18] drm/i915: Simplify request_alloc by returning the allocated request

Joonas Lahtinen
2016-07-21T13:08:19Z

On ke, 2016-07-20 at 14:12 +0100, Chris Wilson wrote:
> If is simpler and leads to more readable code through the callstack if
> the allocation returns the allocated struct through the return value.
>
> The importance of this is that it no longer looks like we accidentally
> allocate requests as side-effect of calling certain functions.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  3 +-
>  drivers/gpu/drm/i915/i915_gem.c            | 75 ++++++++----------------------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++---
>  drivers/gpu/drm/i915/i915_gem_request.c    | 58 ++++++++---------------
>  drivers/gpu/drm/i915/i915_trace.h          | 13 +++---
>  drivers/gpu/drm/i915/intel_display.c       | 36 ++++++--------
>  drivers/gpu/drm/i915/intel_lrc.c           |  2 +-
>  drivers/gpu/drm/i915/intel_overlay.c       | 20 ++++----
>  8 files changed, 79 insertions(+), 140 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f32ec6db5bfa..3f67431577e3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3168,8 +3168,7 @@ static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object *obj)
>
>  int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
>  int i915_gem_object_sync(struct drm_i915_gem_object *obj,
> -  struct intel_engine_cs *to,
> -  struct drm_i915_gem_request **to_req);
> +  struct drm_i915_gem_request *to);
>  void i915_vma_move_to_active(struct i915_vma *vma,
>        struct drm_i915_gem_request *req);
>  int i915_gem_dumb_create(struct drm_file *file_priv,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 95dbcfd94a80..77d7c0b012f4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2842,51 +2842,35 @@ out:
>
>  static int
>  __i915_gem_object_sync(struct drm_i915_gem_object *obj,
> -        struct intel_engine_cs *to,
> -        struct drm_i915_gem_request *from_req,
> -        struct drm_i915_gem_request **to_req)
> +        struct drm_i915_gem_request *to,
> +        struct drm_i915_gem_request *from)
>  {
> - struct intel_engine_cs *from;
>   int ret;
>
> - from = i915_gem_request_get_engine(from_req);
> - if (to == from)
> + if (to->engine == from->engine)
>   return 0;
>
> - if (i915_gem_request_completed(from_req))
> + if (i915_gem_request_completed(from))
>   return 0;
>
>   if (!i915.semaphores) {
> - struct drm_i915_private *i915 = to_i915(obj->base.dev);
> - ret = __i915_wait_request(from_req,
> -   i915->mm.interruptible,
> + ret = __i915_wait_request(from,
> +   from->i915->mm.interruptible,
>     NULL,
>     NO_WAITBOOST);
>   if (ret)
>   return ret;
>
> - i915_gem_object_retire_request(obj, from_req);
> + i915_gem_object_retire_request(obj, from);
>   } else {
> - int idx = intel_engine_sync_index(from, to);
> - u32 seqno = i915_gem_request_get_seqno(from_req);
> + int idx = intel_engine_sync_index(from->engine, to->engine);
> + u32 seqno = i915_gem_request_get_seqno(from);
>
> - WARN_ON(!to_req);
> -
> - if (seqno <= from->semaphore.sync_seqno[idx])
> + if (seqno <= from->engine->semaphore.sync_seqno[idx])
>   return 0;
>
> - if (*to_req == NULL) {
> - struct drm_i915_gem_request *req;
> -
> - req = i915_gem_request_alloc(to, NULL);
> - if (IS_ERR(req))
> - return PTR_ERR(req);
> -
> - *to_req = req;
> - }
> -
> - trace_i915_gem_ring_sync_to(*to_req, from, from_req);
> - ret = to->semaphore.sync_to(*to_req, from, seqno);
> + trace_i915_gem_ring_sync_to(to, from);

Will somebody go nuts for changing the tracing just like so? I remember
somebody treating it somewhat of an ABI.

> + ret = to->engine->semaphore.sync_to(to, from->engine, seqno);
>   if (ret)
>   return ret;
>
> @@ -2894,8 +2878,8 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
>    * might have just caused seqno wrap under
>    * the radar.
>    */
> - from->semaphore.sync_seqno[idx] =
> - i915_gem_request_get_seqno(obj->last_read_req[from->id]);
> + from->engine->semaphore.sync_seqno[idx] =
> + i915_gem_request_get_seqno(obj->last_read_req[from->engine->id]);
>   }
>
>   return 0;
> @@ -2905,17 +2889,12 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   * i915_gem_object_sync - sync an object to a ring.
>   *
>   * @obj: object which may be in use on another ring.
> - * @to: ring we wish to use the object on. May be NULL.
> - * @to_req: request we wish to use the object for. See below.
> - *          This will be allocated and returned if a request is
> - *          required but not passed in.
> + * @to: request we are wishing to use
>   *
>   * This code is meant to abstract object synchronization with the GPU.
> - * Calling with NULL implies synchronizing the object with the CPU
> - * rather than a particular GPU ring. Conceptually we serialise writes
> - * between engines inside the GPU. We only allow one engine to write
> - * into a buffer at any time, but multiple readers. To ensure each has
> - * a coherent view of memory, we must:
> + * Conceptually we serialise writes between engines inside the GPU.
> + * We only allow one engine to write into a buffer at any time, but
> + * multiple readers. To ensure each has a coherent view of memory, we must:
>   *
>   * - If there is an outstanding write request to the object, the new
>   *   request must wait for it to complete (either CPU or in hw, requests
> @@ -2924,22 +2903,11 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   * - If we are a write request (pending_write_domain is set), the new
>   *   request must wait for outstanding read requests to complete.
>   *
> - * For CPU synchronisation (NULL to) no request is required. For syncing with
> - * rings to_req must be non-NULL. However, a request does not have to be
> - * pre-allocated. If *to_req is NULL and sync commands will be emitted then a
> - * request will be allocated automatically and returned through *to_req. Note
> - * that it is not guaranteed that commands will be emitted (because the system
> - * might already be idle). Hence there is no need to create a request that
> - * might never have any work submitted. Note further that if a request is
> - * returned in *to_req, it is the responsibility of the caller to submit
> - * that request (after potentially adding more work to it).
> - *
>   * Returns 0 if successful, else propagates up the lower layer error.
>   */
>  int
>  i915_gem_object_sync(struct drm_i915_gem_object *obj,
> -      struct intel_engine_cs *to,
> -      struct drm_i915_gem_request **to_req)
> +      struct drm_i915_gem_request *to)
>  {
>   const bool readonly = obj->base.pending_write_domain == 0;
>   struct drm_i915_gem_request *req[I915_NUM_ENGINES];
> @@ -2948,9 +2916,6 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   if (!obj->active)
>   return 0;
>
> - if (to == NULL)
> - return i915_gem_object_wait_rendering(obj, readonly);
> -
>   n = 0;
>   if (readonly) {
>   if (obj->last_write_req)
> @@ -2961,7 +2926,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   req[n++] = obj->last_read_req[i];
>   }
>   for (i = 0; i < n; i++) {
> - ret = __i915_gem_object_sync(obj, to, req[i], to_req);
> + ret = __i915_gem_object_sync(obj, to, req[i]);
>   if (ret)
>   return ret;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 2a4841256f8e..5cea95c6f98b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -981,7 +981,7 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
>   struct drm_i915_gem_object *obj = vma->obj;
>
>   if (obj->active & other_rings) {
> - ret = i915_gem_object_sync(obj, req->engine, &req);
> + ret = i915_gem_object_sync(obj, req);
>   if (ret)
>   return ret;
>   }
> @@ -1426,7 +1426,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  {
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct i915_ggtt *ggtt = &dev_priv->ggtt;
> - struct drm_i915_gem_request *req = NULL;
>   struct eb_vmas *eb;
>   struct drm_i915_gem_object *batch_obj;
>   struct drm_i915_gem_exec_object2 shadow_exec_entry;
> @@ -1614,13 +1613,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>   params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm);
>
>   /* Allocate a request for this batch buffer nice and early. */
> - req = i915_gem_request_alloc(engine, ctx);
> - if (IS_ERR(req)) {
> - ret = PTR_ERR(req);
> + params->request = i915_gem_request_alloc(engine, ctx);
> + if (IS_ERR(params->request)) {
> + ret = PTR_ERR(params->request);
>   goto err_batch_unpin;
>   }
>
> - ret = i915_gem_request_add_to_client(req, file);
> + ret = i915_gem_request_add_to_client(params->request, file);
>   if (ret)
>   goto err_request;
>
> @@ -1636,7 +1635,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>   params->dispatch_flags          = dispatch_flags;
>   params->batch_obj               = batch_obj;
>   params->ctx                     = ctx;
> - params->request                 = req;

Not sure why you especially want to always use params->request form?

>
>   ret = dev_priv->gt.execbuf_submit(params, args, &eb->vmas);
>  err_request:
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 3a566abf5219..2153b4fe4a1f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -292,10 +292,21 @@ static int i915_gem_get_seqno(struct drm_i915_private *dev_priv, u32 *seqno)
>   return 0;
>  }
>
> -static inline int
> -__i915_gem_request_alloc(struct intel_engine_cs *engine,
> -  struct i915_gem_context *ctx,
> -  struct drm_i915_gem_request **req_out)
> +/**
> + * i915_gem_request_alloc - allocate a request structure
> + *
> + * @engine: engine that we wish to issue the request on.
> + * @ctx: context that the request will be associated with.
> + *       This can be NULL if the request is not directly related to
> + *       any specific user context, in which case this function will
> + *       choose an appropriate context to use.
> + *
> + * Returns a pointer to the allocated request if successful,
> + * or an error code if not.
> + */
> +struct drm_i915_gem_request *
> +i915_gem_request_alloc(struct intel_engine_cs *engine,
> +        struct i915_gem_context *ctx)
>  {
>   struct drm_i915_private *dev_priv = engine->i915;
>   unsigned int reset_counter = i915_reset_counter(&dev_priv->gpu_error);
> @@ -303,18 +314,13 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
>   u32 seqno;
>   int ret;
>
> - if (!req_out)
> - return -EINVAL;
> -
> - *req_out = NULL;
> -
>   /* ABI: Before userspace accesses the GPU (e.g. execbuffer), report
>    * EIO if the GPU is already wedged, or EAGAIN to drop the struct_mutex
>    * and restart.
>    */
>   ret = i915_gem_check_wedge(reset_counter, dev_priv->mm.interruptible);
>   if (ret)
> - return ret;
> + return ERR_PTR(ret);
>
>   /* Move the oldest request to the slab-cache (if not in use!) */
>   if (!list_empty(&engine->request_list)) {
> @@ -326,7 +332,7 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
>
>   req = kmem_cache_zalloc(dev_priv->requests, GFP_KERNEL);
>   if (!req)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
>   ret = i915_gem_get_seqno(dev_priv, &seqno);
>   if (ret)
> @@ -359,39 +365,13 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
>   if (ret)
>   goto err_ctx;
>
> - *req_out = req;
> - return 0;
> + return req;
>
>  err_ctx:
>   i915_gem_context_put(ctx);
>  err:
>   kmem_cache_free(dev_priv->requests, req);
> - return ret;
> -}
> -
> -/**
> - * i915_gem_request_alloc - allocate a request structure
> - *
> - * @engine: engine that we wish to issue the request on.
> - * @ctx: context that the request will be associated with.
> - *       This can be NULL if the request is not directly related to
> - *       any specific user context, in which case this function will
> - *       choose an appropriate context to use.
> - *
> - * Returns a pointer to the allocated request if successful,
> - * or an error code if not.
> - */
> -struct drm_i915_gem_request *
> -i915_gem_request_alloc(struct intel_engine_cs *engine,
> -        struct i915_gem_context *ctx)
> -{
> - struct drm_i915_gem_request *req;
> - int err;
> -
> - if (!ctx)
> - ctx = engine->i915->kernel_context;
> - err = __i915_gem_request_alloc(engine, ctx, &req);
> - return err ? ERR_PTR(err) : req;
> + return ERR_PTR(ret);
>  }
>
>  static void i915_gem_mark_busy(const struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index 007112d1e049..9e43c0aa6e3b 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -449,10 +449,9 @@ TRACE_EVENT(i915_gem_evict_vm,
>  );
>
>  TRACE_EVENT(i915_gem_ring_sync_to,
> -     TP_PROTO(struct drm_i915_gem_request *to_req,
> -      struct intel_engine_cs *from,
> -      struct drm_i915_gem_request *req),
> -     TP_ARGS(to_req, from, req),
> +     TP_PROTO(struct drm_i915_gem_request *to,
> +      struct drm_i915_gem_request *from),
> +     TP_ARGS(to, from),
>
>       TP_STRUCT__entry(
>        __field(u32, dev)
> @@ -463,9 +462,9 @@ TRACE_EVENT(i915_gem_ring_sync_to,
>
>       TP_fast_assign(
>      __entry->dev = from->i915->drm.primary->index;
> -    __entry->sync_from = from->id;
> -    __entry->sync_to = to_req->engine->id;
> -    __entry->seqno = req->fence.seqno;
> +    __entry->sync_from = from->engine->id;
> +    __entry->sync_to = to->engine->id;
> +    __entry->seqno = from->fence.seqno;
>      ),
>
>       TP_printk("dev=%u, sync-from=%u, sync-to=%u, seqno=%u",
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bff172c45ff7..5d4420b67642 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11583,7 +11583,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>   struct intel_flip_work *work;
>   struct intel_engine_cs *engine;
>   bool mmio_flip;
> - struct drm_i915_gem_request *request = NULL;
> + struct drm_i915_gem_request *request;
>   int ret;
>
>   /*
> @@ -11690,22 +11690,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
>   mmio_flip = use_mmio_flip(engine, obj);
>
> - /* When using CS flips, we want to emit semaphores between rings.
> -  * However, when using mmio flips we will create a task to do the
> -  * synchronisation, so all we want here is to pin the framebuffer
> -  * into the display plane and skip any waits.
> -  */
> - if (!mmio_flip) {
> - ret = i915_gem_object_sync(obj, engine, &request);
> - if (!ret && !request) {
> - request = i915_gem_request_alloc(engine, NULL);
> - ret = PTR_ERR_OR_ZERO(request);
> - }
> -
> - if (ret)
> - goto cleanup_pending;
> - }
> -
>   ret = intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
>   if (ret)
>   goto cleanup_pending;
> @@ -11723,14 +11707,24 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
>   schedule_work(&work->mmio_work);
>   } else {
> - i915_gem_request_assign(&work->flip_queued_req, request);
> + request = i915_gem_request_alloc(engine, engine->last_context);
> + if (IS_ERR(request)) {
> + ret = PTR_ERR(request);
> + goto cleanup_unpin;
> + }
> +
> + ret = i915_gem_object_sync(obj, request);
> + if (ret)
> + goto cleanup_request;
> +
>   ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request,
>      page_flip_flags);
>   if (ret)
> - goto cleanup_unpin;
> + goto cleanup_request;
>
>   intel_mark_page_flip_active(intel_crtc, work);
>
> + work->flip_queued_req = i915_gem_request_get(request);

If I understood it correctly, result should be equivalent, no
functional changes.

Regards, Joonas

>   i915_add_request_no_flush(request);
>   }
>
> @@ -11745,11 +11739,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
>   return 0;
>
> +cleanup_request:
> + i915_add_request_no_flush(request);
>  cleanup_unpin:
>   intel_unpin_fb_obj(fb, crtc->primary->state->rotation);
>  cleanup_pending:
> - if (!IS_ERR_OR_NULL(request))
> - i915_add_request_no_flush(request);
>   atomic_dec(&intel_crtc->unpin_work_count);
>   mutex_unlock(&dev->struct_mutex);
>  cleanup:
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 3158a1a38644..6cd0e24ed50c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -655,7 +655,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
>   struct drm_i915_gem_object *obj = vma->obj;
>
>   if (obj->active & other_rings) {
> - ret = i915_gem_object_sync(obj, req->engine, &req);
> + ret = i915_gem_object_sync(obj, req);
>   if (ret)
>   return ret;
>   }
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index a5071e281088..356a1f6f95aa 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -229,11 +229,18 @@ static int intel_overlay_do_wait_request(struct intel_overlay *overlay,
>   return 0;
>  }
>
> +static struct drm_i915_gem_request *alloc_request(struct intel_overlay *overlay)
> +{
> + struct drm_i915_private *dev_priv = overlay->i915;
> + struct intel_engine_cs *engine = &dev_priv->engine[RCS];
> +
> + return i915_gem_request_alloc(engine, dev_priv->kernel_context);
> +}
> +
>  /* overlay needs to be disable in OCMD reg */
>  static int intel_overlay_on(struct intel_overlay *overlay)
>  {
>   struct drm_i915_private *dev_priv = overlay->i915;
> - struct intel_engine_cs *engine = &dev_priv->engine[RCS];
>   struct drm_i915_gem_request *req;
>   struct intel_ring *ring;
>   int ret;
> @@ -241,7 +248,7 @@ static int intel_overlay_on(struct intel_overlay *overlay)
>   WARN_ON(overlay->active);
>   WARN_ON(IS_I830(dev_priv) && !(dev_priv->quirks & QUIRK_PIPEA_FORCE));
>
> - req = i915_gem_request_alloc(engine, NULL);
> + req = alloc_request(overlay);
>   if (IS_ERR(req))
>   return PTR_ERR(req);
>
> @@ -268,7 +275,6 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
>     bool load_polyphase_filter)
>  {
>   struct drm_i915_private *dev_priv = overlay->i915;
> - struct intel_engine_cs *engine = &dev_priv->engine[RCS];
>   struct drm_i915_gem_request *req;
>   struct intel_ring *ring;
>   u32 flip_addr = overlay->flip_addr;
> @@ -285,7 +291,7 @@ static int intel_overlay_continue(struct intel_overlay *overlay,
>   if (tmp & (1 << 17))
>   DRM_DEBUG("overlay underrun, DOVSTA: %x\n", tmp);
>
> - req = i915_gem_request_alloc(engine, NULL);
> + req = alloc_request(overlay);
>   if (IS_ERR(req))
>   return PTR_ERR(req);
>
> @@ -338,7 +344,6 @@ static void intel_overlay_off_tail(struct intel_overlay *overlay)
>  static int intel_overlay_off(struct intel_overlay *overlay)
>  {
>   struct drm_i915_private *dev_priv = overlay->i915;
> - struct intel_engine_cs *engine = &dev_priv->engine[RCS];
>   struct drm_i915_gem_request *req;
>   struct intel_ring *ring;
>   u32 flip_addr = overlay->flip_addr;
> @@ -352,7 +357,7 @@ static int intel_overlay_off(struct intel_overlay *overlay)
>    * of the hw. Do it in both cases */
>   flip_addr |= OFC_UPDATE;
>
> - req = i915_gem_request_alloc(engine, NULL);
> + req = alloc_request(overlay);
>   if (IS_ERR(req))
>   return PTR_ERR(req);
>
> @@ -412,7 +417,6 @@ static int intel_overlay_recover_from_interrupt(struct intel_overlay *overlay)
>  static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
>  {
>   struct drm_i915_private *dev_priv = overlay->i915;
> - struct intel_engine_cs *engine = &dev_priv->engine[RCS];
>   int ret;
>
>   lockdep_assert_held(&dev_priv->drm.struct_mutex);
> @@ -428,7 +432,7 @@ static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
>   struct drm_i915_gem_request *req;
>   struct intel_ring *ring;
>
> - req = i915_gem_request_alloc(engine, NULL);
> + req = alloc_request(overlay);
>   if (IS_ERR(req))
>   return PTR_ERR(req);
>
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation