[Intel-xe] [PATCH v2 1/2] drm/xe: Add helper functions to toggle CT communication
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Oct 12 10:23:58 UTC 2023
On 12.10.2023 08:32, Riana Tauro wrote:
> During Runtime suspend/resume, GuC is reloaded for both
> D3hot/D3Cold-> D0 transistions. It is not necessary for GuC to be
> loaded everytime for D3hot->D0, only enable/disable ctb communication.
>
> Add helper functions to toggle CT communication. Modify existing
> guc communication function
s/guc/GuC
s/ctb/CTB
typo transistions
>
> v2: do not register/unregister ctb, toggle the enabled variable
> instead.
> enable irqs while toggling CT (Daniele)
>
> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
> ---
> drivers/gpu/drm/xe/xe_guc.c | 36 ++++++++++++++++++++++++++++++----
> drivers/gpu/drm/xe/xe_guc.h | 3 ++-
> drivers/gpu/drm/xe/xe_guc_ct.c | 15 ++++++++++++--
> drivers/gpu/drm/xe/xe_guc_ct.h | 1 +
> drivers/gpu/drm/xe/xe_uc.c | 2 +-
> 5 files changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index 84f0b5488783..40c7bf0c50c5 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -507,7 +507,7 @@ int xe_guc_min_load_for_hwconfig(struct xe_guc *guc)
> if (ret)
> return ret;
>
> - ret = xe_guc_enable_communication(guc);
> + ret = xe_guc_enable_communication(guc, true);
> if (ret)
> return ret;
>
> @@ -560,16 +560,31 @@ static void guc_enable_irq(struct xe_guc *guc)
> xe_mmio_rmw32(gt, GUC_SG_INTR_MASK, events, 0);
> }
>
> -int xe_guc_enable_communication(struct xe_guc *guc)
> +/**
> + * xe_guc_enable_communication - Enable GuC communication
> + * @guc: The GuC object
> + * @ctb_register: boolean to indicate if ctb registration needs to be done
> + * or toggling of the value.
> + *
> + * This function enables guc interrupts, toggles or registers CT communication
> + * and handles mmio messages.
> + *
> + * Return: 0 on success, negative error code on error.
> + */
> +int xe_guc_enable_communication(struct xe_guc *guc, bool ctb_register)
I'm not super happy with the naming here...
maybe there should be separate functions like:
xe_guc_enable_communication(guc) { ct_enable(); irq(on) }
xe_guc_suspend_communication(guc) { irq(off); ct_toggle(off) }
xe_guc_resume_communication(guc) { ct_toggle(on); irq(on) }
xe_guc_disable_communication(guc) { irq(off); ct_disable() }
> {
> - int err;
> + int err = 0;
>
> guc_enable_irq(guc);
>
> xe_mmio_rmw32(guc_to_gt(guc), PMINTRMSK,
> ARAT_EXPIRED_INTRMSK, 0);
>
> - err = xe_guc_ct_enable(&guc->ct);
> + if (ctb_register)
> + err = xe_guc_ct_enable(&guc->ct);
> + else
> + xe_guc_ct_toggle(&guc->ct, true);
> +
> if (err)
> return err;
>
> @@ -578,6 +593,19 @@ int xe_guc_enable_communication(struct xe_guc *guc)
> return 0;
> }
>
> +/**
> + * xe_guc_disable_communication - disables GuC communication
> + * @guc: The GuC object
> + *
> + * This function disables CT communication
maybe: "This function disables all communication channels with the GuC."
> + */
> +void xe_guc_disable_communication(struct xe_guc *guc)
> +{
> + xe_guc_ct_toggle(&guc->ct, false);
> +
> + guc_handle_mmio_msg(guc);
do we need to handle that here?
note that if we just toggle CTB flag in the driver, GuC will still
believe that CTB is available and will send any G2H over CTB, not MMIO
> +}
> +
> int xe_guc_suspend(struct xe_guc *guc)
> {
> int ret;
> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
> index 3addd8fc674a..71dac0673e61 100644
> --- a/drivers/gpu/drm/xe/xe_guc.h
> +++ b/drivers/gpu/drm/xe/xe_guc.h
> @@ -18,7 +18,8 @@ int xe_guc_post_load_init(struct xe_guc *guc);
> int xe_guc_reset(struct xe_guc *guc);
> int xe_guc_upload(struct xe_guc *guc);
> int xe_guc_min_load_for_hwconfig(struct xe_guc *guc);
> -int xe_guc_enable_communication(struct xe_guc *guc);
> +void xe_guc_disable_communication(struct xe_guc *guc);
> +int xe_guc_enable_communication(struct xe_guc *guc, bool ctb_register);
> int xe_guc_suspend(struct xe_guc *guc);
> void xe_guc_notify(struct xe_guc *guc);
> int xe_guc_auth_huc(struct xe_guc *guc, u32 rsa_addr);
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 8b686c8b3339..ff89a1a09f1f 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -319,14 +319,25 @@ int xe_guc_ct_enable(struct xe_guc_ct *ct)
> return err;
> }
>
> -void xe_guc_ct_disable(struct xe_guc_ct *ct)
> +/**
> + * xe_guc_ct_toggle - Toggle GuC CT
> + * @ct: GuC CT
> + * @toggle: 0-disable, 1-enable
maybe this should be named just as @enable ?
> + *
> + * This function enables or disables CT communication
to be precise this function just toggles a flag whether CTB is enabled
(aka ready for use by other layers) but does not enable it with the GuC
> + */
> +void xe_guc_ct_toggle(struct xe_guc_ct *ct, bool toggle)
> {
> mutex_lock(&ct->lock); /* Serialise dequeue_one_g2h() */
> spin_lock_irq(&ct->fast_lock); /* Serialise CT fast-path */
> - ct->enabled = false; /* Finally disable CT communication */
> + ct->enabled = toggle; /* Toggle CT communication */
> spin_unlock_irq(&ct->fast_lock);
> mutex_unlock(&ct->lock);
> +}
>
> +void xe_guc_ct_disable(struct xe_guc_ct *ct)
> +{
> + xe_guc_ct_toggle(ct, false);
> xa_destroy(&ct->fence_lookup);
> }
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.h b/drivers/gpu/drm/xe/xe_guc_ct.h
> index f15f8a4857e0..11166a7ce958 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.h
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.h
> @@ -14,6 +14,7 @@ int xe_guc_ct_init(struct xe_guc_ct *ct);
> int xe_guc_ct_enable(struct xe_guc_ct *ct);
> void xe_guc_ct_disable(struct xe_guc_ct *ct);
> void xe_guc_ct_fast_path(struct xe_guc_ct *ct);
> +void xe_guc_ct_toggle(struct xe_guc_ct *ct, bool toggle);
>
> struct xe_guc_ct_snapshot *
> xe_guc_ct_snapshot_capture(struct xe_guc_ct *ct, bool atomic);
> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> index 784f53c5f282..bf75c39d929d 100644
> --- a/drivers/gpu/drm/xe/xe_uc.c
> +++ b/drivers/gpu/drm/xe/xe_uc.c
> @@ -148,7 +148,7 @@ int xe_uc_init_hw(struct xe_uc *uc)
> if (ret)
> return ret;
>
> - ret = xe_guc_enable_communication(&uc->guc);
> + ret = xe_guc_enable_communication(&uc->guc, true);
> if (ret)
> return ret;
>
More information about the Intel-xe
mailing list