Messages in this thread | | | Date | Thu, 27 Jan 2022 07:59:13 -0800 | From | Lucas De Marchi <> | Subject | Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map |
| |
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
| |