Re: [PATCH 1/2] drm/amdgpu: add userq specific kernel config for fence ioctls

["Paneer Selvam, Arunpravin" <arunpravin.paneerselvam@amd.com>]

On 10/29/2024 7:33 PM, Christian König wrote:
> Am 29.10.24 um 14:32 schrieb Alex Deucher:
>> On Tue, Oct 29, 2024 at 5:38 AM Christian König
>> <christian.koenig@amd.com> wrote:
>>> Am 24.10.24 um 14:10 schrieb Arunpravin Paneer Selvam:
>>>> Keep the user queue fence signal and wait IOCTLs in the
>>>> kernel config CONFIG_DRM_AMDGPU_NAVI3X_USERQ.
>>>>
>>>> Signed-off-by: Arunpravin Paneer Selvam 
>>>> <Arunpravin.PaneerSelvam@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c         |  4 ++++
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 16 
>>>> ++++++++++++++++
>>>>    2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> index 70cb3b794a8a..04eb6611d19b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> @@ -2971,9 +2971,11 @@ static int __init amdgpu_init(void)
>>>>        if (r)
>>>>                goto error_sync;
>>>>
>>>> +#ifdef CONFIG_DRM_AMDGPU_NAVI3X_USERQ
>>>>        r = amdgpu_fence_slab_init();
>>>>        if (r)
>>>>                goto error_fence;
>>>> +#endif
>>> That here makes no sense. This is for the kernel queues and not for the
>>> user queues.
>>>
>>>>        r = amdgpu_userq_fence_slab_init();
>>>>        if (r)
>>>> @@ -3003,7 +3005,9 @@ static void __exit amdgpu_exit(void)
>>>>        amdgpu_unregister_atpx_handler();
>>>>        amdgpu_acpi_release();
>>>>        amdgpu_sync_fini();
>>>> +#ifdef CONFIG_DRM_AMDGPU_NAVI3X_USERQ
>>>>        amdgpu_fence_slab_fini();
>>>> +#endif
>>>>        amdgpu_userq_fence_slab_fini();
>>>>        mmu_notifier_synchronize();
>>>>        amdgpu_xcp_drv_release();
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>> index 279dece6f6d7..bec53776fe5f 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>> @@ -318,6 +318,7 @@ static const struct dma_fence_ops 
>>>> amdgpu_userq_fence_ops = {
>>>>        .release = amdgpu_userq_fence_release,
>>>>    };
>>>>
>>>
>>>> +#ifdef CONFIG_DRM_AMDGPU_NAVI3X_USERQ
>>>>    /**
>>>>     * amdgpu_userq_fence_read_wptr - Read the userq wptr value
>>>>     *
>>>> @@ -502,7 +503,15 @@ int amdgpu_userq_signal_ioctl(struct 
>>>> drm_device *dev, void *data,
>>>>
>>>>        return r;
>>>>    }
>>>> +#else
>>>> +int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>>>> +                           struct drm_file *filp)
>>>> +{
>>>> +     return 0;
>>>> +}
>>>> +#endif
>>>>
>>>> +#ifdef CONFIG_DRM_AMDGPU_NAVI3X_USERQ
>>>>    int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>>>>                            struct drm_file *filp)
>>>>    {
>>>> @@ -797,3 +806,10 @@ int amdgpu_userq_wait_ioctl(struct drm_device 
>>>> *dev, void *data,
>>>>
>>>>        return r;
>>>>    }
>>>> +#else
>>>> +int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>>>> +                         struct drm_file *filp)
>>>> +{
>>>> +     return 0;
>>>> +}
>>>> +#endif
>>> Not nice, but since CONFIG_DRM_AMDGPU_NAVI3X_USERQ depends on
>>> CONFIG_BROKEN at the moment probably ok as intermediate step.
>> Wouldn't it be better to return an error in these cases?
>
> Good point, the functions should never be called in the first place 
> but better save than sorry.
Can I return -EINVAL instead of 0.

Thanks,
Arun.
>
> Christian.
>
>>
>> Alex
>>
>>> Regards,
>>> Christian.
>>>
>