[PATCH] drm/xe/pf: Skip LMTT update if no LMEM was provisioned

[Michal Wajdeczko]

On 7/31/2025 10:14 PM, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Michal Wajdeczko
> Sent: Thursday, July 31, 2025 12:47 PM
> To: intel-xe at lists.freedesktop.org
> Cc: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>
> Subject: [PATCH] drm/xe/pf: Skip LMTT update if no LMEM was provisioned
>>
>> During VF unprovisioning, if VF was not provisioned with LMEM,
>> there is no need to trigger LMTT update, as VF LMTT was never
>> set. This will spare us sending full TLB invalidation requests.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> 
> Some nits below, but I'm not going to make any of the suggestions a requirement,
> so:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> 
>> ---
>>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
>> index f00cce81f1ff..692b174dd5b8 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
>> @@ -1434,8 +1434,10 @@ static int pf_update_vf_lmtt(struct xe_device *xe, unsigned int vfid)
>>  	return err;
>>  }
>>  
>> -static void pf_release_vf_config_lmem(struct xe_gt *gt, struct xe_gt_sriov_config *config)
>> +static bool pf_release_vf_config_lmem(struct xe_gt *gt, struct xe_gt_sriov_config *config)
>>  {
>> +	bool released = false;
>> +
>>  	xe_gt_assert(gt, IS_DGFX(gt_to_xe(gt)));
>>  	xe_gt_assert(gt, xe_gt_is_main_type(gt));
>>  	lockdep_assert_held(xe_gt_sriov_pf_master_mutex(gt));
>> @@ -1443,7 +1445,9 @@ static void pf_release_vf_config_lmem(struct xe_gt *gt, struct xe_gt_sriov_confi
>>  	if (config->lmem_obj) {
>>  		xe_bo_unpin_map_no_vm(config->lmem_obj);
>>  		config->lmem_obj = NULL;
>> +		released = true;
>>  	}
>> +	return released;
> 
> NIT:
> Storing the return value of this function doesn't seem immediately necessary,
> as we could probably just return the value directly as a part of this branch:
> 
> """
> 	if (config->lmem_obj) {
> 		xe_bo_unpin_map_no_vm(config->lmem_obj);
> 		config->lmem_obj = NULL;
> 		return true;
> 	}
> 	return false;
> """
> 
> But I had a similar thought in a prior patch of yours, and I agree with
> the sentiment that it may be useful in the future if we ever need to use
> the return value for anything else.  It also doesn't hurt anything to keep,
> so I won't make it a requirement.
> 
>>  }
>>  
>>  static int pf_provision_vf_lmem(struct xe_gt *gt, unsigned int vfid, u64 size)
>> @@ -2021,12 +2025,13 @@ static void pf_release_vf_config(struct xe_gt *gt, unsigned int vfid)
>>  {
>>  	struct xe_gt_sriov_config *config = pf_pick_vf_config(gt, vfid);
>>  	struct xe_device *xe = gt_to_xe(gt);
>> +	bool released;
>>  
>>  	if (xe_gt_is_main_type(gt)) {
>>  		pf_release_vf_config_ggtt(gt, config);
>>  		if (IS_DGFX(xe)) {
>> -			pf_release_vf_config_lmem(gt, config);
>> -			if (xe_device_has_lmtt(xe))
>> +			released = pf_release_vf_config_lmem(gt, config);
>> +			if (released && xe_device_has_lmtt(xe))
> 
> NIT:
> Same as above: storing the value of pf_release_vf_config_lmem isn't
> immediately necessary, and we could just pass the result directly into
> the if-statement:
> 
> """
> 	if (pf_release_vf_config_lmem(gt, config) &&
> 	    xe_device_has_lmtt(xe))
> """

I'm really against obfuscating the code just to some save few characters/lines

modern compilers can deal with that and our priority should be clear code

making complex if-statements with nested function calls is not making code
easier to understand or more resilient to mistakes

> 
> But, again, it doesn't hurt to have, and it'll be useful if we ever expand this
> function in the future and need that value, so there's no required changes
> here.
> 
> -Jonathan Cavitt
> 
> 
>>  				pf_update_vf_lmtt(xe, vfid);
>>  		}
>>  	}
>> -- 
>> 2.47.1
>>
>>