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

["Christian König" <christian.koenig@amd.com>]

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.

Christian.

>
> Alex
>
>> Regards,
>> Christian.
>>