Re: [PATCH] drm/i915: Refactor golden render state emission to unconfuse gcc

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On pe, 2016-07-22 at 11:16 +0100, Chris Wilson wrote:
> GCC was inlining the init and setup functions, but was getting itself
> confused into thinking that variables could be used uninitialised. If we
> do the inline for gcc, it is happy! As a bonus we shrink the code.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_render_state.c | 93 ++++++++--------------------
>  1 file changed, 26 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index a9b56d18a93b..b50412c205e5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -32,7 +32,6 @@ struct render_state {
>  	const struct intel_renderstate_rodata *rodata;
>  	struct drm_i915_gem_object *obj;
>  	u64 ggtt_offset;
> -	int gen;
>  	u32 aux_batch_size;
>  	u32 aux_batch_offset;
>  };
> @@ -54,36 +53,6 @@ render_state_get_rodata(const int gen)
>  	return NULL;
>  }
>  
> -static int render_state_init(struct render_state *so,
> -			     struct drm_i915_private *dev_priv)
> -{
> -	int ret;
> -
> -	so->gen = INTEL_GEN(dev_priv);
> -	so->ggtt_offset = 0; /* keep gcc quiet */
> -	so->rodata = render_state_get_rodata(so->gen);
> -	if (so->rodata == NULL)
> -		return 0;
> -
> -	if (so->rodata->batch_items * 4 > 4096)
> -		return -EINVAL;
> -
> -	so->obj = i915_gem_object_create(&dev_priv->drm, 4096);
> -	if (IS_ERR(so->obj))
> -		return PTR_ERR(so->obj);
> -
> -	ret = i915_gem_obj_ggtt_pin(so->obj, 4096, 0);
> -	if (ret)
> -		goto free_gem;
> -
> -	so->ggtt_offset = i915_gem_obj_ggtt_offset(so->obj);
> -	return 0;
> -
> -free_gem:
> -	i915_gem_object_put(so->obj);
> -	return ret;
> -}
> -
>  /*
>   * Macro to add commands to auxiliary batch.
>   * This macro only checks for page overflow before inserting the commands,
> @@ -106,6 +75,7 @@ static int render_state_setup(struct render_state *so)
>  {
>  	struct drm_device *dev = so->obj->base.dev;
>  	const struct intel_renderstate_rodata *rodata = so->rodata;
> +	const bool has_64bit_reloc = INTEL_GEN(dev) >= 8;
>  	unsigned int i = 0, reloc_index = 0;
>  	struct page *page;
>  	u32 *d;
> @@ -124,7 +94,7 @@ static int render_state_setup(struct render_state *so)
>  		if (i * 4  == rodata->reloc[reloc_index]) {
>  			u64 r = s + so->ggtt_offset;
>  			s = lower_32_bits(r);
> -			if (so->gen >= 8) {
> +			if (has_64bit_reloc) {
>  				if (i + 1 >= rodata->batch_items ||
>  				    rodata->batch[i + 1] != 0) {
>  					ret = -EINVAL;
> @@ -202,53 +172,40 @@ err_out:
>  
>  #undef OUT_BATCH
>  
> -static void render_state_fini(struct render_state *so)
> -{
> -	i915_gem_object_ggtt_unpin(so->obj);
> -	i915_gem_object_put(so->obj);
> -}
> -
> -static int render_state_prepare(struct intel_engine_cs *engine,
> -				struct render_state *so)
> +int i915_gem_render_state_init(struct drm_i915_gem_request *req)
>  {
> +	struct render_state so;
>  	int ret;
>  
> -	if (WARN_ON(engine->id != RCS))
> -		return -ENOENT;
> -
> -	ret = render_state_init(so, engine->i915);
> -	if (ret)
> -		return ret;
> -
> -	if (so->rodata == NULL)
> +	if (WARN_ON(req->engine->id != RCS))
>  		return 0;

Why not -ENOENT anymore, it was propagated up previously.

>  
> -	ret = render_state_setup(so);
> -	if (ret) {
> -		render_state_fini(so);
> -		return ret;
> -	}
> +	so.rodata = render_state_get_rodata(INTEL_GEN(req->i915));

While you revamp, maybe let the function take req->i915 directly?

Otherwise looks quite good.

> +	if (so.rodata == NULL)
> +		return 0;
> 
>  
> -	return 0;
> -}
> +	if (so.rodata->batch_items * 4 > 4096)
> +		return -EINVAL;
>  
> -int i915_gem_render_state_init(struct drm_i915_gem_request *req)
> -{
> -	struct render_state so;
> -	int ret;
> +	so.obj = i915_gem_object_create(&req->i915->drm, 4096);
> +	if (IS_ERR(so.obj))
> +		return PTR_ERR(so.obj);
>  
> -	ret = render_state_prepare(req->engine, &so);
> +	ret = i915_gem_obj_ggtt_pin(so.obj, 4096, 0);
>  	if (ret)
> -		return ret;
> +		goto err_obj;
>  
> -	if (so.rodata == NULL)
> -		return 0;
> +	so.ggtt_offset = i915_gem_obj_ggtt_offset(so.obj);
> +
> +	ret = render_state_setup(&so);
> +	if (ret)
> +		goto err_unpin;
>  
>  	ret = req->engine->emit_bb_start(req, so.ggtt_offset,
>  					 so.rodata->batch_items * 4,
>  					 I915_DISPATCH_SECURE);
>  	if (ret)
> -		goto out;
> +		goto err_unpin;
>  
>  	if (so.aux_batch_size > 8) {
>  		ret = req->engine->emit_bb_start(req,
> @@ -257,11 +214,13 @@ int i915_gem_render_state_init(struct drm_i915_gem_request *req)
>  						 so.aux_batch_size,
>  						 I915_DISPATCH_SECURE);
>  		if (ret)
> -			goto out;
> +			goto err_unpin;
>  	}
>  
>  	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), req);
> -out:
> -	render_state_fini(&so);
> +err_unpin:
> +	i915_gem_object_ggtt_unpin(so.obj);
> +err_obj:
> +	i915_gem_object_put(so.obj);
>  	return ret;
>  }
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx