[Linaro-mm-sig] [RFC] dma-shared-buf: Add buffer sharing framework

[Clark, Rob]

On Fri, Sep 9, 2011 at 1:43 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Thu, Sep 08, 2011 at 05:56:19PM -0500, Clark, Rob wrote:
>> On Thu, Sep 8, 2011 at 3:28 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> > On Thu, Sep 08, 2011 at 12:39:41PM -0500, Clark, Rob wrote:
>> >> On Thu, Sep 8, 2011 at 10:55 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> >> > On Thu, Sep 08, 2011 at 08:45:43PM +0530, Sumit Semwal wrote:
>> >> >> On 8 September 2011 19:48, Clark, Rob <rob at ti.com> wrote:
>> >> >> > actually, all I think we needed was to add 'struct device *'  to
>> >> >> > get_scatterlist (and maybe doesn't hurt for put)..  I'm not entirely
>> >> >> > sure why we need create/attach_device?  The create fxn especially
>> >> >> > seems wrong.. what if you are the 2nd device to import the buffer?
>> >> >> >
>> >> >> Ah... I guess I misunderstood your 'it seems like a sane approach' to mean
>> >> >> you agree with the 2-step process suggested by Daniel :) - my bad!
>> >> >
>> >> > On re-reading Rob's response I think there's a misunderstanding. I don't
>> >> > want a create function in the dma_buf_ops. The create would be a generic
>> >> > dma_buf subsystem function that (like in the patch) allocates the struct
>> >> > and properly registers the fd and what not else. The attach_device would
>> >> > then be the only new function in dma_buf_ops.
>> >> >
>> >> > The confusion probably stems from my other proposal to hide these function
>> >> > pointers behind small inline helpers for the in-kernel api.
>> >>
>> >> I guess/assume that what you are trying to get to is a way to
>> >> differentiate between "I am done DMA'ing to the buffer now, but will
>> >> likely use it again (ie. feel free to cache the mapping)", and "I am
>> >> done forever w/ the buffer (don't bother caching)"?
>> >
>> > Hm, no, I haven't really thought much about the release side. dri2/gem
>> > doesn't have any automatic caching of shared buffers, btw, that's all done
>> > manually (i.e. in the higher levels like the mesa/ddx drivers) by keeping
>> > track of recently shared buffers (and hanging onto them for as long as
>> > possible without creating havoc on the other side).
>>
>> ok, then I've lost sync.. (ok, I admit to be only having time to read
>> email for 5 min here and there, which might be contributing to the
>> problem)..
>>
>> what was the purpose of attach() then (which couldn't be accomplished
>> in the first get_scatterlist())?
>
> Ok, I'll try to explain with this a use-case we've discussed: A video
> input device, some yuv plane scanout engine and an mpeg encoder, i.e. the
> video-shooting usecase. mpeg encoder is optional and only engaged when the
> user presse the record button.
>
> Now for live-view, userspaces grabs some dma_buf ids (i.e. fds) from the
> v4l device and then creates a bunch of yuv planes at the scanout device
> with these. For this case you're right that we could arbitrage the memory
> layout at the get_scatterlist call of the scanout device (the other user,
> the frame-grabber, is already known because the dma_buf orginitated
> there).
>
> But if we throw a third device into the mix, this won't work. Somewhen
> (and it doesn't matter how much we delay this till the complete pipeline
> is set up), either the the scanout device or the encoder has to call
> get_scatterlist before the other one. And as soon is the allocator
> (respectively the originator) has handed out a scatterlist (either in
> system address space or device address space) it cannot move the backing
> storage anymore. If the other device then calls get_scatterlist and has
> more stringet buffer placement requirements, things stop working.
>
> Now my idea is that when userspace establishes the shared buffer (i.e.
> subsystems convert the dma_buf fd number into a subsystem specific id),
> the device taking part in this buffer sharing only calls attach_device,
> but not get_scatterlist.
>
> After this has taken place for all devices, userspace starts the
> pipelined. Kernel drivers only then call get_scatterlist and so force the
> allocation of the backing storage. And by then all participating devices
> are known, so the right placement can be chosen for the buffer.

My one concern is that, depending one the app/framework above, it may
not be possible to know all the involved devices at the start.. or at
least this would require more extensive userspace changes, not just
userspace framework (like GStreamer) changes..

But how about this.. and I think this approach ensures that there will
be no later buffer migration.  But in the other case, it ensures that
there is at most one migration.  (This all assuming the devices can
physically share the same buffer.)

-----
struct dma_buf_attachment * dma_buf_attach(struct dma_buf *buf, struct
device *dev);
  --> establish intent to use the buffer w/ particular device

for (... # of times buffer is used ...) {
struct scatterlist * dma_buf_get_scatterlist(struct dma_buf_attachment *attach);
  --> indicate begin of DMA, first call might actually back the buffer
w/ pages or migrate buffer

void dma_buf_put_scatterlist(struct dma_buf_attachment *attach, struct
scatterlist *);
  --> indication end of DMA
}

void dma_buf_detach(struct dma_buf_attachment *attach);
  --> indicate buffer is no longer to be used by importing device
-----

with this sort of approach, if a new device is attached after the
first get_scatterlist the buffer can be, if needed, migrated using the
union of all the devices requirements at a point in time when no DMA
is active to/from the buffer.  But if all the devices are known up
front, then you never need to migrate unnecessarily.

This way, if userspace is able to ensure all devices are known
upfront, everything is great.  But if not, things don't explode and
you aren't forced to reallocate buffers in userspace or anything ugly
like this.

Optionally perhaps attach could be combined w/ dma_buf_get and detach
w/ dma_buf_put.. (although then they probably wouldn't return a
'struct dma_buf *'.. not sure if that is weird.  But I guess
get+attach and detach+put would always go together and this saves an
extra call.  I don't have (yet) a strong opinion one way or another on
that.

BR,
-R

> So the reason for attach_device is to make sharing between 3 devices
> reliably possible (without userspace needing to know in which sequence to
> share buffers with devices). For 2 devices, attach_device isn't really
> necessary because the first device does an implicit attach_device when
> creating the buffer - it better not forget it's own memory placement
> requirements ;-) So for just 2 devices delaying the allocation up to the
> call to get_scatterlist is good enough.
>
> Cheers, Daniel
> --
> Daniel Vetter
> Mail: daniel at ffwll.ch
> Mobile: +41 (0)79 365 57 48
>