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
