On Fri, 2017-09-29 at 19:52 +0530, Sagar Arun Kamble wrote:
>
> On 9/29/2017 5:30 PM, Joonas Lahtinen wrote:
> > On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
> > > Currently GPU is reset at the end of suspend via i915_gem_sanitize.
> > > On resume, GuC will not be loaded until intel_uc_init_hw happens
> > > during GEM resume flow but action to exit sleep can be sent to GuC
> > > considering the FW load status. To make sure we don't invoke that
> > > action update GuC FW load status at the end of GPU reset as NONE.
> > > load_status indicates HW state and it is sanitized through this new
> > > function intel_uc_sanitize.
> > >
> > > v2: Rebase.
> > >
> > > v3: Removed intel_guc_sanitize. Marking load status as NONE at the
> > > GPU reset point. (Chris/Michal)
> > >
> > > v4: Reinstated the uC function intel_uc_sanitize. (Michal Wajdeczko)
> > >
> > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >
> > <SNIP>
> >
> > > @@ -508,6 +508,18 @@ int intel_uc_resume(struct drm_i915_private *dev_priv)
> > > return intel_guc_resume(dev_priv);
> > > }
> > >
> > > +void intel_uc_sanitize(struct drm_i915_private *dev_priv)
> > > +{
> > > + /*
> > > + * FIXME: intel_uc_resume currently depends on load_status to resume
> > > + * GuC. Since we are resetting Full GPU at the end of suspend, let us
> > > + * mark the load status as NONE. Once intel_uc_resume is updated to take
> > > + * into consideration GuC load state based on WOPCM, we can skip this
> > > + * state update.
> > > + */
> > > + dev_priv->guc.fw.load_status = INTEL_UC_FIRMWARE_NONE;
> >
> > With what I suggested to Michal, this would be call to
> > intel_guc_sanitize() (and in future also intel_huc_sanitize()
> > intel_dmc_sanitize()).
>
> Yes.
> >
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -1763,6 +1763,9 @@ int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
> > > }
> > > intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> > >
> > > + if (engine_mask == ALL_ENGINES)
> > > + intel_uc_sanitize(dev_priv);
> >
> > We could propagate engine_mask to intel_uc_sanitize and let it decide
> > what it does to keep a clear top level code flow. This also doesn't
> > seem to depend on if GuC submission is enabled or not.
>
> Sure. will make this change.
> > If we want to be unconditional, wouldn't intel_guc_select_fw would not
> > be more appropriate in intel_uc_sanitize?
>
> Do we want to select different fw across resets? That would mean
> changing i915.guc_firmware_path at runtime which I guess we don't want
> do right?
That's a good point, intel_uc_sanitize could call intel_guc_sanitize
and intel_guc_sanitize could be called at the beginning of
intel_guc_select_fw too. Quasi-randomly setting the guc firmware load
status was odd.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
