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-desk2While doing this series I had code like this when using the API in a function toparse/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. Andin most of the cases I can just have this single line in the beggining of thefunction 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 MarchiBest 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
