[Intel-gfx] [PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Thu Feb 14 22:46:25 UTC 2019
On 2/14/19 1:45 PM, Sujaritha Sundaresan wrote:
> The aim of this patch is to allow enabling and disabling
> of CTB without requiring the mutex lock.
>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> ---
> drivers/gpu/drm/i915/intel_guc.c | 12 ++++
> drivers/gpu/drm/i915/intel_guc_ct.c | 85 +++++++++++++++++++++--------
> drivers/gpu/drm/i915/intel_guc_ct.h | 3 +
> 3 files changed, 77 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 8660af3fd755..8ecb47087457 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -203,11 +203,19 @@ int intel_guc_init(struct intel_guc *guc)
> goto err_log;
> GEM_BUG_ON(!guc->ads_vma);
>
> + if (HAS_GUC_CT(dev_priv)) {
> + ret = intel_guc_ct_init(&guc->ct);
> + if (ret)
> + goto err_ads;
> + }
> +
> /* We need to notify the guc whenever we change the GGTT */
> i915_ggtt_enable_guc(dev_priv);
>
> return 0;
>
> +err_ads:
> + intel_guc_ads_destroy(guc);
> err_log:
> intel_guc_log_destroy(&guc->log);
> err_shared:
> @@ -222,6 +230,10 @@ void intel_guc_fini(struct intel_guc *guc)
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
>
> i915_ggtt_disable_guc(dev_priv);
> +
> + if (HAS_GUC_CT(dev_priv))
> + intel_guc_ct_fini(&guc->ct);
> +
> intel_guc_ads_destroy(guc);
> intel_guc_log_destroy(&guc->log);
> guc_shared_data_destroy(guc);
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index a52883e9146f..fbf9da247975 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -140,9 +140,9 @@ static int guc_action_deregister_ct_buffer(struct intel_guc *guc,
> return err;
> }
>
> -static bool ctch_is_open(struct intel_guc_ct_channel *ctch)
> +static bool ctch_is_enabled(struct intel_guc_ct_channel *ctch)
> {
> - return ctch->vma != NULL;
> + return ctch->is_enabled;
Bikeshed: now that the check is simpler, doing ctch->enabled is actually
simpler then ctch_is_enabled(ctch), so I'd prefer to just switch.
> }
>
> static int ctch_init(struct intel_guc *guc,
> @@ -217,22 +217,14 @@ static void ctch_fini(struct intel_guc *guc,
> i915_vma_unpin_and_release(&ctch->vma, I915_VMA_RELEASE_MAP);
> }
>
> -static int ctch_open(struct intel_guc *guc,
> +static int ctch_enable(struct intel_guc *guc,
> struct intel_guc_ct_channel *ctch)
> {
> u32 base;
> int err;
> int i;
>
> - CT_DEBUG_DRIVER("CT: channel %d reopen=%s\n",
> - ctch->owner, yesno(ctch_is_open(ctch)));
> -
> - if (!ctch->vma) {
> - err = ctch_init(guc, ctch);
> - if (unlikely(err))
> - goto err_out;
> - GEM_BUG_ON(!ctch->vma);
> - }
> + GEM_BUG_ON(!ctch->vma);
GEM_BUG_ON(ctch->enabled) as well?
>
> /* vma should be already allocated and map'ed */
> base = intel_guc_ggtt_offset(guc, ctch->vma);
> @@ -255,7 +247,7 @@ static int ctch_open(struct intel_guc *guc,
> base + PAGE_SIZE/4 * CTB_RECV,
> INTEL_GUC_CT_BUFFER_TYPE_RECV);
> if (unlikely(err))
> - goto err_fini;
> + goto err_out;
>
> err = guc_action_register_ct_buffer(guc,
> base + PAGE_SIZE/4 * CTB_SEND,
> @@ -263,23 +255,25 @@ static int ctch_open(struct intel_guc *guc,
> if (unlikely(err))
> goto err_deregister;
>
> + ctch->is_enabled = true;
> +
> return 0;
>
> err_deregister:
> guc_action_deregister_ct_buffer(guc,
> ctch->owner,
> INTEL_GUC_CT_BUFFER_TYPE_RECV);
> -err_fini:
> - ctch_fini(guc, ctch);
> err_out:
> DRM_ERROR("CT: can't open channel %d; err=%d\n", ctch->owner, err);
> return err;
> }
>
> -static void ctch_close(struct intel_guc *guc,
> +static void ctch_disable(struct intel_guc *guc,
> struct intel_guc_ct_channel *ctch)
> {
> - GEM_BUG_ON(!ctch_is_open(ctch));
> + GEM_BUG_ON(!ctch_is_enabled(ctch));
> +
> + ctch->is_enabled = false;
>
> guc_action_deregister_ct_buffer(guc,
> ctch->owner,
> @@ -287,7 +281,6 @@ static void ctch_close(struct intel_guc *guc,
> guc_action_deregister_ct_buffer(guc,
> ctch->owner,
> INTEL_GUC_CT_BUFFER_TYPE_RECV);
> - ctch_fini(guc, ctch);
> }
>
> static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
> @@ -481,7 +474,7 @@ static int ctch_send(struct intel_guc_ct *ct,
> u32 fence;
> int err;
>
> - GEM_BUG_ON(!ctch_is_open(ctch));
> + GEM_BUG_ON(!ctch_is_enabled(ctch));
> GEM_BUG_ON(!len);
> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> GEM_BUG_ON(!response_buf && response_buf_size);
> @@ -817,7 +810,7 @@ static void ct_process_host_channel(struct intel_guc_ct *ct)
> u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
> int err = 0;
>
> - if (!ctch_is_open(ctch))
> + if (!ctch_is_enabled(ctch))
> return;
>
> do {
> @@ -848,6 +841,49 @@ static void intel_guc_to_host_event_handler_ct(struct intel_guc *guc)
> ct_process_host_channel(ct);
> }
>
> +/**
> + * intel_guc_ct_init - Allocate CT
You're not really allocating the CT struct, but the internal channel.
Maybe just say "init CT communication" here and then say in the
description below "allocate memory required for communication via the CT
channel".
Similar thing for the fini path.
> + * @ct: pointer to CT struct
> + *
> + * Shall only be called for platforms with HAS_GUC_CT.
> + *
> + * Return: 0 on success, a negative errno code on failure.
> + */
> +int intel_guc_ct_init(struct intel_guc_ct *ct)
> +{
> + struct intel_guc *guc = ct_to_guc(ct);
> + struct intel_guc_ct_channel *ctch = &ct->host_channel;
> + int err;
> +
> + GEM_BUG_ON(ctch->vma);
the BUG_ON is already in ctch_init(), no need to duplicate.
> +
> + err = ctch_init(guc, ctch);
> + if (unlikely(err)) {
> + DRM_ERROR("CT: can't open channel %d; err=%d\n",
> + ctch->owner, err);
> + return err;
> + }
> +
> + GEM_BUG_ON(!ctch->vma);
> + return 0;
> +}
> +
> +/**
> + * intel_guc_ct_fini - Deallocate CT
> + * @ct: pointer to CT struct
> + *
> + * Shall only be called for platforms with HAS_GUC_CT.
> + */
> +void intel_guc_ct_fini(struct intel_guc_ct *ct)
> +{
> + struct intel_guc *guc = ct_to_guc(ct);
> + struct intel_guc_ct_channel *ctch = &ct->host_channel;
> +
> + GEM_BUG_ON(ctch_is_enabled(ctch));
We could move this inside ctch_fini() to keep all the ctch BUG_ONs
inside ctch_* functions.
> +
> + ctch_fini(guc, ctch);
> +}
> +
> /**
> * intel_guc_ct_enable - Enable buffer based command transport.
> * @ct: pointer to CT struct
> @@ -865,7 +901,10 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
>
> GEM_BUG_ON(!HAS_GUC_CT(i915));
>
> - err = ctch_open(guc, ctch);
> + if (ctch_is_enabled(ctch))
> + return 0;
> +
> + err = ctch_enable(guc, ctch);
> if (unlikely(err))
> return err;
>
> @@ -890,10 +929,10 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct)
>
> GEM_BUG_ON(!HAS_GUC_CT(i915));
>
> - if (!ctch_is_open(ctch))
> + if (!ctch_is_enabled(ctch))
> return;
>
> - ctch_close(guc, ctch);
> + ctch_disable(guc, ctch);
>
> /* Disable send */
> guc->send = intel_guc_send_nop;
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
> index d774895ab143..e05fdc3cf0b0 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
> @@ -64,6 +64,7 @@ struct intel_guc_ct_buffer {
> struct intel_guc_ct_channel {
> struct i915_vma *vma;
> struct intel_guc_ct_buffer ctbs[2];
> + bool is_enabled;
just "enabled" is fine.
Daniele
> u32 owner;
> u32 next_fence;
> };
> @@ -90,6 +91,8 @@ struct intel_guc_ct {
> };
>
> void intel_guc_ct_init_early(struct intel_guc_ct *ct);
> +int intel_guc_ct_init(struct intel_guc_ct *ct);
> +void intel_guc_ct_fini(struct intel_guc_ct *ct);
> int intel_guc_ct_enable(struct intel_guc_ct *ct);
> void intel_guc_ct_disable(struct intel_guc_ct *ct);
>
>
More information about the Intel-gfx
mailing list