Re: [PATCH 01/17] drm/i915: Decouple GuC log setup from verbosity parameter

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/07/16 14:41, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> GuC Log buffer allocation was tied up with verbosity level module param
> i915.guc_log_level. User would be given a provision to enable firmware
> logging at runtime, through a host2guc action, and not necessarily during
> Driver load time. But the address of log buffer can be passed only in
> init params, at firmware load time, so GuC has to be reset and firmware
> needs to be reloaded to pass the log buffer address at runtime.
> To avoid reset of GuC & reload of firmware, allocation of log buffer will
> be done always but logging would be enabled initially on GuC side based on
> the value of module paramter guc_log_level.
>
> v2: Update commit message to describe the constraint with allocation of
>      log buffer at runtime. (Tvrtko)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 3 ---
>   drivers/gpu/drm/i915/intel_guc_loader.c    | 8 +++++---
>   2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 2112e02..8a9a0cb 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -832,9 +832,6 @@ static void guc_create_log(struct intel_guc *guc)
>   	unsigned long offset;
>   	uint32_t size, flags;
>
> -	if (i915.guc_log_level < GUC_LOG_VERBOSITY_MIN)
> -		return;
> -
>   	if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
>   		i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 605c696..b211bd0 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -175,11 +175,13 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
>   	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
>   			GUC_CTL_VCS2_ENABLED;
>
> -	if (i915.guc_log_level >= 0) {
> -		params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
> +	params[GUC_CTL_LOG_PARAMS] = guc->log_flags;

guc->log_flags will be zero when logging is not configured because guc 
is a part of dev_priv. So it looks safe - although I reckon it would be 
clearer to set this (GUC_CTL_LOG_PARAMS) explicitly inside the if-else 
below?

> +
> +	if (i915.guc_log_level >= 0)
>   		params[GUC_CTL_DEBUG] =
>   			i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
> -	}
> +	else
> +		params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;

I also wonder how come GUC_LOG_DISABLED isn't set today when 
i915.guc_log_level == -1, given that:

#define   GUC_LOG_DISABLED             (1 << 6)

Is that bit set by default somehow if i915 does not program it?

>
>   	if (guc->ads_obj) {
>   		u32 ads = (u32)i915_gem_obj_ggtt_offset(guc->ads_obj)
>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx