Re: [PATCH v3 1/5] drm/xe/vf: Helper for telling whether CCS migration BBs are needed

["Lis, Tomasz" <tomasz.lis@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4765 bytes --]


On 10/16/2025 9:57 PM, Michal Wajdeczko wrote:
>
> On 10/16/2025 2:05 PM, Tomasz Lis wrote:
>> To avoid having duplicated checks, and also provide clear
>> documentation of what are we checking for, the conditions
>> for initing CCS BBs for VF migration are closed into a function.
> I would drop "BBs" from title and commit message, as this is
> just an implementation detail of the "VF CCS migration" feature
> that doesn't need to be exposed right now.

Is it though? Does that mean DGPUs do not need "VF CCS migration"?

VF CCS migration, like migration of all chunks/blocks of VF state, is 
handled by PF side.

What we have here is VF-side workaround for hardware limitation of few 
integrated platforms. When it's no longer needed on the next platform, 
will we say VF CCS migration was eliminated?

This workaround has been named under main feature name. That wasn't the 
best idea, but it's done. Though in future changes, I'd propose we move 
towards naming it what it is.

We may change the name to something else, but disguising it under main 
feature name does not feel right to me. It is important part of this 
feature for certain platforms, but it is not this feature.

>> Signed-off-by: Tomasz Lis<tomasz.lis@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_sriov_vf_ccs.c | 15 ++++++++++++++-
>>   drivers/gpu/drm/xe/xe_sriov_vf_ccs.h |  2 ++
>>   2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
>> index 790249801364..7688d1baf42f 100644
>> --- a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
>> +++ b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
>> @@ -260,6 +260,19 @@ int xe_sriov_vf_ccs_register_context(struct xe_device *xe)
>>   	return err;
>>   }
>>   
>> +/**
>> + * xe_sriov_vf_ccs_migration_bb_needed - Whether GuC requires CCS copy BBs for VF migration.
> all functions in xe_sriov_vf_ccs.c are related to "migration"
> but only this one mention it in its name, either rename other
> functions to match this one, or name this one simply as:
>
>        xe_sriov_vf_ccs_is_needed()

When I imagine anyone trying to understand this code and reading 
"vf_ccs_is_needed", it's hard for me to find a path when it doesn't just 
end with confusion. That would be a confusing name.

So I guess renaming everything in the file is needed. But that should be 
done as separate patch, and most likely will end up being a separate series.

We may still discuss whether "ccs_migration_bb" should be the name, or 
something else/shorter. I considered "ccs_wa" - shorter, but rather 
generic, and by this logic the whole recovery should have "_wa" in 
function names. I also considered "ccs_lrcas"/"ccs_migration_lrcas", but 
the additional contexts are really just required accompanying stuff to 
what is the central part here - the batch buffers. And that's how I 
settled on the current name.

The word "migration", when not accompanying directly by "vf", also has a 
different meaning in the Xe driver; but I can't find a more unique but 
still true word. We have "recovery", but that is only connected to 
"restore" part of the process. The words "vf" and "sriov" are still 
close in the name, so I consider that good enough.

>> + * @xe: the &xe_device instance.
>> + *
>> + * Only selected platforms require VF KMD to maintain CCS copy BBs and linked LRCAs.
>> + *
>> + * Return: true if the BBs are needed, false otherwise.
> "true if VF driver must participate in the CCS migration"
that's good. Ack.
>
>> + */
>> +bool xe_sriov_vf_ccs_migration_bb_needed(struct xe_device *xe)
>> +{
> this should be always called from VF code, so even if not strictly required:
>
> 	xe_assert(xe, IS_SRIOV(xe))

ack.

-Tomasz

>
>> +	return !IS_DGFX(xe) && xe_device_has_flat_ccs(xe);
>> +}
>> +
>>   static void xe_sriov_vf_ccs_fini(void *arg)
>>   {
>>   	struct xe_sriov_vf_ccs_ctx *ctx = arg;
>> @@ -294,7 +307,7 @@ int xe_sriov_vf_ccs_init(struct xe_device *xe)
>>   	xe_assert(xe, IS_SRIOV_VF(xe));
>>   	xe_assert(xe, xe_sriov_vf_migration_supported(xe));
>>   
>> -	if (IS_DGFX(xe) || !xe_device_has_flat_ccs(xe))
>> +	if (!xe_sriov_vf_ccs_migration_bb_needed(xe))
>>   		return 0;
>>   
>>   	for_each_ccs_rw_ctx(ctx_id) {
>> diff --git a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.h b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.h
>> index f8ca6efce9ee..dcba4173c712 100644
>> --- a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.h
>> +++ b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.h
>> @@ -14,6 +14,8 @@ struct drm_printer;
>>   struct xe_device;
>>   struct xe_bo;
>>   
>> +bool xe_sriov_vf_ccs_migration_bb_needed(struct xe_device *xe);
>> +
>>   int xe_sriov_vf_ccs_init(struct xe_device *xe);
>>   int xe_sriov_vf_ccs_attach_bo(struct xe_bo *bo);
>>   int xe_sriov_vf_ccs_detach_bo(struct xe_bo *bo);

[-- Attachment #2: Type: text/html, Size: 7101 bytes --]