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

[Danilo Krummrich <dakr@kernel.org>]

On Wed, Jul 02, 2025 at 11:23:26AM +0200, Pierre-Eric Pelloux-Prayer wrote:
> 
> 
> 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.

I mean, you're right, you can definitely do that, it's entirely up to the driver
what to pass as a debug cookie to drm_sched_job_init().

I'm just saying that you're completely on your own if the implementation of
file->client_id would change (which admittedly is unlikely). In such a case
you'd have to accept that potentially a change silently breaks your driver and
that people are free to ignore this fact.

In this case it's probably not that big a deal, but still I like to create some
awareness that this class of solutions (i.e. rely on how generic infrastructure
works internally) is usually not a good idea at all, since it's error prone and
is giving maintainers a hard time.