On 11/12/2018 05:32 PM, Paul Kocialkowski wrote:
> Hi,
>
> On Mon, 2018-11-12 at 09:33 +0100, Hans Verkuil wrote:
>> As was discussed here (among other places):
>>
>> https://lkml.org/lkml/2018/10/19/440
>>
>> using capture queue buffer indices to refer to reference frames is
>> not a good idea. A better idea is to use a 'tag' (thanks to Alexandre
>> for the excellent name; it's much better than 'cookie') where the
>> application can assign a u64 tag to an output buffer, which is then
>> copied to the capture buffer(s) derived from the output buffer.
>>
>> A u64 is chosen since this allows userspace to also use pointers to
>> internal structures as 'tag'.
>
> As I mentionned in the dedicated patch, this approach is troublesome on
> 32-bit platforms. Do we really need this equivalency?
It's just a bug in the header that I need to fix. I just need to use
the right cast. I think it is a desirable feature.
>> The first two patches add core tag support, the next two patches
>> add tag support to vim2m and vicodec, and the final patch (compile
>> tested only!) adds support to the cedrus driver.
>>
>> I also removed the 'pad' fields from the mpeg2 control structs (it
>> should never been added in the first place) and aligned the structs
>> to a u32 boundary (u64 for the tag values).
>>
>> The cedrus code now also copies the timestamps (didn't happen before)
>> but the sequence counter is still not set, that's something that should
>> still be added.
>>
>> Note: if no buffer is found for a certain tag, then the dma address
>> is just set to 0. That happened before as well with invalid buffer
>> indices. This should be checked in the driver!
>
> Thanks for making these changes!
>
>> Also missing in this series are documentation updates, which is why
>> it is marked RFC.
>>
>> I would very much appreciate it if someone can test the cedrus driver
>> with these changes. If it works, then I can prepare a real patch series
>> for 4.20. It would be really good if the API is as stable as we can make
>> it before 4.20 is released.
>
> I just had a go at testing the patches on cedrus with minimal userspace
> adaptation to deal with the tags and everything looks good!
Great!
> I only set the tag when queing each OUTPUT buffer and the driver
> properly matched the CAPTURE reference buffer.
>
> I think we should make it clear in the stateless spec that multiple
> OUTPUT buffers can be allowed for the same tag, but that a single
> CAPTURE buffer should be used. Otherwise, the hardware can't use
> different partly-decoded buffers as references (and the tag API doesn't
> allow that either, since a single buffer index is returned for a tag).
Actually, the tag API allows for multiple buffers for the same tag: that's
what the last argument of vb2_find_tag() is for: you can continue looking
for buffers with the given tag from where you left off:
second_idx = -1;
first_idx = vb2_find_tag(q, tag, 0);
if (first_idx >= 0)
second_idx = vb2_find_tag(q, tag, first_idx + 1);
I think how OUTPUT buffers relate to CAPTURE buffers is really a property
of the codec that's used (I mean H.264 vs MPEG vs VP8 etc). The tag API
supports any combination.
For the stateless MPEG codec it is simple: one OUTPUT frame produces one
CAPTURE frame. So this can be documented for the control that has the
buffer references.
Thank you very much for testing this. I'll prepare a new patch series this
week which will hopefully be the final version.
Regards,
Hans
>
> What do you think?
>
> Cheers,
>
> Paul
>
>> Regards,
>>
>> Hans
>>
>> Changes since v1:
>>
>> - cookie -> tag
>> - renamed v4l2_tag to v4l2_buffer_tag
>> - dropped spurious 'to' in the commit log of patch 1
>>
>> Hans Verkuil (5):
>> videodev2.h: add tag support
>> vb2: add tag support
>> vim2m: add tag support
>> vicodec: add tag support
>> cedrus: add tag support
>>
>> .../media/common/videobuf2/videobuf2-v4l2.c | 43 ---
>> drivers/media/platform/vicodec/vicodec-core.c | 3 ++
>> drivers/media/platform/vim2m.c| 3 ++
>> drivers/media/v4l2-core/v4l2-ctrls.c | 9
>> drivers/staging/media/sunxi/cedrus/cedrus.h | 8 ++--
>> .../staging/media/sunxi/cedrus/cedrus_dec.c | 10 +
>> .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 -
>> include/media/videobuf2-v4l2.h| 18
>> include/uapi/linux/v4l2-controls.h| 14 +++---
>> include/uapi/linux/videodev2.h| 37 +++-
>> 10 files changed, 127 insertions(+), 39 deletions(-)
>>
