[Intel-gfx] [PATCH 1/3] drm/i915/guc: Move notification code into virtual function

[Michal Wajdeczko]

On Tue, May 02, 2017 at 09:37:45AM -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 02/05/17 05:39, Michal Wajdeczko wrote:
> > Prepare for alternate GuC notification 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>
> > ---
> >  drivers/gpu/drm/i915/intel_uc.c | 10 +++++++++-
> >  drivers/gpu/drm/i915/intel_uc.h |  7 +++++++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 7fd75ca..72f49e6 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -94,12 +94,20 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> >  		i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
> >  }
> > 
> > +static void guc_write_irq_trigger(struct intel_guc *guc)
> > +{
> > +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > +
> > +	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> > +}
> > +
> >  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_nop;
> > +	guc->notify = guc_write_irq_trigger;
> >  }
> > 
> >  static void fetch_uc_fw(struct drm_i915_private *dev_priv,
> > @@ -413,7 +421,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> > 
> >  	POSTING_READ(SOFT_SCRATCH(i - 1));
> > 
> > -	I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
> > +	intel_guc_notify(guc);
> > 
> >  	/*
> >  	 * No GuC command should ever take longer than 10ms.
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > index 1e0eecd..097289b 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -210,6 +210,9 @@ struct intel_guc {
> > 
> >  	/* GuC's FW specific send function */
> >  	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
> > +
> > +	/* GuC's FW specific notify function */
> > +	void (*notify)(struct intel_guc *guc);
> >  };
> > 
> >  struct intel_huc {
> > @@ -233,6 +236,10 @@ static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 l
> >  {
> >  	return guc->send(guc, action, len);
> >  }
> > +static inline void intel_guc_notify(struct intel_guc *guc)
> > +{
> > +	guc->notify(guc);
> > +}
> > 
> 
> personal preference: I would use guc->notify directly instead of adding
> intel_guc_notify(). intel_guc_send() made more sense because a function with
> that name already existed and by keeping it we minimized the change, but I
> don't see such benefit with intel_guc_notify() and calling the function
> pointer directly feels more in sync with what we do elsewhere in the driver.
> 

Note that thanks to the compiler, resulting code is the same, but by keeping
intel_guc_notify() we are more flexible than when using hardcoded guc->notify.
Note that the same reason why we added intel_guc_send() "minimize the change"
can also be valid when in the future we may want to switch from vfun pointer
to other implementation that then can be hidden in that tiny inline that you
just wanted to kill.

-Michal