Re: [PATCH 2/2] dma-buf: add helpers for attacher dma-parms
On Fri, Jul 20, 2012 at 10:09 PM, Rob Clark rob.cl...@linaro.org 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 rob.cl...@linaro.org wrote:
From: Rob Clark r...@ti.com
Add some helpers to iterate through all attachers and get the most
restrictive segment size/count/boundary.
Signed-off-by: Rob Clark r...@ti.com
---
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-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] omap_vout: Set DSS overlay_info only if paddr is non zero
On Mon, Mar 19, 2012 at 5:16 PM, Archit Taneja a0393...@ti.com 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 Tanejaarc...@ti.com
---
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 Valkeinentomi.valkei...@ti.com
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 from you containing this?
Thanks and best regards,
~Sumit.
Thanks,
Vaibhav
Regards,
Archit
Will fix this and submit.
Archit
Thanks,
Vaibhav
/* First save the configuration in ovelray structure */
ret = omapvid_init(vout, addr);
if (ret)
@@ -2071,11 +2050,12 @@ static int __init
omap_vout_create_video_devices(struct platform_device *pdev)
}
video_set_drvdata(vfd, vout);
- /* Configure the overlay structure */
- ret = omapvid_init(vid_dev-vouts[k], 0);
- if (!ret)
- goto success;
+ dev_info(pdev-dev, : registered and initialized
+ video device %d\n, vfd-minor);
+ if (k
Re: [PATCHv6 00/13] Integration of videobuf2 with dmabuf
On Tue, May 29, 2012 at 6:25 AM, Laurent Pinchart
laurent.pinch...@ideasonboard.com 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 | 7 +
include/media/videobuf2-core.h | 34 ++
15 files changed, 924 insertions(+), 99 deletions(-)
--
Regards,
Laurent Pinchart
Re: [RFCv2 PATCH 0/9] Integration of videobuf2 with dmabuf
Hi Tomasz, On Tue, Mar 6, 2012 at 5:08 PM, Tomasz Stanislawski t.stanisl...@samsung.com 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. snip 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 2/3] dma-buf: add support for kernel cpu access
Hi Daniel, On Tue, Mar 6, 2012 at 12:27 AM, Daniel Vetter daniel.vet...@ffwll.ch wrote: On Fri, Mar 2, 2012 at 23:24, Rob Clark robdcl...@gmail.com 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 laurent.pinch...@ideasonboard.com 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. snip *** 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 sumit.sem...@ti.com 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 t.stanisl...@samsung.com
Signed-off-by: Sumit Semwal sumit.sem...@ti.com
---
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: [Linaro-mm-sig] [PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism
On Fri, Jan 20, 2012 at 6:53 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com 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
[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)
snip
+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.
snip
--
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 Wed, Jan 25, 2012 at 4:04 PM, Tomasz Stanislawski t.stanisl...@samsung.com 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 t.stanisl...@samsung.com wrote: Hi Mauro, snip 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. snip 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 snip 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 1/3] dma-buf: Introduce dma buffer sharing mechanism
On Thu, Jan 26, 2012 at 1:37 AM, Daniel Vetter dan...@ffwll.ch 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 Mon, Jan 23, 2012 at 8:55 PM, Tomasz Stanislawski t.stanisl...@samsung.com wrote: Hi Mauro, snip 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. snip 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 sumit.sem...@ti.com wrote:
On Sun, Jan 15, 2012 at 2:08 AM, Sakari Ailus sakari.ai...@iki.fi wrote:
Hi Sumit,
Thanks for the patch!
Hi Sakari,
Thanks for reviewing this :)
snip
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.
snip
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 r...@ti.com wrote: On Mon, Jan 9, 2012 at 7:34 PM, InKi Dae daei...@gmail.com wrote: 2012/1/10 Rob Clark r...@ti.com: 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 r...@ti.com wrote: On Fri, Dec 23, 2011 at 4:08 AM, Semwal, Sumit sumit.sem...@ti.com wrote: On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie airl...@gmail.com wrote: snip Hence for both patches: Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch 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 airl...@redhat.com 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: [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
konrad.w...@oracle.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.
- 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.
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?
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.
+ */
+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 device.
Not seeing those two.
Oops; removed in v3 - will correct.
+ */
snip
+ /* TODO: Add try_map_dma_buf version, to return immed with
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 a...@arndb.de 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: [Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism
On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie airl...@gmail.com wrote: snip 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 daniel.vet...@ffwll.ch 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 airl...@redhat.com 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 v3 0/2] Introduce DMA buffer sharing mechanism
On Wed, Dec 21, 2011 at 3:56 AM, Rob Clark r...@ti.com wrote: On Tue, Dec 20, 2011 at 2:20 PM, Dave Airlie airl...@gmail.com wrote: snip 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 daniel.vet...@ffwll.ch 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 airl...@redhat.com 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 rob.cl...@linaro.org 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: [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
On Wed, Dec 7, 2011 at 3:41 PM, Arnd Bergmann a...@arndb.de 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 Daniel, Rob,
On Tue, Dec 6, 2011 at 3:41 AM, Rob Clark r...@ti.com wrote:
On Mon, Dec 5, 2011 at 3:23 PM, Daniel Vetter dan...@ffwll.ch 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 a...@arndb.de 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...
snip
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 very good learning
for the relatively-inexperienced me :)
So, for the purpose of dma-buf framework, could I summarize the
following and rework accordingly?:
1. remove mmap() dma_buf_op [and mmap fop], and
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 a...@arndb.de 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 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
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
konrad.w...@oracle.com 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.
snip
[1]: https://wiki.linaro.org/OfficeofCTO/MemoryManagement
[2]: http://lwn.net/Articles/454389
Signed-off-by: Sumit Semwal sumit.sem...@linaro.org
Signed-off-by: Sumit Semwal sumit.sem...@ti.com
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.
snip
+ * Copyright(C) 2011 Linaro Limited. All rights reserved.
+ * Author: Sumit Semwal sumit.sem...@ti.com
OK, so the SOB should be from @ti.com then.
+ *
snip
+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]
snip
+
+/**
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_file(file)) {
+ fput(file);
+ return ERR_PTR(-EINVAL);
+ }
+
+ return file-private_data;
+}
+EXPORT_SYMBOL(dma_buf_get);
GPL
sure; will change it.
+
+/**
+ *
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 robdcl...@gmail.com wrote: On Sat, Nov 26, 2011 at 8:00 AM, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Nov 25, 2011 at 17:28, Dave Airlie airl...@gmail.com 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 rdun...@xenotime.net 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 sumit.sem...@linaro.org
Signed-off-by: Sumit Semwal sumit.sem...@ti.com
---
Documentation/dma-buf-sharing.txt | 210
+
snip
+ 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 3/5] OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr
On Wed, Sep 28, 2011 at 8:19 PM, Archit Taneja arc...@ti.com 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 arc...@ti.com Reviewed-by: Sumit Semwal sumit.sem...@ti.com snip -- 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 arc...@ti.com 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 arc...@ti.com Reviewed-by: Sumit Semwal sumit.sem...@ti.com snip -- 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 hvaib...@ti.com 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, snip 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 snip -- 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 hvaib...@ti.com 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, snip - 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. snip -- 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
