Re: [Intel-gfx] [PATCH] dma-buf: add struct dma_buf_attach_info v2

From: Daniel Vetter <daniel@ffwll.ch>
To: christian.koenig@amd.com
Cc: maxime.ripard@bootlin.com, dri-devel@lists.freedesktop.org,
	digetx@gmail.com, m.szyprowski@samsung.com,
	devel@driverdev.osuosl.org, sstabellini@kernel.org,
	arnd@arndb.de,
	Russell King - ARM Linux admin <linux@armlinux.org.uk>,
	jonathanh@nvidia.com, tomi.valkeinen@ti.com,
	xen-devel@lists.xenproject.org, linux-media@vger.kernel.org,
	pawel@osciak.com, intel-gfx@lists.freedesktop.org,
	linux-tegra@vger.kernel.org, boris.ostrovsky@oracle.com,
	mchehab@kernel.org, jgross@suse.com, gregkh@linuxfoundation.org,
	linux-kernel@vger.kernel.org, kyungmin.park@samsung.com
Subject: Re: [Intel-gfx] [PATCH] dma-buf: add struct dma_buf_attach_info v2
Date: Fri, 3 May 2019 14:09:33 +0200	[thread overview]
Message-ID: <20190503120933.GL3271@phenom.ffwll.local> (raw)
In-Reply-To: <cbcbb076-a8b0-67b0-8c16-daf1d060fc1d@gmail.com>

On Fri, May 03, 2019 at 02:05:47PM +0200, Christian König wrote:
> Am 30.04.19 um 19:31 schrieb Russell King - ARM Linux admin:
> > On Tue, Apr 30, 2019 at 01:10:02PM +0200, Christian König wrote:
> > > Add a structure for the parameters of dma_buf_attach, this makes it much easier
> > > to add new parameters later on.
> > I don't understand this reasoning.  What are the "new parameters" that
> > are being proposed, and why do we need to put them into memory to pass
> > them across this interface?
> > 
> > If the intention is to make it easier to change the interface, passing
> > parameters in this manner mean that it's easy for the interface to
> > change and drivers not to notice the changes, since the compiler will
> > not warn (unless some member of the structure that the driver is using
> > gets removed, in which case it will error.)
> > 
> > Additions to the structure will go unnoticed by drivers - what if the
> > caller is expecting some different kind of behaviour, and the driver
> > ignores that new addition?
> 
> Well, exactly that's the intention here: That the drivers using this
> interface should be able to ignore the new additions for now as long as they
> are not going to use them.
> 
> The background is that we have multiple interface changes in the pipeline,
> and each step requires new optional parameters.
> 
> > This doesn't seem to me like a good idea.
> 
> Well, the obvious alternatives are:
> 
> a) Change all drivers to explicitly provide NULL/0 for the new parameters.
> 
> b) Use a wrapper, so that the function signature of dma_buf_attach stays the
> same.
> 
> Key point here is that I have an invalidation callback change, a P2P patch
> set and some locking changes which all require adding new parameters or
> flags. And at each step I would then start to change all drivers, adding
> some more NULL pointers or flags with 0 default value.
> 
> I'm actually perfectly fine going down any route, but this just seemed to me
> simplest and with the least risk of breaking anything. Opinions?

I think given all our discussions and plans the argument object makes tons
of sense. Much easier to document well than a long list of parameters.
Maybe we should make it const, so it could work like an ops/func table and
we could store it as a pointer in the dma_buf_attachment?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel