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

Thomas Zimmermann <tzimmermann@xxxxxxx> · Thu, 27 Jan 2022 15:52:50 +0100

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 <daniel.vetter@xxxxxxxx>

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 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
https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C0654a16ea3444271d7c308d9e17bd35d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788744226808874%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Q6soluBglaZLhLszdapaWuUVsqMq5qvJOKiJjO%2B9BTg%3D&amp;reserved=0 

--
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