| Message ID | 20241206131706.203324-1-mcanal@igalia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/vc4: Clean-up the BO seqnos | expand |
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 :) > > 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. 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(-) >
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. 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(-) >> >
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(-) >>> >> >