Message-ID: <YfJedaoeJjE3grum@phenom.ffwll.local>
Date:   Thu, 27 Jan 2022 09:57:25 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Christian König <christian.koenig@....com>
Cc:     Lucas De Marchi <lucas.demarchi@...el.com>,
        linaro-mm-sig@...ts.linaro.org, intel-gfx@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-media@...r.kernel.org
Subject: Re: [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.

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.

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

Powered by blists - more mailing lists