[Intel-gfx] [CI 1/2] drm/i915/guc: Enable send function only after successful init

[Chris Wilson]

On Wed, May 03, 2017 at 05:54:43PM +0200, Michal Wajdeczko wrote:
> On Wed, May 03, 2017 at 11:47:12AM +0100, Chris Wilson wrote:
> > On Tue, May 02, 2017 at 10:32:42AM +0000, Michal Wajdeczko wrote:
> > > It is safer to setup valid send function after successful GuC
> > > hardware initialization. In addition we prepare placeholder
> > > where we can setup any alternate GuC communication mechanism.
> > > 
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_uc.c | 27 ++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_uc.h |  1 +
> > >  2 files changed, 27 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > > index 900e376..5957a95 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.c
> > > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > > @@ -99,7 +99,7 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> > >  	struct intel_guc *guc = &dev_priv->guc;
> > >  
> > >  	mutex_init(&guc->send_mutex);
> > > -	guc->send = intel_guc_send_mmio;
> > > +	guc->send = intel_guc_send_nop;
> > >  }
> > >  
> > >  static void fetch_uc_fw(struct drm_i915_private *dev_priv,
> > > @@ -252,13 +252,27 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
> > >  	__intel_uc_fw_fini(&dev_priv->huc.fw);
> > >  }
> > >  
> > > +static int guc_enable_communication(struct intel_guc *guc)
> > > +{
> > > +	/* XXX: placeholder for alternate setup */
> > > +	guc->send = intel_guc_send_mmio;
> > > +	return 0;
> > > +}
> > > +
> > > +static void guc_disable_communication(struct intel_guc *guc)
> > > +{
> > > +	guc->send = intel_guc_send_nop;
> > > +}
> > > +
> > >  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> > >  {
> > > +	struct intel_guc *guc = &dev_priv->guc;
> > >  	int ret, attempts;
> > >  
> > >  	if (!i915.enable_guc_loading)
> > >  		return 0;
> > >  
> > > +	guc_disable_communication(guc);
> > >  	gen9_reset_guc_interrupts(dev_priv);
> > >  
> > >  	/* We need to notify the guc whenever we change the GGTT */
> > > @@ -308,6 +322,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> > >  	if (ret)
> > >  		goto err_submission;
> > >  
> > > +	ret = guc_enable_communication(guc);
> > > +	if (ret)
> > > +		goto err_submission;
> > > +
> > >  	intel_guc_auth_huc(dev_priv);
> > >  	if (i915.enable_guc_submission) {
> > >  		if (i915.guc_log_level >= 0)
> > > @@ -330,6 +348,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> > >  	 * marks the GPU as wedged until reset).
> > >  	 */
> > >  err_interrupts:
> > > +	guc_disable_communication(guc);
> > >  	gen9_disable_guc_interrupts(dev_priv);
> > >  err_submission:
> > >  	if (i915.enable_guc_submission)
> > > @@ -364,6 +383,12 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
> > >  	i915_ggtt_disable_guc(dev_priv);
> > >  }
> > >  
> > > +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
> > > +{
> > > +	WARN(1, "Unexpected send: action=%#x\n", *action);
> > > +	return -ENOSYS;
> > > +}
> > > +
> > >  /*
> > >   * This function implements the MMIO based host to GuC interface.
> > >   */
> > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > > index 2f0229d..1e0eecd 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.h
> > > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > > @@ -227,6 +227,7 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
> > >  int intel_uc_init_hw(struct drm_i915_private *dev_priv);
> > >  void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> > >  int intel_guc_sample_forcewake(struct intel_guc *guc);
> > > +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
> > 
> > Was there a reason for exporting the nop? Its a purely internal BUG_ON()
> > function that should only be set when disabling the guc. Afaict, it
> > should be private.
> 
> By looking only at this patch, agree, this nop function should be private.
> 
> But at the same time, this function, to some extend, is similar to the send_mmio
> function, that we agree to keep it exposed, as it also may be needed in other 
> code paths (like upcoming alternate GuC communication implementation) in case
> of the recovery or other plumbing.
> 
> I should mention that in the commit message to avoid your concern ;)

Always a benefit to mention the grand design if things look out of place
in individual patches. I'll be surprised if you can conjure up a
situation where we want to call intel_guc_send_nop() :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre