Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC][PATCHv2 05/12] videobuf2: handle
 V4L2_FLAG_MEMORY_NON_CONSISTENT flag
To:     Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     Hans Verkuil <hans.verkuil@cisco.com>,
        Tomasz Figa <tfiga@chromium.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Sakari Ailus <sakari.ailus@iki.fi>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Pawel Osciak <posciak@chromium.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20200204025641.218376-1-senozhatsky@chromium.org>
 <20200204025641.218376-6-senozhatsky@chromium.org>
 <7f3310f9-8a12-1299-726d-2ca04207b32c@xs4all.nl>
 <20200219085945.GE122464@google.com>
From:   Hans Verkuil <hverkuil@xs4all.nl>
Message-ID: <2ec710c4-1471-afe2-abbc-6a499ffadfec@xs4all.nl>
Date:   Wed, 19 Feb 2020 10:04:25 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <20200219085945.GE122464@google.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2/19/20 9:59 AM, Sergey Senozhatsky wrote:
> On (20/02/19 09:25), Hans Verkuil wrote:
> [..]
>>> diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
>>> index bd08e4f77ae4..68185e94b686 100644
>>> --- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
>>> +++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
>>> @@ -121,7 +121,14 @@ than the number requested.
>>>  	other changes, then set ``count`` to 0, ``memory`` to
>>>  	``V4L2_MEMORY_MMAP`` and ``format.type`` to the buffer type.
>>>      * - __u32
>>> -      - ``reserved``\ [7]
>>> +      - ``flags``
>>> +      - Specifies additional buffer management attributes. Valid only when
>>> +	queue reports :ref:`V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS` capability.
>>> +	Old drivers and applications must set it to zero.
>>
>> Drop the 'Valid only' sentence. The V4L2_FLAG_MEMORY_NON_CONSISTENT depends
>> on that capability, but other flags added in the future may not.
> 
> The whole sentence, right?

Yes, "Valid only ... capability."

> 
>> Inside add a reference to the memory flags section created in patch 3.
> 
> Sorry. Inside?

Oops: Inside -> Instead

> 
> [..]
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 72efc1c544cd..169a8cf345ed 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -938,7 +938,10 @@ struct v4l2_requestbuffers {
>>>  	__u32			type;		/* enum v4l2_buf_type */
>>>  	__u32			memory;		/* enum v4l2_memory */
>>>  	__u32			capabilities;
>>> -	__u32			reserved[1];
>>> +	union {
>>> +		__u32		flags;
>>> +		__u32		reserved[1];
>>> +	};
>>
>> How about this:
>>
>> 	__u8			flags;
>> 	__u8			reserved[3];
>>
>> That avoids the anonymous union and allows some space for future additions.
> 
> Hmm. This way old apps, which clear out ->reserved, e.g.
> memset(&x.reserved, 0x00, sizeof(x.reserved)), won't clear
> out x.flags and can accidentally submit some unintended
> garbage. It's not the case with anon union.

Hmm. I need to think about this some more, so leave in the anon union
for now.

Regards,

	Hans

> 
> 	-ss
>