[Intel-gfx] [PATCH v2 2/3] drm/i915/guc: Drop union guc_log_control

[Michal Wajdeczko]

On Mon, 19 Mar 2018 16:32:05 +0100, Michał Winiarski  
<michal.winiarski at intel.com> wrote:

> On Mon, Mar 19, 2018 at 01:49:23PM +0000, Michal Wajdeczko wrote:
>> Usually we use shift/mask macros for bit field definitions.
>> Union guc_log_control was not following that pattern.
>>
>> Additional bonus:
>>
>> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-25 (-25)
>> Function                                     old     new   delta
>> intel_guc_log_level_set                      388     363     -25
>>
>> 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_fwif.h | 16 +++++-----------
>>  drivers/gpu/drm/i915/intel_guc_log.c  | 11 +++--------
>>  2 files changed, 8 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h  
>> b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> index 4971685..72941bd 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> @@ -534,17 +534,6 @@ struct guc_log_buffer_state {
>>  	u32 version;
>>  } __packed;
>>
>> -union guc_log_control {
>> -	struct {
>> -		u32 logging_enabled:1;
>> -		u32 reserved1:3;
>> -		u32 verbosity:4;
>> -		u32 default_logging:1;
>> -		u32 reserved2:23;
>> -	};
>> -	u32 value;
>> -} __packed;
>> -
>>  struct guc_ctx_report {
>>  	u32 report_return_status;
>>  	u32 reserved1[64];
>> @@ -603,6 +592,11 @@ enum intel_guc_report_status {
>>  	INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4,
>>  };
>>
>> +#define GUC_LOG_CONTROL_LOGGING_ENABLED	(1 << 0)
>> +#define GUC_LOG_CONTROL_VERBOSITY_SHIFT	4
>> +#define GUC_LOG_CONTROL_VERBOSITY_MASK	(0xF <<  
>> GUC_LOG_CONTROL_VERBOSITY_SHIFT)
>
> You're not using the mask anywhere. Is it just to describe the iface?

yep, only bit-field interface

> Or was it supposed to be used in guc_action_control_log?
> (there's a slight change in behavior here, which may lead to confusion  
> when
> passing out-of-range verbosity)

As you already check for out-of-range verbosity level passed by user:

	if (val < GUC_LOG_LEVEL_DISABLED ||
	    val > GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
		return -EINVAL;

I can make my check in guc_action() more assertive:

	GEM_BUG_ON(verbosity > GUC_LOG_VERBOSITY_MAX)

Thanks,
/m