Re: [PATCH v1 3/3] drm/xe/pf: Add functions to save and restore VF LMEM and CCS data

["Laguna, Lukasz" <lukasz.laguna@intel.com>]

On 10/17/2024 13:12, Michal Wajdeczko wrote:
>
> On 16.10.2024 11:57, Lukasz Laguna wrote:
>> State of VF LMEM and corresponding CCS metadata needs to be saved and
>> restored during migration of VM with attached dGPU VF. Add helpers that
>> allow it. We will start using them in upcoming patches.
>>
>> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com
>> ---
>>   drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.c | 100 ++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.h |   7 ++
>>   2 files changed, 107 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.c
>> index c712111aa30d..feb22d3e5dd5 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.c
>> @@ -12,6 +12,7 @@
>>   #include "xe_gt_sriov_printk.h"
>>   #include "xe_guc.h"
>>   #include "xe_guc_ct.h"
>> +#include "xe_migrate.h"
>>   #include "xe_sriov.h"
>>   
>>   /* Return: number of dwords saved/restored/required or a negative error code on failure */
>> @@ -382,6 +383,105 @@ ssize_t xe_gt_sriov_pf_migration_write_guc_state(struct xe_gt *gt, unsigned int
>>   }
>>   #endif /* CONFIG_DEBUG_FS */
>>   
>> +static int pf_save_restore_lmem(struct xe_gt *gt, unsigned int vfid, struct xe_bo *sysmem,
>> +				struct xe_bo *ccs, u64 offset, size_t size, bool save)
> bool flags are not welcomed by the maintainters

I understand that bool flags should be avoided, but in this particular 
case using bool seems to be the best option. It doesn't impact code 
readability, doesn't add any complexity in the function logic and 
doesn't introduce any confusion.

