Re: [PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

[Hans Verkuil <hverkuil@xs4all.nl>]

On 02/25/14 12:44, 'Sakari Ailus' wrote:
> Hi Kamil and Hans,
> 
> On Mon, Feb 24, 2014 at 05:13:49PM +0100, Kamil Debski wrote:
>>> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
>>> Sent: Monday, February 24, 2014 4:35 PM
>>>
>>> Hans Verkuil wrote:
>>>> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
>>>>> The buffer flags field is 32 bits but the defined only used 16. This
>>>>> is fine, but as more than 16 bits will be used in the very near
>>>>> future, define them as 32-bit numbers for consistency.
>>>>>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>>>> ---
>>>>>  Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++---------
>>> ----
>>>>>  include/uapi/linux/videodev2.h         |   38 +++++++++++++++++++--
>>> -----------
>>>>>  2 files changed, 38 insertions(+), 30 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/DocBook/media/v4l/io.xml
>>>>> b/Documentation/DocBook/media/v4l/io.xml
>>>>> index 8facac4..46d24b3 100644
>>>>> --- a/Documentation/DocBook/media/v4l/io.xml
>>>>> +++ b/Documentation/DocBook/media/v4l/io.xml
>>>>
>>>> <snip>
>>>>
>>>>> @@ -1115,7 +1115,7 @@ in which case caches have not been
>>> used.</entry>
>>>>>  	  </row>
>>>>>  	  <row>
>>>>>
>>> <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
>>>>> -	    <entry>0x4000</entry>
>>>>> +	    <entry>0x00004000</entry>
>>>>>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>>>>>  	    corresponding OUTPUT buffer. This flag applies only to
>>> mem2mem devices.</entry>
>>>>>  	  </row>
>>>>
>>>> Should we add here that if TIMESTAMP_COPY is set and the TIMECODE
>>> flag
>>>> is set, then drivers should copy the TIMECODE struct as well? This is
>>>> happening already in various drivers and I think that is appropriate.
>>>> Although to be honest nobody is actually using the timecode struct,
>>>> but we plan to hijack that for hardware timestamps in the future
>>> anyway.
>>>
>>> Is there a single driver which uses the timecode field? The fact is
>>> that many m2m drivers copy it but that's probably mostly copying what
>>> one of them happened to do by accident. :-)
>>
>> Let's focus on not breaking m2m drivers with timestamp patches this time.
>> I'm sure it was a matter of accident with the initial timestamp patches.
> 
> This patch extends the documentation of the buffer flags from 16 bits to 32
> bits. There are no other changes in functionality nor documentation.
> 
> The patchset does indeed change the way timestamp and timestamp flags are
> copied: from source to destination rather than the other way around. I'd
> appreciate if you'd review especially that one (patch 5/7).
> 
> There are no other changes to the way timestamps (or timecode) are handled.
> 
>> I agree with Hans here, not sure about hijacking it in the future, though.
> 
> This patchset does not change the handling of the timecode field, other than
> the fixes in patch 5/7. I would prefer to get this old patchset in and unify
> the timecode field handling once it has been discussed and agreed on.
> 

That's fine by me with respect to timecode handling. That can be handled later.
My comment about patch 7/7 still stands (about having no guarantees when
dealing with TIMESTAMP_COPY). I think this should be mentioned, perhaps with
a note that since it is userspace that provides the source timestamps in this
case, it is also userspace's problem :-) Garbage in, garbage out.

Regards,

	Hans