[PATCH v5 01/23] Introduce drm_gpuvm_sm_map_ops_flags enums for sm_map_ops

[Ghimiray, Himal Prasad]

On 24-07-2025 16:02, Caterina Shablia wrote:
> El jueves, 24 de julio de 2025 2:43:56 (hora de verano de Europa central),
> Matthew Brost escribió:
>> On Tue, Jul 22, 2025 at 03:38:14PM +0200, Danilo Krummrich wrote:
>>> (Cc: Caterina)
>>>
>>> On Tue Jul 22, 2025 at 3:35 PM CEST, Himal Prasad Ghimiray wrote:
>>>> - DRM_GPUVM_SM_MAP_NOT_MADVISE: Default sm_map operations for the input
>>>>
>>>>    range.
>>>>
>>>> - DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE: This flag is used by
>>>>
>>>>    drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
>>>>
>>>> user-provided range and split the existing non-GEM object VMA if the
>>>> start or end of the input range lies within it. The operations can
>>>> create up to 2 REMAPS and 2 MAPs. The purpose of this operation is to be
>>>> used by the Xe driver to assign attributes to GPUVMA's within the
>>>> user-defined range. Unlike drm_gpuvm_sm_map_ops_flags in default mode,
>>>> the operation with this flag will never have UNMAPs and
>>>> merges, and can be without any final operations.
>>>>
>>>> v2
>>>> - use drm_gpuvm_sm_map_ops_create with flags instead of defining new
>>>>
>>>>    ops_create (Danilo)
>>>>
>>>> - Add doc (Danilo)
>>>>
>>>> v3
>>>> - Fix doc
>>>> - Fix unmapping check
>>>>
>>>> v4
>>>> - Fix mapping for non madvise ops
>>>>
>>>> Cc: Danilo Krummrich <dakr at redhat.com>
>>>> Cc: Matthew Brost <matthew.brost at intel.com>
>>>> Cc: Boris Brezillon <bbrezillon at kernel.org>
>>>> Cc: <dri-devel at lists.freedesktop.org>
>>>> Signed-off-by: Himal Prasad Ghimiray<himal.prasad.ghimiray at intel.com>
>>>> ---
>>>>
>>>>   drivers/gpu/drm/drm_gpuvm.c            | 93 ++++++++++++++++++++------
>>>>   drivers/gpu/drm/nouveau/nouveau_uvmm.c |  1 +
>>>>   drivers/gpu/drm/xe/xe_vm.c             |  1 +
>>>
>>> What about the other drivers using GPUVM, aren't they affected by the
>>> changes?
>> Yes, this seemly would break the build or other users. If the baseline
>> includes the patch below that I suggest to pull in this is a moot point
>> though.
>>
>>>>   include/drm/drm_gpuvm.h                | 25 ++++++-
>>>>   4 files changed, 98 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>>>> index e89b932e987c..c7779588ea38 100644
>>>> --- a/drivers/gpu/drm/drm_gpuvm.c
>>>> +++ b/drivers/gpu/drm/drm_gpuvm.c
>>>> @@ -2103,10 +2103,13 @@ static int
>>>>
>>>>   __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>>>   
>>>>   		   const struct drm_gpuvm_ops *ops, void *priv,
>>>>   		   u64 req_addr, u64 req_range,
>>>>
>>>> +		   enum drm_gpuvm_sm_map_ops_flags flags,
>>>
>>> Please coordinate with Boris and Caterina here. They're adding a new
>>> request structure, struct drm_gpuvm_map_req.
>>>
>>> I think we can define it as
>>>
>>> 	struct drm_gpuvm_map_req {
>>> 	
>>> 		struct drm_gpuva_op_map map;
>>> 		struct drm_gpuvm_sm_map_ops_flags flags;
>>> 	
>>> 	}
>>
>> +1, I see the patch [2] and the suggested change to drm_gpuva_op_map
>> [3]. Both patch and your suggestion look good to me.
>>
>> Perhaps we try to accelerate [2] landing ahead of either series as
>> overall just looks like a good cleanup which can be merged asap.
> I'm not sure my patchset would be in a mergeable state any time soon -- I've
> discovered some issues with split/merge of repeated mappings while writing the
> doc, so it will be a while before I'll be submitting that again. [2] itself is
> in a good shape, absolutely feel free to submit that as part of your series.

Thanks for the confirmation. Will update next rev accordingly.

>>
>> Himal - I'd rebase on top [2], with Danilo suggestion in [3] if this
>> hasn't landed by your next rev.
>>
>> [2]
>> https://lore.kernel.org/all/20250707170442.1437009-4-caterina.shablia@colla
>> bora.com/ [3]
>> https://lore.kernel.org/all/DB61N61AKIJ3.FG7GUJBG386P@kernel.org/
>>> eventually.
>>>
>>> Please also coordinate on the changes in __drm_gpuvm_sm_map() below
>>> regarding Caterina's series [1], it looks like they're conflicting.
>>
>> It looks pretty minor actually. I'm sure if really matter who this is
>> race but yes, always good to coordinate.
>>
>>> [1]
>>> https://lore.kernel.org/all/20250707170442.1437009-1-caterina.shablia@col
>>> labora.com/>
>>>> +/**
>>>> + * enum drm_gpuvm_sm_map_ops_flags - flags for drm_gpuvm split/merge
>>>> ops
>>>> + */
>>>> +enum drm_gpuvm_sm_map_ops_flags {
>>>> +	/**
>>>> +	 * @DRM_GPUVM_SM_MAP_NOT_MADVISE: DEFAULT sm_map ops
>>>> +	 */
>>>> +	DRM_GPUVM_SM_MAP_NOT_MADVISE = 0,
>>>
>>> Why would we name this "NOT_MADVISE"? What if we add more flags for other
>>> purposes?
>>
>> How about...
>>
>> s/DRM_GPUVM_SM_MAP_NOT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_NONE/
>>
>>>> +	/**
>>>> +	 * @DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE: This flag is used by
>>>> +	 * drm_gpuvm_sm_map_ops_create to iterate over GPUVMA's in the
>>>> +	 * user-provided range and split the existing non-GEM object VMA
> if
>>>> the
>>>> +	 * start or end of the input range lies within it. The operations
> can
>>>> +	 * create up to 2 REMAPS and 2 MAPs. Unlike
> drm_gpuvm_sm_map_ops_flags
>>>> +	 * in default mode, the operation with this flag will never have
>>>> UNMAPs and +	 * merges, and can be without any final operations.
>>>> +	 */
>>>> +	DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE = BIT(0),
>>
>> Then normalize this one...
>>
>> s/DRM_GPUVM_SKIP_GEM_OBJ_VA_SPLIT_MADVISE/DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MA
>> DVISE/
>>
>> Matt
>>
>>>> +};
> 
> 
> 
>