On (20/06/04 13:43), Hans Verkuil wrote:
> With this patch the compliance test passes.
>
> But I have some comments about this code:
Hi Hans,
[..]
> > +
> > + if (cache_hints_cap) {
> > + crbufs.count = 2;
> > + crbufs.memory = q.g_memory();
> > + /*
> > + * Different memory consistency model. Should fail for MMAP
> > + * queues which support cache hints.
> > + */
> > + crbufs.flags = 0;
> > + if (m == V4L2_MEMORY_MMAP)
> > + fail_on_test(doioctl(node, VIDIOC_CREATE_BUFS, &crbufs) != EINVAL);
> > + else
> > + fail_on_test(doioctl(node, VIDIOC_CREATE_BUFS, &crbufs));
> > + }
>
> The code above can be integrated into testReqBufs() in the
> 'for (m = V4L2_MEMORY_MMAP; m <= V4L2_MEMORY_DMABUF; m++)' loop.
OK, do you want me to stop testing ->flags = 0 case, or shall I test both?
I wanted to test those separately yet keep the size of testReqBufs() more
or less reasonable (I just prefer to have small functions all over the place
and let the compiler decide what should be inlined)
> Currently it sets reqbufs.flags/crbufs.flags to 0, but you can just set it to
> V4L2_FLAG_MEMORY_NON_CONSISTENT instead and add the relevant tests.
Ah, OK, so no test for ->flags = 0 case.
> > + q.reqbufs(node);
> > +
> > + /* For the time being only MMAP is tested */
> > + if (m != V4L2_MEMORY_MMAP)
> > + return 0;
> > +
> > + node->g_fmt(fmt, q.g_type());
> > + q.create_bufs(node, 2, &fmt, 0);
> > + fail_on_test(setupMmap(node, q));
> > +
> > + q.reqbufs(node);
> > + q.create_bufs(node, 2, &fmt, V4L2_FLAG_MEMORY_NON_CONSISTENT);
> > + fail_on_test(setupMmap(node, q));
>
> This test should be part of testMmap instead.
OK. Let me take a look.
-ss
