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

[Danilo Krummrich <dakr@kernel.org>]

(Cc: dri-devel)

On Tue, Jun 10, 2025 at 03:05:34PM +0200, Christian König wrote:
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> > index 5a8bc6342222..6108a6f9dba7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> > @@ -44,6 +44,22 @@
> >  struct amdgpu_fence;
> >  enum amdgpu_ib_pool_type;
> >  
> > +/* Internal kernel job ids. (decreasing values, starting from U64_MAX). */
> > +#define AMDGPU_KERNEL_JOB_ID_VM_UPDATE              (18446744073709551615ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_VM_UPDATE_PDES         (18446744073709551614ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_VM_UPDATE_RANGE        (18446744073709551613ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_VM_PT_CLEAR            (18446744073709551612ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_TTM_MAP_BUFFER         (18446744073709551611ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_TTM_ACCESS_MEMORY_SDMA (18446744073709551610ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_TTM_COPY_BUFFER        (18446744073709551609ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_CLEAR_ON_RELEASE       (18446744073709551608ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_MOVE_BLIT              (18446744073709551607ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_TTM_CLEAR_BUFFER       (18446744073709551606ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_CLEANER_SHADER         (18446744073709551605ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_FLUSH_GPU_TLB          (18446744073709551604ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_KFD_GART_MAP           (18446744073709551603ULL)
> > +#define AMDGPU_KERNEL_JOB_ID_VCN_RING_TEST          (18446744073709551602ULL)

Why not

	(U64_MAX - {1, 2, ...})?

> Mhm, reiterating our internal discussion on the mailing list.
> 
> I think it would be nicer if we could use negative values for the kernel submissions and positive for userspace. But as discussed internally we would need to adjust the scheduler trace points for that once more.
> 
> @Philip and @Danilo any opinion on that?

Both, the U64_MAX and the positive-negative approach, are a bit hacky. I wonder
why we need client_id to be a u64, wouldn't a u32 not be enough?

Anyways, if client_id remains to be a u64, we could add a global DRM constant
instead, e.g.

	#define DRM_CLIENT_ID_MAX	0x0fffffffffffffff
	#define DRM_KERNEL_ID_BASE	(DRM_CLIENT_ID_MAX + 1)

And in drm_file_alloc() fail if we're out of IDs.

I think this would be much cleaner.