Hi Subash, On Thursday 22 March 2012 20:29:57 Subash Patel wrote: > On 03/22/2012 07:37 PM, Laurent Pinchart wrote: > > 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. Somewhere on my TODO list is a Kalman filter implementation for a microcontroller with 512 bytes of RAM. I know what you mean :-) > 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 :) For objects that will exist in many instances, saving memory is important (you would need to be really convincing to add a single bit to struct page for instance), but struct v4l2_exportbuffer is only used to hold parameters for an ioctl. That's temporary memory, so I think we can spare a couple of extra bytes if it can help with ABI stability. > > 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. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel