Re: [PATCH v4 1/2] drm/xe: modify GuC CTB enable communication

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 17.07.2024 07:55, Riana Tauro wrote:
> Modify GuC CTB enable communication function to only set the
> CTB state if registration is not required.
> This will be used while resuming from D3Hot where
> registration of CTB is not necessary.
> 
> Also move xe_guc_ct_set_state to header file.

hmm, but this will expose internals of the guc_ct layer to the outside
see below for alternate solution

> 
> v2: split patches (Rodrigo)
> 
> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc.c    | 25 +++++++++++++++++++------
>  drivers/gpu/drm/xe/xe_guc.h    |  2 +-
>  drivers/gpu/drm/xe/xe_guc_ct.c |  3 +--
>  drivers/gpu/drm/xe/xe_guc_ct.h |  2 ++
>  drivers/gpu/drm/xe/xe_uc.c     |  4 ++--
>  5 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index eb655cee19f7..8131c290a4f4 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -755,7 +755,7 @@ static int vf_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;
>  
> @@ -800,7 +800,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;
>  
> @@ -854,7 +854,16 @@ 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 CTB communication
> + * @guc: GuC object
> + * @register_ctb: true to register CTB, false otherwise
> + *
> + * Enables GuC CTB Communication and IRQ
> + *
> + * Return: 0 on success, negative error code otherwise
> + */
> +int xe_guc_enable_communication(struct xe_guc *guc, bool register_ctb)
>  {
>  	struct xe_device *xe = guc_to_xe(guc);
>  	int err;
> @@ -870,9 +879,13 @@ int xe_guc_enable_communication(struct xe_guc *guc)
>  		guc_enable_irq(guc);
>  	}
>  
> -	err = xe_guc_ct_enable(&guc->ct);
> -	if (err)
> -		return err;
> +	if (register_ctb) {
> +		err = xe_guc_ct_enable(&guc->ct);
> +		if (err)
> +			return err;
> +	} else {
> +		xe_guc_ct_set_state(&guc->ct, XE_GUC_CT_STATE_ENABLED);
> +	}

maybe better option would be to pass the register_ctb flag to the
xe_guc_ct_enable() so it can do either full enabling (including CTB
registration) or just change the internal state:

int xe_guc_enable_communication(struct xe_guc *guc, bool register_ctb)
{
	err = xe_guc_ct_enable(&guc->ct, register_ctb);
	...
}

int xe_guc_ct_enable(struct xe_guc_ct *ct, bool register_ctb)
{
	if (register_ctb) {
		...
	}

	ct_set_state(ct, XE_GUC_CT_STATE_ENABLED);
	return 0;
}

>  
>  	guc_handle_mmio_msg(guc);
>  
> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
> index af59c9545753..b49444297eb0 100644
> --- a/drivers/gpu/drm/xe/xe_guc.h
> +++ b/drivers/gpu/drm/xe/xe_guc.h
> @@ -20,7 +20,7 @@ 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);
> +int xe_guc_enable_communication(struct xe_guc *guc, bool register_ctb);
>  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 7d2e937da1d8..44c44e086a98 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -318,8 +318,7 @@ static int guc_ct_control_toggle(struct xe_guc_ct *ct, bool enable)
>  	return ret > 0 ? -EPROTO : ret;
>  }
>  
> -static void xe_guc_ct_set_state(struct xe_guc_ct *ct,
> -				enum xe_guc_ct_state state)
> +void xe_guc_ct_set_state(struct xe_guc_ct *ct, enum xe_guc_ct_state state)
>  {
>  	mutex_lock(&ct->lock);		/* Serialise dequeue_one_g2h() */
>  	spin_lock_irq(&ct->fast_lock);	/* Serialise CT fast-path */
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.h b/drivers/gpu/drm/xe/xe_guc_ct.h
> index 190202fce2d0..60d46739548c 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.h
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.h
> @@ -15,6 +15,8 @@ int xe_guc_ct_enable(struct xe_guc_ct *ct);
>  void xe_guc_ct_disable(struct xe_guc_ct *ct);
>  void xe_guc_ct_stop(struct xe_guc_ct *ct);
>  void xe_guc_ct_fast_path(struct xe_guc_ct *ct);
> +void xe_guc_ct_set_state(struct xe_guc_ct *ct,
> +			 enum xe_guc_ct_state state);
>  
>  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 0f240534fb72..eb9c1443c849 100644
> --- a/drivers/gpu/drm/xe/xe_uc.c
> +++ b/drivers/gpu/drm/xe/xe_uc.c
> @@ -154,7 +154,7 @@ static int vf_uc_init_hw(struct xe_uc *uc)
>  	if (err)
>  		return err;
>  
> -	err = xe_guc_enable_communication(&uc->guc);
> +	err = xe_guc_enable_communication(&uc->guc, true);
>  	if (err)
>  		return err;
>  
> @@ -194,7 +194,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;
>