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.
