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

[Christian König]

Am 14.08.2017 um 17:10 schrieb 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.

That's usually not such a good idea, cause interface rare upstream as 
they are designed internally.

Probably better to just add them to the end of the list.

>
> 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?

That's better, but I think something in the direction of VA address 
config would be better.

BTW: What exactly this this good for?

Regards,
Christian.

>
> Regards,
>    Felix
>
>>>   #endif
>>> --
>>> 2.7.4
>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx