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

[Riana Tauro <riana.tauro@intel.com>]

Hi Michal

Thank you for the review comments

On 7/17/2024 1:34 PM, Michal Wajdeczko wrote:
> 
> 
> 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


yeah that is the reason i had split the functions in the last rev.
Will follow the alternate solution of passing the register_ctb
to the CT layer.

Thanks,
Riana

> 
>>
>> 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;
>>