[Intel-gfx] [RFC] drm/i915/firmware: Load GuC and HuC firmware using async work.

[Rodrigo Vivi]

On Wed, Aug 16, 2017 at 5:41 PM, Joseph Garvey <joseph1.garvey at intel.com> wrote:
> The DMC firmware is currently being loaded using async work.

what would be the advantage?
why do we need to load asynchronously?

> We can do the same for the GuC and HuC firmware. Also wait for
> the work to finish before the firmware transfer.

how are we waiting? I'm afraid I didn't understand that...

>
> Cc: Sagar Kamble <sagar.a.kamble at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> CC: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>

Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>

> Signed-off-by: Joseph Garvey <joseph1.garvey at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_uc.c | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 907603c..56998fa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2167,6 +2167,7 @@ struct drm_i915_private {
>
>         struct intel_huc huc;
>         struct intel_guc guc;
> +       struct work_struct uc_load_work;

By looking to DMC I'm not sure we are doing the right thing with using
own wq instead of the official nowait firmware interface...
I believe nowait provides the advantage of loading the firmware
without initrd... but  I might be wrong..
maybe I'm expecting to much on the nowait interface...

>
>         struct intel_csr csr;
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 27e072c..8d40e1d 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -250,12 +250,21 @@ static void fetch_uc_fw(struct drm_i915_private *dev_priv,
>         uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
>  }
>
> -void intel_uc_init_fw(struct drm_i915_private *dev_priv)
> +static void load_uc_fw_work(struct work_struct *work)
>  {
> +       struct drm_i915_private *dev_priv;
> +
> +       dev_priv = container_of(work, typeof(*dev_priv), uc_load_work);
>         fetch_uc_fw(dev_priv, &dev_priv->huc.fw);
>         fetch_uc_fw(dev_priv, &dev_priv->guc.fw);

I believe this is handling on the wrong place....
I believe it should be inside fetch_uc_fw around firmrare_request, not here...

>  }
>
> +void intel_uc_init_fw(struct drm_i915_private *dev_priv)
> +{
> +       INIT_WORK(&dev_priv->uc_load_work, load_uc_fw_work);

apparently init is on the right place, but please double check if ther
is absolutelly no wai of flush being called on cases where this
INIT_WORK was never ever executed or you get a oops..

> +       schedule_work(&dev_priv->uc_load_work);

this should be on where currently request firmware is...

> +}
> +
>  void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>  {
>         __intel_uc_fw_fini(&dev_priv->guc.fw);
> @@ -336,6 +345,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>         if (!i915.enable_guc_loading)
>                 return 0;
>
> +       flush_work(&dev_priv->uc_load_work);
>         guc_disable_communication(guc);
>         gen9_reset_guc_interrupts(dev_priv);
>
> --
> 2.7.4
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br