Hi, On Mon, 2018-11-19 at 12:18 +0100, Hans Verkuil wrote: > On 11/14/2018 02:47 PM, Hans Verkuil wrote: > > From: Hans Verkuil <hansv...@cisco.com> > > > > 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' 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'. > > > > The first three patches add core tag support, the next patch document > > the tag support, then a new helper function is added to v4l2-mem2mem.c > > to easily copy data from a source to a destination buffer that drivers > > can use. > > > > Next a new supports_tags vb2_queue flag is added to indicate that > > the driver supports tags. Ideally this should not be necessary, but > > that would require that all m2m drivers are converted to using the > > new helper function introduced in the previous patch. That takes more > > time then I have now since we want to get this in for 4.20. > > > > Finally the vim2m, vicodec and cedrus drivers are converted to support > > tags. > > > > 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). > > > > Note that this might change further (Paul suggested using bitfields). > > > > Also note that the cedrus code doesn't set the sequence counter, that's > > something that should still be added before this driver can be moved > > out of staging. > > > > 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! > > > > The previous RFC series was tested successfully with the cedrus driver. > > > > Regards, > > > > Hans > > I'd like to get some Acked-by or Reviewed-by replies for this series. > Or comments if you don't like something.
The series looks good to me, so consider it: Reviewed-by: Paul Kocialkowski <paul.kocialkow...@bootlin.com> Also, I'm glad you made the v4l2_m2m_buf_copy_data helper function, since all drivers will need to do these same operations anyway. > I would really like to get this in for 4.20 so the cedrus API is stable > (hopefully), since this is a last outstanding API item. I see a few more items that we need to tackle before we can consider the MPEG-2 stateless API as stable. * I mentionned using flags in the mpeg2 structures to solve the alignment issues, but I failed to provide a patch implementing it at this point. This would make the MPEG-2 controls more similar to the proposed H.264 ones and generally makes more sense than using a u8 for a binary value. * Some time ago, we also discussed adding extra fields to the structures for later use. Do you think that is still relevant? Adding a flags elements (with unused bits) would certainly help in this direction anyway. * During the discussions on the spec, the concensus was also to use one slice_params per-slice (instead of per-picture) which requires splitting the per-picture and the per-slice parameters. On this as well, I fell behind and didn't send a proposal yet. So my question is: does it need to be done for 4.20 or can we affoard going through another version cycle for these changes? It was my impression that we wanted to wait for another driver to use the API to declare it stable (and move the driver out of staging). Cheers, Paul > Tomasz: you commented that the text still referred to the tag as a u64, > but that was only in the cover letter, not the patches themselves. So > I don't plan to post a v3 just for that. > > Regards, > > Hans > > > Changes since v1: > > > > - changed to a u32 tag. Using a 64 bit tag was overly complicated due > > to the bad layout of the v4l2_buffer struct, and there is no real > > need for it by applications. > > > > Main changes since the RFC: > > > > - Added new buffer capability flag > > - Added m2m helper to copy data between buffers > > - Added documentation > > - Added tag logging in v4l2-ioctl.c > > > > Hans Verkuil (9): > > videodev2.h: add tag support > > vb2: add tag support > > v4l2-ioctl.c: log v4l2_buffer tag > > buffer.rst: document the new buffer tag feature. > > v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function > > vb2: add new supports_tags queue flag > > vim2m: add tag support > > vicodec: add tag support > > cedrus: add tag support > > > > Documentation/media/uapi/v4l/buffer.rst | 32 +++++++++---- > > .../media/uapi/v4l/vidioc-reqbufs.rst | 4 ++ > > .../media/common/videobuf2/videobuf2-v4l2.c | 45 ++++++++++++++++--- > > drivers/media/platform/vicodec/vicodec-core.c | 14 ++---- > > drivers/media/platform/vim2m.c | 14 ++---- > > drivers/media/v4l2-core/v4l2-ctrls.c | 9 ---- > > drivers/media/v4l2-core/v4l2-ioctl.c | 9 ++-- > > drivers/media/v4l2-core/v4l2-mem2mem.c | 23 ++++++++++ > > drivers/staging/media/sunxi/cedrus/cedrus.h | 9 ++-- > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 2 + > > .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++----- > > .../staging/media/sunxi/cedrus/cedrus_video.c | 2 + > > include/media/v4l2-mem2mem.h | 21 +++++++++ > > include/media/videobuf2-core.h | 2 + > > include/media/videobuf2-v4l2.h | 21 ++++++++- > > include/uapi/linux/v4l2-controls.h | 14 +++--- > > include/uapi/linux/videodev2.h | 9 +++- > > 17 files changed, 178 insertions(+), 73 deletions(-) > > -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com
signature.asc
Description: This is a digitally signed message part
