On 08/06/2020 13:09, Sergey Senozhatsky wrote:
> 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.
Right.
I don't think explicitly testing for flags = 0 is useful (famous last words, I
may have to eat them in the future :-) ).
>
>>> + 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.
I don't think I explained why: setupMmap will queue buffers, and that is something
that should only be done if v4l2-compliance is started with the -s (streaming) option.
Without -s buffers shall not be queued during compliance testing.
Regards,
Hans
