Re: [PATCH 02/12] dma-buf: add explicit buffer pinning v2

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 30, 2019 at 04:26:32PM +0200, Christian König wrote:
> Am 30.04.19 um 15:59 schrieb Daniel Vetter:
> > On Tue, Apr 30, 2019 at 03:42:22PM +0200, Christian König wrote:
> > > Am 29.04.19 um 10:40 schrieb Daniel Vetter:
> > > > On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote:
> > > > > [SNIP]
> > > > > +/**
> > > > > + * dma_buf_pin - Lock down the DMA-buf
> > > > > + *
> > > > > + * @dmabuf:	[in]	DMA-buf to lock down.
> > > > > + *
> > > > > + * Returns:
> > > > > + * 0 on success, negative error code on failure.
> > > > > + */
> > > > > +int dma_buf_pin(struct dma_buf *dmabuf)
> > > > I think this should be on the attachment, not on the buffer. Or is the
> > > > idea that a pin is for the entire buffer, and all subsequent
> > > > dma_buf_map_attachment must work for all attachments? I think this matters
> > > > for sufficiently contrived p2p scenarios.
> > > This is indeed for the DMA-buf, cause we are pinning the underlying backing
> > > store and not just one attachment.
> > You can't move the buffer either way, so from that point there's no
> > difference. I more meant from an account/api point of view of whether it's
> > ok to pin a buffer you haven't even attached to yet. From the code it
> > seems like first you always want to attach, hence it would make sense to
> > put the pin/unpin onto the attachment. That might also help with
> > debugging, we could check whether everyone balances their pin/unpin
> > correctly (instead of just counting for the overall dma-buf).
> > 
> > There's also a slight semantic difference between a pin on an attachment
> > (which would mean this attachment is always going to be mappable and wont
> > move around), whereas a pin on the entire dma-buf means the entire dma-buf
> > and all it's attachment must always be mappable. Otoh, global pin is
> > probably easier, you just need to check all current attachments again
> > whether they still work or whether you might need to move the buffer
> > around to a more suitable place (e.g. if you not all can do p2p it needs
> > to go into system ram before it's pinned).
> > 
> > For the backing storage you only want one per-bo ->pinned_count, that's
> > clear, my suggestion was more about whether having more information about
> > who's pinning is useful. Exporters can always just ignore that added
> > information.
> > 
> > > Key point is I want to call this in the exporter itself in the long run.
> > > E.g. that the exporter stops working with its internal special handling
> > > functions and uses this one instead.
> > Hm, not exactly following why the exporter needs to call
> > dma_buf_pin/unpin, instead of whatever is used to implement that. Or do
> > you mean that you want a ->pinned_count in dma_buf itself, so that there's
> > only one book-keeping for this?
> 
> Yes, exactly that is one of the final goals of this.
> 
> We could of course use the attachment here, but that would disqualify the
> exporter calling this directly without an attachment.

Hm exporter calling dma_buf_pin/unpin sounds like one seriously inverted
lasagna :-)

I do understand the goal, all these imported/exporter special cases in
code are a bit silly, but I think the proper fix would be if you just
always import a buffer, even the private ones, allocated against some
exporter-only thing. Then it's always the same function to call.

But I'm not sure this is a good reasons to guide the dma-buf interfaces
for everyone else. Moving pin to the attachment sounds like a better idea
(if the above is the only reason to keep it on the dma-buf).
-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