[Intel-gfx] [PATCH] drm/i915/guc: Unify naming of private GuC action functions

[Michał Winiarski]

On Thu, Mar 15, 2018 at 05:19:27PM +0100, Michal Wajdeczko wrote:
> On Thu, 15 Mar 2018 16:57:26 +0100, Michał Winiarski
> <michal.winiarski at intel.com> wrote:
> 
> > On Wed, Mar 14, 2018 at 06:37:15PM +0000, Michal Wajdeczko wrote:
> > > We should avoid using guc_log prefix for functions that don't
> > > operate on GuC log, but rather request action from the GuC.
> > > Better to use guc_action prefix.
> > > 
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > Cc: Michal Winiarski <michal.winiarski at intel.com>
> > > Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_guc_log.c | 16 +++++++++-------
> > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
> > > b/drivers/gpu/drm/i915/intel_guc_log.c
> > > index b9c7bd7..457168a 100644
> > > --- a/drivers/gpu/drm/i915/intel_guc_log.c
> > > +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> > > @@ -39,7 +39,7 @@
> > >   * registers value.
> > >   */
> > > 
> > > -static int guc_log_flush_complete(struct intel_guc *guc)
> > > +static int guc_action_flush_log_complete(struct intel_guc *guc)
> > >  {
> > >  	u32 action[] = {
> > >  		INTEL_GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE
> > > @@ -48,7 +48,7 @@ static int guc_log_flush_complete(struct intel_guc
> > > *guc)
> > >  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> > >  }
> > > 
> > > -static int guc_log_flush(struct intel_guc *guc)
> > > +static int guc_action_flush_log(struct intel_guc *guc)
> > >  {
> > >  	u32 action[] = {
> > >  		INTEL_GUC_ACTION_FORCE_LOG_BUFFER_FLUSH,
> > > @@ -58,7 +58,8 @@ static int guc_log_flush(struct intel_guc *guc)
> > >  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> > >  }
> > > 
> > > -static int guc_log_control(struct intel_guc *guc, bool enable, u32
> > > verbosity)
> > > +static int guc_action_enable_log(struct intel_guc *guc, bool enable,
> > > +				 u32 verbosity)
> > 
> > Let's hide the fact that the actual action is called "ENABLE_LOGGING",
> > and stick
> > with guc_action_log_control, especially since we're using
> > guc_log_control union,
> > and the action itself is also used for verbosity (and default log...
> > more than
> > just enable/disable switch).
> 
> Hmm, I think that using action name as base for function is right thing.
> If in your opinion action name is not correct, we should start with action
> rename first.

Nooo, then we're going to have i915/GuC mismatch :/

> And I would rather prefer to drop definition of union guc_log_control
> and replace it with set of SHIFT/MASK macros as we do for other bitfields.

Sure - why not.

> Also using actual action name as base for new function name, we could
> avoid having yet another [log|control|log] function name permutation.

We're not consistent with maching action/function name, and I think 4 arguments
"enable" function is going to be really confusing. But I don't have a strong
opinion here, it's going to be used in a single place, and it has "guc_action_*"
warning sign now ;)

-Michał
 
> But I'm flexible ;)
> 
> > 
> > With that:
> > 
> > Reviewed-by: Michał Winiarski <michal.winiarski at intel.com>
> > 
> > -Michał
> > 
> > > 
> > >  	/* GuC would have updated log buffer by now, so capture it */
> > > @@ -639,10 +640,11 @@ int intel_guc_log_control_set(struct intel_guc
> > > *guc, u64 val)
> > >  	}
> > > 
> > >  	intel_runtime_pm_get(dev_priv);
> > > -	ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val));
> > > +	ret = guc_action_enable_log(guc, enabled,
> > > LOG_LEVEL_TO_VERBOSITY(val));
> > >  	intel_runtime_pm_put(dev_priv);
> > >  	if (ret) {
> > > -		DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret);
> > > +		DRM_DEBUG_DRIVER("GuC action to %s log failed (%d)\n",
> > > +				 enabled ? "enable" : "disable", ret);
> > >  		goto out_unlock;
> > >  	}
> > > 
> > > --
> > > 1.9.1