Re: [PATCH] drm/xe/pf: Fix VF config validation on multi-GT platforms

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

On 06.08.2024 21:06, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Michal Wajdeczko
> Sent: Tuesday, August 6, 2024 11:05 AM
> To: intel-xe@lists.freedesktop.org
> Cc: Wajdeczko, Michal <Michal.Wajdeczko@intel.com>; Piorkowski, Piotr <piotr.piorkowski@intel.com>
> Subject: [PATCH] drm/xe/pf: Fix VF config validation on multi-GT platforms
>>
>> When validating VF config on the media GT, we may wrongly report
>> that VF is already partially configured on it, as we consider GGTT
>> and LMEM provisioning done on the primary GT (since both GGTT and
>> LMEM are tile-level resources, not a GT-level).
>>
>> This will cause skipping a VF auto-provisioning on the media-GT and
>> in result will block a VF from successfully initialize that GT.
>>
>> Fix that by considering GGTT and LMEM configurations only when
>> checking if a VF provisioning is complete, and omit GGTT and LMEM
>> when reporting empty/partial provisioning.
>>
>> Fixes: 234670cea9a2 ("drm/xe/pf: Skip fair VFs provisioning if already provisioned")
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Piotr Piórkowski <piotr.piorkowski@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 52c7277d243d..227527785afd 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
>> @@ -1928,6 +1928,7 @@ static int pf_validate_vf_config(struct xe_gt *gt, unsigned int vfid)
>>  {
>>  	struct xe_gt *primary_gt = gt_to_tile(gt)->primary_gt;
>>  	struct xe_device *xe = gt_to_xe(gt);
>> +	bool is_primary = !xe_gt_is_media_type(gt);
>>  	bool valid_ggtt, valid_ctxs, valid_dbs;
>>  	bool valid_any, valid_all;
>>  
>> @@ -1936,13 +1937,17 @@ static int pf_validate_vf_config(struct xe_gt *gt, unsigned int vfid)
>>  	valid_dbs = pf_get_vf_config_dbs(gt, vfid);
>>  
>>  	/* note that GuC doorbells are optional */
>> -	valid_any = valid_ggtt || valid_ctxs || valid_dbs;
>> -	valid_all = valid_ggtt && valid_ctxs;
>> +	valid_any = valid_ctxs || valid_dbs;
>> +	valid_all = valid_ctxs;
>> +
>> +	/* and GGTT/LMEM is configured on primary GT only */
>> +	valid_all = valid_all && valid_ggtt;
>> +	valid_any = valid_any || (valid_ggtt && is_primary);
> 
> I can see why you would want to keep the ggtt configuration separate from
> the rest of the config, though it might be cleaner to just keep it all collected
> together like it was before. 

it's still collected together as it was, just final flags are calculated
separately ;)

> Not blocking on this, though:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>

thanks!

> -Jonathan Cavitt
> 
>>  
>>  	if (IS_DGFX(xe)) {
>>  		bool valid_lmem = pf_get_vf_config_ggtt(primary_gt, vfid);
>>  
>> -		valid_any = valid_any || valid_lmem;
>> +		valid_any = valid_any || (valid_lmem && is_primary);
>>  		valid_all = valid_all && valid_lmem;
>>  	}
>>  
>> -- 
>> 2.43.0
>>
>>