On Wed, Aug 16, 2017 at 5:41 PM, Joseph Garvey <joseph1.garvey@xxxxxxxxx> 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@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> CC: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Signed-off-by: Joseph Garvey <joseph1.garvey@xxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
