[PATCH v1] drm/amdgpu: give each kernel job a unique id

[Pierre-Eric Pelloux-Prayer]

Le 18/06/2025 à 11:18, Pierre-Eric Pelloux-Prayer a écrit :
> 
> 
> 
>>>
>>> Adding an API to reserve fixed numbers would work but:
>>> * if the fixed numbers are chosen by the driver ("drm_reserve_id(u64_max
>>> -1)"), I don't see the benefit over the current patch
>>> * if the fixed numbers are allocated by drm (drm_reserve_id("vm_update") ->
>>> u64), it would then require a way to expose them to userspace (through
>>> /sys/kernel/debug/dri/x/clients?)
>>
>> Yeah, both is possible, I'm fine with either.
>>
>> The benefit is that this way it becomes an official API, which can (and must)
>> be considered should there ever be a change on drm_file::client_id.
>>
>> If someone would patch drm_file::client_id to start allocating IDs from high to
>> low, your corresponding driver code would silently break, because it relies on
>> an implementation detail of something that is not an official API.
>>
>> Yes, I know that this exact case won't happen, but I guess you get the idea. :-)
> 
> After looking a bit more into this, I came to the conclusion that IMO the 2 above options aren't great:
> * the first adds a function that expose the possibility of reserving an id so we'll have to keep 
> track of such reserved ids for a benefit that is limited at best
> * the second option is nicer on the surface because it doesn't make the tools dependent on hard- 
> coded kernel ids. But it also requires quite a bit of changes and memory usage allocations.
> 
> Honestly I'm wondering if adding a comment to drm_file_alloc like this would be enough;
> 
>     /* Get a unique identifier for fdinfo.
>      * The highest ids may be used by drivers for tracing purposes. Overlapping is
>      * unlikely to occur, and if it does the impact will be limited to tracing.
>      */
>     file->client_id = atomic64_inc_return(&ident);
> 
> What do you think?
> 

ping?

btw, I don't think that adding a comment to drm_file is even useful.

What the original patch does is passing opaque ids to a function that expects
client_id (drm_sched_job_init).
These opaque ids could have any values, they won't interfere with fdinfo statistics
nor the driver inner working - they're just for tracing purpose.

Pierre-Eric


> Pierre-Eric
> 
>