Re: [PATCH 02/19] dma-buf-map: Add helper to initialize second map

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

 



Hi

Am 27.01.22 um 16:59 schrieb Lucas De Marchi:
On Thu, Jan 27, 2022 at 03:33:12PM +0100, Thomas Zimmermann wrote:


Am 26.01.22 um 21:36 schrieb Lucas De Marchi:
When dma_buf_map struct is passed around, it's useful to be able to
initialize a second map that takes care of reading/writing to an offset
of the original map.

Add a helper that copies the struct and add the offset to the proper
address.

Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
Cc: Christian König <christian.koenig@xxxxxxx>
Cc: linux-media@xxxxxxxxxxxxxxx
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx
Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
---
 include/linux/dma-buf-map.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
index 65e927d9ce33..3514a859f628 100644
--- a/include/linux/dma-buf-map.h
+++ b/include/linux/dma-buf-map.h
@@ -131,6 +131,35 @@ struct dma_buf_map {
         .is_iomem = false, \
     }
+/**
+ * DMA_BUF_MAP_INIT_OFFSET - Initializes struct dma_buf_map from another dma_buf_map
+ * @map_:    The dma-buf mapping structure to copy from
+ * @offset:    Offset to add to the other mapping
+ *
+ * Initializes a new dma_buf_struct based on another. This is the equivalent of doing:
+ *
+ * .. code-block: c
+ *
+ *    dma_buf_map map = other_map;
+ *    dma_buf_map_incr(&map, &offset);
+ *
+ * Example usage:
+ *
+ * .. code-block: c
+ *
+ *    void foo(struct device *dev, struct dma_buf_map *base_map)
+ *    {
+ *        ...
+ *        struct dma_buf_map = DMA_BUF_MAP_INIT_OFFSET(base_map, FIELD_OFFSET);
+ *        ...
+ *    }
+ */
+#define DMA_BUF_MAP_INIT_OFFSET(map_, offset_)    (struct dma_buf_map)    \
+    {                                \
+        .vaddr = (map_)->vaddr + (offset_),            \
+        .is_iomem = (map_)->is_iomem,                \
+    }
+

It's illegal to access .vaddr  with raw pointer. Always use a dma_buf_memcpy_() interface. So why would you need this macro when you have dma_buf_memcpy_*() with an offset parameter?

I did a better job with an example in 20220127093332.wnkd2qy4tvwg5i5l@ldmartin-desk2

While doing this series I had code like this when using the API in a function to
parse/update part of the struct mapped:

     int bla_parse_foo(struct dma_buf_map *bla_map)
     {
         struct dma_buf_map foo_map = *bla_map;
         ...

         dma_buf_map_incr(&foo_map, offsetof(struct bla, foo));

         ...
     }

Pasting the rest of the reply here:

I had exactly this code above, but after writting quite a few patches
using it, particularly with functions that have to write to 2 maps (see
patch 6 for example), it felt much better to have something to
initialize correctly from the start

         struct dma_buf_map other_map = *bla_map;
         /* poor Lucas forgetting dma_buf_map_incr(map, offsetof(...)); */

is error prone and hard to debug since you will be reading/writting
from/to another location rather than exploding

Indeed. We have soem very specific use cases in graphics code, when dma_buf_map_incr() makes sense. But it's really bad for others. I guess that the docs should talk about this.


While with the construct below

         other_map;
         ...
         other_map = INITIALIZER()

I can rely on the compiler complaining about uninitialized var. And
in most of the cases I can just have this single line in the beggining of the
function when the offset is constant:

         struct dma_buf_map other_map = INITIALIZER(bla_map, offsetof(..));


This is useful when you have several small functions in charge of
updating/reading inner struct members.

You won't need an extra variable or the initializer macro if you add an offset parameter to dma_buf_memcpy_{from,to}. Simple pass offsetof(..) to that parameter and it will do the right thing.

It avoids the problems of the current macro and is even more flexible. On top of that, you can build whatever convenience macros you need for i915.

Best regards
Thomas



I've also been very careful to distinguish between .vaddr and .vaddr_iomem, even in places where I wouldn't have to. This macro breaks the assumption.

That's one reason I think if we have this macro, it should be in the
dma_buf_map.h header (or whatever we rename these APIs to). It's the
only place where we can safely add code that relies on the implementation
of the "private" fields in struct dma_buf_map.

Lucas De Marchi


Best regards
Thomas

 /**
  * dma_buf_map_set_vaddr - Sets a dma-buf mapping structure to an address in system memory
  * @map:    The dma-buf mapping structure

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev




--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux