Re: [Linaro-mm-sig] [RFCv2 PATCH 4/9] v4l: add buffer exporting via dmabuf

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 03/22/2012 07:37 PM, Laurent Pinchart wrote:
Hi Subash,

On Thursday 22 March 2012 19:27:01 Subash Patel wrote:
On 03/22/2012 04:46 PM, Laurent Pinchart wrote:
On Tuesday 13 March 2012 11:17:02 Tomasz Stanislawski wrote:

[snip]

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index bb6844e..e71c787 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -680,6 +680,25 @@ struct v4l2_buffer {

   #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x0800
   #define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x1000

+/**
+ * struct v4l2_exportbuffer - export of video buffer as DMABUF file
descriptor
+ *
+ * @fd:		file descriptor associated with DMABUF (set by driver)
+ * @mem_offset:	for non-multiplanar buffers with memory ==
V4L2_MEMORY_MMAP;

I don't think we will ever support exporting anything else than
V4L2_MEMORY_MMAP buffers. What will happen for multiplanar buffers ?

+ *		offset from the start of the device memory for this plane,
+ *		(or a "cookie" that should be passed to mmap() as offset)
+ *

Shouldn't the mem_offset field always be set to the mmap cookie value ?
I'm a bit confused by the "or" part, it seems to have been copied from
the v4l2_buffer documentation directly. We should clarify that.

+ * Contains data used for exporting a video buffer as DMABUF file
+ * descriptor. Uses the same 'cookie' as mmap() syscall. All reserved
fields
+ * must be set to zero.
+ */
+struct v4l2_exportbuffer {
+	__u32		fd;
+	__u32		reserved0;

Why is there a reserved field here ?

+1 to Laurent. Any particular need for reserved0 and reserved[13] below?
I think one void user pointer is sufficient even for future need.

I'd rather have more than one void user pointer, just in case. A couple of
bytes won't be expensive,
Just an off-topic note. When I learnt to hit keyboard for programming(in linux for embedded), I had strict guidelines not to declare variables as I like, as memory and computing was very precious then. A decade later, people no more think its expensive to keep 14*3 bytes*(who knows how many dma_buf objects) in the system.
Just a side note, thats all :)

and they will save lots of hassle in the future if
we need to add a couple of fields. I was just wondering why there was a
reserved field between fd and mem_offset.

+	__u32		mem_offset;
+	__u32		reserved[13];
+};
+

Also, what is the reason for returning the fd through this structure? To
keep it aligned with other v4l2 calls? I liked(or now hate making change
in the app) how it was being returned through the ioctl in your last patch.

Probably to be consistent with the V4L2 API, yes. It won't make a big
difference for applications, I would favor consistency in this case.

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux