Re: [PATCH 2/2] drm/i915/guc : Enable GuC logs even when submission is not enabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 23, 2017 at 03:13:07PM -0700, Sujaritha Sundaresan wrote:
> Currently, we only enable GuC logs when enable_guc_submission is set.
> But we could be interested in getting GuC logs in other cases as well.
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxx>
> Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  2 +-
>  drivers/gpu/drm/i915/i915_guc_submission.c | 13 ++-----------
>  drivers/gpu/drm/i915/intel_guc_log.c       |  6 +++---
>  drivers/gpu/drm/i915/intel_uc.c            | 22 +++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_uc.h            |  1 +
>  5 files changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8e18c67..437c3c6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3065,7 +3065,7 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg)
>   * properties, so we have separate macros to test them.
>   */
>  #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
> -#define HAS_GUC(dev_priv)			((dev_priv)->info.has.guc)
> +#define HAS_GUC(dev_priv)			((dev_priv)->info.has_guc)

This shall be fixed in patch 1/2

>  #define HAS_GUC_UCODE(dev_priv)		((dev_priv)->guc.fw.path != NULL)
>  #define HAS_HUC_UCODE(dev_priv)		((dev_priv)->huc.fw.path != NULL)
>  #define NEEDS_GUC_LOADING(dev_priv) \
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 48a1e93..639e0d8 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1109,16 +1109,12 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>  
>  	vaddr = i915_gem_object_pin_map(guc->stage_desc_pool->obj, I915_MAP_WB);
>  	if (IS_ERR(vaddr)) {
> -		ret = PTR_ERR(vaddr);
> -		goto err_vma;
> +		i915_vma_unpin_and_release(&guc->stage_desc_pool);
> +		return PTR_ERR(vaddr);

Why ?

>  	}
>  
>  	guc->stage_desc_pool_vaddr = vaddr;
>  
> -	ret = intel_guc_log_create(guc);
> -	if (ret < 0)
> -		goto err_vaddr;
> -
>  	ret = guc_ads_create(guc);
>  	if (ret < 0)
>  		goto err_log;
> @@ -1129,10 +1125,6 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>  
>  err_log:
>  	intel_guc_log_destroy(guc);

As you removed log_create(), why log_destroy() is still there ?

> -err_vaddr:
> -	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
> -err_vma:
> -	i915_vma_unpin_and_release(&guc->stage_desc_pool);
>  	return ret;
>  }
>  
> @@ -1142,7 +1134,6 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>  
>  	ida_destroy(&guc->stage_ids);
>  	guc_ads_destroy(guc);
> -	intel_guc_log_destroy(guc);
>  	i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>  	i915_vma_unpin_and_release(&guc->stage_desc_pool);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 16d3b87..592b449 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -502,7 +502,7 @@ static void guc_flush_logs(struct intel_guc *guc)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>  
> -	if (!i915.enable_guc_submission || (i915.guc_log_level < 0))
> +	if (HAS_GUC_UCODE(dev_priv) || (i915.guc_log_level < 0))

Hmm, is this new condition correct ?

>  		return;
>  
>  	/* First disable the interrupts, will be renabled afterwards */
> @@ -641,7 +641,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>  
>  void i915_guc_log_register(struct drm_i915_private *dev_priv)
>  {
> -	if (!i915.enable_guc_submission || i915.guc_log_level < 0)
> +	if (HAS_GUC_UCODE(dev_priv) || i915.guc_log_level < 0)
>  		return;
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -651,7 +651,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
>  
>  void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>  {
> -	if (!i915.enable_guc_submission)
> +	if (HAS_GUC_UCODE(dev_priv))
>  		return;
>  
>  	mutex_lock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index d8fab3a..44faeac 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -298,6 +298,20 @@ static void guc_disable_communication(struct intel_guc *guc)
>  	guc->send = intel_guc_send_nop;
>  }
>  
> +static int guc_shared_objects_init(struct intel_guc *guc)
> +{
> +	int ret;
> +
> +	ret = intel_guc_log_create(guc);
> +	if (ret < 0)
> +		return ret;
> +}
> +
> +static void guc_shared_objects_fini(struct intel_guc *guc)
> +{
> +	intel_guc_log_destroy(guc);
> +}
> +
>  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> @@ -311,6 +325,9 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  
>  	/* We need to notify the guc whenever we change the GGTT */
>  	i915_ggtt_enable_guc(dev_priv);
> +	ret = guc_shared_objects_init(guc);
> +	if (ret)
> +		goto err_guc;
>  
>  	if (i915.enable_guc_submission) {
>  		/*
> @@ -319,7 +336,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  		 */
>  		ret = i915_guc_submission_init(dev_priv);
>  		if (ret)
> -			goto err_guc;
> +			goto err_shared;
>  	}
>  
>  	/* init WOPCM */
> @@ -389,6 +406,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>  err_submission:
>  	if (i915.enable_guc_submission)
>  		i915_guc_submission_fini(dev_priv);
> +err_shared:
> +	guc_shared_objects_fini(guc);
>  err_guc:
>  	i915_ggtt_disable_guc(dev_priv);
>  
> @@ -424,6 +443,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>  		i915_guc_submission_fini(dev_priv);
>  	}
>  
> +	guc_shared_objects_fini(&dev_priv->guc);
>  	i915_ggtt_disable_guc(dev_priv);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 7370b74..ea2a32b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -256,6 +256,7 @@ static inline void intel_guc_notify(struct intel_guc *guc)
>  void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
>  
> +

Unrelated

>  /* intel_guc_log.c */
>  int intel_guc_log_create(struct intel_guc *guc);
>  void intel_guc_log_destroy(struct intel_guc *guc);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux