Re: [RFC PATCHv2 0/5] vb2/cedrus: add tag support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux