Re: [PATCH 28/59] drm/i915: Update i915_gem_object_sync() to take a request structure

[Tomas Elf <tomas.elf@xxxxxxxxx>]

On 19/03/2015 12:30, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> The plan is to pass requests around as the basic submission tracking structure
> rather than rings and contexts. This patch updates the i915_gem_object_sync()
> code path.
>
> v2: Much more complex patch to share a single request between the sync and the
> page flip. The _sync() function now supports lazy allocation of the request
> structure. That is, if one is passed in then that will be used. If one is not,
> then a request will be allocated and passed back out. Note that the _sync() code
> does not necessarily require a request. Thus one will only be created until
> certain situations. The reason the lazy allocation must be done within the
> _sync() code itself is because the decision to need one or not is not really
> something that code above can second guess (except in the case where one is
> definitely not required because no ring is passed in).
>
> The call chains above _sync() now support passing a request through which most
> callers passing in NULL and assuming that no request will be required (because
> they also pass in NULL for the ring and therefore can't be generating any ring
> code).
>
> The exeception is intel_crtc_page_flip() which now supports having a request
> returned from _sync(). If one is, then that request is shared by the page flip
> (if the page flip is of a type to need a request). If _sync() does not generate
> a request but the page flip does need one, then the page flip path will create
> it's own request.

Nitpick: "it's own request" -> "its own request"

> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_drv.h            |    6 ++++--
>   drivers/gpu/drm/i915/i915_gem.c            |   19 ++++++++++++++++---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
>   drivers/gpu/drm/i915/intel_display.c       |   17 ++++++++++++-----
>   drivers/gpu/drm/i915/intel_drv.h           |    3 ++-
>   drivers/gpu/drm/i915/intel_fbdev.c         |    2 +-
>   drivers/gpu/drm/i915/intel_lrc.c           |    2 +-
>   drivers/gpu/drm/i915/intel_overlay.c       |    2 +-
>   8 files changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ca070d3..55d82a7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2665,7 +2665,8 @@ static inline void i915_gem_object_unpin_pages(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 intel_engine_cs *to,
> +			 struct drm_i915_gem_request **to_req);
>   void i915_vma_move_to_active(struct i915_vma *vma,
>   			     struct intel_engine_cs *ring);
>   int i915_gem_dumb_create(struct drm_file *file_priv,
> @@ -2773,7 +2774,8 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
>   int __must_check
>   i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>   				     u32 alignment,
> -				     struct intel_engine_cs *pipelined);
> +				     struct intel_engine_cs *pipelined,
> +				     struct drm_i915_gem_request **pipelined_request);
>   void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
>   int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>   				int align);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0a25461..9114071 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2957,6 +2957,9 @@ out:
>    *
>    * @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. May be NULL.

to_req may only be NULL if to is NULL. And to may only be NULL if to_req 
is NULL.

> + *          This will be allocated and returned if a request is
> + *          required but not passed in.

"This will be allocated and returned if a request is required but not 
passed in" sounds like to_req is NULL. However, what we mean here is 
that *to_req is NULL. And that can coincide with to not being NULL.
So together with the relationship between to_req and to described in my 
remark above this adds another level of complexity.

Could we perhaps formulate the full extent of the NULLability of these 
parameters in a consistent way instead of two arbitrary sentences? Like 
something along the lines of:

- to can be NULL if to_req is NULL
- to_req can be NULL if to is NULL
- If *to_req is NULL it may be allocated and returned.

Thanks,
Tomas

