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
