Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
Hi
Am 28.01.22 um 09:15 schrieb Thomas Zimmermann:
...
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.
And maybe put all changes to the dma_buf_map interface into a single
patch. It makes it easier to review and discuss.
Best regards
Thomas
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
OpenPGP_signature
Description: OpenPGP digital signature
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
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
Cc: Christian König
Cc: linux-me...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-...@lists.linaro.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Lucas De Marchi
---
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(, );
+ *
+ * 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(_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
OpenPGP_signature
Re: [Linaro-mm-sig] Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
On Thu, Jan 27, 2022 at 12:44:21PM +0100, Christian König wrote:
Am 27.01.22 um 12:16 schrieb Daniel Vetter:
On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
Am 27.01.22 um 11:00 schrieb Daniel Vetter:
On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
Well what you propose here can lead to all kind of problems and is
rather bad design as far as I can see.
The struct dma_buf_map is only to be filled in by the exporter and
should not be modified in this way by the importer.
humn... not sure if I was clear. There is no importer and exporter here.
Yeah, and exactly that's what I'm pointing out as problem here.
You are using the inter driver framework for something internal to the
driver. That is an absolutely clear NAK!
We could discuss that, but you guys are just sending around patches to do
this without any consensus that this is a good idea.
Uh I suggested this, also we're already using dma_buf_map all over the
place as a convenient abstraction. So imo that's all fine, it should allow
drivers to simplify some code where on igpu it's in normal kernel memory
and on dgpu it's behind some pci bar.
Maybe we should have a better name for that struct (and maybe also a
better place), but way back when we discussed that bikeshed I didn't come
up with anything better really.
I suggest iosys_map since it abstracts access to IO and system memory.
There is a role delegation on filling out and reading a buffer when
that buffer represents a struct layout.
struct bla {
int a;
int b;
int c;
struct foo foo;
struct bar bar;
int d;
}
This implementation allows you to have:
fill_foo(struct dma_buf_map *bla_map) { ... }
fill_bar(struct dma_buf_map *bla_map) { ... }
and the first thing these do is to make sure the map it's pointing to
is relative to the struct it's supposed to write/read. Otherwise you're
suggesting everything to be relative to struct bla, or to do the same
I'm doing it, but IMO more prone to error:
struct dma_buf_map map = *bla_map;
dma_buf_map_incr(map, offsetof(...));
Wrt the issue at hand I think the above is perfectly fine code. The idea
with dma_buf_map is really that it's just a special pointer, so writing
the code exactly as pointer code feels best. Unfortunately you cannot make
them typesafe (because of C), so the code sometimes looks a bit ugly.
Otherwise we could do stuff like container_of and all that with
typechecking in the macros.
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(..));
Hm yeah that's a good point that this allows us to rely on the compiler to
check for uninitialized variables.
Maybe include the above (with editing, but keeping the examples) in the
kerneldoc to explain why/how to use this? With that the concept at least
has my
Acked-by: Daniel Vetter
I'll leave it up to you & Christian to find a prettier color choice for
the naming bikeshed.
There is one major issue remaining with this and that is dma_buf_vunmap():
void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
Here we expect the original pointer as returned by dma_buf_map(), otherwise
we vunmap() the wrong area!
For all TTM based driver this doesn't matter since we keep the vmap base
separately in the BO anyway (IIRC), but we had at least one case where this
made boom last year.
Yeah but isn't that the same if it's just a void *?
If you pass the wrong pointer to an unmap function and not exactly what
you go from the map function, then things go boom. This is like
complaining that the following code wont work
u32 *stuff
stuff = kmap_local(some_page);
*stuff++ = 0;
*stuff
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
Am 27.01.22 um 11:00 schrieb Daniel Vetter:
On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
Well what you propose here can lead to all kind of problems and is
rather bad design as far as I can see.
The struct dma_buf_map is only to be filled in by the exporter and
should not be modified in this way by the importer.
humn... not sure if I was clear. There is no importer and exporter here.
Yeah, and exactly that's what I'm pointing out as problem here.
You are using the inter driver framework for something internal to the
driver. That is an absolutely clear NAK!
We could discuss that, but you guys are just sending around patches to do
this without any consensus that this is a good idea.
Uh I suggested this, also we're already using dma_buf_map all over the
place as a convenient abstraction. So imo that's all fine, it should allow
drivers to simplify some code where on igpu it's in normal kernel memory
and on dgpu it's behind some pci bar.
Maybe we should have a better name for that struct (and maybe also a
better place), but way back when we discussed that bikeshed I didn't come
up with anything better really.
I suggest iosys_map since it abstracts access to IO and system memory.
There is a role delegation on filling out and reading a buffer when
that buffer represents a struct layout.
struct bla {
int a;
int b;
int c;
struct foo foo;
struct bar bar;
int d;
}
This implementation allows you to have:
fill_foo(struct dma_buf_map *bla_map) { ... }
fill_bar(struct dma_buf_map *bla_map) { ... }
and the first thing these do is to make sure the map it's pointing to
is relative to the struct it's supposed to write/read. Otherwise you're
suggesting everything to be relative to struct bla, or to do the same
I'm doing it, but IMO more prone to error:
struct dma_buf_map map = *bla_map;
dma_buf_map_incr(map, offsetof(...));
Wrt the issue at hand I think the above is perfectly fine code. The idea
with dma_buf_map is really that it's just a special pointer, so writing
the code exactly as pointer code feels best. Unfortunately you cannot make
them typesafe (because of C), so the code sometimes looks a bit ugly.
Otherwise we could do stuff like container_of and all that with
typechecking in the macros.
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(..));
Hm yeah that's a good point that this allows us to rely on the compiler to
check for uninitialized variables.
Maybe include the above (with editing, but keeping the examples) in the
kerneldoc to explain why/how to use this? With that the concept at least
has my
Acked-by: Daniel Vetter
I'll leave it up to you & Christian to find a prettier color choice for
the naming bikeshed.
There is one major issue remaining with this and that is dma_buf_vunmap():
void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
Here we expect the original pointer as returned by dma_buf_map(),
otherwise we vunmap() the wrong area!
yeah... I think the most confusing aspect here is about the name.
void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
this function is the implementation of the dma_buf, not dma_buf_map,
which is another thing entirely. I think the rename will be benefitial
for this to be cleared out, because then it's more obvious the shallow
copy of the map is the equivalent of having
u8 *p = buffer;
...
p += 10;
Etc. You can't kfree(p) and expect it to work.
Lucas De Marchi
For all TTM based driver this doesn't matter since we keep the vmap
base separately in the BO
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
Cc: Christian König
Cc: linux-me...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-...@lists.linaro.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Lucas De Marchi
---
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(, );
+ *
+ * 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(_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
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
Hi
Am 27.01.22 um 11:21 schrieb Christian König:
Am 27.01.22 um 11:00 schrieb Daniel Vetter:
On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
Well what you propose here can lead to all kind of problems and is
rather bad design as far as I can see.
The struct dma_buf_map is only to be filled in by the exporter and
should not be modified in this way by the importer.
humn... not sure if I was clear. There is no importer and
exporter here.
Yeah, and exactly that's what I'm pointing out as problem here.
You are using the inter driver framework for something internal to the
driver. That is an absolutely clear NAK!
We could discuss that, but you guys are just sending around patches
to do
this without any consensus that this is a good idea.
Uh I suggested this, also we're already using dma_buf_map all over the
place as a convenient abstraction. So imo that's all fine, it should
allow
drivers to simplify some code where on igpu it's in normal kernel
memory
and on dgpu it's behind some pci bar.
Maybe we should have a better name for that struct (and maybe also a
better place), but way back when we discussed that bikeshed I didn't
come
up with anything better really.
I suggest iosys_map since it abstracts access to IO and system memory.
There is a role delegation on filling out and reading a buffer when
that buffer represents a struct layout.
struct bla {
int a;
int b;
int c;
struct foo foo;
struct bar bar;
int d;
}
This implementation allows you to have:
fill_foo(struct dma_buf_map *bla_map) { ... }
fill_bar(struct dma_buf_map *bla_map) { ... }
and the first thing these do is to make sure the map it's pointing to
is relative to the struct it's supposed to write/read. Otherwise
you're
suggesting everything to be relative to struct bla, or to do the same
I'm doing it, but IMO more prone to error:
struct dma_buf_map map = *bla_map;
dma_buf_map_incr(map, offsetof(...));
Wrt the issue at hand I think the above is perfectly fine code. The
idea
with dma_buf_map is really that it's just a special pointer, so writing
the code exactly as pointer code feels best. Unfortunately you
cannot make
them typesafe (because of C), so the code sometimes looks a bit ugly.
Otherwise we could do stuff like container_of and all that with
typechecking in the macros.
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(..));
Hm yeah that's a good point that this allows us to rely on the
compiler to
check for uninitialized variables.
Maybe include the above (with editing, but keeping the examples) in the
kerneldoc to explain why/how to use this? With that the concept at least
has my
Acked-by: Daniel Vetter
I'll leave it up to you & Christian to find a prettier color choice for
the naming bikeshed.
There is one major issue remaining with this and that is dma_buf_vunmap():
void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
Here we expect the original pointer as returned by dma_buf_map(),
otherwise we vunmap() the wrong area!
Indeed. It's always been a problem with that API, even when it still
took raw pointers.
The IMHO correct solution would distinguish between a buffer (struct
dma_buf_map) and a pointer into that buffer (struct dma_buf_ptr).
I don't feel like typing that.
Best regards
Thomas
For all TTM based driver this doesn't matter since we keep the vmap base
separately in the BO anyway (IIRC), but we had at least one case where
this made boom last year.
Christian.
-Daniel
Lucas De Marchi
-Daniel
IMO this construct is worse because at a point in time in the
function
the map was pointing to the wrong thing the
Re: [Linaro-mm-sig] Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
On Thu, Jan 27, 2022 at 12:44:21PM +0100, Christian König wrote:
> Am 27.01.22 um 12:16 schrieb Daniel Vetter:
> > On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
> > > Am 27.01.22 um 11:00 schrieb Daniel Vetter:
> > > > On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
> > > > > On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
> > > > > > On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
> > > > > > > Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
> > > > > > > > On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
> > > > > > > > > Well what you propose here can lead to all kind of problems
> > > > > > > > > and is
> > > > > > > > > rather bad design as far as I can see.
> > > > > > > > >
> > > > > > > > > The struct dma_buf_map is only to be filled in by the
> > > > > > > > > exporter and
> > > > > > > > > should not be modified in this way by the importer.
> > > > > > > > humn... not sure if I was clear. There is no importer and
> > > > > > > > exporter here.
> > > > > > > Yeah, and exactly that's what I'm pointing out as problem here.
> > > > > > >
> > > > > > > You are using the inter driver framework for something internal
> > > > > > > to the
> > > > > > > driver. That is an absolutely clear NAK!
> > > > > > >
> > > > > > > We could discuss that, but you guys are just sending around
> > > > > > > patches to do
> > > > > > > this without any consensus that this is a good idea.
> > > > > > Uh I suggested this, also we're already using dma_buf_map all over
> > > > > > the
> > > > > > place as a convenient abstraction. So imo that's all fine, it
> > > > > > should allow
> > > > > > drivers to simplify some code where on igpu it's in normal kernel
> > > > > > memory
> > > > > > and on dgpu it's behind some pci bar.
> > > > > >
> > > > > > Maybe we should have a better name for that struct (and maybe also a
> > > > > > better place), but way back when we discussed that bikeshed I
> > > > > > didn't come
> > > > > > up with anything better really.
> > > > > I suggest iosys_map since it abstracts access to IO and system memory.
> > > > >
> > > > > > > > There is a role delegation on filling out and reading a buffer
> > > > > > > > when
> > > > > > > > that buffer represents a struct layout.
> > > > > > > >
> > > > > > > > struct bla {
> > > > > > > > int a;
> > > > > > > > int b;
> > > > > > > > int c;
> > > > > > > > struct foo foo;
> > > > > > > > struct bar bar;
> > > > > > > > int d;
> > > > > > > > }
> > > > > > > >
> > > > > > > >
> > > > > > > > This implementation allows you to have:
> > > > > > > >
> > > > > > > > fill_foo(struct dma_buf_map *bla_map) { ... }
> > > > > > > > fill_bar(struct dma_buf_map *bla_map) { ... }
> > > > > > > >
> > > > > > > > and the first thing these do is to make sure the map it's
> > > > > > > > pointing to
> > > > > > > > is relative to the struct it's supposed to write/read.
> > > > > > > > Otherwise you're
> > > > > > > > suggesting everything to be relative to struct bla, or to do
> > > > > > > > the same
> > > > > > > > I'm doing it, but IMO more prone to error:
> > > > > > > >
> > > > > > > > struct dma_buf_map map = *bla_map;
> > > > > > > > dma_buf_map_incr(map, offsetof(...));
> > > > > > Wrt the issue at hand I think the above is perfectly fine code. The
> > > > > > idea
> > > > > > with dma_buf_map is really that it's just a special pointer, so
> > > > > > writing
> > > > > > the code exactly as pointer code feels best. Unfortunately you
> > > > > > cannot make
> > > > > > them typesafe (because of C), so the code sometimes looks a bit
> > > > > > ugly.
> > > > > > Otherwise we could do stuff like container_of and all that with
> > > > > > typechecking in the macros.
> > > > > 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
> > > > >
> > >
Re: [Linaro-mm-sig] Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
Am 27.01.22 um 12:16 schrieb Daniel Vetter:
On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
Am 27.01.22 um 11:00 schrieb Daniel Vetter:
On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
Well what you propose here can lead to all kind of problems and is
rather bad design as far as I can see.
The struct dma_buf_map is only to be filled in by the exporter and
should not be modified in this way by the importer.
humn... not sure if I was clear. There is no importer and exporter here.
Yeah, and exactly that's what I'm pointing out as problem here.
You are using the inter driver framework for something internal to the
driver. That is an absolutely clear NAK!
We could discuss that, but you guys are just sending around patches to do
this without any consensus that this is a good idea.
Uh I suggested this, also we're already using dma_buf_map all over the
place as a convenient abstraction. So imo that's all fine, it should allow
drivers to simplify some code where on igpu it's in normal kernel memory
and on dgpu it's behind some pci bar.
Maybe we should have a better name for that struct (and maybe also a
better place), but way back when we discussed that bikeshed I didn't come
up with anything better really.
I suggest iosys_map since it abstracts access to IO and system memory.
There is a role delegation on filling out and reading a buffer when
that buffer represents a struct layout.
struct bla {
int a;
int b;
int c;
struct foo foo;
struct bar bar;
int d;
}
This implementation allows you to have:
fill_foo(struct dma_buf_map *bla_map) { ... }
fill_bar(struct dma_buf_map *bla_map) { ... }
and the first thing these do is to make sure the map it's pointing to
is relative to the struct it's supposed to write/read. Otherwise you're
suggesting everything to be relative to struct bla, or to do the same
I'm doing it, but IMO more prone to error:
struct dma_buf_map map = *bla_map;
dma_buf_map_incr(map, offsetof(...));
Wrt the issue at hand I think the above is perfectly fine code. The idea
with dma_buf_map is really that it's just a special pointer, so writing
the code exactly as pointer code feels best. Unfortunately you cannot make
them typesafe (because of C), so the code sometimes looks a bit ugly.
Otherwise we could do stuff like container_of and all that with
typechecking in the macros.
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(..));
Hm yeah that's a good point that this allows us to rely on the compiler to
check for uninitialized variables.
Maybe include the above (with editing, but keeping the examples) in the
kerneldoc to explain why/how to use this? With that the concept at least
has my
Acked-by: Daniel Vetter
I'll leave it up to you & Christian to find a prettier color choice for
the naming bikeshed.
There is one major issue remaining with this and that is dma_buf_vunmap():
void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
Here we expect the original pointer as returned by dma_buf_map(), otherwise
we vunmap() the wrong area!
For all TTM based driver this doesn't matter since we keep the vmap base
separately in the BO anyway (IIRC), but we had at least one case where this
made boom last year.
Yeah but isn't that the same if it's just a void *?
If you pass the wrong pointer to an unmap function and not exactly what
you go from the map function, then things go boom. This is like
complaining that the following code wont work
u32 *stuff
stuff = kmap_local(some_page);
*stuff++ = 0;
*stuff = 1;
kunmap_locak(stuff);
It's just ... don't
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian König wrote:
> Am 27.01.22 um 11:00 schrieb Daniel Vetter:
> > On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
> > > On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
> > > > On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
> > > > > Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
> > > > > > On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
> > > > > > > Well what you propose here can lead to all kind of problems and is
> > > > > > > rather bad design as far as I can see.
> > > > > > >
> > > > > > > The struct dma_buf_map is only to be filled in by the exporter and
> > > > > > > should not be modified in this way by the importer.
> > > > > > humn... not sure if I was clear. There is no importer and exporter
> > > > > > here.
> > > > > Yeah, and exactly that's what I'm pointing out as problem here.
> > > > >
> > > > > You are using the inter driver framework for something internal to the
> > > > > driver. That is an absolutely clear NAK!
> > > > >
> > > > > We could discuss that, but you guys are just sending around patches
> > > > > to do
> > > > > this without any consensus that this is a good idea.
> > > > Uh I suggested this, also we're already using dma_buf_map all over the
> > > > place as a convenient abstraction. So imo that's all fine, it should
> > > > allow
> > > > drivers to simplify some code where on igpu it's in normal kernel memory
> > > > and on dgpu it's behind some pci bar.
> > > >
> > > > Maybe we should have a better name for that struct (and maybe also a
> > > > better place), but way back when we discussed that bikeshed I didn't
> > > > come
> > > > up with anything better really.
> > > I suggest iosys_map since it abstracts access to IO and system memory.
> > >
> > > > > > There is a role delegation on filling out and reading a buffer when
> > > > > > that buffer represents a struct layout.
> > > > > >
> > > > > > struct bla {
> > > > > > int a;
> > > > > > int b;
> > > > > > int c;
> > > > > > struct foo foo;
> > > > > > struct bar bar;
> > > > > > int d;
> > > > > > }
> > > > > >
> > > > > >
> > > > > > This implementation allows you to have:
> > > > > >
> > > > > > fill_foo(struct dma_buf_map *bla_map) { ... }
> > > > > > fill_bar(struct dma_buf_map *bla_map) { ... }
> > > > > >
> > > > > > and the first thing these do is to make sure the map it's pointing
> > > > > > to
> > > > > > is relative to the struct it's supposed to write/read. Otherwise
> > > > > > you're
> > > > > > suggesting everything to be relative to struct bla, or to do the
> > > > > > same
> > > > > > I'm doing it, but IMO more prone to error:
> > > > > >
> > > > > > struct dma_buf_map map = *bla_map;
> > > > > > dma_buf_map_incr(map, offsetof(...));
> > > > Wrt the issue at hand I think the above is perfectly fine code. The idea
> > > > with dma_buf_map is really that it's just a special pointer, so writing
> > > > the code exactly as pointer code feels best. Unfortunately you cannot
> > > > make
> > > > them typesafe (because of C), so the code sometimes looks a bit ugly.
> > > > Otherwise we could do stuff like container_of and all that with
> > > > typechecking in the macros.
> > > 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(..));
> > Hm yeah that's a good point that this allows us to rely on the compiler to
> > check for uninitialized variables.
> >
> > Maybe include the above (with editing, but keeping the examples) in the
> > kerneldoc to explain why/how to use this? With that the concept at least
> > has my
> >
> >
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
Am 27.01.22 um 11:00 schrieb Daniel Vetter:
On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
Well what you propose here can lead to all kind of problems and is
rather bad design as far as I can see.
The struct dma_buf_map is only to be filled in by the exporter and
should not be modified in this way by the importer.
humn... not sure if I was clear. There is no importer and exporter here.
Yeah, and exactly that's what I'm pointing out as problem here.
You are using the inter driver framework for something internal to the
driver. That is an absolutely clear NAK!
We could discuss that, but you guys are just sending around patches to do
this without any consensus that this is a good idea.
Uh I suggested this, also we're already using dma_buf_map all over the
place as a convenient abstraction. So imo that's all fine, it should allow
drivers to simplify some code where on igpu it's in normal kernel memory
and on dgpu it's behind some pci bar.
Maybe we should have a better name for that struct (and maybe also a
better place), but way back when we discussed that bikeshed I didn't come
up with anything better really.
I suggest iosys_map since it abstracts access to IO and system memory.
There is a role delegation on filling out and reading a buffer when
that buffer represents a struct layout.
struct bla {
int a;
int b;
int c;
struct foo foo;
struct bar bar;
int d;
}
This implementation allows you to have:
fill_foo(struct dma_buf_map *bla_map) { ... }
fill_bar(struct dma_buf_map *bla_map) { ... }
and the first thing these do is to make sure the map it's pointing to
is relative to the struct it's supposed to write/read. Otherwise you're
suggesting everything to be relative to struct bla, or to do the same
I'm doing it, but IMO more prone to error:
struct dma_buf_map map = *bla_map;
dma_buf_map_incr(map, offsetof(...));
Wrt the issue at hand I think the above is perfectly fine code. The idea
with dma_buf_map is really that it's just a special pointer, so writing
the code exactly as pointer code feels best. Unfortunately you cannot make
them typesafe (because of C), so the code sometimes looks a bit ugly.
Otherwise we could do stuff like container_of and all that with
typechecking in the macros.
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(..));
Hm yeah that's a good point that this allows us to rely on the compiler to
check for uninitialized variables.
Maybe include the above (with editing, but keeping the examples) in the
kerneldoc to explain why/how to use this? With that the concept at least
has my
Acked-by: Daniel Vetter
I'll leave it up to you & Christian to find a prettier color choice for
the naming bikeshed.
There is one major issue remaining with this and that is dma_buf_vunmap():
void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map);
Here we expect the original pointer as returned by dma_buf_map(),
otherwise we vunmap() the wrong area!
For all TTM based driver this doesn't matter since we keep the vmap base
separately in the BO anyway (IIRC), but we had at least one case where
this made boom last year.
Christian.
-Daniel
Lucas De Marchi
-Daniel
IMO this construct is worse because at a point in time in the function
the map was pointing to the wrong thing the function was supposed to
read/write.
It's also useful when the function has double duty, updating a global
part of the struct and a table inside it (see example in patch 6)
thanks
Lucas De Marchi
--
Daniel Vetter
Software Engineer, Intel Corporation
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote:
> On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
> > On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
> > > Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
> > > > On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
> > > > >
> > > > > Well what you propose here can lead to all kind of problems and is
> > > > > rather bad design as far as I can see.
> > > > >
> > > > > The struct dma_buf_map is only to be filled in by the exporter and
> > > > > should not be modified in this way by the importer.
> > > >
> > > > humn... not sure if I was clear. There is no importer and exporter
> > > > here.
> > >
> > > Yeah, and exactly that's what I'm pointing out as problem here.
> > >
> > > You are using the inter driver framework for something internal to the
> > > driver. That is an absolutely clear NAK!
> > >
> > > We could discuss that, but you guys are just sending around patches to do
> > > this without any consensus that this is a good idea.
> >
> > Uh I suggested this, also we're already using dma_buf_map all over the
> > place as a convenient abstraction. So imo that's all fine, it should allow
> > drivers to simplify some code where on igpu it's in normal kernel memory
> > and on dgpu it's behind some pci bar.
> >
> > Maybe we should have a better name for that struct (and maybe also a
> > better place), but way back when we discussed that bikeshed I didn't come
> > up with anything better really.
>
> I suggest iosys_map since it abstracts access to IO and system memory.
>
> >
> > > > There is a role delegation on filling out and reading a buffer when
> > > > that buffer represents a struct layout.
> > > >
> > > > struct bla {
> > > > int a;
> > > > int b;
> > > > int c;
> > > > struct foo foo;
> > > > struct bar bar;
> > > > int d;
> > > > }
> > > >
> > > >
> > > > This implementation allows you to have:
> > > >
> > > > fill_foo(struct dma_buf_map *bla_map) { ... }
> > > > fill_bar(struct dma_buf_map *bla_map) { ... }
> > > >
> > > > and the first thing these do is to make sure the map it's pointing to
> > > > is relative to the struct it's supposed to write/read. Otherwise you're
> > > > suggesting everything to be relative to struct bla, or to do the same
> > > > I'm doing it, but IMO more prone to error:
> > > >
> > > > struct dma_buf_map map = *bla_map;
> > > > dma_buf_map_incr(map, offsetof(...));
> >
> > Wrt the issue at hand I think the above is perfectly fine code. The idea
> > with dma_buf_map is really that it's just a special pointer, so writing
> > the code exactly as pointer code feels best. Unfortunately you cannot make
> > them typesafe (because of C), so the code sometimes looks a bit ugly.
> > Otherwise we could do stuff like container_of and all that with
> > typechecking in the macros.
>
> 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(..));
Hm yeah that's a good point that this allows us to rely on the compiler to
check for uninitialized variables.
Maybe include the above (with editing, but keeping the examples) in the
kerneldoc to explain why/how to use this? With that the concept at least
has my
Acked-by: Daniel Vetter
I'll leave it up to you & Christian to find a prettier color choice for
the naming bikeshed.
-Daniel
>
> Lucas De Marchi
>
> > -Daniel
> >
> > > > IMO this construct is worse because at a point in time in the function
> > > > the map was pointing to the wrong thing the function was supposed to
> > > > read/write.
> > > >
> > > > It's also useful when the function has double duty, updating a global
> > > > part of the struct and a table inside it (see example in patch 6)
> > > >
> > > > thanks
> > > > Lucas De Marchi
> > >
> >
> >
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote:
On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote:
Am 27.01.22 um 08:57 schrieb Lucas De Marchi:
> On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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.
> >
> > Well what you propose here can lead to all kind of problems and is
> > rather bad design as far as I can see.
> >
> > The struct dma_buf_map is only to be filled in by the exporter and
> > should not be modified in this way by the importer.
>
> humn... not sure if I was clear. There is no importer and exporter here.
Yeah, and exactly that's what I'm pointing out as problem here.
You are using the inter driver framework for something internal to the
driver. That is an absolutely clear NAK!
We could discuss that, but you guys are just sending around patches to do
this without any consensus that this is a good idea.
Uh I suggested this, also we're already using dma_buf_map all over the
place as a convenient abstraction. So imo that's all fine, it should allow
drivers to simplify some code where on igpu it's in normal kernel memory
and on dgpu it's behind some pci bar.
Maybe we should have a better name for that struct (and maybe also a
better place), but way back when we discussed that bikeshed I didn't come
up with anything better really.
I suggest iosys_map since it abstracts access to IO and system memory.
> There is a role delegation on filling out and reading a buffer when
> that buffer represents a struct layout.
>
> struct bla {
> int a;
> int b;
> int c;
> struct foo foo;
> struct bar bar;
> int d;
> }
>
>
> This implementation allows you to have:
>
> fill_foo(struct dma_buf_map *bla_map) { ... }
> fill_bar(struct dma_buf_map *bla_map) { ... }
>
> and the first thing these do is to make sure the map it's pointing to
> is relative to the struct it's supposed to write/read. Otherwise you're
> suggesting everything to be relative to struct bla, or to do the same
> I'm doing it, but IMO more prone to error:
>
> struct dma_buf_map map = *bla_map;
> dma_buf_map_incr(map, offsetof(...));
Wrt the issue at hand I think the above is perfectly fine code. The idea
with dma_buf_map is really that it's just a special pointer, so writing
the code exactly as pointer code feels best. Unfortunately you cannot make
them typesafe (because of C), so the code sometimes looks a bit ugly.
Otherwise we could do stuff like container_of and all that with
typechecking in the macros.
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(..));
Lucas De Marchi
-Daniel
> IMO this construct is worse because at a point in time in the function
> the map was pointing to the wrong thing the function was supposed to
> read/write.
>
> It's also useful when the function has double duty, updating a global
> part of the struct and a table inside it (see example in patch 6)
>
> thanks
> Lucas De Marchi
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
Am 27.01.22 um 10:12 schrieb Lucas De Marchi: On Thu, Jan 27, 2022 at 09:55:05AM +0100, Christian König wrote: Am 27.01.22 um 09:18 schrieb Lucas De Marchi: On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote: Am 27.01.22 um 08:57 schrieb Lucas De Marchi: On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König wrote: Am 26.01.22 um 21:36 schrieb Lucas De Marchi: [SNIP] humn... not sure if I was clear. There is no importer and exporter here. Yeah, and exactly that's what I'm pointing out as problem here. You are using the inter driver framework for something internal to the driver. That is an absolutely clear NAK! We could discuss that, but you guys are just sending around patches to do this without any consensus that this is a good idea. s/you guys/you/ if you have to blame anyone - I'm the only s-o-b in these patches. I'm sending these to _build consensus_ on what may be a good use for it showing a real problem it's helping to fix. Well a cover letter would have been helpful, my impression was that you have a larger set and just want to upstream some minor DMA-buf changes necessary for it. I missed adding this sentence to the cover letter, as my impression was that dma-buf-map was already used outside inter-driver framework. But there is actually a cover letter: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220126203702.1784589-1-lucas.demarchi%40intel.com%2Fdata=04%7C01%7Cchristian.koenig%40amd.com%7Cb36def4a6ebd4879731c08d9e1753ccd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788715933199161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=gwW05OaUq%2FxlBWnY%2FPuPfl0YDdKp5VTbllaSmn45nE8%3Dreserved=0 And looking at it now, it seems I missed adding Thomas Zimmermann to Cc. Now I know why people are bugging me all the time to add cover letters to add more context to my sets. From its documentation: * The type :c:type:`struct dma_buf_map ` and its helpers are * actually independent from the dma-buf infrastructure. When sharing buffers * among devices, drivers have to know the location of the memory to access * the buffers in a safe way. :c:type:`struct dma_buf_map ` * solves this problem for dma-buf and its users. If other drivers or * sub-systems require similar functionality, the type could be generalized * and moved to a more prominent header file. if there is no consensus and a better alternative, I'm perfectly fine in throwing it out and using the better approach. When Thomas Zimmermann upstreamed the dma_buf_map work we had a discussion if that shouldn't be independent of the DMA-buf framework. The consensus was that as soon as we have more widely use for it this should be made independent. So basically that is what's happening now. I suggest the following approach: 1. Find a funky name for this, something like iomem_, kiomap_ or similar. iosys_map? Works for me. 2. Separate this from all you driver dependent work and move the dma_buf_map structure out of DMA-buf into this new whatever_ prefix. should this be a follow up to the driver work or a prerequisite? Prerequisite. Structural changes like this always separate to the actually work switching over to them because the later needs a much fewer audience for review. Regards, Christian. thanks Lucas De Marchi 3. Ping Thomas, LKML, me and probably a couple of other core people if this is the right idea or not. 4. Work on dropping the map parameter from dma_buf_vunmap(). This is basically why we can't modify the pointers returned from dma_buf_vmap() and has already cause a few problems with dma_buf_map_incr(). Regards, Christian. Lucas De Marchi
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
On Thu, Jan 27, 2022 at 09:55:05AM +0100, Christian König wrote: Am 27.01.22 um 09:18 schrieb Lucas De Marchi: On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote: Am 27.01.22 um 08:57 schrieb Lucas De Marchi: On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König wrote: Am 26.01.22 um 21:36 schrieb Lucas De Marchi: [SNIP] humn... not sure if I was clear. There is no importer and exporter here. Yeah, and exactly that's what I'm pointing out as problem here. You are using the inter driver framework for something internal to the driver. That is an absolutely clear NAK! We could discuss that, but you guys are just sending around patches to do this without any consensus that this is a good idea. s/you guys/you/ if you have to blame anyone - I'm the only s-o-b in these patches. I'm sending these to _build consensus_ on what may be a good use for it showing a real problem it's helping to fix. Well a cover letter would have been helpful, my impression was that you have a larger set and just want to upstream some minor DMA-buf changes necessary for it. I missed adding this sentence to the cover letter, as my impression was that dma-buf-map was already used outside inter-driver framework. But there is actually a cover letter: https://lore.kernel.org/all/20220126203702.1784589-1-lucas.demar...@intel.com/ And looking at it now, it seems I missed adding Thomas Zimmermann to Cc. Now I know why people are bugging me all the time to add cover letters to add more context to my sets. From its documentation: * The type :c:type:`struct dma_buf_map ` and its helpers are * actually independent from the dma-buf infrastructure. When sharing buffers * among devices, drivers have to know the location of the memory to access * the buffers in a safe way. :c:type:`struct dma_buf_map ` * solves this problem for dma-buf and its users. If other drivers or * sub-systems require similar functionality, the type could be generalized * and moved to a more prominent header file. if there is no consensus and a better alternative, I'm perfectly fine in throwing it out and using the better approach. When Thomas Zimmermann upstreamed the dma_buf_map work we had a discussion if that shouldn't be independent of the DMA-buf framework. The consensus was that as soon as we have more widely use for it this should be made independent. So basically that is what's happening now. I suggest the following approach: 1. Find a funky name for this, something like iomem_, kiomap_ or similar. iosys_map? 2. Separate this from all you driver dependent work and move the dma_buf_map structure out of DMA-buf into this new whatever_ prefix. should this be a follow up to the driver work or a prerequisite? thanks Lucas De Marchi 3. Ping Thomas, LKML, me and probably a couple of other core people if this is the right idea or not. 4. Work on dropping the map parameter from dma_buf_vunmap(). This is basically why we can't modify the pointers returned from dma_buf_vmap() and has already cause a few problems with dma_buf_map_incr(). Regards, Christian. Lucas De Marchi
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
Am 27.01.22 um 09:18 schrieb Lucas De Marchi: On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote: Am 27.01.22 um 08:57 schrieb Lucas De Marchi: On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König wrote: Am 26.01.22 um 21:36 schrieb Lucas De Marchi: [SNIP] humn... not sure if I was clear. There is no importer and exporter here. Yeah, and exactly that's what I'm pointing out as problem here. You are using the inter driver framework for something internal to the driver. That is an absolutely clear NAK! We could discuss that, but you guys are just sending around patches to do this without any consensus that this is a good idea. s/you guys/you/ if you have to blame anyone - I'm the only s-o-b in these patches. I'm sending these to _build consensus_ on what may be a good use for it showing a real problem it's helping to fix. Well a cover letter would have been helpful, my impression was that you have a larger set and just want to upstream some minor DMA-buf changes necessary for it. Now I know why people are bugging me all the time to add cover letters to add more context to my sets. From its documentation: * The type :c:type:`struct dma_buf_map ` and its helpers are * actually independent from the dma-buf infrastructure. When sharing buffers * among devices, drivers have to know the location of the memory to access * the buffers in a safe way. :c:type:`struct dma_buf_map ` * solves this problem for dma-buf and its users. If other drivers or * sub-systems require similar functionality, the type could be generalized * and moved to a more prominent header file. if there is no consensus and a better alternative, I'm perfectly fine in throwing it out and using the better approach. When Thomas Zimmermann upstreamed the dma_buf_map work we had a discussion if that shouldn't be independent of the DMA-buf framework. The consensus was that as soon as we have more widely use for it this should be made independent. So basically that is what's happening now. I suggest the following approach: 1. Find a funky name for this, something like iomem_, kiomap_ or similar. 2. Separate this from all you driver dependent work and move the dma_buf_map structure out of DMA-buf into this new whatever_ prefix. 3. Ping Thomas, LKML, me and probably a couple of other core people if this is the right idea or not. 4. Work on dropping the map parameter from dma_buf_vunmap(). This is basically why we can't modify the pointers returned from dma_buf_vmap() and has already cause a few problems with dma_buf_map_incr(). Regards, Christian. Lucas De Marchi
Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map
On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian König wrote: Am 27.01.22 um 08:57 schrieb Lucas De Marchi: On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian König 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. Well what you propose here can lead to all kind of problems and is rather bad design as far as I can see. The struct dma_buf_map is only to be filled in by the exporter and should not be modified in this way by the importer. humn... not sure if I was clear. There is no importer and exporter here. Yeah, and exactly that's what I'm pointing out as problem here. You are using the inter driver framework for something internal to the driver. That is an absolutely clear NAK! We could discuss that, but you guys are just sending around patches to do this without any consensus that this is a good idea. s/you guys/you/ if you have to blame anyone - I'm the only s-o-b in these patches. I'm sending these to _build consensus_ on what may be a good use for it showing a real problem it's helping to fix. From its documentation: * The type :c:type:`struct dma_buf_map ` and its helpers are * actually independent from the dma-buf infrastructure. When sharing buffers * among devices, drivers have to know the location of the memory to access * the buffers in a safe way. :c:type:`struct dma_buf_map ` * solves this problem for dma-buf and its users. If other drivers or * sub-systems require similar functionality, the type could be generalized * and moved to a more prominent header file. if there is no consensus and a better alternative, I'm perfectly fine in throwing it out and using the better approach. Lucas De Marchi
