On Wed, Jun 11, 2025 at 04:57:50PM +0200, Christian König wrote:
> On 6/11/25 16:25, Danilo Krummrich wrote:
> > (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, ...})?
>
> That's what Pierre came up with as well, but I thought that this is ugly
> because it makes it hard to match the numbers from the trace back to
> something in the code.
>
> >> 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?
>
> That can trivially overflow on long running boxes.
I don't know if "trivially" is the word of choice given that the number is
4,294,967,295.
But I did indeed miss that this is a for ever increasing atomic. Why is it an
atomic? Why is it not an IDA?
> > 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.
>
> Mhm, I wouldn't mind printing the client id as hex and then always setting
> the highest bit for kernel submissions.
>
> But when we keep printing them as base 10 it kind of becomes unreadable.
>
> Christian.
>
> >
> > I think this would be much cleaner.
>
