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

["Koenig, Christian" <Christian.Koenig@amd.com>]

Am 03.05.19 um 14:09 schrieb Daniel Vetter:
> [CAUTION: External Email]
>
> 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?

Yeah, the invalidation callback and P2P flags are constant. But the 
importer_priv field isn't.

We could do something like adding the importer_priv field as parameter 
and the other two as const structure.

Third alternative would be to throw out all the DRM abstraction and just 
embed the attachment structure in the buffer object and get completely 
rid of the importer_priv field (probably the cleanest alternative, but 
also the most work todo).

Christian.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel