From: Daniel Vetter <daniel@ffwll.ch>
To: "Christian König" <ckoenig.leichtzumerken@gmail.com>
Cc: linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org,
linux-media@vger.kernel.org
Subject: Re: [PATCH 22/26] dma-buf: add enum dma_resv_usage
Date: Thu, 25 Nov 2021 16:59:53 +0100 [thread overview]
Message-ID: <YZ+y+Uwo809qtvs5@phenom.ffwll.local> (raw)
In-Reply-To: <20211123142111.3885-23-christian.koenig@amd.com>
On Tue, Nov 23, 2021 at 03:21:07PM +0100, Christian König wrote:
> This change adds the dma_resv_usage enum and allows us to specify why a
> dma_resv object is queried for its containing fences.
>
> Additional to that a dma_resv_usage_rw() helper function is added to aid
> retrieving the fences for a read or write userspace submission.
>
> This is then deployed to the different query functions of the dma_resv
> object and all of their users.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Just a few comments on the kenreldoc while I scroll through.
> EXPORT_SYMBOL(ib_umem_dmabuf_map_pages);
> diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
> index 062571c04bca..37552935bca6 100644
> --- a/include/linux/dma-resv.h
> +++ b/include/linux/dma-resv.h
> @@ -49,6 +49,86 @@ extern struct ww_class reservation_ww_class;
>
> struct dma_resv_list;
>
> +/**
> + * enum dma_resv_usage - how the fences from a dma_resv obj are used
> + *
> + * This enum describes the different use cases for a dma_resv object and
> + * controls which fences are returned when queried.
> + *
> + * An important fact is that there is the order KERNEL<WRITE<READ<OTHER and
> + * when the dma_resv object is asked for fences for one use case the fences
> + * for the lower use case are returned as well.
Might be good to replicate this to all functions that take a
dma_resv_usage flag, and then also add a "See enum dma_resv_usage for more
information." so we get a clickable hyperlink too.
> + *
> + * For example when asking for WRITE fences then the KERNEL fences are returned
> + * as well. Similar when asked for READ fences then both WRITE and KERNEL
> + * fences are returned as well.
> + */
> +enum dma_resv_usage {
> + /**
> + * @DMA_RESV_USAGE_KERNEL: For in kernel memory management only.
> + *
> + * This should only be used for things like copying or clearing memory
> + * with a DMA hardware engine for the purpose of kernel memory
> + * management.
> + *
> + * Drivers *always* need to wait for those fences before accessing the
> + * resource protected by the dma_resv object. The only exception for
> + * that is when the resource is known to be locked down in place by
> + * pinning it previously.
Should dma_buf_pin also do the wait for kernel fences? I think that would
also ba e bit clearer semantics in the dma-buf patch which does these
waits for us.
Or should dma_buf_pin be pipelined and it's up to callers to wait? For kms
that's definitely the semantics we want, but it's a bit playing with fire
situation, so maybe dma-buf should get the more idiot proof semantics?
> + */
> + DMA_RESV_USAGE_KERNEL,
> +
> + /**
> + * @DMA_RESV_USAGE_WRITE: Implicit write synchronization.
> + *
> + * This should only be used for userspace command submissions which add
> + * an implicit write dependency.
> + */
> + DMA_RESV_USAGE_WRITE,
> +
> + /**
> + * @DMA_RESV_USAGE_READ: Implicit read synchronization.
> + *
> + * This should only be used for userspace command submissions which add
> + * an implicit read dependency.
> + */
> + DMA_RESV_USAGE_READ,
> +
> + /**
> + * @DMA_RESV_USAGE_OTHER: No implicit sync.
> + *
> + * This should be used for operations which don't want to add an
> + * implicit dependency at all, but still have a dependency on memory
> + * management.
> + *
> + * This might include things like preemption fences as well as device
> + * page table updates or even userspace command submissions.
I think we should highlight a bit more that for explicitly synchronized
userspace like vk OTHER is the normal case. So really not an exception.
Ofc aside from amdkgf there's currently no driver doing this, but really
we should have lots of them ...
> + *
> + * The kernel memory management *always* need to wait for those fences
> + * before moving or freeing the resource protected by the dma_resv
> + * object.
> + */
> + DMA_RESV_USAGE_OTHER
> +};
> +
> +/**
> + * dma_resv_usage_rw - helper for implicit sync
> + * @write: true if we create a new implicit sync write
> + *
> + * This returns the implicit synchronization usage for write or read accesses.
Pls add "See enum dma_resv_usage for more details." or so. Never hurts to
be plentiful with links :-)
> + */
> +static inline enum dma_resv_usage dma_resv_usage_rw(bool write)
> +{
> + /* This looks confusing at first sight, but is indeed correct.
> + *
> + * The rational is that new write operations needs to wait for the
> + * existing read and write operations to finish.
> + * But a new read operation only needs to wait for the existing write
> + * operations to finish.
> + */
> + return write ? DMA_RESV_USAGE_READ : DMA_RESV_USAGE_WRITE;
> +}
> +
> /**
> * struct dma_resv - a reservation object manages fences for a buffer
> *
> @@ -147,8 +227,8 @@ struct dma_resv_iter {
> /** @obj: The dma_resv object we iterate over */
> struct dma_resv *obj;
>
> - /** @all_fences: If all fences should be returned */
> - bool all_fences;
> + /** @usage: Controls which fences are returned */
> + enum dma_resv_usage usage;
>
> /** @fence: the currently handled fence */
> struct dma_fence *fence;
> @@ -178,14 +258,14 @@ struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor);
> * dma_resv_iter_begin - initialize a dma_resv_iter object
> * @cursor: The dma_resv_iter object to initialize
> * @obj: The dma_resv object which we want to iterate over
> - * @all_fences: If all fences should be returned or just the exclusive one
> + * @usage: controls which fences to return
Please add the blurb here I mentioned above. Maybe adjust the text to use
the neatly highlighted @usage.
> */
> static inline void dma_resv_iter_begin(struct dma_resv_iter *cursor,
> struct dma_resv *obj,
> - bool all_fences)
> + enum dma_resv_usage usage)
> {
> cursor->obj = obj;
> - cursor->all_fences = all_fences;
> + cursor->usage = usage;
> cursor->fence = NULL;
> }
>
> @@ -242,7 +322,7 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor)
> * dma_resv_for_each_fence - fence iterator
> * @cursor: a struct dma_resv_iter pointer
> * @obj: a dma_resv object pointer
> - * @all_fences: true if all fences should be returned
> + * @usage: controls which fences to return
> * @fence: the current fence
> *
Same, another place that needs the @usage clarification.
> * Iterate over the fences in a struct dma_resv object while holding the
> @@ -251,8 +331,8 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor)
> * valid as long as the lock is held and so no extra reference to the fence is
> * taken.
> */
> -#define dma_resv_for_each_fence(cursor, obj, all_fences, fence) \
> - for (dma_resv_iter_begin(cursor, obj, all_fences), \
> +#define dma_resv_for_each_fence(cursor, obj, usage, fence) \
> + for (dma_resv_iter_begin(cursor, obj, usage), \
> fence = dma_resv_iter_first(cursor); fence; \
> fence = dma_resv_iter_next(cursor))
>
> @@ -421,14 +501,14 @@ void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context,
> void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence);
> void dma_resv_prune(struct dma_resv *obj);
> void dma_resv_prune_unlocked(struct dma_resv *obj);
> -int dma_resv_get_fences(struct dma_resv *obj, bool write,
> +int dma_resv_get_fences(struct dma_resv *obj, enum dma_resv_usage usage,
> unsigned int *num_fences, struct dma_fence ***fences);
> -int dma_resv_get_singleton(struct dma_resv *obj, bool write,
> +int dma_resv_get_singleton(struct dma_resv *obj, enum dma_resv_usage usage,
> struct dma_fence **fence);
> int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src);
> -long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr,
> - unsigned long timeout);
> -bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all);
> +long dma_resv_wait_timeout(struct dma_resv *obj, enum dma_resv_usage usage,
> + bool intr, unsigned long timeout);
> +bool dma_resv_test_signaled(struct dma_resv *obj, enum dma_resv_usage usage);
> void dma_resv_describe(struct dma_resv *obj, struct seq_file *seq);
I took endless amounts of discussions, but I think we're arriving at
something really neat and tiny here now finally. Both semantics, and how
drivers use them.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
next prev parent reply other threads:[~2021-11-25 16:00 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-23 14:20 completely rework the dma_resv semantic Christian König
2021-11-23 14:20 ` [PATCH 01/26] drm/amdgpu: partially revert "svm bo enable_signal call condition" Christian König
2021-11-23 14:20 ` [PATCH 02/26] dma-buf: cleanup pruning of dma_resv objects Christian König
2021-11-23 14:20 ` [PATCH 03/26] dma-buf: make fence mandatory for dma_resv_add_excl_fence Christian König
2021-11-23 14:20 ` [PATCH 04/26] drm/qxl: use iterator instead of dma_resv_shared_list Christian König
2021-11-23 14:20 ` [PATCH 05/26] dma-buf: add dma_resv_replace_fences Christian König
2021-11-23 14:20 ` [PATCH 06/26] dma-buf: finally make the dma_resv_list private Christian König
2021-11-23 14:20 ` [PATCH 07/26] dma-buf: drop excl_fence parameter from dma_resv_get_fences Christian König
2021-11-23 14:20 ` [PATCH 08/26] dma-buf: add dma_resv_get_singleton Christian König
2021-11-23 14:20 ` [PATCH 09/26] RDMA: use dma_resv_wait() instead of extracting the fence Christian König
2021-11-23 14:20 ` [PATCH 10/26] drm/etnaviv: stop using dma_resv_excl_fence Christian König
2021-11-23 14:20 ` [PATCH 11/26] drm/nouveau: " Christian König
2021-11-23 14:20 ` [PATCH 12/26] drm/vmwgfx: " Christian König
2021-11-23 14:20 ` [PATCH 13/26] drm/radeon: " Christian König
2021-11-23 14:20 ` [PATCH 14/26] drm/amdgpu: remove excl as shared workarounds Christian König
2021-11-23 14:21 ` [PATCH 15/26] drm/amdgpu: use dma_resv_for_each_fence for CS workaround Christian König
2021-11-23 14:21 ` [PATCH 16/26] dma-buf: finally make dma_resv_excl_fence private Christian König
2021-11-23 14:21 ` [PATCH 17/26] dma-buf: drop the DAG approach for the dma_resv object Christian König
2021-11-23 14:21 ` [PATCH 18/26] dma-buf/drivers: make reserving a shared slot mandatory Christian König
2021-11-23 14:21 ` [PATCH 19/26] drm: support more than one write fence in drm_gem_plane_helper_prepare_fb Christian König
2021-11-25 15:47 ` Daniel Vetter
2021-11-26 10:30 ` Christian König
2021-11-26 15:37 ` Daniel Vetter
2021-11-23 14:21 ` [PATCH 20/26] drm/nouveau: support more than one write fence in fenv50_wndw_prepare_fb Christian König
2021-11-23 14:21 ` [PATCH 21/26] drm/amdgpu: use dma_resv_get_singleton in amdgpu_pasid_free_cb Christian König
2021-11-23 14:21 ` [PATCH 22/26] dma-buf: add enum dma_resv_usage Christian König
2021-11-25 15:50 ` Daniel Vetter
2021-11-25 15:59 ` Daniel Vetter [this message]
2021-11-29 12:19 ` Christian König
2021-11-30 8:31 ` Daniel Vetter
2021-11-23 14:21 ` [PATCH 23/26] dma-buf: specify usage while adding fences to dma_resv obj Christian König
2021-11-23 14:21 ` [PATCH 24/26] dma-buf: wait for map to complete for static attachments Christian König
2021-11-23 14:21 ` [PATCH 25/26] amdgpu: remove DMA-buf fence workaround Christian König
2021-11-23 14:21 ` [PATCH 26/26] drm/ttm: remove bo->moving Christian König
2021-11-24 8:31 ` completely rework the dma_resv semantic Pekka Paalanen
2021-11-24 8:39 ` Christian König
2021-11-25 16:04 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YZ+y+Uwo809qtvs5@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=ckoenig.leichtzumerken@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-media@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).