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

[Michał Winiarski]

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?
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)

-Michał

> +#define GUC_LOG_CONTROL_DEFAULT_LOGGING	(1 << 8)
> +
>  /*
>   * The GuC sends its response to a command by overwriting the
>   * command in SS0. The response is distinguishable from a command
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 39928e6..c84b67f 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -60,16 +60,11 @@ static int guc_action_flush_log(struct intel_guc *guc)
>  static int guc_action_control_log(struct intel_guc *guc, bool enable,
>  				  bool default_logging, u32 verbosity)
>  {
> -	union guc_log_control control_val = {
> -		{
> -			.logging_enabled = enable,
> -			.verbosity = verbosity,
> -			.default_logging = default_logging,
> -		},
> -	};
>  	u32 action[] = {
>  		INTEL_GUC_ACTION_UK_LOG_ENABLE_LOGGING,
> -		control_val.value
> +		(enable ? GUC_LOG_CONTROL_LOGGING_ENABLED : 0) |
> +		(verbosity << GUC_LOG_CONTROL_VERBOSITY_SHIFT) |
> +		(default_logging ? GUC_LOG_CONTROL_DEFAULT_LOGGING : 0)
>  	};
>  
>  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> -- 
> 1.9.1
>