Message-ID: <20120103210927.GJ17472@phenom.dumpdata.com>
Date:	Tue, 3 Jan 2012 16:09:27 -0500
From:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To:	"Semwal, Sumit" <sumit.semwal@...com>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mm@...ck.org, linaro-mm-sig@...ts.linaro.org,
	dri-devel@...ts.freedesktop.org, linux-media@...r.kernel.org,
	t.stanislaws@...sung.com, linux@....linux.org.uk, arnd@...db.de,
	patches@...aro.org, rob@...com,
	Sumit Semwal <sumit.semwal@...aro.org>,
	m.szyprowski@...sung.com
Subject: Re: [RFC v3 1/2] dma-buf: Introduce dma buffer sharing mechanism

On Fri, Dec 23, 2011 at 03:22:35PM +0530, Semwal, Sumit wrote:
> Hi Konrad,
> 
> On Tue, Dec 20, 2011 at 10:06 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@...cle.com> wrote:
> > On Mon, Dec 19, 2011 at 02:03:30PM +0530, Sumit Semwal wrote:
> >> This is the first step in defining a dma buffer sharing mechanism.
> >>
> >> A new buffer object dma_buf is added, with operations and API to allow easy
> >> sharing of this buffer object across devices.
> >>
> >> The framework allows:
> >> - different devices to 'attach' themselves to this buffer, to facilitate
> >>   backing storage negotiation, using dma_buf_attach() API.
> >
> > Any thoughts of adding facility to track them? So you can see who is using what?
> Not for version 1, but it would be a useful addition once we start
> using this mechanism.

OK. Looking forward to V2.
> 
> >
> >> - association of a file pointer with each user-buffer and associated
> >>    allocator-defined operations on that buffer. This operation is called the
> >>    'export' operation.
> >
> >  'create'? or 'alloc' ?
> >
> > export implies an import somwhere and I don't think that is the case here.
> I will rephrase it as suggested by Rob as well.
> 
> >
> >> - this exported buffer-object to be shared with the other entity by asking for
> >>    its 'file-descriptor (fd)', and sharing the fd across.
> >> - a received fd to get the buffer object back, where it can be accessed using
> >>    the associated exporter-defined operations.
> >> - the exporter and user to share the scatterlist using map_dma_buf and
> >>    unmap_dma_buf operations.
> >>
> >> Atleast one 'attach()' call is required to be made prior to calling the
> >> map_dma_buf() operation.
> >
> > for the whole memory region or just for the device itself?
> Rob has very eloquently and kindly explained it in his reply.

Can you include his words of wisdom in the git description?

> 
> >
> >>
> <snip>
> >> +/*
> >> + * is_dma_buf_file - Check if struct file* is associated with dma_buf
> >> + */
> >> +static inline int is_dma_buf_file(struct file *file)
> >> +{
> >> +     return file->f_op == &dma_buf_fops;
> >> +}
> >> +
> >> +/**
> >
> > Wrong kerneldoc.
> I looked into scripts/kernel-doc, and
> Documentation/kernel-doc-na-HOWTO.txt => both these places mention
> that the kernel-doc comments have to start with /**, and I couldn't
> spot an error in what's wrong with my usage - would you please
> elaborate on what you think is not right?

The issue I had was with '/**' but let me double-check where I learned
that /** was a bad. Either way, it is a style-guide thing and the
Documentation/* trumps what I recall.

> >
> <snip>
> >> +/**
> >> + * struct dma_buf_attachment - holds device-buffer attachment data
> >
> > OK, but what is the purpose of it?
> I will add that in the comments.
> >
> >> + * @dmabuf: buffer for this attachment.
> >> + * @dev: device attached to the buffer.
> >                                ^^^ this
> >> + * @node: list_head to allow manipulation of list of dma_buf_attachment.
> >
> > Just say: "list of dma_buf_attachment"'
> ok.
> >
> >> + * @priv: exporter-specific attachment data.
> >
> > That "exporter-specific.." brings to my mind custom decleration forms. But maybe that is me.
> :) well, in context of dma-buf 'exporter', it makes sense.

Or just private contents of the backend driver. But the naming is not that important
to inhibit this patch from being merged.
> 
> >
> >> + */
> >> +struct dma_buf_attachment {
> >> +     struct dma_buf *dmabuf;
> >> +     struct device *dev;
> >> +     struct list_head node;
> >> +     void *priv;
> >> +};
> >
> > Why don't you move the decleration of this below 'struct dma_buf'?
> > It would easier than to read this structure..
> I could do that, but then anyways I will have to do a
> forward-declaration of dma_buf_attachment, since I have to use it in
> dma_buf_ops. If it improves readability, I am happy to move it below
> struct dma_buf

