Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 3 May 2019 14:09:33 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     christian.koenig@amd.com
Cc:     Russell King - ARM Linux admin <linux@armlinux.org.uk>,
        maxime.ripard@bootlin.com, dri-devel@lists.freedesktop.org,
        digetx@gmail.com, sumit.semwal@linaro.org,
        m.szyprowski@samsung.com, devel@driverdev.osuosl.org,
        sstabellini@kernel.org, arnd@arndb.de, 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
Message-ID: <20190503120933.GL3271@phenom.ffwll.local>
Mail-Followup-To: christian.koenig@amd.com,
        Russell King - ARM Linux admin <linux@armlinux.org.uk>,
        maxime.ripard@bootlin.com, dri-devel@lists.freedesktop.org,
        digetx@gmail.com, sumit.semwal@linaro.org, m.szyprowski@samsung.com,
        devel@driverdev.osuosl.org, sstabellini@kernel.org, arnd@arndb.de,
        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
References: <20190430111002.106168-1-christian.koenig@amd.com>
 <20190430173127.k5ivpaz6ktbfecgo@shell.armlinux.org.uk>
 <cbcbb076-a8b0-67b0-8c16-daf1d060fc1d@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <cbcbb076-a8b0-67b0-8c16-daf1d060fc1d@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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