[PATCH v2 2/4] drm/vc4: Use DMA Resv to implement VC4 wait BO IOCTL

[Tvrtko Ursulin]

On 18/12/2024 11:36, Maíra Canal wrote:
> Hi Tvrtko,
> 
> On 18/12/24 07:41, Tvrtko Ursulin wrote:
>>
>> On 12/12/2024 20:20, Maíra Canal wrote:
>>> As the BOs used by VC4 have DMA Reservation Objects attached to it,
>>> there is no need to use seqnos wait for the BOs availability. Instead,
>>> we can use `dma_gem_dma_resv_wait()`.
>>>
>>> Signed-off-by: Maíra Canal <mcanal at igalia.com>
>>> ---
>>>   drivers/gpu/drm/vc4/vc4_drv.h | 10 ++++++++++
>>>   drivers/gpu/drm/vc4/vc4_gem.c | 31 +++++++++++++++++++------------
>>>   2 files changed, 29 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/ 
>>> vc4_drv.h
>>> index 4a078ffd9f82..03ed40ab9a93 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_drv.h
>>> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>>> @@ -924,6 +924,16 @@ struct vc4_validated_shader_info {
>>>                              (Wmax))
>>>   #define wait_for(COND, MS)        _wait_for((COND), (MS) * 1000, 
>>> 10, 1000)
>>> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
>>> +{
>>> +    /* nsecs_to_jiffies64() does not guard against overflow */
>>> +    if ((NSEC_PER_SEC % HZ) != 0 &&
>>> +        div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
>>> +        return MAX_JIFFY_OFFSET;
>>> +
>>> +    return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
>>> +}
>>> +
>>>   /* vc4_bo.c */
>>>   struct drm_gem_object *vc4_create_object(struct drm_device *dev, 
>>> size_t size);
>>>   struct vc4_bo *vc4_bo_create(struct drm_device *dev, size_t size,
>>> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/ 
>>> vc4_gem.c
>>> index 1021f45cb53c..4037c65eb269 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_gem.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
>>> @@ -1017,11 +1017,13 @@ int
>>>   vc4_wait_bo_ioctl(struct drm_device *dev, void *data,
>>>             struct drm_file *file_priv)
>>>   {
>>> -    struct vc4_dev *vc4 = to_vc4_dev(dev);
>>>       int ret;
>>> +    struct vc4_dev *vc4 = to_vc4_dev(dev);
>>>       struct drm_vc4_wait_bo *args = data;
>>> -    struct drm_gem_object *gem_obj;
>>> -    struct vc4_bo *bo;
>>> +    unsigned long timeout_jiffies =
>>> +        nsecs_to_jiffies_timeout(args->timeout_ns);
>>
>> What about a potentially simpler alternative:
>>
>> timeout_jiffies = usecs_to_jiffies(div_u64(args->timeout_ns, 1000));
>>
>> No need for a new helper and code cannot sleep for less than one jiffy 
>> anyway.
> 
> I ended up deciding to use a function available on "drm_utils.h",
> `drm_timeout_abs_to_jiffies()`, which seems to deal appropriately with
> overflows.

You will convert relative to absolute to be able to use it? At that 
point usecs_to_jiffies() might still be more elegant.

Regards,

Tvrtko

> 
>>
>>> +    ktime_t start = ktime_get();
>>> +    u64 delta_ns;
>>>       if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
>>>           return -ENODEV;
>>> @@ -1029,17 +1031,22 @@ vc4_wait_bo_ioctl(struct drm_device *dev, 
>>> void *data,
>>>       if (args->pad != 0)
>>>           return -EINVAL;
>>> -    gem_obj = drm_gem_object_lookup(file_priv, args->handle);
>>> -    if (!gem_obj) {
>>> -        DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
>>> -        return -EINVAL;
>>> -    }
>>> -    bo = to_vc4_bo(gem_obj);
>>> +    ret = drm_gem_dma_resv_wait(file_priv, args->handle,
>>> +                    true, timeout_jiffies);
>>> -    ret = vc4_wait_for_seqno_ioctl_helper(dev, bo->seqno,
>>> -                          &args->timeout_ns);
>>> +    /* Decrement the user's timeout, in case we got interrupted
>>> +     * such that the ioctl will be restarted.
>>> +     */
>>> +    delta_ns = ktime_to_ns(ktime_sub(ktime_get(), start));
>>> +    if (delta_ns < args->timeout_ns)
>>> +        args->timeout_ns -= delta_ns;
>>> +    else
>>> +        args->timeout_ns = 0;
>>> +
>>> +    /* Asked to wait beyond the jiffy/scheduler precision? */
>>> +    if (ret == -ETIME && args->timeout_ns)
>>> +        ret = -EAGAIN;
>>
>> EAGAIN is new from here, right? Will userspace cope in the right way?
> 
> Thanks for noticing! I'll address it in the next version.
> 
> Best Regards,
> - Maíra
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>> -    drm_gem_object_put(gem_obj);
>>>       return ret;
>>>   }
>