It is more of just making the readability easier. As in reading from top bottom
one. But if it is too ugly, don't bother.
> 
> >
> >> +
> >> +/**
> >> + * struct dma_buf_ops - operations possible on struct dma_buf
> >> + * @attach: allows different devices to 'attach' themselves to the given
> >
> > register?
> >> + *       buffer. It might return -EBUSY to signal that backing storage
> >> + *       is already allocated and incompatible with the requirements
> >
> > Wait.. allocated or attached?
> This and above comment on 'register' are already answered by Rob in
> his explanation of the sequence in earlier reply. [the Documentation
> patch [2/2] also tries to explain it]

OK. Might want to mention the user to look in the Documentation, in case you don't
already have it.
> 
> >
> >> + *       of requesting device. [optional]
> >
> > What is optional? The return value? Or the 'attach' call? If the later , say
> > that in the first paragraph.
> >
> ok, sure. it is meant for the attach op.
> >
> >> + * @detach: detach a given device from this buffer. [optional]
> >> + * @map_dma_buf: returns list of scatter pages allocated, increases usecount
> >> + *            of the buffer. Requires atleast one attach to be called
> >> + *            before. Returned sg list should already be mapped into
> >> + *            _device_ address space. This call may sleep. May also return
> >
> > Ok, there is some __might_sleep macro you should put on the function.
> >
> That's a nice suggestion; I will add it to the wrapper function for
> map_dma_buf().
> 
> >> + *            -EINTR.
> >
> > Ok. What is the return code if attach has _not_ been called?
> Will document it to return -EINVAL if atleast on attach() hasn't been called.
> 
> >
> >> + * @unmap_dma_buf: decreases usecount of buffer, might deallocate scatter
> >> + *              pages.
> >> + * @release: release this buffer; to be called after the last dma_buf_put.
> >> + * @sync_sg_for_cpu: sync the sg list for cpu.
> >> + * @sync_sg_for_device: synch the sg list for device.
> >
> > Not seeing those two.
> Oops; removed in v3 - will correct.
> 
> >> + */
> <snip>
> >> +     /* TODO: Add try_map_dma_buf version, to return immed with -EBUSY
> >
> > Ewww. Why? Why not just just the 'map_dma_buf' and return that?
> Requirement is to allow for blocking and non-blocking versions of
> map_dma_buf. try_map_dma_buf could be used for the non-blocking
> version.

Ok. What about just adding that in as a nop function operation. And have an
initial implementation in the code base that does .. well, nothing except 
return -ENOSPC and move the TODO to the code instead of it being in the header?

> 
> >
> <snip>
> >> +/**
> >> + * struct dma_buf - shared buffer object
> >
> > Missing the 'size'.
> Will add.
> >
> >> + * @file: file pointer used for sharing buffers across, and for refcounting.
> >> + * @attachments: list of dma_buf_attachment that denotes all devices attached.
> >> + * @ops: dma_buf_ops associated with this buffer object
> >> + * @priv: user specific private data
> >
> >
> > Can you elaborate on this? Is this the "exporter" using this? Or is
> > it for the "user" using it? If so, why provide it? Wouldn't the
> > user of this have something like this:
> >
> > struct my_dma_bufs {
> >        struct dma_buf[20];
> >        void *priv;
> > }
> >
> > Anyhow?
> My bad - it is meant for the exporter - exporter gives this as one of
> the parameters to 'dma_buf_export()' API. I will correct the comment.
> >
> Thanks for your review!

Sure. And please put 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>'.
> Best regards,
> ~Sumit.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists