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

[Tomas Elf <tomas.elf@intel.com>]

On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> 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.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h            |    2 +-
>   drivers/gpu/drm/i915/i915_gem.c            |    7 ++++---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
>   drivers/gpu/drm/i915/intel_lrc.c           |    2 +-
>   4 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 375d4f9..bfd7b47 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2744,7 +2744,7 @@ 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 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,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5897d54..c5b9bc7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2956,7 +2956,7 @@ out:
>    * 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. May be NULL.
>    *
>    * This code is meant to abstract object synchronization with the GPU.
>    * Calling with NULL implies synchronizing the object with the CPU
> @@ -2966,8 +2966,9 @@ out:
>    */
>   int
>   i915_gem_object_sync(struct drm_i915_gem_object *obj,
> -		     struct intel_engine_cs *to)
> +		     struct drm_i915_gem_request *to_req)
>   {
> +	struct intel_engine_cs *to = to_req ? to_req->ring : NULL;
>   	struct intel_engine_cs *from;
>   	u32 seqno;
>   	int ret, idx;
> @@ -3953,7 +3954,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>   			if (ret)
>   				return ret;
>
> -			ret = i915_gem_object_sync(obj, req->ring);
> +			ret = i915_gem_object_sync(obj, req);
>   			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 76f6dcf..2cd0579 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -838,7 +838,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);
>   		if (ret)
>   			return ret;
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 4b42346..0d88e9c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -589,7 +589,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);
>   		if (ret)
>   			return ret;
>
>

This seems to be just fine but if the requests of any of the existing 
i915_gem_object_sync() call sites were to be null they would cause local 
null pointer exceptions in the calling functions even though 
i915_gem_object_sync() itself supports an incoming null request. There 
does not seem to be any call sites in the driver that actually passes or 
supports passing null requests into i915_gem_object_sync(). Does the 
driver do that anywhere today or how does that fit in?

Aside from that open question:

Reviewed-by: Tomas Elf <tomas.elf@intel.com>

Thanks,
Tomas

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