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.
>
