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@linaro.org Cc: Christian König christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linaro-mm-sig@lists.linaro.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com
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
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.
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