Re: [PATCH 1/4] dma-buf: Remove kmap kerneldoc vestiges

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 14.12.20 um 17:01 schrieb Daniel Vetter:
On Mon, Dec 14, 2020 at 11:33:10AM +0100, Christian König wrote:
Am 11.12.20 um 16:58 schrieb Daniel Vetter:
Also try to clarify a bit when dma_buf_begin/end_cpu_access should
be called.

Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
Cc: "Christian König" <christian.koenig@xxxxxxx>
Cc: linux-media@xxxxxxxxxxxxxxx
Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx
---
   drivers/dma-buf/dma-buf.c | 20 ++++++++++++++------
   include/linux/dma-buf.h   | 25 +++++++++----------------
   2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index e63684d4cd90..a12fdffa130f 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1001,15 +1001,15 @@ EXPORT_SYMBOL_GPL(dma_buf_move_notify);
    *   vmalloc space might be limited and result in vmap calls failing.
    *
    *   Interfaces::
+ *
    *      void \*dma_buf_vmap(struct dma_buf \*dmabuf)
    *      void dma_buf_vunmap(struct dma_buf \*dmabuf, void \*vaddr)
    *
    *   The vmap call can fail if there is no vmap support in the exporter, or if
- *   it runs out of vmalloc space. Fallback to kmap should be implemented. Note
- *   that the dma-buf layer keeps a reference count for all vmap access and
- *   calls down into the exporter's vmap function only when no vmapping exists,
- *   and only unmaps it once. Protection against concurrent vmap/vunmap calls is
- *   provided by taking the dma_buf->lock mutex.
+ *   it runs out of vmalloc space. Note that the dma-buf layer keeps a reference
+ *   count for all vmap access and calls down into the exporter's vmap function
+ *   only when no vmapping exists, and only unmaps it once. Protection against
+ *   concurrent vmap/vunmap calls is provided by taking the &dma_buf.lock mutex.
Who is talking the lock? The caller of the dma_buf_vmap/vunmap() functions,
the functions itself or the callback inside the exporter?
That's the part I didn't change at all here, just re-laid out the line
breaking. I only removed the outdated kmap section here.

I just wanted to point out that this still isn't described here very very.


Should I do another patch and remove this one sentence here (it's kinda
pointless and generally we don't muse about implementation details that
callers don't care about)?

Na, works like this for me.

I did try and do a cursory review of the dma-buf docs, but this is kinda
not meant as an all-out revamp. Just a few things I've noticed while
reviewing Thomas' vmap_local stuff.


Fell free to add an Acked-by: Christian König <christian.koenig@xxxxxxx> to the series.

Christian.

-Daniel

Christian.

    *
    * - For full compatibility on the importer side with existing userspace
    *   interfaces, which might already support mmap'ing buffers. This is needed in
@@ -1098,6 +1098,11 @@ static int __dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
    * dma_buf_end_cpu_access(). Only when cpu access is braketed by both calls is
    * it guaranteed to be coherent with other DMA access.
    *
+ * This function will also wait for any DMA transactions tracked through
+ * implicit synchronization in &dma_buf.resv. For DMA transactions with explicit
+ * synchronization this function will only ensure cache coherency, callers must
+ * ensure synchronization with such DMA transactions on their own.
+ *
    * Can return negative error values, returns 0 on success.
    */
   int dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
@@ -1199,7 +1204,10 @@ EXPORT_SYMBOL_GPL(dma_buf_mmap);
    * This call may fail due to lack of virtual mapping address space.
    * These calls are optional in drivers. The intended use for them
    * is for mapping objects linear in kernel space for high use objects.
- * Please attempt to use kmap/kunmap before thinking about these interfaces.
+ *
+ * To ensure coherency users must call dma_buf_begin_cpu_access() and
+ * dma_buf_end_cpu_access() around any cpu access performed through this
+ * mapping.
    *
    * Returns 0 on success, or a negative errno code otherwise.
    */
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index cf72699cb2bc..7eca37c8b10c 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -183,24 +183,19 @@ struct dma_buf_ops {
   	 * @begin_cpu_access:
   	 *
   	 * This is called from dma_buf_begin_cpu_access() and allows the
-	 * exporter to ensure that the memory is actually available for cpu
-	 * access - the exporter might need to allocate or swap-in and pin the
-	 * backing storage. The exporter also needs to ensure that cpu access is
-	 * coherent for the access direction. The direction can be used by the
-	 * exporter to optimize the cache flushing, i.e. access with a different
+	 * exporter to ensure that the memory is actually coherent for cpu
+	 * access. The exporter also needs to ensure that cpu access is coherent
+	 * for the access direction. The direction can be used by the exporter
+	 * to optimize the cache flushing, i.e. access with a different
   	 * direction (read instead of write) might return stale or even bogus
   	 * data (e.g. when the exporter needs to copy the data to temporary
   	 * storage).
   	 *
-	 * This callback is optional.
+	 * Note that this is both called through the DMA_BUF_IOCTL_SYNC IOCTL
+	 * command for userspace mappings established through @mmap, and also
+	 * for kernel mappings established with @vmap.
   	 *
-	 * FIXME: This is both called through the DMA_BUF_IOCTL_SYNC command
-	 * from userspace (where storage shouldn't be pinned to avoid handing
-	 * de-factor mlock rights to userspace) and for the kernel-internal
-	 * users of the various kmap interfaces, where the backing storage must
-	 * be pinned to guarantee that the atomic kmap calls can succeed. Since
-	 * there's no in-kernel users of the kmap interfaces yet this isn't a
-	 * real problem.
+	 * This callback is optional.
   	 *
   	 * Returns:
   	 *
@@ -216,9 +211,7 @@ struct dma_buf_ops {
   	 *
   	 * This is called from dma_buf_end_cpu_access() when the importer is
   	 * done accessing the CPU. The exporter can use this to flush caches and
-	 * unpin any resources pinned in @begin_cpu_access.
-	 * The result of any dma_buf kmap calls after end_cpu_access is
-	 * undefined.
+	 * undo anything else done in @begin_cpu_access.
   	 *
   	 * This callback is optional.
   	 *

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux