[Intel-gfx] [PATCH] drm/i915: Wedge the GPU if command parser setup fails

[Chris Wilson]

Quoting Tvrtko Ursulin (2021-02-26 09:51:54)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Commit 311a50e76a33 ("drm/i915: Add support for mandatory cmdparsing")
> introduced mandatory command parsing but setup failures were not
> translated into wedging the GPU which was probably the intent.
> 
> Possible errors come in two categories. Either the sanity check on
> internal tables has failed, which should be caught in CI unless an
> affected platform would be missed in testing; or memory allocation failure
> happened during driver load, which should be extremely unlikely but for
> correctness should still be handled.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Fixes: 311a50e76a33 ("drm/i915: Add support for mandatory cmdparsing")
> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Chris Wilson <chris.p.wilson at intel.com>
> ---
> To catchup with referenced patch we'd need to copy stable from v5.4+.
> Failures are very unlikely but I think it would still be prudent to do so.
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c |  8 +++++++-
>  drivers/gpu/drm/i915/i915_cmd_parser.c    | 19 +++++++++++++------
>  drivers/gpu/drm/i915/i915_drv.h           |  2 +-
>  3 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 577ebd4a324f..ac6478c4ede4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -696,6 +696,11 @@ static int engine_setup_common(struct intel_engine_cs *engine)
>                 goto err_status;
>         }
>  
> +       err = intel_engine_init_cmd_parser(engine);
> +       if (err) {
> +               goto err_cmd_parser;
> +       }

-{}

> +
>         i915_sched_init(&engine->sched,
>                         engine->i915->drm.dev,
>                         engine->name,
> @@ -703,7 +708,6 @@ static int engine_setup_common(struct intel_engine_cs *engine)
>                         ENGINE_PHYSICAL);
>  
>         intel_engine_init_execlists(engine);
> -       intel_engine_init_cmd_parser(engine);
>         intel_engine_init__pm(engine);
>         intel_engine_init_retire(engine);
>  
> @@ -720,6 +724,8 @@ static int engine_setup_common(struct intel_engine_cs *engine)
>  
>         return 0;
>  
> +err_cmd_parser:
> +       intel_breadcrumbs_free(engine->breadcrumbs);
>  err_status:
>         cleanup_status_page(engine);
>         return err;
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index ced9a96d7c34..13905891aff7 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -940,7 +940,7 @@ static void fini_hash_table(struct intel_engine_cs *engine)
>   * struct intel_engine_cs based on whether the platform requires software
>   * command parsing.
>   */
> -void intel_engine_init_cmd_parser(struct intel_engine_cs *engine)
> +int intel_engine_init_cmd_parser(struct intel_engine_cs *engine)
>  {
>         const struct drm_i915_cmd_table *cmd_tables;
>         int cmd_table_count;
> @@ -948,7 +948,7 @@ void intel_engine_init_cmd_parser(struct intel_engine_cs *engine)
>  
>         if (!IS_GEN(engine->i915, 7) && !(IS_GEN(engine->i915, 9) &&
>                                           engine->class == COPY_ENGINE_CLASS))
> -               return;
> +               return 0;
>  
>         switch (engine->class) {
>         case RENDER_CLASS:
> @@ -1013,19 +1013,19 @@ void intel_engine_init_cmd_parser(struct intel_engine_cs *engine)
>                 break;
>         default:
>                 MISSING_CASE(engine->class);
> -               return;
> +               goto out;
>         }
>  
>         if (!validate_cmds_sorted(engine, cmd_tables, cmd_table_count)) {
>                 drm_err(&engine->i915->drm,
>                         "%s: command descriptions are not sorted\n",
>                         engine->name);
> -               return;
> +               goto out;
>         }
>         if (!validate_regs_sorted(engine)) {
>                 drm_err(&engine->i915->drm,
>                         "%s: registers are not sorted\n", engine->name);
> -               return;
> +               goto out;
>         }
>  
>         ret = init_hash_table(engine, cmd_tables, cmd_table_count);
> @@ -1033,10 +1033,17 @@ void intel_engine_init_cmd_parser(struct intel_engine_cs *engine)
>                 drm_err(&engine->i915->drm,
>                         "%s: initialised failed!\n", engine->name);
>                 fini_hash_table(engine);
> -               return;
> +               goto out;
>         }
>  
>         engine->flags |= I915_ENGINE_USING_CMD_PARSER;
> +
> +out:
> +       if (intel_engine_requires_cmd_parser(engine) &&
> +           !intel_engine_using_cmd_parser(engine))
> +               return -EINVAL;

Ah. I see, a catch all to pick up the allocation failures.


> +       else

No else after return

Reviewed-by: Chris Wilson <chris.p.wilson at intel.com>
-Chris
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.