>    *
>    * This code is meant to abstract object synchronization with the GPU.
>    * Calling with NULL implies synchronizing the object with the CPU
> @@ -2966,7 +2969,8 @@ out:
>    */
>   int
>   i915_gem_object_sync(struct drm_i915_gem_object *obj,
> -		     struct intel_engine_cs *to)
> +		     struct intel_engine_cs *to,
> +		     struct drm_i915_gem_request **to_req)
>   {
>   	struct intel_engine_cs *from;
>   	u32 seqno;
> @@ -2980,6 +2984,8 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   	if (to == NULL || !i915_semaphore_is_enabled(obj->base.dev))
>   		return i915_gem_object_wait_rendering(obj, false);
>
> +	WARN_ON(!to_req);
> +
>   	idx = intel_ring_sync_index(from, to);
>
>   	seqno = i915_gem_request_get_seqno(obj->last_read_req);
> @@ -2988,6 +2994,12 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   	if (seqno <= from->semaphore.sync_seqno[idx])
>   		return 0;
>
> +	if (*to_req == NULL) {
> +		ret = i915_gem_request_alloc(to, to->default_context, to_req);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	ret = i915_gem_check_olr(obj->last_read_req);
>   	if (ret)
>   		return ret;
> @@ -3953,14 +3965,15 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
>   int
>   i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>   				     u32 alignment,
> -				     struct intel_engine_cs *pipelined)
> +				     struct intel_engine_cs *pipelined,
> +				     struct drm_i915_gem_request **pipelined_request)
>   {
>   	u32 old_read_domains, old_write_domain;
>   	bool was_pin_display;
>   	int ret;
>
>   	if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
> -		ret = i915_gem_object_sync(obj, pipelined);
> +		ret = i915_gem_object_sync(obj, pipelined, pipelined_request);
>   		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 eca88a6..1978f9c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -902,7 +902,7 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
>
>   	list_for_each_entry(vma, vmas, exec_list) {
>   		struct drm_i915_gem_object *obj = vma->obj;
> -		ret = i915_gem_object_sync(obj, req->ring);
> +		ret = i915_gem_object_sync(obj, req->ring, &req);
>   		if (ret)
>   			return ret;
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f1c0295..c71c523 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2245,7 +2245,8 @@ intel_fb_align_height(struct drm_device *dev, int height,
>   int
>   intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>   			   struct drm_framebuffer *fb,
> -			   struct intel_engine_cs *pipelined)
> +			   struct intel_engine_cs *pipelined,
> +			   struct drm_i915_gem_request **pipelined_request)
>   {
>   	struct drm_device *dev = fb->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2304,7 +2305,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>   	intel_runtime_pm_get(dev_priv);
>
>   	dev_priv->mm.interruptible = false;
> -	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
> +	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined, pipelined_request);
>   	if (ret)
>   		goto err_interruptible;
>
> @@ -9882,6 +9883,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>   	enum pipe pipe = intel_crtc->pipe;
>   	struct intel_unpin_work *work;
>   	struct intel_engine_cs *ring;
> +	struct drm_i915_gem_request *request = NULL;
>   	int ret;
>
>   	/*
> @@ -9979,7 +9981,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>   		ring = &dev_priv->ring[RCS];
>   	}
>
> -	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, ring);
> +	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, ring, &request);
>   	if (ret)
>   		goto cleanup_pending;
>
> @@ -10004,6 +10006,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>   					intel_ring_get_request(ring));
>   	}
>
> +	if (request)
> +		i915_add_request_no_flush(request->ring);
> +
>   	work->flip_queued_vblank = drm_crtc_vblank_count(crtc);
>   	work->enable_stall_check = true;
>
> @@ -10021,6 +10026,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>   cleanup_unpin:
>   	intel_unpin_fb_obj(obj);
>   cleanup_pending:
> +	if (request)
> +		i915_gem_request_cancel(request);
>   	atomic_dec(&intel_crtc->unpin_work_count);
>   	mutex_unlock(&dev->struct_mutex);
>   cleanup:
> @@ -11975,7 +11982,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>   		if (ret)
>   			DRM_DEBUG_KMS("failed to attach phys object\n");
>   	} else {
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> +		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL, NULL);
>   	}
>
>   	if (ret == 0)
> @@ -13900,7 +13907,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>
>   		if (intel_pin_and_fence_fb_obj(c->primary,
>   					       c->primary->fb,
> -					       NULL)) {
> +					       NULL, NULL)) {
>   			DRM_ERROR("failed to pin boot fb on pipe %d\n",
>   				  to_intel_crtc(c)->pipe);
>   			drm_framebuffer_unreference(c->primary->fb);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8bb18e5..6ad2823 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -961,7 +961,8 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>   				    struct intel_load_detect_pipe *old);
>   int intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>   			       struct drm_framebuffer *fb,
> -			       struct intel_engine_cs *pipelined);
> +			       struct intel_engine_cs *pipelined,
> +			       struct drm_i915_gem_request **pipelined_request);
>   struct drm_framebuffer *
>   __intel_framebuffer_create(struct drm_device *dev,
>   			   struct drm_mode_fb_cmd2 *mode_cmd,
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 757c0d2..4e7e7da 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -151,7 +151,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>   	}
>
>   	/* Flush everything out, we'll be doing GTT only from now on */
> -	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL);
> +	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL);
>   	if (ret) {
>   		DRM_ERROR("failed to pin obj: %d\n", ret);
>   		goto out_fb;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0480ad4..93faf11 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -590,7 +590,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
>   	list_for_each_entry(vma, vmas, exec_list) {
>   		struct drm_i915_gem_object *obj = vma->obj;
>
> -		ret = i915_gem_object_sync(obj, req->ring);
> +		ret = i915_gem_object_sync(obj, req->ring, &req);
>   		if (ret)
>   			return ret;
>
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 6af2581..ce7797c 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -720,7 +720,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>   	if (ret != 0)
>   		return ret;
>
> -	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
> +	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL, NULL);
>   	if (ret != 0)
>   		return ret;

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