>
>> +{
>> +	struct xe_bo *vram;
>> +	struct dma_fence *fence;
>> +	int ret;
> you may want to assert that vfid != PFID

OK.

>
>> +
>> +	if (!sysmem && !ccs)
>> +		return -EINVAL;
> input parameters shall be validated at the top public function, in all
> internal static functions/helpers we should just have asserts

OK.

>
>> +
>> +	vram = gt->sriov.pf.vfs[vfid].config.lmem_obj;
> access to config.lmem_obj should be under xe_gt_sriov_pf_master_mutex

OK.

>
>> +	if (!vram)
>> +		return -ENXIO;
>> +
>> +	if (!xe_bo_trylock(vram))
>> +		return -EBUSY;
>> +
>> +	if (sysmem) {
>> +		if (!xe_bo_trylock(sysmem)) {
>> +			ret = -EBUSY;
>> +			goto err_vram_unlock;
>> +		}
>> +	}
>> +	if (ccs) {
>> +		if (!xe_bo_trylock(ccs)) {
>> +			ret = -EBUSY;
>> +			goto err_sysmem_unlock;
>> +		}
>> +	}
>> +
>> +	fence = xe_migrate_raw_vram_copy(vram, offset, sysmem, 0, ccs, 0, size, save);
>> +	if (IS_ERR(fence)) {
>> +		ret = PTR_ERR(fence);
>> +		goto err_ccs_unlock;
>> +	}
>> +	ret = dma_fence_wait_timeout(fence, false, 5 * HZ);
> do you want to keep this hardcoded 5s for all cases?
> maybe the fence should be returned to the caller?

Good point, will do it.

>
>> +	if (!ret)
>> +		ret = -ETIME;
>> +
>> +	dma_fence_put(fence);
>> +
>> +err_ccs_unlock:
>> +	if (ccs)
>> +		xe_bo_unlock(ccs);
>> +err_sysmem_unlock:
>> +	if (sysmem)
>> +		xe_bo_unlock(sysmem);
>> +err_vram_unlock:
>> +	xe_bo_unlock(vram);
>> +
>> +	return ret < 0 ? ret : 0;
>> +}
>> +
>> +/**
>> + * xe_gt_sriov_pf_migration_save_lmem() - Save a VF LMEM and corresponding CCS.
>> + * @gt: the &xe_gt
>> + * @vfid: the VF identifier
>> + * @smem: the sysmem BO to save LMEM data to (NULL if not needed)
> likely the vfio driver will not be able to provide such BO, so do we
> plan to introduce yet another layer for it?

Yes, for now it will be operating on virtual addresses of mapped objects 
and in the future we'll probably switch to dma-bufs.

>
>> + * @ccs: the sysmem BO to save CCS data to (NULL if not needed)
> shouldn't the logic about need of the CCS data be somehow hidden from
> the caller?

No, it should be a caller decision, plus we want to have a separate BO 
for CCS anyway. It was added on purpose.

>
>> + * @offset: the offset of VF LMEM chunk
>> + * @size: the size of VF LMEM chunk
>> + *
>> + * This function is for PF only.
>> + *
>> + * This functions saves VF LMEM and corresponding CCS data into sysmem buffer
>> + * objects.
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int xe_gt_sriov_pf_migration_save_lmem(struct xe_gt *gt, unsigned int vfid,
>> +				       struct xe_bo *sysmem, struct xe_bo *ccs, u64 offset,
>> +				       size_t size)
>> +{
>> +	return pf_save_restore_lmem(gt, vfid, sysmem, ccs, offset, size, true);
>> +}
>> +
>> +/**
>> + * xe_gt_sriov_pf_migration_restore_lmem() - Restore a VF LMEM and corresponding CCS.
>> + * @gt: the &xe_gt
>> + * @vfid: the VF identifier
>> + * @smem: the sysmem BO to restore LMEM data from (NULL if not needed)
>> + * @ccs: the sysmem BO to restore CCS data from (NULL if not needed)
>> + * @offset: the offset of VF LMEM chunk
>> + * @size: the size of VF LMEM chunk
>> + *
>> + * This function is for PF only.
>> + *
>> + * This functions restores VF LMEM and corresponding CCS data from sysmem
>> + * buffer objects.
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int xe_gt_sriov_pf_migration_restore_lmem(struct xe_gt *gt, unsigned int vfid,
>> +					  struct xe_bo *sysmem, struct xe_bo *ccs, u64 offset,
>> +					  size_t size)
>> +{
>> +	return pf_save_restore_lmem(gt, vfid, sysmem, ccs, offset, size, false);
>> +}
>> +
>>   static bool pf_check_migration_support(struct xe_gt *gt)
>>   {
>>   	/* GuC 70.25 with save/restore v2 is required */
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.h b/drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.h
>> index 09faeae00ddb..dc1e6c5b44fa 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_migration.h
>> @@ -8,11 +8,18 @@
>>   
>>   #include <linux/types.h>
>>   
>> +struct xe_bo;
>>   struct xe_gt;
>>   
>>   int xe_gt_sriov_pf_migration_init(struct xe_gt *gt);
>>   int xe_gt_sriov_pf_migration_save_guc_state(struct xe_gt *gt, unsigned int vfid);
>>   int xe_gt_sriov_pf_migration_restore_guc_state(struct xe_gt *gt, unsigned int vfid);
>> +int xe_gt_sriov_pf_migration_save_lmem(struct xe_gt *gt, unsigned int vfid,
>> +				       struct xe_bo *sysmem, struct xe_bo *ccs, u64 offset,
>> +				       size_t size);
>> +int xe_gt_sriov_pf_migration_restore_lmem(struct xe_gt *gt, unsigned int vfid,
>> +					  struct xe_bo *sysmem, struct xe_bo *ccs, u64 offset,
>> +					  size_t size);
>>   
>>   #ifdef CONFIG_DEBUG_FS
>>   ssize_t xe_gt_sriov_pf_migration_read_guc_state(struct xe_gt *gt, unsigned int vfid,