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? > 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! 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). 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(-) > -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com
Attachment:
signature.asc
Description: This is a digitally signed message part
