lkml.org 
[lkml]   [2020]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter
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

\
 
 \ /
  Last update: 2020-03-07 08:51    [W:0.122 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site