[PATCH 2/4] drm/amdkfd: Adding new IOCTL for scratch memory

[Felix Kuehling]

[dropping Moses and Ben, they no longer work for AMD and the email
addresses bounce]


On 2017-08-13 05:04 AM, Oded Gabbay wrote:
> On Sat, Aug 12, 2017 at 7:47 AM, Felix Kuehling <Felix.Kuehling at amd.com> wrote:
>> From: Moses Reuben <moses.reuben at amd.com>
>>
>> Signed-off-by: Moses Reuben <moses.reuben at amd.com>
>> Signed-off-by: Ben Goz <ben.goz at amd.com>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
[snip]
>> @@ -286,7 +293,17 @@ struct kfd_ioctl_wait_events_args {
>>  #define AMDKFD_IOC_DBG_WAVE_CONTROL            \
>>                 AMDKFD_IOW(0x10, struct kfd_ioctl_dbg_wave_control_args)
>>
>> +/* TODO:
>> + * - AMDKFD_IOC_ALLOC_MEMORY_OF_GPU
>> + * - AMDKFD_IOC_FREE_MEMORY_OF_GPU
>> + * - AMDKFD_IOC_MAP_MEMORY_TO_GPU
>> + * - AMDKFD_IOC_UNMAP_MEMORY_FROM_GPU
>> + */
>> +
>> +#define AMDKFD_IOC_ALLOC_MEMORY_OF_SCRATCH     \
>> +               AMDKFD_IOWR(0x15, struct kfd_ioctl_alloc_memory_of_scratch_args)
>> +
>>  #define AMDKFD_COMMAND_START           0x01
>> -#define AMDKFD_COMMAND_END             0x11
>> +#define AMDKFD_COMMAND_END             0x16
> You create a hole here, between 0x11 to 0x16. This would make the
> sanity check in kfd_ioctl() to be useless.

I'm creating the hole to match the ABI with our current ROCm releases.
The TODO comment lists the ioctls that will slot into the hole in future
patches.

kfd_ioctl checks that the function pointer is initialized and fails
otherwise. So I think this hole should work OK and attempts to call
unassigned ioctls should fail as expected:

        func = ioctl->func;

        if (unlikely(!func)) {
                dev_dbg(kfd_device, "no function\n");
                retcode = -EINVAL;
                goto err_i1;
        }


>
> Also, why not do a generic IOCTL for allocating GPU memory, and pass
> parameter if its scratch or something else, similar to amdgpu's
> "AMDGPU_GEM_DOMAIN_*" defines ?

The real memory allocation ioctl has more parameters. Also, I always
thought the name "ALLOC_MEMORY_OF_SCRATCH" was misleading, because this
ioctl doesn't really allocate anything, and there is no corresponding
"free". What it really does, it configures the virtual address of the
scratch backing memory for the process.

Maybe we could change the name of the ioctl (without changing the ABI)
to something like SET_SCRATCH_BACKING_VA. What do you think?

Regards,
  Felix

>
>>  #endif
>> --
>> 2.7.4
>>