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

Joonas Lahtinen
2016-07-22T10:34:27Z

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