[PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Ghimiray, Himal Prasad]

On 12-08-2025 21:36, Danilo Krummrich wrote:
> On Mon Aug 11, 2025 at 8:52 AM CEST, Himal Prasad Ghimiray wrote:
>> On 09-08-2025 18:53, Danilo Krummrich wrote:
>> Possible scenarios for ops functionality based on input start and end
>> address from user:
>>
>> a) User-provided range is a subset of an existing drm_gpuva
>> Expected Result: Same behavior as the default sm_map logic.
>> Reference: Case 1 from [1].
>>
>> b) Either start or end (but not both) is not aligned with a drm_gpuva
>> boundary
>> Expected Result: One REMAP and one MAP operation.
>> Reference: Case 3 from [1].
>>
>> Existing GPUVMAs:
>>
>>            drm_gpuva1                        drm_gpuva2
>> 	[a----------------------------b-1][b-------------------c-1]
>>
>> User Input to ops:
>>     start = inside drm_gpuva1
>>     end   = exactly at c-1 (end of drm_gpuva2)
>>
>> Resulting Mapping:
>> 	drm_gpuva1:pre       drm_gpuva:New map     drm_gpuva2
>> 	[a---------start-1][start------- b-1] [b------------c-1]
>>
>> Ops Created:
>>     REMAP:UNMAP drm_gpuva1 a to b
>>     REMAP:PREV a to start - 1
>>     MAP: start to b-1
>>
>> Note: No unmap of drm_gpuvma2 and no merging of New map and drm_gpuva2.
>>
>> c) Both start and end are not aligned with drm_gpuva boundaries, and
>> they fall within different drm_gpuva regions
>> Expected Result: Two REMAP operations and two MAP operations.
>> Reference: Case 2 from [1].
>>
>>
>> d) User-provided range does not overlap with any existing drm_gpuva
>> Expected Result: No operations.
>> start and end exactly match the boundaries of one or more existing
>> drm_gpuva regions
>>
>> e) This includes cases where start is at the beginning of drm_gpuva1 and
>> end is at the end of drm_gpuva2 (drm_gpuva1 and drm_gpuva2 can be same
>> or different).
>> Expected Result: No operations
>>
>> [1]
>> https://lore.kernel.org/intel-xe/4203f450-4b49-401d-81a8-cdcca02035f9@intel.com/
> 
> <snip>
> 
>> I’ve tried to explain the behavior/usecase with madvise and expected
>> outcomes of the ops logic in detail in [1]. Could you please take a
>> moment to review that and let me know if the explanation is sufficient
>> or if any part needs further clarification?
> 
> Thanks a lot for writing this up!
> 
> I think this clarifies everything, the examples from [1] are good (sorry that
> your reply from the RFC got lost somehow on my end).
> 
>>> Please add a separate section about madvise operations to the documentation at
>>> the beginning of the drivers/gpu/drm/drm_gpuvm.c file.
>>
>> Sure will do that.
> 
> Great, this will help users (as well as reviewers) a lot. Please also add your
> examples from [1] to this entry, similar to the existing examples for sm_map.
> 
>>>> v2
>>>> - use drm_gpuvm_sm_map_ops_create with flags instead of defining new
>>>>     ops_create (Danilo)
>>>
>>> If this turns out not to be what I thought semantically and we still agree it's
>>> the correct approach, I think I have to take this back and it should indeed be
>>> an entirely separate code path. But let's wait for your answers above.
> 
> Having the correct understanding of how this is supposed to work (and seeing how
> the code turns out) I think it's still OK to integrate it into sm_map().
> 
> However, it probably makes sense to factor out the code into a common function
> and then build the madvise() and sm_map() functions on top of it.

__drm_gpuvm_sm_map is that common function, and does 
drm_gpuvm_madvise_ops_create sound OK? With separate functions for 
sm_map and madvise, I see there's no need to add a flag to 
drm_gpuvm_map_req at this moment. I will drop [1] in the next version.

[1] https://patchwork.freedesktop.org/patch/667561/?series=149550&rev=6

Thanks
> 
> Please also find some more comments on the patch itself.
> 
>>> Again, I really think this needs some proper documentation like in the
>>> "DOC: Split and Merge" documentation section.
>>
>> Sure
> 
> Thanks!