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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 30, 2019 at 4:41 PM Koenig, Christian
<Christian.Koenig@amd.com> wrote:
>
> Am 30.04.19 um 16:34 schrieb Daniel Vetter:
> > [CAUTION: External Email]
> >
> > 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).
>
> Yeah, that's on my mind as well. But I'm running into a chicken and egg
> problem here again.

Yeah the usual refactor story :-/

> Basically we need to convert the drivers to do their internal operation
> using the DMA-buf as top level object first and then we can switch all
> internal operation to using a "normal" attachment.
>
> Additional to that it simply doesn't looks like the right idea to use
> the attachment as parameter here. After all we pin the underlying
> DMA-buf and NOT the attachment.

We pin both imo - I'd be really surprised as an importer if I attach,
pin, and then the exporter tells me I can't map the attachment I just
made anymore because the buffer is in the wrong place. That's imo what
pin really should make sure is assured - we already require this for
the static importers to be true (which is also why caching makes
sense). Hence for me global pin = pin all the attachments.

I also dropped some questions on the amdgpu patches to hopefully
better understand all this with actual implementations.

btw totally different thought: For dynamic exporters, should we drop
the cached sgt on the floor if it's not in use? the drm_prime.c
helpers don't need this since they map all the time, but afaiui at
least v4l and i915 don't hang onto a mapping forever, only when
actually needed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel