[PATCH 0/4] drm/vc4: Clean-up the BO seqnos

[Christian König]

Am 06.12.24 um 16:11 schrieb Maíra Canal:
> On 06/12/24 11:17, Christian König wrote:
>> Am 06.12.24 um 14:12 schrieb Maíra Canal:
>>> This series introduces a series of clean-ups in BO reservations for the
>>> VC4 driver. Currently, VC4 uses shared fences to track BO 
>>> reservations as
>>> several other DRM devices. But apart from the DMA reservation 
>>> objects, VC4
>>> also attaches seqnos to its BOs with the intention to track if the 
>>> job that
>>> uses such BO has finished.
>>>
>>> The seqno tracking system was introduced before the DMA reservation 
>>> objects
>>> and we ended up lefting it in the code after DMA resv was 
>>> introduced. This
>>> is redundant, as we can perfectly track the end of a job with a fence.
>>> Therefore, this series focuses on eliminating the seqnos from the BO.
>>>
>>> This series introduces a series of clean-ups for BO reservations in 
>>> the VC4
>>> driver. Currently, VC4 uses shared fences to track BO reservations, 
>>> similar
>>> to many other DRM devices. However, in addition to DMA reservation 
>>> objects,
>>> VC4 has been maintaining a separate seqno tracking system for BOs to 
>>> track
>>> job completion.
>>>
>>> The seqno tracking system was implemented before DMA reservation 
>>> objects
>>> and was left in the code after DMA reservation's introduction. This
>>> approach is now redundant, as job completion can be effectively tracked
>>> using fences. Consequently, this series focuses on eliminating 
>>> seqnos from
>>> the BO implementation.
>>
>> Just FYI, you duplicated the text somehow :)
>
> Oops, that's a non-native English-speaker trying to write a v2 of a
> paragraph :)
>
>>
>>>
>>> Patch Breakdown
>>> ===============
>>>
>>> * Patch #1: Uses the DRM GEM implementation of 
>>> `drm_gem_lock_reservations()`
>>> and `drm_gem_unlock_reservations()` to replace the open-coded 
>>> implementation
>>> of this functions by VC4. A straightforward change with no functional
>>> changes.
>>
>> I actually pushed to remove drm_gem_lock_reservations() in favor of 
>> using the drm_exec object.
>>
>> It would be nice if you could use that one instead.
>
> Okay, I'll use DRM exec in the next version. I'd appreciate if you could
> take a look at the patches that remove the BOs seqnos.

Of hand it looks good to me, e.g. no problematic usages for the objects 
defined by DMA-buf, e.g. dma_resv and dma_fence.

But I have absolutely zero experience with the VC4 code base, so I can't 
say anything about that.

Regards,
Christian.

>
> Thanks for comments!
>
> Best Regards,
> - Maíra
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> * Patch #2: Implements the VC4 wait BO IOCTL using DMA reservations. 
>>> The
>>> new implementation closely mirrors the V3D approach and removes the 
>>> IOCTL's
>>> dependency on BO sequence numbers.
>>>
>>> * Patch #3: The central piece of this patchset. It removes `bo->seqno`,
>>> `bo->write_seqno`, and `exec->bin_dep_seqno` from the driver. As the 
>>> seqno
>>> tracking system is redundant, its deletion is relatively 
>>> straightforward.
>>> The only notable change is `vc4_async_set_fence_cb()`, which now uses
>>> `dma_fence_add_callback()` to add the VC4 callback.
>>>
>>> * Patch #4: Deletes the now-unused function `vc4_queue_seqno_cb()`.
>>>
>>> Best Regards,
>>> - Maíra
>>>
>>> Maíra Canal (4):
>>>    drm/vc4: Use `drm_gem_lock_reservations()` instead of hard-coding
>>>    drm/vc4: Use DMA Resv to implement VC4 wait BO IOCTL
>>>    drm/vc4: Remove BOs seqnos
>>>    drm/vc4: Remove `vc4_queue_seqno_cb()`
>>>
>>>   drivers/gpu/drm/vc4/vc4_crtc.c     |  22 ++---
>>>   drivers/gpu/drm/vc4/vc4_drv.h      |  26 ++---
>>>   drivers/gpu/drm/vc4/vc4_gem.c      | 147 
>>> ++++++-----------------------
>>>   drivers/gpu/drm/vc4/vc4_validate.c |  11 ---
>>>   4 files changed, 48 insertions(+), 158 deletions(-)
>>>
>>
>