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

[Michal Wajdeczko]

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.

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.

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

But I'm flexible ;)

>
> With that:
>
> Reviewed-by: Michał Winiarski <michal.winiarski at intel.com>
>
> -Michał
>
>>  {
>>  	union guc_log_control control_val = {
>>  		{
>> @@ -525,7 +526,7 @@ static void guc_log_capture_logs(struct intel_guc  
>> *guc)
>>  	 * time, so get/put should be really quick.
>>  	 */
>>  	intel_runtime_pm_get(dev_priv);
>> -	guc_log_flush_complete(guc);
>> +	guc_action_flush_log_complete(guc);
>>  	intel_runtime_pm_put(dev_priv);
>>  }
>>
>> @@ -541,7 +542,7 @@ static void guc_flush_logs(struct intel_guc *guc)
>>
>>  	/* Ask GuC to update the log buffer state */
>>  	intel_runtime_pm_get(dev_priv);
>> -	guc_log_flush(guc);
>> +	guc_action_flush_log(guc);
>>  	intel_runtime_pm_put(dev_priv);
>>
>>  	/* 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