Re: [PATCH 2/2] dma-buf: add helpers for attacher dma-parms
On Fri, Jul 20, 2012 at 10:09 PM, Rob Clark wrote:
> Fyi, Daniel Vetter had suggested on IRC that it would be cleaner to
> have a single helper fxn that most-restrictive union of all attached
> device's dma_parms. Really this should include dma_mask and
> coherent_dma_mask, I think. But that touches a lot of other places in
> the code. If no one objects to the cleanup of moving
> dma_mask/coherent_dma_mask into dma_parms, I'll do this first.
>
> So anyways, don't consider this patch yet for inclusion, I'll make an
> updated one based on dma_parms..
Hi Rob,
Any news on this patch-set?
>
> BR,
> -R
BR,
~Sumit.
>
> On Thu, Jul 19, 2012 at 11:23 AM, Rob Clark wrote:
>> From: Rob Clark
>>
>> Add some helpers to iterate through all attachers and get the most
>> restrictive segment size/count/boundary.
>>
>> Signed-off-by: Rob Clark
>> ---
>> drivers/base/dma-buf.c | 63
>> +++
>> include/linux/dma-buf.h | 19 ++
>> 2 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> index 24e88fe..757ee20 100644
>> --- a/drivers/base/dma-buf.c
>> +++ b/drivers/base/dma-buf.c
>> @@ -192,6 +192,69 @@ void dma_buf_put(struct dma_buf *dmabuf)
>> EXPORT_SYMBOL_GPL(dma_buf_put);
>>
>> /**
>> + * dma_buf_max_seg_size - helper for exporters to get the minimum of
>> + * all attached device's max segment size
>> + */
>> +unsigned int dma_buf_max_seg_size(struct dma_buf *dmabuf)
>> +{
>> + struct dma_buf_attachment *attach;
>> + unsigned int max = (unsigned int)-1;
>> +
>> + if (WARN_ON(!dmabuf))
>> + return 0;
>> +
>> + mutex_lock(&dmabuf->lock);
>> + list_for_each_entry(attach, &dmabuf->attachments, node)
>> + max = min(max, dma_get_max_seg_size(attach->dev));
>> + mutex_unlock(&dmabuf->lock);
>> +
>> + return max;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_max_seg_size);
>> +
>> +/**
>> + * dma_buf_max_seg_count - helper for exporters to get the minimum of
>> + * all attached device's max segment count
>> + */
>> +unsigned int dma_buf_max_seg_count(struct dma_buf *dmabuf)
>> +{
>> + struct dma_buf_attachment *attach;
>> + unsigned int max = (unsigned int)-1;
>> +
>> + if (WARN_ON(!dmabuf))
>> + return 0;
>> +
>> + mutex_lock(&dmabuf->lock);
>> + list_for_each_entry(attach, &dmabuf->attachments, node)
>> + max = min(max, dma_get_max_seg_count(attach->dev));
>> + mutex_unlock(&dmabuf->lock);
>> +
>> + return max;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_max_seg_count);
>> +
>> +/**
>> + * dma_buf_get_seg_boundary - helper for exporters to get the most
>> + * restrictive segment alignment of all the attached devices
>> + */
>> +unsigned int dma_buf_get_seg_boundary(struct dma_buf *dmabuf)
>> +{
>> + struct dma_buf_attachment *attach;
>> + unsigned int mask = (unsigned int)-1;
>> +
>> + if (WARN_ON(!dmabuf))
>> + return 0;
>> +
>> + mutex_lock(&dmabuf->lock);
>> + list_for_each_entry(attach, &dmabuf->attachments, node)
>> + mask &= dma_get_seg_boundary(attach->dev);
>> + mutex_unlock(&dmabuf->lock);
>> +
>> + return mask;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_get_seg_boundary);
>> +
>> +/**
>> * dma_buf_attach - Add the device to dma_buf's attachments list;
>> optionally,
>> * calls attach() of dma_buf_ops to allow device-specific attach
>> functionality
>> * @dmabuf:[in]buffer to attach device to.
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index eb48f38..9533b9b 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -167,6 +167,10 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
>> struct dma_buf *dma_buf_get(int fd);
>> void dma_buf_put(struct dma_buf *dmabuf);
>>
>> +unsigned int dma_buf_max_seg_size(struct dma_buf *dmabuf);
>> +unsigned int dma_buf_max_seg_count(struct dma_buf *dmabuf);
>> +unsigned int dma_buf_get_seg_boundary(struct dma_buf *dmabuf);
>> +
>> struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>> enum dma_data_direction);
>> void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table
>> *,
>> @@ -220,6 +224,21 @@ static inline void dma_buf_put(struct dma_buf *dmabuf)
>> return;
>> }
>>
>> +static inline unsigned int dma_buf_max_seg_size(struct dma_buf *dmabuf)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline unsigned int dma_buf_max_seg_count(struct dma_buf *dmabuf)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline unsigned int dma_buf_get_seg_boundary(struct dma_buf *dmabuf)
>> +{
>> + return 0;
>> +}
>> +
>> static inline struct sg_table *dma_buf_map_attachment(
>> struct dma_buf_attachment *attach, enum dma_data_direction write)
>> {
>> --
>> 1.7.9.5
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-
Re: [PATCH] omap_vout: Set DSS overlay_info only if paddr is non zero
On Mon, Mar 19, 2012 at 5:16 PM, Archit Taneja wrote:
> On Monday 19 March 2012 02:15 PM, Hiremath, Vaibhav wrote:
>>
>> On Fri, Mar 16, 2012 at 16:41:27, Taneja, Archit wrote:
>>>
>>> Hi,
>>>
>>> On Friday 16 March 2012 03:46 PM, Archit Taneja wrote:
On Monday 12 March 2012 03:34 PM, Hiremath, Vaibhav wrote:
>
> On Wed, Mar 07, 2012 at 14:31:16, Taneja, Archit wrote:
>>
>> The omap_vout driver tries to set the DSS overlay_info using
>> set_overlay_info()
>> when the physical address for the overlay is still not configured.
>> This happens
>> in omap_vout_probe() and vidioc_s_fmt_vid_out().
>>
>> The calls to omapvid_init(which internally calls set_overlay_info())
>> are removed
>> from these functions. They don't need to be called as the
>> omap_vout_device
>> struct anyway maintains the overlay related changes made. Also,
>> remove the
>> explicit call to set_overlay_info() in vidioc_streamon(), this was
>> used to set
>> the paddr, this isn't needed as omapvid_init() does the same thing
>> later.
>>
>> These changes are required as the DSS2 driver since 3.3 kernel
>> doesn't let you
>> set the overlay info with paddr as 0.
>>
>> Signed-off-by: Archit Taneja
>> ---
>> drivers/media/video/omap/omap_vout.c | 36
>> -
>> 1 files changed, 5 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/media/video/omap/omap_vout.c
>> b/drivers/media/video/omap/omap_vout.c
>> index 1fb7d5b..dffcf66 100644
>> --- a/drivers/media/video/omap/omap_vout.c
>> +++ b/drivers/media/video/omap/omap_vout.c
>> @@ -1157,13 +1157,6 @@ static int vidioc_s_fmt_vid_out(struct file
>> *file, void *fh,
>> /* set default crop and win */
>> omap_vout_new_format(&vout->pix,&vout->fbuf,&vout->crop,&vout->win);
>>
>> - /* Save the changes in the overlay strcuture */
>> - ret = omapvid_init(vout, 0);
>> - if (ret) {
>> - v4l2_err(&vout->vid_dev->v4l2_dev, "failed to change mode\n");
>> - goto s_fmt_vid_out_exit;
>> - }
>> -
>> ret = 0;
>>
>> s_fmt_vid_out_exit:
>> @@ -1664,20 +1657,6 @@ static int vidioc_streamon(struct file *file,
>> void *fh, enum v4l2_buf_type i)
>>
>> omap_dispc_register_isr(omap_vout_isr, vout, mask);
>>
>> - for (j = 0; j< ovid->num_overlays; j++) {
>> - struct omap_overlay *ovl = ovid->overlays[j];
>> -
>> - if (ovl->manager&& ovl->manager->device) {
>> - struct omap_overlay_info info;
>> - ovl->get_overlay_info(ovl,&info);
>> - info.paddr = addr;
>> - if (ovl->set_overlay_info(ovl,&info)) {
>> - ret = -EINVAL;
>> - goto streamon_err1;
>> - }
>> - }
>> - }
>> -
>
>
> Have you checked for build warnings? I am getting build warnings
>
> CC drivers/media/video/omap/omap_vout.o
> CC drivers/media/video/omap/omap_voutlib.o
> CC drivers/media/video/omap/omap_vout_vrfb.o
> drivers/media/video/omap/omap_vout.c: In function 'vidioc_streamon':
> drivers/media/video/omap/omap_vout.c:1619:25: warning: unused variable
> 'ovid'
> drivers/media/video/omap/omap_vout.c:1615:15: warning: unused variable
> 'j'
> LD drivers/media/video/omap/omap-vout.o
> LD drivers/media/video/omap/built-in.o
>
> Can you fix this and submit the next version?
>>>
>>>
>>> I applied the patch on the current mainline kernel, it doesn't give any
>>> build warnings. Even after applying the patch, 'j and ovid' are still
>>> used in vidioc_streamon().
>>>
>>> Can you check if it was applied correctly?
>>>
>>
>> Archit,
>>
>> I could able to trace what's going on here,
>>
>> I am using "v4l_for_linus" branch, which has one missing patch,
>>
>> commit aaa874a985158383c4b394c687c716ef26288741
>> Author: Tomi Valkeinen
>> Date: Tue Nov 15 16:37:53 2011 +0200
>>
>> OMAPDSS: APPLY: rewrite overlay enable/disable
>>
>>
>> So, I do not have below changes,
>>
>> @@ -1686,6 +1681,16 @@ static int vidioc_streamon(struct file *file, void
>> *fh, enum v4l2_buf_type i)
>> if (ret)
>> v4l2_err(&vout->vid_dev->v4l2_dev, "failed to change
>> mode\n");
>>
>> + for (j = 0; j< ovid->num_overlays; j++) {
>> + struct omap_overlay *ovl = ovid->overlays[j];
>> +
>> + if (ovl->manager&& ovl->manager->device) {
>>
>> + ret = ovl->enable(ovl);
>> + if (ret)
>> + goto streamon_err1;
>> + }
>> + }
>> +
>>
>> This explains why I am seeing these warnings. Let me give pull request
>> based on master branch.
>
>
> Okay. Thanks for looking into this.
>
> Archit
Hi Vaibhav,
This patch has been outstanding since March, and we have received
reports from various users. Could you please push it ASAP to Mauro for
the upcoming -rc?
Or Did I miss a pull request fr
Re: [PATCHv6 00/13] Integration of videobuf2 with dmabuf
On Tue, May 29, 2012 at 6:25 AM, Laurent Pinchart
wrote:
> Hi Tomasz,
Hi Tomasz, Laurent, Mauro,
>
> On Wednesday 23 May 2012 14:10:14 Tomasz Stanislawski wrote:
>> Hello everyone,
>> This patchset adds support for DMABUF [2] importing to V4L2 stack.
>> The support for DMABUF exporting was moved to separate patchset
>> due to dependency on patches for DMA mapping redesign by
>> Marek Szyprowski [4].
>
> Except for the small issue with patches 01/13 and 02/13, the set is ready for
> upstream as far as I'm concerned.
+1; Mauro: how do you think about this series? Getting it landed into
3.5 would make life lot easier :)
>
>> v6:
>> - fixed missing entry in v4l2_memory_names
>> - fixed a bug occuring after get_user_pages failure
>
> I've missed that one, what was it ?
>
>> - fixed a bug caused by using invalid vma for get_user_pages
>> - prepare/finish no longer call dma_sync for dmabuf buffers
>>
>> v5:
>> - removed change of importer/exporter behaviour
>> - fixes vb2_dc_pages_to_sgt basing on Laurent's hints
>> - changed pin/unpin words to lock/unlock in Doc
>>
>> v4:
>> - rebased on mainline 3.4-rc2
>> - included missing importing support for s5p-fimc and s5p-tv
>> - added patch for changing map/unmap for importers
>> - fixes to Documentation part
>> - coding style fixes
>> - pairing {map/unmap}_dmabuf in vb2-core
>> - fixing variable types and semantic of arguments in videobufb2-dma-contig.c
>>
>> v3:
>> - rebased on mainline 3.4-rc1
>> - split 'code refactor' patch to multiple smaller patches
>> - squashed fixes to Sumit's patches
>> - patchset is no longer dependant on 'DMA mapping redesign'
>> - separated path for handling IO and non-IO mappings
>> - add documentation for DMABUF importing to V4L
>> - removed all DMABUF exporter related code
>> - removed usage of dma_get_pages extension
>>
>> v2:
>> - extended VIDIOC_EXPBUF argument from integer memoffset to struct
>> v4l2_exportbuffer
>> - added patch that breaks DMABUF spec on (un)map_atachment callcacks but
>> allows to work with existing implementation of DMABUF prime in DRM
>> - all dma-contig code refactoring patches were squashed
>> - bugfixes
>>
>> v1: List of changes since [1].
>> - support for DMA api extension dma_get_pages, the function is used to
>> retrieve pages used to create DMA mapping.
>> - small fixes/code cleanup to videobuf2
>> - added prepare and finish callbacks to vb2 allocators, it is used keep
>> consistency between dma-cpu acess to the memory (by Marek Szyprowski)
>> - support for exporting of DMABUF buffer in V4L2 and Videobuf2, originated
>> from [3].
>> - support for dma-buf exporting in vb2-dma-contig allocator
>> - support for DMABUF for s5p-tv and s5p-fimc (capture interface) drivers,
>> originated from [3]
>> - changed handling for userptr buffers (by Marek Szyprowski, Andrzej
>> Pietrasiewicz)
>> - let mmap method to use dma_mmap_writecombine call (by Marek Szyprowski)
>>
>> [1]
>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/4296
>> 6/focus=42968 [2] https://lkml.org/lkml/2011/12/26/29
>> [3]
>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/3635
>> 4/focus=36355 [4]
>> http://thread.gmane.org/gmane.linux.kernel.cross-arch/12819
>>
>> Laurent Pinchart (2):
>> v4l: vb2-dma-contig: Shorten vb2_dma_contig prefix to vb2_dc
>> v4l: vb2-dma-contig: Reorder functions
>>
>> Marek Szyprowski (2):
>> v4l: vb2: add prepare/finish callbacks to allocators
>> v4l: vb2-dma-contig: add prepare/finish to dma-contig allocator
>>
>> Sumit Semwal (4):
>> v4l: Add DMABUF as a memory type
>> v4l: vb2: add support for shared buffer (dma_buf)
>> v4l: vb: remove warnings about MEMORY_DMABUF
>> v4l: vb2-dma-contig: add support for dma_buf importing
>>
>> Tomasz Stanislawski (5):
>> Documentation: media: description of DMABUF importing in V4L2
>> v4l: vb2-dma-contig: Remove unneeded allocation context structure
>> v4l: vb2-dma-contig: add support for scatterlist in userptr mode
>> v4l: s5p-tv: mixer: support for dmabuf importing
>> v4l: s5p-fimc: support for dmabuf importing
>>
>> Documentation/DocBook/media/v4l/compat.xml | 4 +
>> Documentation/DocBook/media/v4l/io.xml | 179 +++
>> .../DocBook/media/v4l/vidioc-create-bufs.xml | 1 +
>> Documentation/DocBook/media/v4l/vidioc-qbuf.xml | 15 +
>> Documentation/DocBook/media/v4l/vidioc-reqbufs.xml | 45 +-
>> drivers/media/video/s5p-fimc/Kconfig | 1 +
>> drivers/media/video/s5p-fimc/fimc-capture.c | 2 +-
>> drivers/media/video/s5p-tv/Kconfig | 1 +
>> drivers/media/video/s5p-tv/mixer_video.c | 2 +-
>> drivers/media/video/v4l2-ioctl.c | 1 +
>> drivers/media/video/videobuf-core.c | 4 +
>> drivers/media/video/videobuf2-core.c | 207 +++-
>> drivers/media/video/videobuf2-dma-contig.c | 520 ++---
>> include/linux/videodev2.h
Re: [RFCv2 PATCH 0/9] Integration of videobuf2 with dmabuf
Hi Tomasz, On Tue, Mar 6, 2012 at 5:08 PM, Tomasz Stanislawski wrote: > Hello everyone, > This patchset is an incremental patch to patchset created by Sumit Semwal [1]. > The patches are dedicated to help find a better solution for support of buffer > sharing by V4L2 API. It is expected to start discussion on the final > installment for dma-buf in vb2-dma-contig allocator. Current version of the > patches contain little documentation. It is going to be fixed after achieving > consensus about design for buffer exporting. Moreover the API between > vb2-core > and the allocator should be revised. I like your approach in general quite a bit. May I request you, though, to maybe split it over into two portions - the preparation patches, and the exporter portion. This would help as the exporter portion is quite dependent on dma_get_pages and dma-mapping patches. (Maybe also indirectly on DRM prime?) With that split, we could try to target the preparation patches for 3.4 while we continue to debate on the exporter patches? I just saw the dma-mapping pull request from Marek, so the dependencies might become available soon. If you agree, then I can post the patch version of my 'v4l2 as dma-buf user' patches, [except the patch that you've included in this series] so we can try to hit 3.4 merge window. > Best regards, ~Sumit. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 PATCH 1/9] v4l: vb2: fixes for DMABUF support
On Tue, Mar 6, 2012 at 5:08 PM, Tomasz Stanislawski
wrote:
> This patch contains fixes to DMABUF support in vb2-core.
> - fixes number of arguments of call_memop macro
> - fixes setup of plane length
> - fixes handling of error pointers
>
> Signed-off-by: Tomasz Stanislawski
> Signed-off-by: Kyungmin Park
Reviewed-by: Sumit Semwal
> ---
> drivers/media/video/videobuf2-core.c | 24 +++-
> include/media/videobuf2-core.h | 6 +++---
> 2 files changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c
> index 951cb56..e7df560 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -118,7 +118,7 @@ static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
> void *mem_priv = vb->planes[plane].mem_priv;
>
> if (mem_priv) {
> - call_memop(q, plane, detach_dmabuf, mem_priv);
> + call_memop(q, detach_dmabuf, mem_priv);
> dma_buf_put(vb->planes[plane].dbuf);
> vb->planes[plane].dbuf = NULL;
> vb->planes[plane].mem_priv = NULL;
> @@ -905,6 +905,8 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, const
> struct v4l2_buffer *b,
> }
> if (b->memory == V4L2_MEMORY_DMABUF) {
> for (plane = 0; plane < vb->num_planes; ++plane) {
> + v4l2_planes[plane].bytesused =
> + b->m.planes[plane].bytesused;
> v4l2_planes[plane].m.fd =
> b->m.planes[plane].m.fd;
> }
> }
> @@ -1052,17 +1054,13 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const
> struct v4l2_buffer *b)
> if (IS_ERR_OR_NULL(dbuf)) {
> dprintk(1, "qbuf: invalid dmabuf fd for "
> "plane %d\n", plane);
> - ret = PTR_ERR(dbuf);
> + ret = -EINVAL;
> goto err;
> }
>
> - /* this doesn't get filled in until __fill_vb2_buffer(),
> - * since it isn't known until after dma_buf_get()..
> - */
> - planes[plane].length = dbuf->size;
> -
> /* Skip the plane if already verified */
> if (dbuf == vb->planes[plane].dbuf) {
> + planes[plane].length = dbuf->size;
> dma_buf_put(dbuf);
> continue;
> }
> @@ -1072,7 +1070,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const
> struct v4l2_buffer *b)
>
> /* Release previously acquired memory if present */
> if (vb->planes[plane].mem_priv) {
> - call_memop(q, plane, detach_dmabuf,
> + call_memop(q, detach_dmabuf,
> vb->planes[plane].mem_priv);
> dma_buf_put(vb->planes[plane].dbuf);
> }
> @@ -1080,8 +1078,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const
> struct v4l2_buffer *b)
> vb->planes[plane].mem_priv = NULL;
>
> /* Acquire each plane's memory */
> - mem_priv = q->mem_ops->attach_dmabuf(
> - q->alloc_ctx[plane], dbuf);
> + mem_priv = call_memop(q, attach_dmabuf, q->alloc_ctx[plane],
> + dbuf, q->plane_sizes[plane], write);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "qbuf: failed acquiring dmabuf "
> "memory for plane %d\n", plane);
> @@ -1089,6 +1087,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const
> struct v4l2_buffer *b)
> goto err;
> }
>
> + planes[plane].length = dbuf->size;
> vb->planes[plane].dbuf = dbuf;
> vb->planes[plane].mem_priv = mem_priv;
> }
> @@ -1098,8 +1097,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const
> struct v4l2_buffer *b)
> * the buffer(s)..
> */
> for (plane = 0; plane < vb->num_planes; ++plane) {
> - ret = q->mem_ops->map_dmabuf(
> - vb->planes[plane].mem_priv, write);
> + ret = call_memop(q, map_dmabuf, vb->planes[plane].mem_priv);
> if (ret) {
> dprintk(1, "qbuf: failed mapping dmabuf "
> "memory for plane %d\n", plane);
> @@ -1527,7 +1525,7 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer
> *b, bool nonblocking)
> */
> if (q->memory == V4L2_MEMORY_DMABUF)
> for (plane = 0; plane < vb->num_planes; ++plane)
> - call_memop(q, plane, unmap_dmabuf,
> +
Re: [PATCH 2/3] dma-buf: add support for kernel cpu access
Hi Daniel, On Tue, Mar 6, 2012 at 12:27 AM, Daniel Vetter wrote: > On Fri, Mar 2, 2012 at 23:24, Rob Clark wrote: >> Perhaps we should check somewhere for required dmabuf ops fxns (like >> kmap_atomic here), rather than just calling unconditionally what might >> be a null ptr. At least put it in the WARN_ON(), but it might be >> nicer to catch a missing required fxns at export time, rather than >> waiting for an importer to try and call it. Less likely that way, for >> newly added required functions go unnoticed. >> >> (same comment applies below for the non-atomic variant.. and possibly >> some other existing dmabuf ops) > > Agreed, I'll rework the patch to do that when rebasing onto Sumit's latest > tree. In addition, you'd not need to check for !dmabuf->ops since the export should already catch it. As I sent in the other mail a while back, could you please rebase on for-next at git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git Best regards, ~Sumit. > -Daniel > -- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Kernel Display and Video API Consolidation mini-summit at ELC 2012 - Notes
Hello Laurent, Everyone: On Fri, Feb 17, 2012 at 4:55 AM, Laurent Pinchart wrote: > Hello everybody, > > First of all, I would like to thank all the attendees for their participation > in the mini-summit that helped make the meeting a success. > > *** dma-buf Implementation in V4L2 *** > > Goal: Implement the dma-buf API in V4L2. > > Sumit Semwal has submitted patches to implement the dma-buf importer role in > videobuf2. Tomasz Stanislawski has then submitted incremental patches to add > exporter role support. > > Action points: > - Create a git branch to host all the latest patches. Sumit will provide > that. > > Against my Action Item: I have created the following branch at my github (obviously, it is an RFC branch only) tree: git://github.com/sumitsemwal/kernel-omap4.git branch: 3.3rc3-v4l2-dmabuf-RFCv1 As the name partially suggests, it is based out of: 3.3-rc3 + dmav6 [1] + some minor dma-buf updates [2] + my v4l2-as-importer RFC [3] + Tomasz' RFC for v4l2-as-exporter (and related patches) [4] Since Tomasz' RFC had a patch-pair which first removed and then added drivers/media/video/videobuf2-dma-contig.c file, I 'combined' these into one - but since the patch-pair heavily refactored the file, I am not able to take responsibility of completeness / correctness of the same. [1]: http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/3.3-rc2-dma-v6 [2]: git://git.linaro.org/people/sumitsemwal/linux-3.x.git 'dev' branch [3]: http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968 [4]: http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/43793 Best regards, ~Sumit. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma-buf: add dma_data_direction to unmap dma_buf_op
Please ignore! I will send out a new version in a minute.
Thanks and best regards,
~Sumit.
On Fri, Jan 27, 2012 at 3:04 PM, Sumit Semwal wrote:
> Some exporters may use DMA map/unmap APIs in dma-buf ops, which require
> enum dma_data_direction while unmapping.
>
> Thus, the unmap dma_buf_op also needs to have enum dma_data_direction as
> a parameter.
>
> Reported-by: Tomasz Stanislawski
> Signed-off-by: Sumit Semwal
> ---
> drivers/base/Kconfig | 2 +-
> drivers/base/dma-buf.c | 7 +--
> include/linux/dma-buf.h | 8 +---
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 7be9f79..5edc5db 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -179,7 +179,7 @@ config GENERIC_CPU_DEVICES
> source "drivers/base/regmap/Kconfig"
>
> config DMA_SHARED_BUFFER
> - bool
> + bool "Temporary mechanism to enable build of dma-buf"
> default n
> select ANON_INODES
> depends on EXPERIMENTAL
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> index 8afe2dd..c9a945f 100644
> --- a/drivers/base/dma-buf.c
> +++ b/drivers/base/dma-buf.c
> @@ -271,16 +271,19 @@ EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
> * dma_buf_ops.
> * @attach: [in] attachment to unmap buffer from
> * @sg_table: [in] scatterlist info of the buffer to unmap
> + * @direction: [in] direction of DMA transfer
> *
> */
> void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> - struct sg_table *sg_table)
> + struct sg_table *sg_table,
> + enum dma_data_direction direction)
> {
> if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
> return;
>
> mutex_lock(&attach->dmabuf->lock);
> - attach->dmabuf->ops->unmap_dma_buf(attach, sg_table);
> + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> + direction);
> mutex_unlock(&attach->dmabuf->lock);
>
> }
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 86f6241..847b026 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -63,7 +63,8 @@ struct dma_buf_ops {
> struct sg_table * (*map_dma_buf)(struct dma_buf_attachment *,
> enum dma_data_direction);
> void (*unmap_dma_buf)(struct dma_buf_attachment *,
> - struct sg_table *);
> + struct sg_table *,
> + enum dma_data_direction);
> /* TODO: Add try_map_dma_buf version, to return immed with -EBUSY
> * if the call would block.
> */
> @@ -122,7 +123,8 @@ void dma_buf_put(struct dma_buf *dmabuf);
>
> struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
> enum dma_data_direction);
> -void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table
> *);
> +void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
> + enum dma_data_direction);
> #else
>
> static inline struct dma_buf_attachment *dma_buf_attach(struct dma_buf
> *dmabuf,
> @@ -166,7 +168,7 @@ static inline struct sg_table *dma_buf_map_attachment(
> }
>
> static inline void dma_buf_unmap_attachment(struct dma_buf_attachment
> *attach,
> - struct sg_table *sg)
> + struct sg_table *sg, enum dma_data_direction write)
> {
> return;
> }
> --
> 1.7.5.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism
On Thu, Jan 26, 2012 at 1:37 AM, Daniel Vetter wrote: > On Wed, Jan 25, 2012 at 06:02:41PM +0100, Tomasz Stanislawski wrote: >> Hi Sumit, >> >> On 12/26/2011 10:23 AM, Sumit Semwal wrote: >> >This is the first step in defining a dma buffer sharing mechanism. >> > >> [snip] >> >> >+struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *, >> >+ enum dma_data_direction); >> >+void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table >> >*); >> >> I think that you should add enum dma_data_direction as an argument >> unmap function. It was mentioned that the dma_buf_attachment should keep >> cached and mapped sg_table for performance reasons. The field >> dma_buf_attachment::priv seams to be a natural place to keep this sg_table. >> To map a buffer the exporter calls dma_map_sg. It needs dma direction >> as an argument. The problem is that dma_unmap_sg also needs this >> argument but dma direction is not available neither in >> dma_buf_unmap_attachment nor in unmap callback. Therefore the exporter >> is forced to embed returned sg_table into a bigger structure where >> dma direction is remembered. Refer to function vb2_dc_dmabuf_ops_map >> at > > Oops, makes sense. I've totally overlooked that we need to pass in the dma > direction also for the unmap call to the dma subsystem. Sumit, can you > stitch together that small patch? Right, of course. I will do that by tomorrow; it is a bank holiday today here in India, so. regards, ~Sumit. > -Daniel > -- > Daniel Vetter > Mail: dan...@ffwll.ch > Mobile: +41 (0)79 365 57 48 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] v4l: vb2: fixes for DMABUF support
Hi Tomasz, On Wed, Jan 25, 2012 at 4:04 PM, Tomasz Stanislawski wrote: > > Hi Sumit, > > On 01/25/2012 06:35 AM, Semwal, Sumit wrote: >> >> Hi Tomasz, >> On Mon, Jan 23, 2012 at 8:55 PM, Tomasz Stanislawski >> wrote: >>> >>> Hi Mauro, >>> >>> >> >>> >>> >>> Ok. I should have given more details about the patch. I am sorry for >>> missing >>> it. My kernel tree failed to compile after applying patches from >>> >>> [1] >>> >>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968 >>> >>> I had to generate this patch to compile the code and test it. Most of the >>> fixes refer to Sumit's code and I think he will take care of those bugs. >> >> Is your kernel tree a mainline kernel? I am pretty sure I posted out >> the RFC after compile testing. > > > Our development kernel often contains patches that are not posted to > opensource. The tree presented in the cover letter contains only patches > that were approved for opensource submission. Right; I understand that - I just wanted to make sure you didn't hit some problem with my patches that I didn't. Thanks for confirming that it was your dev kernel. > > Some of the patches that are not merged into the mainline may break > compilation if patches from the mailing list are applied on the top. The > example is 'media: vb2: remove plane argument from call_memop and cleanup > mempriv usage'. I had to add fixes to compile the code. Moreover I had to > test a working application that makes use of DMABUF exporting/importing via > V4L2 API. So I had to fix other issues that are not only compilation > related. > I understand. > As I remember we agreed that I had to post an incremental patchset. > Therefore all needed fixes had to be present in the tree. > > The fixes were posted in this patchset to keep the whole work together. > I expect that you already prepared a patch fixing majority of issues from > this patch. Many of them were mentioned in Pawel's and Laurent's and > Sakari's reviews. If you find fixes in this patch useful you can merge them > into next version of RFC 'v4l: DMA buffer sharing support as a user'. > > OK - this makes it quite clear; I will re-work my RFC then. >> >>> >> >>> >>> >>> >>> I wanted to post the complete set of patches that produce compilable >>> kernel. >>> Therefore most important bugs/issues had to be fixed and attached to the >>> patchset. Some of the issues in [1] were mentioned by Laurent and Sakari. >>> I >>> hope Sumit will take care of those problems. >> >> I must've misunderstood when you said 'I would like to take care of >> these patches'. Please let me know if you'd like me to submit next >> version of my RFC separately with fixes for these issues, or would you >> manage that as part of your RFC patch series submission. > > > This patchset is an RFC. It was my big mistake that I forgot to add this to > the title of the patchset. I am not going to post the patch with fixes to > your part any more. It would be great if you merged it into new version of > 'DMA buffer sharing support as a user'. > That's OK with me. > IMO, some parts should go as separate threads: > - extension to DMA subsystem, introduction of dma_get_pages. This would > probably go to DMA mailing list. > - redesign of dma-contig allocator (w/o dmabuf exporting/importing) > - buffer importing via dmabuf in V4L2 and vb2-dma-contig > - buffer exporting via dmabuf in V4L2 and vb2-dma-contig > > BTW. Could you state your opinion on presented solution for dma-buf > exporting in vb2-core and vb2-dma-contig allocator? > I agree with your ordering of these parts; Also, with this ordering, I guess I should pay more attention to parts 1. (extension to DMA...) and 2. (redesign of dma-contig allocator...) - which I hope you are going to do? I can then base out my next version of RFC on these. I was OoO for past couple of days, hence missed all the action :) - I will try and go through your approach, and comment as soon as I can. Hopefully in a couple of days. > Regards, > Tomasz Stanislawski > > Best regards, ~Sumit. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism
On Fri, Jan 20, 2012 at 6:53 PM, Laurent Pinchart
wrote:
> Hi Summit,
>
> Sorry for the late review. I know that this code is now in mainline, but I
> still have a couple of comments. I'll send patches if you agree with them.
Hi Laurent,
Thanks for your review; apologies for being late in replying - I was
OoO for last couple of days.
>
> On Monday 26 December 2011 10:23:15 Sumit Semwal wrote:
>>
>
> [snip]
>
>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> new file mode 100644
>> index 000..e38ad24
>> --- /dev/null
>> +++ b/drivers/base/dma-buf.c
>> @@ -0,0 +1,291 @@
>
> [snip]
>
>> +/**
>> + * dma_buf_export - Creates a new dma_buf, and associates an anon file
>> + * with this buffer, so it can be exported.
>> + * Also connect the allocator specific data and ops to the buffer.
>> + *
>> + * @priv: [in] Attach private data of allocator to this buffer
>> + * @ops: [in] Attach allocator-defined dma buf ops to the new buffer.
>> + * @size: [in] Size of the buffer
>> + * @flags: [in] mode flags for the file.
>> + *
>> + * Returns, on success, a newly created dma_buf object, which wraps the
>> + * supplied private data and operations for dma_buf_ops. On either missing
>> + * ops, or error in allocating struct dma_buf, will return negative error.
>> + *
>> + */
>> +struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,
>> + size_t size, int flags)
>> +{
>> + struct dma_buf *dmabuf;
>> + struct file *file;
>> +
>> + if (WARN_ON(!priv || !ops
>> + || !ops->map_dma_buf
>> + || !ops->unmap_dma_buf
>> + || !ops->release)) {
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
>> + if (dmabuf == NULL)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + dmabuf->priv = priv;
>> + dmabuf->ops = ops;
>
> dmabuf->ops will never but NULL, but (see below)
>
>> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> + struct device *dev)
>> +{
>> + struct dma_buf_attachment *attach;
>> + int ret;
>> +
>> + if (WARN_ON(!dmabuf || !dev || !dmabuf->ops))
>
> you still check dmabuf->ops here, as well as in several places below.
> Shouldn't these checks be removed ?
You're right - these can be removed.
>
>> + return ERR_PTR(-EINVAL);
>> +
>> + attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
>> + if (attach == NULL)
>> + goto err_alloc;
>
> What about returning ERR_PTR(-ENOMEM) directly here ?
>
Right; we can do that.
>> +
>> + mutex_lock(&dmabuf->lock);
>> +
>> + attach->dev = dev;
>> + attach->dmabuf = dmabuf;
>
> These two lines can be moved before mutex_lock().
>
:) Yes - thanks for catching this.
> --
> Regards,
>
> Laurent Pinchart
Let me know if you'd send patches for these, or should I just go ahead
and correct.
Best regards,
~Sumit.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] v4l: vb2: fixes for DMABUF support
Hi Tomasz, On Mon, Jan 23, 2012 at 8:55 PM, Tomasz Stanislawski wrote: > Hi Mauro, > > > > Ok. I should have given more details about the patch. I am sorry for missing > it. My kernel tree failed to compile after applying patches from > > [1] > http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968 > > I had to generate this patch to compile the code and test it. Most of the > fixes refer to Sumit's code and I think he will take care of those bugs. Is your kernel tree a mainline kernel? I am pretty sure I posted out the RFC after compile testing. > > > > I wanted to post the complete set of patches that produce compilable kernel. > Therefore most important bugs/issues had to be fixed and attached to the > patchset. Some of the issues in [1] were mentioned by Laurent and Sakari. I > hope Sumit will take care of those problems. I must've misunderstood when you said 'I would like to take care of these patches'. Please let me know if you'd like me to submit next version of my RFC separately with fixes for these issues, or would you manage that as part of your RFC patch series submission. > >> >> Failing to do that will mean that important fixes for upstream >> will be missed. > > > Ok. It will be fixed. > >> >> Regards, >> Mauro >> > > Regards, > Tomasz Stanislawski > Best regards, ~Sumit. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL]: dma-buf tree: maintainer update
Hi Linus, Post the merge of dma-buf tree that was (very kindly) sent by Dave Airlie, various people involved in this project feel it is natural and practical for me to be the maintainer of this code. This is my first pull request to you, which only changes the MAINTAINERS file - could you please pull from it? [If you'd just prefer the patch, I could post that out as well.] Thanks and best regards, ~Sumit. The following changes since commit dcd6c92267155e70a94b3927bce681ce74b80d1f: Linux 3.3-rc1 (2012-01-19 15:04:48 -0800) are available in the git repository at: git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git for-linus-3.3 Sumit Semwal (1): MAINTAINERS: Add dma-buf sharing framework maintainer MAINTAINERS | 11 +++ 1 files changed, 11 insertions(+), 0 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [RFCv1 2/4] v4l:vb2: add support for shared buffer (dma_buf)
On Mon, Jan 16, 2012 at 11:03 AM, Semwal, Sumit wrote:
>
> On Sun, Jan 15, 2012 at 2:08 AM, Sakari Ailus wrote:
>>
>> Hi Sumit,
>>
>> Thanks for the patch!
>
Hi Sakari,
Thanks for reviewing this :)
>>
>>
>>
>> Shouldn't the buffer mapping only be done at the first call to
>> __qbuf_dmabuf()? On latter calls, the cache would need to be handled
>> according to presence of V4L2_BUF_FLAG_NO_CACHE_CLEAN /
>> V4L2_BUF_FLAG_NO_CACHE_INVALIDATE in v4l2_buffer.
>
Well, the 'map / unmap' implementation is by design exporter-specific;
so the exporter of the buffer may choose to, depending on the use
case, 'map-and-keep' on first call to map_dmabuf, and do actual unmap
only at 'release' time. This will mean that the {map,unmap}_dmabuf
calls will be used mostly for 'access-bracketing' between multiple
users, and not for actual map/unmap each time.
Again, the framework is flexible enough to allow exporters to actually
map/unmap as required (think cases where backing-storage migration
might be needed while buffers are in use - in that case, when all
current users have called unmap_XXX() on a buffer, it can be migrated,
and the next map_XXX() calls could give different mappings back to the
users).
> The kernel 'users' of dma-buf [in case of this RFC, v4l2] should not ideally
> need to worry about the actual mapping/unmapping that is done; the buffer
> exporter in a particular use-case should be able to handle it.
>
>>
>> Same here, except reverse: this only should be done when the buffer is
>> destroyed --- either when the user explicitly calls reqbufs(0) or when
>> the file handle owning this buffer is being closed.
>>
>> Mapping buffers at every prepare_buf and unmapping them in dqbuf is
>> prohibitively expensive. Same goes for many other APIs than V4L2, I think.
>>
>> I wonder if the means to do this exists already.
>>
>> I have to admit I haven't followed the dma_buf discussion closely so I
>> might be missing something relevant here.
>
Hope the above explanation helps. Please do not hesitate to contact if
you need more details.
>>
>>
>> Kind regards,
>>
>> --
>> Sakari Ailus
>> sakari.ai...@iki.fi
Best regards,
~Sumit.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
On Tue, Jan 10, 2012 at 7:44 AM, Rob Clark wrote: > On Mon, Jan 9, 2012 at 7:34 PM, InKi Dae wrote: >> 2012/1/10 Rob Clark : >> at least with no IOMMU, the memory information(containing physical >> memory address) would be copied to vb2_xx_buf object if drm gem >> exported its own buffer and vb2 wants to use that buffer at this time, >> sg table is used to share that buffer. and the problem I pointed out >> is that this buffer(also physical memory region) could be released by >> vb2 framework(as you know, vb2_xx_buf object and the memory region for >> buf->dma_addr pointing) but the Exporter(drm gem) couldn't know that >> so some problems would be induced once drm gem tries to release or >> access that buffer. and I have tried to resolve this issue adding >> get_shared_cnt() callback to dma-buf.h but I'm not sure that this is >> good way. maybe there would be better way. Hi Inki, As also mentioned in the documentation patch, importer (the user of the buffer) - in this case for current RFC patches on v4l2-as-a-user[1] vb2 framework - shouldn't release the backing memory of the buffer directly - it should only use the dma-buf callbacks in the right sequence to let the exporter know that it is done using this buffer, so the exporter can release it if allowed and needed. > > the exporter (in this case your driver's drm/gem bits) shouldn't > release that mapping / sgtable until the importer (in this case v4l2) > calls dma_buf_unmap fxn.. > > It would be an error if the importer did a dma_buf_put() without first > calling dma_buf_unmap_attachment() (if currently mapped) and then > dma_buf_detach() (if currently attached). Perhaps somewhere there > should be some sanity checking debug code which could be enabled to do > a WARN_ON() if the importer does the wrong thing. It shouldn't really > be part of the API, I don't think, but it actually does seem like a > good thing, esp. as new drivers start trying to use dmabuf, to have > some debug options which could be enabled. > > It is entirely possible that something was missed on the vb2 patches, > but the way it is intended to work is like this: > https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3a66a96185f/drivers/media/video/videobuf2-core.c#L92 > > where it does a detach() before the dma_buf_put(), and the vb2-contig > backend checks here that it is also unmapped(): > https://github.com/robclark/kernel-omap4/blob/0961428143cd10269223e3d0f24bc3a66a96185f/drivers/media/video/videobuf2-dma-contig.c#L251 The proposed RFC for V4L2 adaptation at [1] does exactly the same thing; detach() before dma_buf_put(), and check in vb2-contig backend for unmapped() as mentioned above. > > BR, > -R > BR, Sumit. [1]: V4l2 as a dma-buf user RFC: http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism
On Fri, Dec 23, 2011 at 10:50 PM, Rob Clark wrote: > On Fri, Dec 23, 2011 at 4:08 AM, Semwal, Sumit wrote: >> On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie wrote: >> >>>> >>>> Hence for both patches: >>>> Reviewed-by: Daniel Vetter >>> >>> Yeah I'm with Daniel, I like this one, I can definitely build the drm >>> buffer sharing layer on top of this. >>> >>> How do we see this getting merged? I'm quite happy to push it to Linus >>> if we don't have an identified path, though it could go via a Linaro >>> tree as well. >>> >>> so feel free to add: >>> Reviewed-by: Dave Airlie >> Thanks Daniel and Dave! >> >> I guess we can start with staging for 3.3, and see how it shapes up. I >> will post the latest patch version pretty soon. > > not sure about staging, but could make sense to mark as experimental. Thanks, I will mark it experimental for the first version; we can remove that once it is more widely used and tested. > >> Arnd, Dave: do you have any preference on the path it takes to get >> merged? In my mind, Linaro tree might make more sense, but I would >> leave it upto you gentlemen. > > Looks like Dave is making some progress on drm usage of buffer sharing > between gpu's.. if that is ready to go in at the same time, it might > be a bit logistically simpler for him to put dmabuf in the same pull > req. I don't have strong preference one way or another, so do what is > collectively simpler ;-) :) Right - I am quite happy for it to get merged in either ways :) > > BR, > -R Best regards, ~Sumit. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism
On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie wrote: >> >> I think this is a really good v1 version of dma_buf. It contains all the >> required bits (with well-specified semantics in the doc patch) to >> implement some basic use-cases and start fleshing out the integration with >> various subsystem (like drm and v4l). All the things still under >> discussion like >> - userspace mmap support >> - more advanced (and more strictly specified) coherency models >> - and shared infrastructure for implementing exporters >> are imo much clearer once we have a few example drivers at hand and a >> better understanding of some of the insane corner cases we need to be able >> to handle. >> >> And I think any risk that the resulting clarifications will break a basic >> use-case is really minimal, so I think it'd be great if this could go into >> 3.3 (maybe as some kind of staging/experimental infrastructure). >> >> Hence for both patches: >> Reviewed-by: Daniel Vetter > > Yeah I'm with Daniel, I like this one, I can definitely build the drm > buffer sharing layer on top of this. > > How do we see this getting merged? I'm quite happy to push it to Linus > if we don't have an identified path, though it could go via a Linaro > tree as well. > > so feel free to add: > Reviewed-by: Dave Airlie Thanks Daniel and Dave! I guess we can start with staging for 3.3, and see how it shapes up. I will post the latest patch version pretty soon. Arnd, Dave: do you have any preference on the path it takes to get merged? In my mind, Linaro tree might make more sense, but I would leave it upto you gentlemen. > > Dave. Best regards, ~Sumit. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
On Wed, Dec 21, 2011 at 10:57 PM, Arnd Bergmann wrote: > On Tuesday 20 December 2011, Daniel Vetter wrote: >> > I'm thinking for a first version, we can get enough mileage out of it by >> > saying: >> > 1) only exporter can mmap to userspace >> > 2) only importers that do not need CPU access to buffer.. Thanks Rob - and the exporter can do the mmap outside of dma-buf usage, right? I mean, we don't need to provide an mmap to dma_buf() and restrict it to exporter, when the exporter has more 'control' of the buffer anyways. > BR, ~Sumit. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 1/2] dma-buf: Introduce dma buffer sharing mechanism
Hi Konrad,
On Tue, Dec 20, 2011 at 10:06 PM, Konrad Rzeszutek Wilk
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.
>
>> - 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.
>
>>
>> +/*
>> + * 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?
>
>> +/**
>> + * 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.
>
>> + */
>> +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.
>
>> +
>> +/**
>> + * 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]
>
>> + * 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
Re: [Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism
On Wed, Dec 21, 2011 at 3:56 AM, Rob Clark wrote: > On Tue, Dec 20, 2011 at 2:20 PM, Dave Airlie wrote: >>> >>> I think this is a really good v1 version of dma_buf. It contains all the >>> required bits (with well-specified semantics in the doc patch) to >>> implement some basic use-cases and start fleshing out the integration with >>> various subsystem (like drm and v4l). All the things still under >>> discussion like >>> - userspace mmap support >>> - more advanced (and more strictly specified) coherency models >>> - and shared infrastructure for implementing exporters >>> are imo much clearer once we have a few example drivers at hand and a >>> better understanding of some of the insane corner cases we need to be able >>> to handle. >>> >>> And I think any risk that the resulting clarifications will break a basic >>> use-case is really minimal, so I think it'd be great if this could go into >>> 3.3 (maybe as some kind of staging/experimental infrastructure). >>> >>> Hence for both patches: >>> Reviewed-by: Daniel Vetter >> >> Yeah I'm with Daniel, I like this one, I can definitely build the drm >> buffer sharing layer on top of this. >> >> How do we see this getting merged? I'm quite happy to push it to Linus >> if we don't have an identified path, though it could go via a Linaro >> tree as well. >> >> so feel free to add: >> Reviewed-by: Dave Airlie > > fwiw, patches to share buffers between drm and v4l2 are here: > > https://github.com/robclark/kernel-omap4/commits/drmplane-dmabuf > > (need a bit of cleanup before the vb2 patches are submitted.. but that > is unrelated to the dmabuf patches) > > so, > > Reviewed-and-Tested-by: Rob Clark Thanks Daniel, Dave, and Rob! BR, Sumit. > >> Dave. >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-media" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Linaro-mm-sig] [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
Hi Arnd, Daniel, On Mon, Dec 12, 2011 at 10:18 PM, Arnd Bergmann wrote: > On Saturday 10 December 2011, Daniel Vetter wrote: >> If userspace (through some driver calls) >> tries to do stupid things, it'll just get garbage. See >> Message-ID: >> >> for my reasons why it think this is the right way to go forward. So in >> essence I'm really interested in the reasons why you want the kernel >> to enforce this (or I'm completely missing what's the contentious >> issue here). > > This has nothing to do with user space mappings. Whatever user space does, > you get garbage if you don't invalidate cache lines that were introduced > through speculative prefetching before you access cache lines that were > DMA'd from a device. I didn't see a consensus on whether dma_buf should enforce some form of serialization within the API - so atleast for v1 of dma-buf, I propose to 'not' impose a restriction, and we can tackle it (add new ops or enforce as design?) whenever we see the first need of it - will that be ok? [I am bending towards the thought that it is a problem to solve at a bigger platform than dma_buf.] > > Arnd Best regards, ~Sumit. > > -- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
Hi Daniel, Rob,
On Tue, Dec 6, 2011 at 3:41 AM, Rob Clark wrote:
> On Mon, Dec 5, 2011 at 3:23 PM, Daniel Vetter wrote:
>> On Mon, Dec 05, 2011 at 02:46:47PM -0600, Rob Clark wrote:
>>> On Mon, Dec 5, 2011 at 11:18 AM, Arnd Bergmann wrote:
>>> > In the patch 2, you have a section about migration that mentions that
>>> > it is possible to export a buffer that can be migrated after it
>>> > is already mapped into one user driver. How does that work when
>>> > the physical addresses are mapped into a consumer device already?
>>>
>>> I think you can do physical migration if you are attached, but
>>> probably not if you are mapped.
>>
>> Yeah, that's very much how I see this, and also why map/unmap (at least
>> for simple users like v4l) should only bracket actual usage. GPU memory
>> managers need to be able to move around buffers while no one is using
>> them.
>>
>> [snip]
>>
>>> >> + /* allow allocator to take care of cache ops */
>>> >> + void (*sync_sg_for_cpu) (struct dma_buf *, struct device *);
>>> >> + void (*sync_sg_for_device)(struct dma_buf *, struct device *);
>>> >
>>> > I don't see how this works with multiple consumers: For the streaming
>>> > DMA mapping, there must be exactly one owner, either the device or
>>> > the CPU. Obviously, this rule needs to be extended when you get to
>>> > multiple devices and multiple device drivers, plus possibly user
>>> > mappings. Simply assigning the buffer to "the device" from one
>>> > driver does not block other drivers from touching the buffer, and
>>> > assigning it to "the cpu" does not stop other hardware that the
>>> > code calling sync_sg_for_cpu is not aware of.
>>> >
>>> > The only way to solve this that I can think of right now is to
>>> > mandate that the mappings are all coherent (i.e. noncachable
>>> > on noncoherent architectures like ARM). If you do that, you no
>>> > longer need the sync_sg_for_* calls.
>>>
>>> My original thinking was that you either need DMABUF_CPU_{PREP,FINI}
>>> ioctls and corresponding dmabuf ops, which userspace is required to
>>> call before / after CPU access. Or just remove mmap() and do the
>>> mmap() via allocating device and use that device's equivalent
>>> DRM_XYZ_GEM_CPU_{PREP,FINI} or DRM_XYZ_GEM_SET_DOMAIN ioctls. That
>>> would give you a way to (a) synchronize with gpu/asynchronous
>>> pipeline, (b) synchronize w/ multiple hw devices vs cpu accessing
>>> buffer (ie. wait all devices have dma_buf_unmap_attachment'd). And
>>> that gives you a convenient place to do cache operations on
>>> noncoherent architecture.
>>>
>>> I sort of preferred having the DMABUF shim because that lets you pass
>>> a buffer around userspace without the receiving code knowing about a
>>> device specific API. But the problem I eventually came around to: if
>>> your GL stack (or some other userspace component) is batching up
>>> commands before submission to kernel, the buffers you need to wait for
>>> completion might not even be submitted yet. So from kernel
>>> perspective they are "ready" for cpu access. Even though in fact they
>>> are not in a consistent state from rendering perspective. I don't
>>> really know a sane way to deal with that. Maybe the approach instead
>>> should be a userspace level API (in libkms/libdrm?) to provide
>>> abstraction for userspace access to buffers rather than dealing with
>>> this at the kernel level.
>>
>> Well, there's a reason GL has an explicit flush and extensions for sync
>> objects. It's to support such scenarios where the driver batches up gpu
>> commands before actually submitting them.
>
> Hmm.. what about other non-GL APIs.. maybe vaapi/vdpau or similar?
> (Or something that I haven't thought of.)
>
>> Also, recent gpus have all (or
>> shortly will grow) multiple execution pipelines, so it's also important
>> that you sync up with the right command stream. Syncing up with all of
>> them is generally frowned upon for obvious reasons ;-)
>
> Well, I guess I am happy enough with something that is at least
> functional. Usespace access would (I think) mainly be weird edge case
> type stuff. But...
>
>
>> On the topic of a coherency model for dmabuf, I think we need to look at
>> dma_buf_attachment_map/unmap (and also the mmap variants cpu_start and
>> cpu_finish or whatever they might get called) as barriers:
>>
>> So after a dma_buf_map, all previsously completed dma operations (i.e.
>> unmap already called) and any cpu writes (i.e. cpu_finish called) will be
>> coherent. Similar rule holds for cpu access through the userspace mmap,
>> only writes completed before the cpu_start will show up.
>>
>> Similar, writes done by the device are only guaranteed to show up after
>> the _unmap. Dito for cpu writes and cpu_finish.
>>
>> In short we always need two function calls to denote the start/end of the
>> "critical section".
>
> Yup, this was exactly my assumption. But I guess it is better to spell it
> out.
Thanks for the excellent discussion - it indeed is ve
Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
On Wed, Dec 7, 2011 at 3:41 PM, Arnd Bergmann wrote: > On Wednesday 07 December 2011, Semwal, Sumit wrote: >> > >> > Do you have a use case for making the interface compile-time disabled? >> > I had assumed that any code using it would make no sense if it's not >> > available so you don't actually need this. >> >> Ok. Though if we keep the interface compile-time disabled, the users >> can actually check and fail or fall-back gracefully when the API is >> not available; If I remove it, anyways the users would need to do the >> same compile time check whether API is available or not, right? > > If you have to do a compile-time check for the config symbol, it's better > to do it the way you did here than in the caller. > > My guess was that no caller would actually require this, because when you > write a part of a subsystem to interact with the dma-buf infrastructure, > you would always disable compilation of an extire file that deals with > everything related to struct dma_buf, not just stub out the calls. Right; that would be ideal, but we may not be able to ask each user to do so - especially when the sharing part might be interspersed in existing buffer handling code. So for now, I would like to keep it as it-is. > > Arnd > BR, ~Sumit. > -- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
Hi Arnd,
Thanks for your review!
On Mon, Dec 5, 2011 at 10:48 PM, Arnd Bergmann wrote:
> On Friday 02 December 2011, Sumit Semwal wrote:
>> This is the first step in defining a dma buffer sharing mechanism.
>
> This looks very nice, but there are a few things I don't understand yet
> and a bunch of trivial comments I have about things I spotted.
>
> Do you have prototype exporter and consumer drivers that you can post
> for clarification?
>
> In the patch 2, you have a section about migration that mentions that
> it is possible to export a buffer that can be migrated after it
> is already mapped into one user driver. How does that work when
> the physical addresses are mapped into a consumer device already?
I guess I need to clear it up in the documentation - when I said "once
all ongoing access is completed" - I meant to say "once all current
users have finished accessing and have unmapped this buffer". So I
agree with Rob - the migration would only be possible for "attached
but unmapped" buffers. I will update the documentation.
>
>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> index 21cf46f..07d8095 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -174,4 +174,14 @@ config SYS_HYPERVISOR
>>
>> source "drivers/base/regmap/Kconfig"
>>
>> +config DMA_SHARED_BUFFER
>> + bool "Buffer framework to be shared between drivers"
>> + default n
>> + depends on ANON_INODES
>
> I would make this 'select ANON_INODES', like the other users of this
> feature.
Sure.
>
>> + return dmabuf;
>> +}
>> +EXPORT_SYMBOL(dma_buf_export);
>
> I agree with Konrad, this should definitely be EXPORT_SYMBOL_GPL,
> because it's really a low-level function that I would expect
> to get used by in-kernel subsystems providing the feature to
> users and having back-end drivers, but it's not the kind of thing
> we want out-of-tree drivers to mess with.
Agreed.
>
>> +/**
>> + * dma_buf_fd - returns a file descriptor for the given dma_buf
>> + * @dmabuf: [in] pointer to dma_buf for which fd is required.
>> + *
>> + * On success, returns an associated 'fd'. Else, returns error.
>> + */
>> +int dma_buf_fd(struct dma_buf *dmabuf)
>> +{
>> + int error, fd;
>> +
>> + if (!dmabuf->file)
>> + return -EINVAL;
>> +
>> + error = get_unused_fd_flags(0);
>
> Why not simply get_unused_fd()?
:) oversight. Will correct.
>
>> +/**
>> + * dma_buf_attach - Add the device to dma_buf's attachments list;
>> optionally,
>> + * calls attach() of dma_buf_ops to allow device-specific attach
>> functionality
>> + * @dmabuf: [in] buffer to attach device to.
>> + * @dev: [in] device to be attached.
>> + *
>> + * Returns struct dma_buf_attachment * for this attachment; may return NULL.
>> + *
>
> Or may return a negative error code. It's better to be consistent here:
> either always return NULL on error, or change the allocation error to
> ERR_PTR(-ENOMEM).
Ok, that makes sense.
>
>> + */
>> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> + struct device *dev)
>> +{
>> + struct dma_buf_attachment *attach;
>> + int ret;
>> +
>> + BUG_ON(!dmabuf || !dev);
>> +
>> + attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
>> + if (attach == NULL)
>> + goto err_alloc;
>> +
>> + mutex_lock(&dmabuf->lock);
>> +
>> + attach->dev = dev;
>> + attach->dmabuf = dmabuf;
>> + if (dmabuf->ops->attach) {
>> + ret = dmabuf->ops->attach(dmabuf, dev, attach);
>> + if (!ret)
>> + goto err_attach;
>
> You probably mean "if (ret)" here instead of "if (!ret)", right?
yes - a stupid one! will correct.
>
>> + /* allow allocator to take care of cache ops */
>> + void (*sync_sg_for_cpu) (struct dma_buf *, struct device *);
>> + void (*sync_sg_for_device)(struct dma_buf *, struct device *);
>
> I don't see how this works with multiple consumers: For the streaming
> DMA mapping, there must be exactly one owner, either the device or
> the CPU. Obviously, this rule needs to be extended when you get to
> multiple devices and multiple device drivers, plus possibly user
> mappings. Simply assigning the buffer to "the device" from one
> driver does not block other drivers from touching the buffer, and
> assigning it to "the cpu" does not stop other hardware that the
> code calling sync_sg_for_cpu is not aware of.
>
> The only way to solve this that I can think of right now is to
> mandate that the mappings are all coherent (i.e. noncachable
> on noncoherent architectures like ARM). If you do that, you no
> longer need the sync_sg_for_* calls.
I will take yours and Daniel's suggestion, and remove these; if at all
they're needed, we can add them back again later, with
/s/device/attachment as suggested by Daniel.
>
>> +#ifdef CONFIG_DMA_SHARED_BUFFER
>
> Do you have a use case for making the interface compile-time disabled?
> I had as
Re: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
Hi Konrad,
On Fri, Dec 2, 2011 at 10:41 PM, Konrad Rzeszutek Wilk
wrote:
> On Fri, Dec 02, 2011 at 02:27:31PM +0530, Sumit Semwal wrote:
>> This is the first step in defining a dma buffer sharing mechanism.
>>
>>
>> [1]: https://wiki.linaro.org/OfficeofCTO/MemoryManagement
>> [2]: http://lwn.net/Articles/454389
>>
>> Signed-off-by: Sumit Semwal
>> Signed-off-by: Sumit Semwal
>
> You have a clone? You only need one SOB.
:) Thanks for your review - Well, not a clone, but I have two 'employers' :))
I have a rather weird reason for this - I am employed with Texas
Instruments, but working with Linaro as well. And due to some
'non-technical' reasons, I need to send this work from @ti.com mail
ID. At the same time, I would like to acknowledge that this work was
done as part of the Linaro umbrella, so I put another SOB @linaro.org.
>
>
>> + * Copyright(C) 2011 Linaro Limited. All rights reserved.
>> + * Author: Sumit Semwal
>
> OK, so the SOB should be from @ti.com then.
>
>> + *
>> +static int dma_buf_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> + struct dma_buf *dmabuf;
>> +
>> + if (!is_dma_buf_file(file))
>> + return -EINVAL;
>> +
>> + dmabuf = file->private_data;
>> +
>
> Should you check if dmabuf is NULL and or dmabuf->ops is NULL too?
>
> Hm, you probably don't need to check for dmabuf, but from
> looking at dma_buf_export one could pass a NULL for the ops.
see next comment
>
>> + if (!dmabuf->ops->mmap)
>> + return -EINVAL;
>> +
>> + return dmabuf->ops->mmap(dmabuf, vma);
>> +}
>> +
>> +static int dma_buf_release(struct inode *inode, struct file *file)
>> +{
>> + struct dma_buf *dmabuf;
>> +
>> + if (!is_dma_buf_file(file))
>> + return -EINVAL;
>> +
>> + dmabuf = file->private_data;
>> +
>
> No checking here for ops or ops->release?
Hmmm.. you're right, of course. for this common check in mmap and
release, I guess I'd add it to 'is_dma_buf_file()' helper [maybe call
it is_valid_dma_buf_file() or something similar]
>
>> +
>> +/**
>
> I don't think the ** is anymore the current kernel doc format.
thanks for catching this :) - will correct.
>
>> + * dma_buf_export - Creates a new dma_buf, and associates an anon file
>> + * with this buffer,so it can be exported.
>
> Put a space there.
ok
>
>> + * Also connect the allocator specific data and ops to the buffer.
>> + *
>> + * @priv: [in] Attach private data of allocator to this buffer
>> + * @ops: [in] Attach allocator-defined dma buf ops to the new buffer.
>> + * @flags: [in] mode flags for the file.
>> + *
>> + * Returns, on success, a newly created dma_buf object, which wraps the
>> + * supplied private data and operations for dma_buf_ops. On failure to
>> + * allocate the dma_buf object, it can return NULL.
>
> "it can" I think the right word is "it will".
Right.
>
>> + *
>> + */
>> +struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,
>> + int flags)
>> +{
>> + struct dma_buf *dmabuf;
>> + struct file *file;
>> +
>> + BUG_ON(!priv || !ops);
>
> Whoa. Crash the whole kernel b/c of this? No no. You should
> use WARN_ON and just return NULL.
ok
>
>> +
>> + dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
>> + if (dmabuf == NULL)
>> + return dmabuf;
>
> Hmm, why not return ERR_PTR(-ENOMEM); ?
ok
>
>> +
>> + dmabuf->priv = priv;
>> + dmabuf->ops = ops;
>> +
>> + file = anon_inode_getfile("dmabuf", &dma_buf_fops, dmabuf, flags);
>> +
>> + dmabuf->file = file;
>> +
>> + mutex_init(&dmabuf->lock);
>> + INIT_LIST_HEAD(&dmabuf->attachments);
>> +
>> + return dmabuf;
>> +}
>> +EXPORT_SYMBOL(dma_buf_export);
>
> _GPL ?
sure; will change it.
>
>> +
>> +
>> +/**
>> + * dma_buf_fd - returns a file descriptor for the given dma_buf
>> + * @dmabuf: [in] pointer to dma_buf for which fd is required.
>> + *
>> + * On success, returns an associated 'fd'. Else, returns error.
>> + */
>> +int dma_buf_fd(struct dma_buf *dmabuf)
>> +{
>> + int error, fd;
>> +
>
> Should you check if dmabuf is NULL first?
yes.
>
>> + if (!dmabuf->file)
>> + return -EINVAL;
>> +
>> + error = get_unused_fd_flags(0);
>> + if (error < 0)
>> + return error;
>> + fd = error;
>> +
>> + fd_install(fd, dmabuf->file);
>> +
>> + return fd;
>> +}
>> +EXPORT_SYMBOL(dma_buf_fd);
>
> GPL?
sure; will change it.
>> +
>> +/**
>> + * dma_buf_get - returns the dma_buf structure related to an fd
>> + * @fd: [in] fd associated with the dma_buf to be returned
>> + *
>> + * On success, returns the dma_buf structure associated with an fd; uses
>> + * file's refcounting done by fget to increase refcount. returns ERR_PTR
>> + * otherwise.
>> + */
>> +struct dma_buf *dma_buf_get(int fd)
>> +{
>> + struct file *file;
>> +
>> + file = fget(fd);
>> +
>> + if (!file)
>> + return ERR_PTR(-EBADF);
>> +
>> + if (!is_dma_buf_
Re: [Linaro-mm-sig] [RFC 1/2] dma-buf: Introduce dma buffer sharing mechanism
Hi Dave, Daniel, Rob, > > On Sun, Nov 27, 2011 at 12:29 PM, Rob Clark wrote: >> >> On Sat, Nov 26, 2011 at 8:00 AM, Daniel Vetter wrote: >> > On Fri, Nov 25, 2011 at 17:28, Dave Airlie wrote: >> >> I've rebuilt my PRIME interface on top of dmabuf to see how it would >> >> work, >> >> >> >> I've got primed gears running again on top, but I expect all my object >> >> lifetime and memory ownership rules need fixing up (i.e. leaks like a >> >> sieve). >> >> >> >> http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-prime-dmabuf >> >> >> >> has the i915/nouveau patches for the kernel to produce the prime >> >> interface. >> > >> > I've noticed that your implementations for get_scatterlist (at least >> > for the i915 driver) doesn't return the sg table mapped into the >> > device address space. I've checked and the documentation makes it >> > clear that this should be the case (and we really need this to support >> > certain insane hw), but the get/put_scatterlist names are a bit >> > misleading. Proposal: >> > >> > - use struct sg_table instead of scatterlist like you've already done >> > in you branch. Simply more consistent with the dma api. >> >> yup >> >> > - rename get/put_scatterlist into map/unmap for consistency with all >> > the map/unmap dma api functions. The attachement would then serve as >> > the abstract cookie to the backing storage, similar to how struct page >> > * works as an abstract cookie for dma_map/unmap_page. The only special >> > thing is that struct device * parameter because that's already part of >> > the attachment. >> >> yup >> >> > - add new wrapper functions dma_buf_map_attachment and >> > dma_buf_unmap_attachement to hide all the pointer/vtable-chasing that >> > we currently expose to users of this interface. >> >> I thought that was one of the earlier comments on the initial dmabuf >> patch, but either way: yup > Thanks for your comments; I will incorporate all of these in the next version I'll send out. >> >> >> BR, >> -R > BR, Sumit. >> >> >> > Comments? >> > >> > Cheers, Daniel >> > -- >> > Daniel Vetter >> > daniel.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-media" >> > in >> > the body of a message to majord...@vger.kernel.org >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/2] dma-buf: Documentation for buffer sharing framework
Hi Randy,
On Thu, Oct 13, 2011 at 4:00 AM, Randy Dunlap wrote:
> On 10/11/2011 02:23 AM, Sumit Semwal wrote:
>> Add documentation for dma buffer sharing framework, explaining the
>> various operations, members and API of the dma buffer sharing
>> framework.
>>
>> Signed-off-by: Sumit Semwal
>> Signed-off-by: Sumit Semwal
>> ---
>> Documentation/dma-buf-sharing.txt | 210
>> +
>> + if the new buffer-user has stricter 'backing-storage constraints', and
>> the
>> + exporter can handle these constraints, the exporter can just stall on
>> the
>> + get_scatterlist till all outstanding access is completed (as signalled
>> by
>
> until
>
Thanks for your review; I will update all these in the next version.
>> + put_scatterlist).
>> + Once all ongoing access is completed, the exporter could potentially
>> move
>> + the buffer to the stricter backing-storage, and then allow further
>> + {get,put}_scatterlist operations from any buffer-user from the migrated
>> + backing-storage.
>> +
>> + If the exporter cannot fulfill the backing-storage constraints of the new
>> + buffer-user device as requested, dma_buf_attach() would return an error
>> to
>> + denote non-compatibility of the new buffer-sharing request with the
>> current
>> + buffer.
>> +
>> + If the exporter chooses not to allow an attach() operation once a
>> + get_scatterlist has been called, it simply returns an error.
>> +
>> +- mmap file operation
>> + An mmap() file operation is provided for the fd associated with the
>> buffer.
>> + If the exporter defines an mmap operation, the mmap() fop calls this to
>> allow
>> + mmap for devices that might need it; if not, it returns an error.
>> +
>> +References:
>> +[1] struct dma_buf_ops in include/linux/dma-buf.h
>> +[2] All interfaces mentioned above defined in include/linux/dma-buf.h
>
>
> --
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>
Best regards,
~Sumit.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/5] OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr
On Wed, Sep 28, 2011 at 8:19 PM, Archit Taneja wrote: > Currently, there is a lot of redundant code is between DPI and VENC panels, > this > can be made common by moving out field/interlace specific code to a separate > function called omapvid_handle_interlace_display(). There is no functional > change made. > > Signed-off-by: Archit Taneja Reviewed-by: Sumit Semwal -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr
On Wed, Sep 28, 2011 at 8:19 PM, Archit Taneja wrote: > Currently, in omap_vout_isr(), if the panel type is DPI, and if we > get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the > current buffers state to VIDEOBUF_DONE and prepare to display the > next frame in the queue. > > On OMAP4, because we have 2 LCD managers, the panel type itself is not > sufficient to tell if we have received the correct irq, i.e, we shouldn't > proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2 > interrupt for LCD manager. > > Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager > to VSYNC2 interrupt. > > Signed-off-by: Archit Taneja Reviewed-by: Sumit Semwal -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr
Hi Vaibhav, On Tue, Sep 27, 2011 at 12:09 PM, Hiremath, Vaibhav wrote: >> -Original Message- >> From: Semwal, Sumit >> Sent: Tuesday, September 27, 2011 11:12 AM >> To: Hiremath, Vaibhav >> Cc: Taneja, Archit; Valkeinen, Tomi; linux-o...@vger.kernel.org; linux- >> me...@vger.kernel.org >> Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in >> omap_vout_isr >> >> Hi Vaibhav, >> >>> Archit, >> >>> >> >>> I think I am still not understood or convinced with your description >> above, >> >>> >> >>> The code snippet which we are referring here, does check whether the >> >>> interrupt is either VSYNC or VSYNC2, else fall back to "vout_isr_err". >> > >> > >> I am not quite sure I understand what is the confusing part here - >> below is my understanding; please correct me if you think otherwise. >> As I understand, this patch creates a (missing) correlation between a >> manager and the corresponding ISR. The earlier code would accept a >> VSYNC2 for LCD1 manager, which is not the correct thing to do. >> That is why the check of 'if ((mgr==LCD) && (IRQ==VSYNC))' kind of >> thing is needed; Which part of this do you think the above patch >> doesn't do? Or, do you think it is not needed / done correctly? > Sumit, > > Please look at the patch carefully, it does exactly same thing. I understand > the use-case what Archit explained in the last email but in this patch > context, the use-case change anything here in this patch. > > Can you review it carefully again? Thanks - I did review it carefully (the first time, and again), and maybe it is something that you're able to see which I can't. Could you please explain why you think (1) if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2))) goto vout_isr_err; is *exactly* the same as (2) if (!((mgr==LCD) && (irqstatus & DISPC_IRQ_VSYNC)) || (mgr==LCD2) && (irqstatus & DISPC_IRQ_VSYNC2)) ) goto vout_isr_err; [which I understand is what Archit's patch does] I am not able to see any correlation in (1) between mgr and irq, whereas it is quite clear in (2). Let me know if I missed something? Best regards, ~Sumit. > > Thanks, > Vaibhav -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr
Hi Vaibhav, >> >> On Mon, Sep 26, 2011 at 3:49 PM, Hiremath, Vaibhav wrote: >>> >>> > -Original Message- >>> > From: Taneja, Archit >>> > Sent: Thursday, September 22, 2011 11:46 AM >>> > To: Hiremath, Vaibhav >>> > Cc: Valkeinen, Tomi; linux-o...@vger.kernel.org; Semwal, Sumit; linux- >>> > me...@vger.kernel.org >>> > Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in >>> > omap_vout_isr >>> > >>> > Hi, > > >>> >>> > >> - if (!(irqstatus& (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2))) >>> > >> + if (mgr_id == OMAP_DSS_CHANNEL_LCD) >>> > >> + irq = DISPC_IRQ_VSYNC; >>> > >> + else if (mgr_id == OMAP_DSS_CHANNEL_LCD2) >>> > >> + irq = DISPC_IRQ_VSYNC2; >>> > >> + else >>> > >> + goto vout_isr_err; >>> > >> + >>> > >> + if (!(irqstatus& irq)) >>> > >> goto vout_isr_err; >>> > >> break; >>> > > I understand what this patch meant for, but I am more curious to know >>> > > What is value addition of this patch? >>> > > >>> > > if (!(irqstatus& (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2))) >>> > > >>> > > Vs >>> > > >>> > > if (mgr_id == OMAP_DSS_CHANNEL_LCD) >>> > > irq = DISPC_IRQ_VSYNC; >>> > > else if (mgr_id == OMAP_DSS_CHANNEL_LCD2) >>> > > irq = DISPC_IRQ_VSYNC2; >>> > > >>> > > Isn't this same, because we are not doing anything separate and special >>> > > for VSYNC and VSYNC2? >>> > >>> > Consider a use case where the primary LCD panel(connected to LCD >>> > manager) is configured at a 60 Hz refresh rate, and the secondary >>> > panel(connected to LCD2) refreshes at 30 Hz. Let the overlay >>> > configuration be something like this: >>> > >>> > /dev/video1->overlay1->LCD manager->Primary panel(60 Hz) >>> > >>> > >>> > /dev/video2->overlay2->LCD2 manager->Secondary Panel(30 Hz) >>> > >>> > >>> > Now if we are doing streaming on both video1 and video2, since we deque >>> > on either VSYNC or VSYNC2, video2 buffers will deque faster than the >>> > panels refresh, which is incorrect. So for video2, we should only deque >>> > when we get a VSYNC2 interrupt. Thats why there is a need to correlate >>> > the interrupt and the overlay manager. >>> > >>> >>> Archit, >>> >>> I think I am still not understood or convinced with your description above, >>> >>> The code snippet which we are referring here, does check whether the >>> interrupt is either VSYNC or VSYNC2, else fall back to "vout_isr_err". > > I am not quite sure I understand what is the confusing part here - below is my understanding; please correct me if you think otherwise. As I understand, this patch creates a (missing) correlation between a manager and the corresponding ISR. The earlier code would accept a VSYNC2 for LCD1 manager, which is not the correct thing to do. That is why the check of 'if ((mgr==LCD) && (IRQ==VSYNC))' kind of thing is needed; Which part of this do you think the above patch doesn't do? Or, do you think it is not needed / done correctly? >>> >>> For me both are doing same thing, the original code is optimized way of >>> implementation. Can you please review it again? >>> >>> Thanks, >>> Vaibhav > > Thanks and best regards, ~Sumit. >>> >>> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
