Re: [PATCH 20/53] drm/i915: Update ppgtt_init_ring() & context_enable() to take requests

[Tomas Elf <tomas.elf@xxxxxxxxx>]

On 19/02/2015 17:17, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> The final step in removing the OLR from i915_gem_init_hw() is to pass the newly
> allocated request structure in to each step rather than passing a ring
> structure. This patch updates both i915_ppgtt_init_ring() and
> i915_gem_context_enable() to take request pointers.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |    2 +-
>   drivers/gpu/drm/i915/i915_gem.c         |    4 ++--
>   drivers/gpu/drm/i915/i915_gem_context.c |    7 ++++---
>   drivers/gpu/drm/i915/i915_gem_gtt.c     |    6 +++---
>   drivers/gpu/drm/i915/i915_gem_gtt.h     |    2 +-
>   5 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea0da6b..618a841 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2990,7 +2990,7 @@ int __must_check i915_gem_context_init(struct drm_device *dev);
>   void i915_gem_context_fini(struct drm_device *dev);
>   void i915_gem_context_reset(struct drm_device *dev);
>   int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> -int i915_gem_context_enable(struct intel_engine_cs *ring);
> +int i915_gem_context_enable(struct drm_i915_gem_request *req);
>   void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
>   int i915_switch_context(struct intel_engine_cs *ring,
>   			struct intel_context *to);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index efed49a..379bf44 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4877,7 +4877,7 @@ i915_gem_init_hw(struct drm_device *dev)
>   				i915_gem_l3_remap(ring, i);
>   		}
>
> -		ret = i915_ppgtt_init_ring(ring);
> +		ret = i915_ppgtt_init_ring(req);
>   		if (ret && ret != -EIO) {
>   			DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret);
>   			i915_gem_request_unreference(req);
> @@ -4885,7 +4885,7 @@ i915_gem_init_hw(struct drm_device *dev)
>   			return ret;
>   		}
>
> -		ret = i915_gem_context_enable(ring);
> +		ret = i915_gem_context_enable(req);
>   		if (ret && ret != -EIO) {
>   			DRM_ERROR("Context enable ring #%d failed %d\n", i, ret);
>   			i915_gem_request_unreference(req);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index dd83d61..04d2a20 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -403,17 +403,18 @@ void i915_gem_context_fini(struct drm_device *dev)
>   	i915_gem_context_unreference(dctx);
>   }
>
> -int i915_gem_context_enable(struct intel_engine_cs *ring)
> +int i915_gem_context_enable(struct drm_i915_gem_request *req)
>   {
> +	struct intel_engine_cs *ring = req->ring;
>   	int ret;
>
>   	if (i915.enable_execlists) {
>   		if (ring->init_context == NULL)
>   			return 0;
>
> -		ret = ring->init_context(ring, ring->default_context);
> +		ret = ring->init_context(req->ring, ring->default_context);
>   	} else
> -		ret = i915_switch_context(ring, ring->default_context);
> +		ret = i915_switch_context(req->ring, ring->default_context);

You don't have to make any more changes to this function aside from 
setting up the ring variable at the top. ring = req->ring and if you 
don't change these lines the will by default use ring like they always did.

>   	if (ret) {
>   		DRM_ERROR("ring init context: %d\n", ret);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 428d2f6..cd00080 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1227,15 +1227,15 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
>   	return 0;
>   }
>
> -int i915_ppgtt_init_ring(struct intel_engine_cs *ring)
> +int i915_ppgtt_init_ring(struct drm_i915_gem_request *req)
>   {
> -	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct drm_i915_private *dev_priv = req->ring->dev->dev_private;
>   	struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
>
>   	if (!ppgtt)
>   		return 0;
>
> -	return ppgtt->switch_mm(ppgtt, ring);
> +	return ppgtt->switch_mm(ppgtt, req->ring);
>   }

If you want to uphold a pattern that you've already established you 
could just make a single change in the function above by setting up ring 
= req->ring and then make no more changes to the function body. In this 
case it's one new line vs. two changes of existing code so it's doesn't 
make that much of a difference but it is nice to stick to patterns. 
Also, you wouldn't have to make a 4-level indirection 
(req->ring->dev->dev_private), only a 3-level one.

>   struct i915_hw_ppgtt *
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 5a6cef9..e7e202f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -300,7 +300,7 @@ void i915_global_gtt_cleanup(struct drm_device *dev);
>
>   int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt);
>   int i915_ppgtt_init_hw(struct drm_device *dev);
> -int i915_ppgtt_init_ring(struct intel_engine_cs *ring);
> +int i915_ppgtt_init_ring(struct drm_i915_gem_request *req);
>   void i915_ppgtt_release(struct kref *kref);
>   struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev,
>   					struct drm_i915_file_private *fpriv);

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