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

[Maíra Canal]

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.

> 
>> +    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;
>>   }