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
