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;
Date:   Sat, 7 Mar 2020 16:50:46 +0900
From:   Sergey Senozhatsky <senozhatsky@chromium.org>
To:     Hans Verkuil <hverkuil@xs4all.nl>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        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
Subject: Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter
Message-ID: <20200307075046.GC176460@google.com>
References: <20200302041213.27662-1-senozhatsky@chromium.org>
 <20200302041213.27662-5-senozhatsky@chromium.org>
 <7ab74b32-441d-1a1a-0112-6c4d0c0b900c@xs4all.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7ab74b32-441d-1a1a-0112-6c4d0c0b900c@xs4all.nl>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On (20/03/06 15:04), Hans Verkuil wrote:
[..]
> > +static bool verify_consistency_attr(struct vb2_queue *q, bool consistent_mem)
> > +{
> > +	bool queue_attr = q->dma_attrs & DMA_ATTR_NON_CONSISTENT;
> > +
> > +	if (consistent_mem != queue_attr) {
> 
> This is the wrong way around!
> 
> It's much better to write it like this:
> 
>        bool queue_is_consistent = !(q->dma_attrs & DMA_ATTR_NON_CONSISTENT);
> 
>        if (consistent_mem != queue_is_consistent) {

Hmm... That's a great catch. Thanks for spotting this.
Puzzled, how come I've never seen problems.

> What concerns me more is that this means that this series has not been
> tested properly. I found this when testing with v4l2-compliance and vivid.

I fully understand your concerns. Give me a moment to figure
out what's going on...


OK.

Apparently, the user-space I'm using for tests, utilizes different
call path. vb2_core_create_bufs() is never even invoked. Hence queue
consistency vs. request consistency checks are not performed.

What happens, instead, is v4l_reqbufs()->vb2_core_reqbufs() path.
It orphans existing buffers (if any), sets queue memory model, sets
queue consistency model (DMA attr), then allocates buffers.

On my test environment, I see that vb2_core_reqbufs() orphans the
buffers, but it's always due to "*count == 0 || q->num_buffers != 0"
conditions. The user-space I'm using does not twist queue ->memory
or consistency attr, so the tests I'm running are limited in scenarios.

verify_consistency_attr() is not on the list of reasons to orphan
allocated buffer. It probably should be, tho.

===
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index afb3c21a5902..d6b1d32bef3f 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -730,7 +730,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 	}
 
 	if (*count == 0 || q->num_buffers != 0 ||
-	    (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
+	    (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory) ||
+	    !verify_consistency_attr(q, consistent_mem)) {
 		/*
 		 * We already have buffers allocated, so first check if they
 		 * are not in use and can be freed.
===

> > +		dprintk(1, "memory consistency model mismatch\n");
> > +		return false;
> > +	}
> > +	return true;
> > +}
> > +
> >  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> > -		unsigned int *count, unsigned requested_planes,
> > -		const unsigned requested_sizes[])
> > +			 bool consistent_mem, unsigned int *count,
> > +			 unsigned requested_planes,
> > +			 const unsigned requested_sizes[])
> 
> Use 'unsigned int' in the two lines above, as per checkpatch suggestion.

OK, will do.

This comes from the original code. There are 'unsigned'-s in the
existing code, I saw it and didn't want to modify, in order to keep
diffstats shorter.

	-ss