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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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