2018-08-28 14:49:24

by Corentin Labbe

[permalink] [raw]

(No filename) (1.00 kB)
signature.asc (849.00 B)
Download all attachments

2018-08-29 01:09:40

by Ezequiel Garcia

[permalink] [raw]

Subject: Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

On Tue, 2018-08-28 at 09:34 +0200, Paul Kocialkowski wrote:
> This introduces the Cedrus VPU driver that supports the VPU found in
> Allwinner SoCs, also known as Video Engine. It is implemented through
> a v4l2 m2m decoder device and a media device (used for media requests).
> So far, it only supports MPEG2 decoding.
>
> Since this VPU is stateless, synchronization with media requests is
> required in order to ensure consistency between frame headers that
> contain metadata about the frame to process and the raw slice data that
> is used to generate the frame.
>
> This driver was made possible thanks to the long-standing effort
> carried out by the linux-sunxi community in the interest of reverse
> engineering, documenting and implementing support for Allwinner VPU.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> MAINTAINERS | 7 +
> drivers/staging/media/Kconfig | 2 +
> drivers/staging/media/Makefile | 1 +
> drivers/staging/media/sunxi/Kconfig | 15 +
> drivers/staging/media/sunxi/Makefile | 1 +
> drivers/staging/media/sunxi/cedrus/Kconfig | 14 +
> drivers/staging/media/sunxi/cedrus/Makefile | 3 +
> drivers/staging/media/sunxi/cedrus/cedrus.c | 420 +++++++++++++
> drivers/staging/media/sunxi/cedrus/cedrus.h | 167 +++++
> .../staging/media/sunxi/cedrus/cedrus_dec.c | 116 ++++
> .../staging/media/sunxi/cedrus/cedrus_dec.h | 28 +
> .../staging/media/sunxi/cedrus/cedrus_hw.c | 322 ++++++++++
> .../staging/media/sunxi/cedrus/cedrus_hw.h | 30 +
> .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 235 +++++++
> .../staging/media/sunxi/cedrus/cedrus_regs.h | 233 +++++++
> .../staging/media/sunxi/cedrus/cedrus_video.c | 574 ++++++++++++++++++
> .../staging/media/sunxi/cedrus/cedrus_video.h | 32 +
> 17 files changed, 2200 insertions(+)
> create mode 100644 drivers/staging/media/sunxi/Kconfig
> create mode 100644 drivers/staging/media/sunxi/Makefile
> create mode 100644 drivers/staging/media/sunxi/cedrus/Kconfig
> create mode 100644 drivers/staging/media/sunxi/cedrus/Makefile
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.c
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus.h
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_dec.h
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_video.c
> create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_video.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 435e6c08c694..08065d53c69d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -656,6 +656,13 @@ L: [email protected]
> S: Maintained
> F: drivers/crypto/sunxi-ss/
>
> +ALLWINNER VPU DRIVER
> +M: Maxime Ripard <[email protected]>
> +M: Paul Kocialkowski <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/staging/media/sunxi/cedrus/
> +
> ALPHA PORT
> M: Richard Henderson <[email protected]>
> M: Ivan Kokshaysky <[email protected]>
> diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig
> index db5cf67047ad..b3620a8f2d9f 100644
> --- a/drivers/staging/media/Kconfig
> +++ b/drivers/staging/media/Kconfig
> @@ -31,6 +31,8 @@ source "drivers/staging/media/mt9t031/Kconfig"
>
> source "drivers/staging/media/omap4iss/Kconfig"
>
> +source "drivers/staging/media/sunxi/Kconfig"
> +
> source "drivers/staging/media/tegra-vde/Kconfig"
>
> source "drivers/staging/media/zoran/Kconfig"
> diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile
> index 503fbe47fa58..42948f805548 100644
> --- a/drivers/staging/media/Makefile
> +++ b/drivers/staging/media/Makefile
> @@ -5,5 +5,6 @@ obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074/
> obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031/
> obj-$(CONFIG_VIDEO_DM365_VPFE) += davinci_vpfe/
> obj-$(CONFIG_VIDEO_OMAP4) += omap4iss/
> +obj-$(CONFIG_VIDEO_SUNXI) += sunxi/
> obj-$(CONFIG_TEGRA_VDE) += tegra-vde/
> obj-$(CONFIG_VIDEO_ZORAN) += zoran/
> diff --git a/drivers/staging/media/sunxi/Kconfig b/drivers/staging/media/sunxi/Kconfig
> new file mode 100644
> index 000000000000..c78d92240ceb
> --- /dev/null
> +++ b/drivers/staging/media/sunxi/Kconfig
> @@ -0,0 +1,15 @@
> +config VIDEO_SUNXI
> + bool "Allwinner sunXi family Video Devices"
> + depends on ARCH_SUNXI || COMPILE_TEST
> + help
> + If you have an Allwinner SoC based on the sunXi family, say Y.
> +
> + Note that this option doesn't include new drivers in the
> + kernel: saying N will just cause Kconfig to skip all the
> + questions about Allwinner media devices.
> +
> +if VIDEO_SUNXI
> +
> +source "drivers/staging/media/sunxi/cedrus/Kconfig"
> +
> +endif
> diff --git a/drivers/staging/media/sunxi/Makefile b/drivers/staging/media/sunxi/Makefile
> new file mode 100644
> index 000000000000..cee2846c3ecf
> --- /dev/null
> +++ b/drivers/staging/media/sunxi/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_SUNXI_CEDRUS) += cedrus/
> diff --git a/drivers/staging/media/sunxi/cedrus/Kconfig b/drivers/staging/media/sunxi/cedrus/Kconfig
> new file mode 100644
> index 000000000000..afd7d7ee0388
> --- /dev/null
> +++ b/drivers/staging/media/sunxi/cedrus/Kconfig
> @@ -0,0 +1,14 @@
> +config VIDEO_SUNXI_CEDRUS
> + tristate "Allwinner Cedrus VPU driver"
> + depends on VIDEO_DEV && VIDEO_V4L2 && MEDIA_CONTROLLER
> + depends on HAS_DMA
> + depends on OF
> + select VIDEOBUF2_DMA_CONTIG
> + select MEDIA_REQUEST_API
> + select V4L2_MEM2MEM_DEV
> + help
> + Support for the VPU found in Allwinner SoCs, also known as the Cedar
> + video engine.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called sunxi-cedrus.
> diff --git a/drivers/staging/media/sunxi/cedrus/Makefile b/drivers/staging/media/sunxi/cedrus/Makefile
> new file mode 100644
> index 000000000000..e9dc68b7bcb6
> --- /dev/null
> +++ b/drivers/staging/media/sunxi/cedrus/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_VIDEO_SUNXI_CEDRUS) += sunxi-cedrus.o
> +
> +sunxi-cedrus-y = cedrus.o cedrus_video.o cedrus_hw.o cedrus_dec.o cedrus_mpeg2.o
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> new file mode 100644
> index 000000000000..7c0f90253135
> --- /dev/null
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -0,0 +1,420 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cedrus VPU driver
> + *
> + * Copyright (C) 2016 Florent Revest <[email protected]>
> + * Copyright (C) 2018 Paul Kocialkowski <[email protected]>
> + * Copyright (C) 2018 Bootlin
> + *
> + * Based on the vim2m driver, that is:
> + *
> + * Copyright (c) 2009-2010 Samsung Electronics Co., Ltd.
> + * Pawel Osciak, <[email protected]>
> + * Marek Szyprowski, <[email protected]>
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-mem2mem.h>
> +
> +#include "cedrus.h"
> +#include "cedrus_video.h"
> +#include "cedrus_dec.h"
> +#include "cedrus_hw.h"
> +
> +static const struct cedrus_control cedrus_controls[] = {
> + {
> + .id = V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS,
> + .elem_size = sizeof(struct v4l2_ctrl_mpeg2_slice_params),
> + .codec = CEDRUS_CODEC_MPEG2,
> + .required = true,
> + },
> + {
> + .id = V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION,
> + .elem_size = sizeof(struct v4l2_ctrl_mpeg2_quantization),
> + .codec = CEDRUS_CODEC_MPEG2,
> + .required = false,
> + },
> +};
> +
> +#define CEDRUS_CONTROLS_COUNT ARRAY_SIZE(cedrus_controls)
> +
> +void *cedrus_find_control_data(struct cedrus_ctx *ctx, u32 id)
> +{
> + unsigned int i;
> +
> + for (i = 0; ctx->ctrls[i] != NULL; i++)
> + if (ctx->ctrls[i]->id == id)
> + return ctx->ctrls[i]->p_cur.p;
> +
> + return NULL;
> +}
> +
> +static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx)
> +{
> + struct v4l2_ctrl_handler *hdl = &ctx->hdl;
> + struct v4l2_ctrl *ctrl;
> + unsigned int ctrl_size;
> + unsigned int i;
> +
> + v4l2_ctrl_handler_init(hdl, CEDRUS_CONTROLS_COUNT);
> + if (hdl->error) {
> + v4l2_err(&dev->v4l2_dev,
> + "Failed to initialize control handler\n");
> + return hdl->error;
> + }
> +
> + ctrl_size = sizeof(ctrl) * CEDRUS_CONTROLS_COUNT + 1;
> +
> + ctx->ctrls = kzalloc(ctrl_size, GFP_KERNEL);
> + memset(ctx->ctrls, 0, ctrl_size);
> +
> + for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> + struct v4l2_ctrl_config cfg = { 0 };
> +
> + cfg.elem_size = cedrus_controls[i].elem_size;
> + cfg.id = cedrus_controls[i].id;
> +
> + ctrl = v4l2_ctrl_new_custom(hdl, &cfg, NULL);
> + if (hdl->error) {
> + v4l2_err(&dev->v4l2_dev,
> + "Failed to create new custom control\n");
> +
> + v4l2_ctrl_handler_free(hdl);
> + kfree(ctx->ctrls);
> + return hdl->error;
> + }
> +
> + ctx->ctrls[i] = ctrl;
> + }
> +
> + ctx->fh.ctrl_handler = hdl;
> + v4l2_ctrl_handler_setup(hdl);
> +
> + return 0;
> +}
> +
> +static int cedrus_request_validate(struct media_request *req)
> +{
> + struct media_request_object *obj, *obj_safe;
> + struct v4l2_ctrl_handler *parent_hdl, *hdl;
> + struct cedrus_ctx *ctx = NULL;
> + struct v4l2_ctrl *ctrl_test;
> + unsigned int i;
> +
> + list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
> + struct vb2_buffer *vb;
> +
> + if (vb2_request_object_is_buffer(obj)) {
> + vb = container_of(obj, struct vb2_buffer, req_obj);
> + ctx = vb2_get_drv_priv(vb->vb2_queue);
> +
> + break;
> + }
> + }
> +
> + if (!ctx)
> + return -ENOENT;
> +
> + parent_hdl = &ctx->hdl;
> +
> + hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
> + if (!hdl) {
> + v4l2_err(&ctx->dev->v4l2_dev, "Missing codec control(s)\n");
> + return -ENOENT;
> + }
> +
> + for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> + if (cedrus_controls[i].codec != ctx->current_codec ||
> + !cedrus_controls[i].required)
> + continue;
> +
> + ctrl_test = v4l2_ctrl_request_hdl_ctrl_find(hdl,
> + cedrus_controls[i].id);
> + if (!ctrl_test) {
> + v4l2_err(&ctx->dev->v4l2_dev,
> + "Missing required codec control\n");
> + return -ENOENT;
> + }
> + }
> +
> + v4l2_ctrl_request_hdl_put(hdl);
> +
> + return vb2_request_validate(req);
> +}
> +
> +static int cedrus_open(struct file *file)
> +{
> + struct cedrus_dev *dev = video_drvdata(file);
> + struct cedrus_ctx *ctx = NULL;
> + int ret;
> +
> + if (mutex_lock_interruptible(&dev->dev_mutex))
> + return -ERESTARTSYS;
> +
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx) {
> + mutex_unlock(&dev->dev_mutex);
> + return -ENOMEM;
> + }
> +
> + v4l2_fh_init(&ctx->fh, video_devdata(file));
> + file->private_data = &ctx->fh;
> + ctx->dev = dev;
> +
> + ret = cedrus_init_ctrls(dev, ctx);
> + if (ret)
> + goto err_free;
> +
> + ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev, ctx,
> + &cedrus_queue_init);
> + if (IS_ERR(ctx->fh.m2m_ctx)) {
> + ret = PTR_ERR(ctx->fh.m2m_ctx);
> + goto err_ctrls;
> + }
> +
> + v4l2_fh_add(&ctx->fh);
> +
> + mutex_unlock(&dev->dev_mutex);
> +
> + return 0;
> +
> +err_ctrls:
> + v4l2_ctrl_handler_free(&ctx->hdl);
> +err_free:
> + kfree(ctx);
> + mutex_unlock(&dev->dev_mutex);
> +
> + return ret;
> +}
> +
> +static int cedrus_release(struct file *file)
> +{
> + struct cedrus_dev *dev = video_drvdata(file);
> + struct cedrus_ctx *ctx = container_of(file->private_data,
> + struct cedrus_ctx, fh);
> +
> + mutex_lock(&dev->dev_mutex);
> +
> + v4l2_fh_del(&ctx->fh);
> + v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> +
> + v4l2_ctrl_handler_free(&ctx->hdl);
> + kfree(ctx->ctrls);
> +
> + v4l2_fh_exit(&ctx->fh);
> +
> + kfree(ctx);
> +
> + mutex_unlock(&dev->dev_mutex);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_file_operations cedrus_fops = {
> + .owner = THIS_MODULE,
> + .open = cedrus_open,
> + .release = cedrus_release,
> + .poll = v4l2_m2m_fop_poll,
> + .unlocked_ioctl = video_ioctl2,
> + .mmap = v4l2_m2m_fop_mmap,
> +};
> +
> +static const struct video_device cedrus_video_device = {
> + .name = CEDRUS_NAME,
> + .vfl_dir = VFL_DIR_M2M,
> + .fops = &cedrus_fops,
> + .ioctl_ops = &cedrus_ioctl_ops,
> + .minor = -1,
> + .release = video_device_release_empty,
> + .device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING,
> +};
> +
> +static const struct v4l2_m2m_ops cedrus_m2m_ops = {
> + .device_run = cedrus_device_run,
> + .job_abort = cedrus_job_abort,
> +};
> +

I think you can get rid of this .job_abort. It should
simplify your .device_run quite a bit.

.job_abort is optional now since 5525b8314389a0c558d15464e86f438974b94e32.

Regards,
Ezequiel

2018-09-03 08:34:49

by Hans Verkuil

[permalink] [raw]

Subject: Re: [PATCH v8 1/8] media: v4l: Add definitions for MPEG2 slice format and metadata

This looks very nice. I have two more comments, but they can be added using
a follow-up patch (unless you need a v9 anyway):

On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> Stateless video decoding engines require both the MPEG slices and
> associated metadata from the video stream in order to decode frames.
>
> This introduces definitions for a new pixel format, describing buffers
> with MPEG2 slice data, as well as a control structure for passing the
> frame metadata to drivers.
>
> This is based on work from both Florent Revest and Hugues Fruchet.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> .../media/uapi/v4l/extended-controls.rst | 177 ++++++++++++++++++
> .../media/uapi/v4l/pixfmt-compressed.rst | 14 ++
> .../media/uapi/v4l/vidioc-queryctrl.rst | 14 +-
> .../media/videodev2.h.rst.exceptions | 2 +
> drivers/media/v4l2-core/v4l2-ctrls.c | 63 +++++++
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> include/media/v4l2-ctrls.h | 18 +-
> include/uapi/linux/v4l2-controls.h | 65 +++++++
> include/uapi/linux/videodev2.h | 5 +
> 9 files changed, 350 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> index 9f7312bf3365..a9252225b63e 100644
> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> @@ -1497,6 +1497,183 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>
>
>
> +.. _v4l2-mpeg-mpeg2:
> +
> +``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS (struct)``
> + Specifies the slice parameters (as extracted from the bitstream) for the
> + associated MPEG-2 slice data. This includes the necessary parameters for
> + configuring a stateless hardware decoding pipeline for MPEG-2.
> + The bitstream parameters are defined according to the ISO/IEC 13818-2 and
> + ITU-T Rec. H.262 specifications.

You need to use references to the specs in the biblio.rst file. ISO/IEC 13818-2
is there already. As far as I can see ISO/IEC 13818-2 == ITU-T Rec. H.262, so
it's only one spec, not two.

> +
> +.. c:type:: v4l2_ctrl_mpeg2_slice_params
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_ctrl_mpeg2_slice_params
> + :header-rows: 0
> + :stub-columns: 0
> + :widths: 1 1 2
> +
> + * - __u32
> + - ``bit_size``
> + - Size (in bits) of the current slice data.
> + * - __u32
> + - ``data_bit_offset``
> + - Offset (in bits) to the video data in the current slice data.
> + * - struct :c:type:`v4l2_mpeg2_sequence`
> + - ``sequence``
> + - Structure with MPEG-2 sequence metadata, merging relevant fields from
> + the sequence header and sequence extension parts of the bitstream.
> + * - struct :c:type:`v4l2_mpeg2_picture`
> + - ``picture``
> + - Structure with MPEG-2 picture metadata, merging relevant fields from
> + the picture header and picture coding extension parts of the bitstream.
> + * - __u8
> + - ``quantiser_scale_code``
> + - Code used to determine the quantization scale to use for the IDCT.
> + * - __u8
> + - ``backward_ref_index``
> + - Index for the V4L2 buffer to use as backward reference, used with
> + B-coded and P-coded frames.
> + * - __u8
> + - ``forward_ref_index``
> + - Index for the V4L2 buffer to use as forward reference, used with
> + P-coded frames.
> +
> +.. c:type:: v4l2_mpeg2_sequence
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_mpeg2_sequence
> + :header-rows: 0
> + :stub-columns: 0
> + :widths: 1 1 2
> +
> + * - __u16
> + - ``horizontal_size``
> + - The width of the displayable part of the frame's luminance component.
> + * - __u16
> + - ``vertical_size``
> + - The height of the displayable part of the frame's luminance component.
> + * - __u32
> + - ``vbv_buffer_size``
> + - Used to calculate the required size of the video buffering verifier,
> + defined (in bits) as: 16 * 1024 * vbv_buffer_size.
> + * - __u8
> + - ``profile_and_level_indication``
> + - The current profile and level indication as extracted from the
> + bitstream.
> + * - __u8
> + - ``progressive_sequence``
> + - Indication that all the frames for the sequence are progressive instead
> + of interlaced.
> + * - __u8
> + - ``chroma_format``
> + - The chrominance sub-sampling format (1: 4:2:0, 2: 4:2:2, 3: 4:4:4).
> +
> +.. c:type:: v4l2_mpeg2_picture
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_mpeg2_picture
> + :header-rows: 0
> + :stub-columns: 0
> + :widths: 1 1 2
> +
> + * - __u8
> + - ``picture_coding_type``
> + - Picture coding type for the frame covered by the current slice
> + (V4L2_MPEG2_PICTURE_CODING_TYPE_I, V4L2_MPEG2_PICTURE_CODING_TYPE_P or
> + V4L2_MPEG2_PICTURE_CODING_TYPE_B).
> + * - __u8
> + - ``f_code[2][2]``
> + - Motion vector codes.
> + * - __u8
> + - ``intra_dc_precision``
> + - Precision of Discrete Cosine transform (0: 8 bits precision,
> + 1: 9 bits precision, 2: 10 bits precision, 3: 11 bits precision).
> + * - __u8
> + - ``picture_structure``
> + - Picture structure (1: interlaced top field, 2: interlaced bottom field,
> + 3: progressive frame).
> + * - __u8
> + - ``top_field_first``
> + - If set to 1 and interlaced stream, top field is output first.
> + * - __u8
> + - ``frame_pred_frame_dct``
> + - If set to 1, only frame-DCT and frame prediction are used.
> + * - __u8
> + - ``concealment_motion_vectors``
> + - If set to 1, motion vectors are coded for intra macroblocks.
> + * - __u8
> + - ``q_scale_type``
> + - This flag affects the inverse quantization process.
> + * - __u8
> + - ``intra_vlc_format``
> + - This flag affects the decoding of transform coefficient data.
> + * - __u8
> + - ``alternate_scan``
> + - This flag affects the decoding of transform coefficient data.
> + * - __u8
> + - ``repeat_first_field``
> + - This flag affects the decoding process of progressive frames.
> + * - __u8
> + - ``progressive_frame``
> + - Indicates whether the current frame is progressive.
> +
> +``V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION (struct)``
> + Specifies quantization matrices (as extracted from the bitstream) for the
> + associated MPEG-2 slice data.
> +
> +.. c:type:: v4l2_ctrl_mpeg2_quantization
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: struct v4l2_ctrl_mpeg2_quantization
> + :header-rows: 0
> + :stub-columns: 0
> + :widths: 1 1 2
> +
> + * - __u8
> + - ``load_intra_quantiser_matrix``
> + - One bit to indicate whether to load the ``intra_quantiser_matrix`` data.
> + * - __u8
> + - ``load_non_intra_quantiser_matrix``
> + - One bit to indicate whether to load the ``non_intra_quantiser_matrix``
> + data.
> + * - __u8
> + - ``load_chroma_intra_quantiser_matrix``
> + - One bit to indicate whether to load the
> + ``chroma_intra_quantiser_matrix`` data, only relevant for non-4:2:0 YUV
> + formats.
> + * - __u8
> + - ``load_chroma_non_intra_quantiser_matrix``
> + - One bit to indicate whether to load the
> + ``chroma_non_intra_quantiser_matrix`` data, only relevant for non-4:2:0
> + YUV formats.
> + * - __u8
> + - ``intra_quantiser_matrix[64]``
> + - The quantization matrix coefficients for intra-coded frames, in zigzag
> + scanning order. It is relevant for both luma and chroma components,
> + although it can be superseded by the chroma-specific matrix for
> + non-4:2:0 YUV formats.
> + * - __u8
> + - ``non_intra_quantiser_matrix[64]``
> + - The quantization matrix coefficients for non-intra-coded frames, in
> + zigzag scanning order. It is relevant for both luma and chroma
> + components, although it can be superseded by the chroma-specific matrix
> + for non-4:2:0 YUV formats.
> + * - __u8
> + - ``chroma_intra_quantiser_matrix[64]``
> + - The quantization matrix coefficients for the chominance component of
> + intra-coded frames, in zigzag scanning order. Only relevant for
> + non-4:2:0 YUV formats.
> + * - __u8
> + - ``chroma_non_intra_quantiser_matrix[64]``
> + - The quantization matrix coefficients for the chrominance component of
> + non-intra-coded frames, in zigzag scanning order. Only relevant for
> + non-4:2:0 YUV formats.
>
> MFC 5.1 MPEG Controls
> ---------------------
> diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> index d382e7a5c38e..a86b59f770dd 100644
> --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
> @@ -60,6 +60,20 @@ Compressed Formats
> - ``V4L2_PIX_FMT_MPEG2``
> - 'MPG2'
> - MPEG2 video elementary stream.
> + * .. _V4L2-PIX-FMT-MPEG2-SLICE:
> +
> + - ``V4L2_PIX_FMT_MPEG2_SLICE``
> + - 'MG2S'
> + - MPEG-2 parsed slice data, as extracted from the MPEG-2 bitstream.
> + This format is adapted for stateless video decoders that implement a
> + MPEG-2 pipeline (using the Memory to Memory and Media Request APIs).

Use references to the corresponding sections in the spec so the reader can just
click on the link.

Regards,

Hans

> + Metadata associated with the frame to decode is required to be passed
> + through the ``V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS`` control and
> + quantization matrices can optionally be specified through the
> + ``V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION`` control.
> + See the :ref:`associated Codec Control IDs <v4l2-mpeg-mpeg2>`.
> + Buffers associated with this pixel format must contain the appropriate
> + number of macroblocks to decode a full corresponding frame.
> * .. _V4L2-PIX-FMT-MPEG4:
>
> - ``V4L2_PIX_FMT_MPEG4``
> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> index 5bd26e8c9a1a..258f5813f281 100644
> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> @@ -424,8 +424,18 @@ See also the examples in :ref:`control`.
> - any
> - An unsigned 32-bit valued control ranging from minimum to maximum
> inclusive. The step value indicates the increment between values.
> -
> -
> + * - ``V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS``
> + - n/a
> + - n/a
> + - n/a
> + - A struct :c:type:`v4l2_ctrl_mpeg2_slice_params`, containing MPEG-2
> + slice parameters for stateless video decoders.
> + * - ``V4L2_CTRL_TYPE_MPEG2_QUANTIZATION``
> + - n/a
> + - n/a
> + - n/a
> + - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2
> + quantization matrices for stateless video decoders.
>
> .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
>
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index 99256a2c003e..30ba0d6f546f 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -129,6 +129,8 @@ replace symbol V4L2_CTRL_TYPE_STRING :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_U16 :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_U32 :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_U8 :c:type:`v4l2_ctrl_type`
> +replace symbol V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS :c:type:`v4l2_ctrl_type`
> +replace symbol V4L2_CTRL_TYPE_MPEG2_QUANTIZATION :c:type:`v4l2_ctrl_type`
>
> # V4L2 capability defines
> replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index ccaf3068de6d..bcf520e0f61c 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -844,6 +844,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE: return "Vertical MV Search Range";
> case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER: return "Repeat Sequence Header";
> case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME: return "Force Key Frame";
> + case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS: return "MPEG-2 Slice Parameters";
> + case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION: return "MPEG-2 Quantization Matrices";
>
> /* VPX controls */
> case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS: return "VPX Number of Partitions";
> @@ -1292,6 +1294,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_RDS_TX_ALT_FREQS:
> *type = V4L2_CTRL_TYPE_U32;
> break;
> + case V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS:
> + *type = V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS;
> + break;
> + case V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION:
> + *type = V4L2_CTRL_TYPE_MPEG2_QUANTIZATION;
> + break;
> default:
> *type = V4L2_CTRL_TYPE_INTEGER;
> break;
> @@ -1550,6 +1558,7 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
> union v4l2_ctrl_ptr ptr)
> {
> + struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> size_t len;
> u64 offset;
> s64 val;
> @@ -1612,6 +1621,54 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
> return -ERANGE;
> return 0;
>
> + case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> + p_mpeg2_slice_params = ptr.p;
> +
> + switch (p_mpeg2_slice_params->sequence.chroma_format) {
> + case 1: /* 4:2:0 */
> + case 2: /* 4:2:2 */
> + case 3: /* 4:4:4 */
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + switch (p_mpeg2_slice_params->picture.intra_dc_precision) {
> + case 0: /* 8 bits */
> + case 1: /* 9 bits */
> + case 11: /* 11 bits */
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + switch (p_mpeg2_slice_params->picture.picture_structure) {
> + case 1: /* interlaced top field */
> + case 2: /* interlaced bottom field */
> + case 3: /* progressive */
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + switch (p_mpeg2_slice_params->picture.picture_coding_type) {
> + case V4L2_MPEG2_PICTURE_CODING_TYPE_I:
> + case V4L2_MPEG2_PICTURE_CODING_TYPE_P:
> + case V4L2_MPEG2_PICTURE_CODING_TYPE_B:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (p_mpeg2_slice_params->backward_ref_index >= VIDEO_MAX_FRAME ||
> + p_mpeg2_slice_params->forward_ref_index >= VIDEO_MAX_FRAME)
> + return -EINVAL;
> +
> + return 0;
> +
> + case V4L2_CTRL_TYPE_MPEG2_QUANTIZATION:
> + return 0;
> +
> default:
> return -EINVAL;
> }
> @@ -2186,6 +2243,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> case V4L2_CTRL_TYPE_U32:
> elem_size = sizeof(u32);
> break;
> + case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> + elem_size = sizeof(struct v4l2_ctrl_mpeg2_slice_params);
> + break;
> + case V4L2_CTRL_TYPE_MPEG2_QUANTIZATION:
> + elem_size = sizeof(struct v4l2_ctrl_mpeg2_quantization);
> + break;
> default:
> if (type < V4L2_CTRL_COMPOUND_TYPES)
> elem_size = sizeof(s32);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 87dba0b9c0a7..1a8feaf6c3f7 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1309,6 +1309,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> case V4L2_PIX_FMT_H263: descr = "H.263"; break;
> case V4L2_PIX_FMT_MPEG1: descr = "MPEG-1 ES"; break;
> case V4L2_PIX_FMT_MPEG2: descr = "MPEG-2 ES"; break;
> + case V4L2_PIX_FMT_MPEG2_SLICE: descr = "MPEG-2 Parsed Slice Data"; break;
> case V4L2_PIX_FMT_MPEG4: descr = "MPEG-4 part 2 ES"; break;
> case V4L2_PIX_FMT_XVID: descr = "Xvid"; break;
> case V4L2_PIX_FMT_VC1_ANNEX_G: descr = "VC-1 (SMPTE 412M Annex G)"; break;
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 53ca4df0c353..0dae03dd5b06 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -35,13 +35,15 @@ struct poll_table_struct;
>
> /**
> * union v4l2_ctrl_ptr - A pointer to a control value.
> - * @p_s32: Pointer to a 32-bit signed value.
> - * @p_s64: Pointer to a 64-bit signed value.
> - * @p_u8: Pointer to a 8-bit unsigned value.
> - * @p_u16: Pointer to a 16-bit unsigned value.
> - * @p_u32: Pointer to a 32-bit unsigned value.
> - * @p_char: Pointer to a string.
> - * @p: Pointer to a compound value.
> + * @p_s32: Pointer to a 32-bit signed value.
> + * @p_s64: Pointer to a 64-bit signed value.
> + * @p_u8: Pointer to a 8-bit unsigned value.
> + * @p_u16: Pointer to a 16-bit unsigned value.
> + * @p_u32: Pointer to a 32-bit unsigned value.
> + * @p_char: Pointer to a string.
> + * @p_mpeg2_slice_params: Pointer to a MPEG2 slice parameters structure.
> + * @p_mpeg2_quantization: Pointer to a MPEG2 quantization data structure.
> + * @p: Pointer to a compound value.
> */
> union v4l2_ctrl_ptr {
> s32 *p_s32;
> @@ -50,6 +52,8 @@ union v4l2_ctrl_ptr {
> u16 *p_u16;
> u32 *p_u32;
> char *p_char;
> + struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> + struct v4l2_ctrl_mpeg2_quantization *p_mpeg2_quantization;
> void *p;
> };
>
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index e4ee10ee917d..51b095898f4b 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -402,6 +402,9 @@ enum v4l2_mpeg_video_multi_slice_mode {
> #define V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE (V4L2_CID_MPEG_BASE+228)
> #define V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME (V4L2_CID_MPEG_BASE+229)
>
> +#define V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS (V4L2_CID_MPEG_BASE+250)
> +#define V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION (V4L2_CID_MPEG_BASE+251)
> +
> #define V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP (V4L2_CID_MPEG_BASE+300)
> #define V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP (V4L2_CID_MPEG_BASE+301)
> #define V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP (V4L2_CID_MPEG_BASE+302)
> @@ -1092,4 +1095,66 @@ enum v4l2_detect_md_mode {
> #define V4L2_CID_DETECT_MD_THRESHOLD_GRID (V4L2_CID_DETECT_CLASS_BASE + 3)
> #define V4L2_CID_DETECT_MD_REGION_GRID (V4L2_CID_DETECT_CLASS_BASE + 4)
>
> +#define V4L2_MPEG2_PICTURE_CODING_TYPE_I 1
> +#define V4L2_MPEG2_PICTURE_CODING_TYPE_P 2
> +#define V4L2_MPEG2_PICTURE_CODING_TYPE_B 3
> +#define V4L2_MPEG2_PICTURE_CODING_TYPE_D 4
> +
> +struct v4l2_mpeg2_sequence {
> + /* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence header */
> + __u16 horizontal_size;
> + __u16 vertical_size;
> + __u32 vbv_buffer_size;
> +
> + /* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence extension */
> + __u8 profile_and_level_indication;
> + __u8 progressive_sequence;
> + __u8 chroma_format;
> +};
> +
> +struct v4l2_mpeg2_picture {
> + /* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture header */
> + __u8 picture_coding_type;
> +
> + /* ISO/IEC 13818-2, ITU-T Rec. H.262: Picture coding extension */
> + __u8 f_code[2][2];
> + __u8 intra_dc_precision;
> + __u8 picture_structure;
> + __u8 top_field_first;
> + __u8 frame_pred_frame_dct;
> + __u8 concealment_motion_vectors;
> + __u8 q_scale_type;
> + __u8 intra_vlc_format;
> + __u8 alternate_scan;
> + __u8 repeat_first_field;
> + __u8 progressive_frame;
> +};
> +
> +struct v4l2_ctrl_mpeg2_slice_params {
> + __u32 bit_size;
> + __u32 data_bit_offset;
> +
> + struct v4l2_mpeg2_sequence sequence;
> + struct v4l2_mpeg2_picture picture;
> +
> + /* ISO/IEC 13818-2, ITU-T Rec. H.262: Slice */
> + __u8 quantiser_scale_code;
> +
> + __u8 backward_ref_index;
> + __u8 forward_ref_index;
> +};
> +
> +struct v4l2_ctrl_mpeg2_quantization {
> + /* ISO/IEC 13818-2, ITU-T Rec. H.262: Quant matrix extension */
> + __u8 load_intra_quantiser_matrix;
> + __u8 load_non_intra_quantiser_matrix;
> + __u8 load_chroma_intra_quantiser_matrix;
> + __u8 load_chroma_non_intra_quantiser_matrix;
> +
> + __u8 intra_quantiser_matrix[64];
> + __u8 non_intra_quantiser_matrix[64];
> + __u8 chroma_intra_quantiser_matrix[64];
> + __u8 chroma_non_intra_quantiser_matrix[64];
> +};
> +
> #endif
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 490fc9964d97..ad7064fcccea 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -635,6 +635,7 @@ struct v4l2_pix_format {
> #define V4L2_PIX_FMT_H263 v4l2_fourcc('H', '2', '6', '3') /* H263 */
> #define V4L2_PIX_FMT_MPEG1 v4l2_fourcc('M', 'P', 'G', '1') /* MPEG-1 ES */
> #define V4L2_PIX_FMT_MPEG2 v4l2_fourcc('M', 'P', 'G', '2') /* MPEG-2 ES */
> +#define V4L2_PIX_FMT_MPEG2_SLICE v4l2_fourcc('M', 'G', '2', 'S') /* MPEG-2 parsed slice data */
> #define V4L2_PIX_FMT_MPEG4 v4l2_fourcc('M', 'P', 'G', '4') /* MPEG-4 part 2 ES */
> #define V4L2_PIX_FMT_XVID v4l2_fourcc('X', 'V', 'I', 'D') /* Xvid */
> #define V4L2_PIX_FMT_VC1_ANNEX_G v4l2_fourcc('V', 'C', '1', 'G') /* SMPTE 421M Annex G compliant stream */
> @@ -1601,6 +1602,8 @@ struct v4l2_ext_control {
> __u8 __user *p_u8;
> __u16 __user *p_u16;
> __u32 __user *p_u32;
> + struct v4l2_ctrl_mpeg2_slice_params __user *p_mpeg2_slice_params;
> + struct v4l2_ctrl_mpeg2_quantization __user *p_mpeg2_quantization;
> void __user *ptr;
> };
> } __attribute__ ((packed));
> @@ -1646,6 +1649,8 @@ enum v4l2_ctrl_type {
> V4L2_CTRL_TYPE_U8 = 0x0100,
> V4L2_CTRL_TYPE_U16 = 0x0101,
> V4L2_CTRL_TYPE_U32 = 0x0102,
> + V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103,
> + V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104,
> };
>
> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>

2018-09-03 09:12:59

by Hans Verkuil

On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
> Hi and thanks for the review!
>
> Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
>> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
>>> +static int cedrus_request_validate(struct media_request *req)
>>> +{
>>> + struct media_request_object *obj, *obj_safe;
>>> + struct v4l2_ctrl_handler *parent_hdl, *hdl;
>>> + struct cedrus_ctx *ctx = NULL;
>>> + struct v4l2_ctrl *ctrl_test;
>>> + unsigned int i;
>>> +
>>> + list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
>>
>> You don't need to use the _safe variant during validation.
>
> Okay, I'll use the regular one then.
>
>>> + struct vb2_buffer *vb;
>>> +
>>> + if (vb2_request_object_is_buffer(obj)) {
>>> + vb = container_of(obj, struct vb2_buffer, req_obj);
>>> + ctx = vb2_get_drv_priv(vb->vb2_queue);
>>> +
>>> + break;
>>> + }
>>> + }
>>
>> Interesting question: what happens if more than one buffer is queued in the
>> request? This is allowed by the request API and in that case the associated
>> controls in the request apply to all queued buffers.
>>
>> Would this make sense at all for this driver? If not, then you need to
>> check here if there is more than one buffer in the request and document in
>> the spec that this is not allowed.
>
> Well, our driver was written with the (unformal) assumption that we
> only deal with a pair of one output and one capture buffer. So I will
> add a check for this at request validation time and document it in the
> spec. Should that be part of the MPEG-2 PIXFMT documentation (and
> duplicated for future formats we add support for)?

Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c
renaming it to vb2_request_buffer_cnt() and returning the number of buffers
in the request?

Then you can call it here to check that you have only one buffer.

And this has to be documented with the PIXFMT.

Multiple buffers are certainly possible in non-codec scenarios (vim2m and
vivid happily accept that), so this is an exception that should be
documented and checked in the codec driver.

>
>> If it does make sense, then you need to test this.
>>
>> Again, this can be corrected in a follow-up patch, unless there will be a
>> v9 anyway.
>
> [...]

>>> +static int cedrus_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
>>> + unsigned int *nplanes, unsigned int sizes[],
>>> + struct device *alloc_devs[])
>>> +{
>>> + struct cedrus_ctx *ctx = vb2_get_drv_priv(vq);
>>> + struct cedrus_dev *dev = ctx->dev;
>>> + struct v4l2_pix_format_mplane *mplane_fmt;
>>> + struct cedrus_format *fmt;
>>> + unsigned int i;
>>> +
>>> + switch (vq->type) {
>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>> + mplane_fmt = &ctx->src_fmt;
>>> + fmt = cedrus_find_format(mplane_fmt->pixelformat,
>>> + CEDRUS_DECODE_SRC,
>>> + dev->capabilities);
>>> + break;
>>> +
>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>> + mplane_fmt = &ctx->dst_fmt;
>>> + fmt = cedrus_find_format(mplane_fmt->pixelformat,
>>> + CEDRUS_DECODE_DST,
>>> + dev->capabilities);
>>> + break;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!fmt)
>>> + return -EINVAL;
>>> +
>>> + if (fmt->num_buffers == 1) {
>>> + sizes[0] = 0;
>>> +
>>> + for (i = 0; i < fmt->num_planes; i++)
>>> + sizes[0] += mplane_fmt->plane_fmt[i].sizeimage;
>>> + } else if (fmt->num_buffers == fmt->num_planes) {
>>> + for (i = 0; i < fmt->num_planes; i++)
>>> + sizes[i] = mplane_fmt->plane_fmt[i].sizeimage;
>>> + } else {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + *nplanes = fmt->num_buffers;
>>
>> This code does not take VIDIOC_CREATE_BUFFERS into account.
>>
>> If it is called from that ioctl, then *nplanes is non-zero and you need
>> to check if *nplanes equals fmt->num_buffers and that sizes[n] is >=
>> the required size of the format. If so, then return 0, otherwise return
>> -EINVAL.
>
> Thanks for spotting this, I'll fix it as you suggested in the next
> revision.
>
>> Doesn't v4l2-compliance fail on that? Or is that test skipped because this
>> is a decoder for which streaming is not supported (yet)?
>
> Apparently, v4l2-compliance doesn't fail since I'm getting:
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK

It is tested, but only with the -s option. I'll see if I can improve the
tests.

Regards,

Hans

2018-09-06 07:24:41

by Hans Verkuil

[permalink] [raw]

Subject: Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

On 09/06/2018 09:01 AM, Hans Verkuil wrote:
> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
>> Hi and thanks for the review!
>>
>> Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
>>> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
>>>> +static int cedrus_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
>>>> + unsigned int *nplanes, unsigned int sizes[],
>>>> + struct device *alloc_devs[])
>>>> +{
>>>> + struct cedrus_ctx *ctx = vb2_get_drv_priv(vq);
>>>> + struct cedrus_dev *dev = ctx->dev;
>>>> + struct v4l2_pix_format_mplane *mplane_fmt;
>>>> + struct cedrus_format *fmt;
>>>> + unsigned int i;
>>>> +
>>>> + switch (vq->type) {
>>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>>> + mplane_fmt = &ctx->src_fmt;
>>>> + fmt = cedrus_find_format(mplane_fmt->pixelformat,
>>>> + CEDRUS_DECODE_SRC,
>>>> + dev->capabilities);
>>>> + break;
>>>> +
>>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>>> + mplane_fmt = &ctx->dst_fmt;
>>>> + fmt = cedrus_find_format(mplane_fmt->pixelformat,
>>>> + CEDRUS_DECODE_DST,
>>>> + dev->capabilities);
>>>> + break;
>>>> +
>>>> + default:
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if (!fmt)
>>>> + return -EINVAL;
>>>> +
>>>> + if (fmt->num_buffers == 1) {
>>>> + sizes[0] = 0;
>>>> +
>>>> + for (i = 0; i < fmt->num_planes; i++)
>>>> + sizes[0] += mplane_fmt->plane_fmt[i].sizeimage;
>>>> + } else if (fmt->num_buffers == fmt->num_planes) {
>>>> + for (i = 0; i < fmt->num_planes; i++)
>>>> + sizes[i] = mplane_fmt->plane_fmt[i].sizeimage;
>>>> + } else {
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + *nplanes = fmt->num_buffers;
>>>
>>> This code does not take VIDIOC_CREATE_BUFFERS into account.
>>>
>>> If it is called from that ioctl, then *nplanes is non-zero and you need
>>> to check if *nplanes equals fmt->num_buffers and that sizes[n] is >=
>>> the required size of the format. If so, then return 0, otherwise return
>>> -EINVAL.
>>
>> Thanks for spotting this, I'll fix it as you suggested in the next
>> revision.
>>
>>> Doesn't v4l2-compliance fail on that? Or is that test skipped because this
>>> is a decoder for which streaming is not supported (yet)?
>>
>> Apparently, v4l2-compliance doesn't fail since I'm getting:
>> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>
> It is tested, but only with the -s option. I'll see if I can improve the
> tests.

I've improved the tests. v4l2-compliance should now fail when run (without the
-s option) against this driver. Can you check that that is indeed the case?

Thanks!

Hans

2018-09-06 07:27:41

by Tomasz Figa

[permalink] [raw]

Subject: Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil <[email protected]> wrote:
>
> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
> > Hi and thanks for the review!
> >
> > Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
> >> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> >>> +static int cedrus_request_validate(struct media_request *req)
> >>> +{
> >>> + struct media_request_object *obj, *obj_safe;
> >>> + struct v4l2_ctrl_handler *parent_hdl, *hdl;
> >>> + struct cedrus_ctx *ctx = NULL;
> >>> + struct v4l2_ctrl *ctrl_test;
> >>> + unsigned int i;
> >>> +
> >>> + list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
> >>
> >> You don't need to use the _safe variant during validation.
> >
> > Okay, I'll use the regular one then.
> >
> >>> + struct vb2_buffer *vb;
> >>> +
> >>> + if (vb2_request_object_is_buffer(obj)) {
> >>> + vb = container_of(obj, struct vb2_buffer, req_obj);
> >>> + ctx = vb2_get_drv_priv(vb->vb2_queue);
> >>> +
> >>> + break;
> >>> + }
> >>> + }
> >>
> >> Interesting question: what happens if more than one buffer is queued in the
> >> request? This is allowed by the request API and in that case the associated
> >> controls in the request apply to all queued buffers.
> >>
> >> Would this make sense at all for this driver? If not, then you need to
> >> check here if there is more than one buffer in the request and document in
> >> the spec that this is not allowed.
> >
> > Well, our driver was written with the (unformal) assumption that we
> > only deal with a pair of one output and one capture buffer. So I will
> > add a check for this at request validation time and document it in the
> > spec. Should that be part of the MPEG-2 PIXFMT documentation (and
> > duplicated for future formats we add support for)?
>
> Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c
> renaming it to vb2_request_buffer_cnt() and returning the number of buffers
> in the request?
>
> Then you can call it here to check that you have only one buffer.
>
> And this has to be documented with the PIXFMT.
>
> Multiple buffers are certainly possible in non-codec scenarios (vim2m and
> vivid happily accept that), so this is an exception that should be
> documented and checked in the codec driver.

Hmm, isn't it still 1 buffer per 1 queue and just multiple queues
included in the request?

If we indeed allow multiple buffers for the same queue in a request,
we shouldn't restrict this on a per-driver basis. It's definitely not
a hardware limitation, since the driver could just do the same as if 2
requests with the same controls were given.

Best regards,
Tomasz

2018-09-06 07:41:00

by Hans Verkuil

[permalink] [raw]

Subject: Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

On 09/06/2018 09:25 AM, Tomasz Figa wrote:
> On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil <[email protected]> wrote:
>>
>> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
>>> Hi and thanks for the review!
>>>
>>> Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
>>>> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
>>>>> +static int cedrus_request_validate(struct media_request *req)
>>>>> +{
>>>>> + struct media_request_object *obj, *obj_safe;
>>>>> + struct v4l2_ctrl_handler *parent_hdl, *hdl;
>>>>> + struct cedrus_ctx *ctx = NULL;
>>>>> + struct v4l2_ctrl *ctrl_test;
>>>>> + unsigned int i;
>>>>> +
>>>>> + list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
>>>>
>>>> You don't need to use the _safe variant during validation.
>>>
>>> Okay, I'll use the regular one then.
>>>
>>>>> + struct vb2_buffer *vb;
>>>>> +
>>>>> + if (vb2_request_object_is_buffer(obj)) {
>>>>> + vb = container_of(obj, struct vb2_buffer, req_obj);
>>>>> + ctx = vb2_get_drv_priv(vb->vb2_queue);
>>>>> +
>>>>> + break;
>>>>> + }
>>>>> + }
>>>>
>>>> Interesting question: what happens if more than one buffer is queued in the
>>>> request? This is allowed by the request API and in that case the associated
>>>> controls in the request apply to all queued buffers.
>>>>
>>>> Would this make sense at all for this driver? If not, then you need to
>>>> check here if there is more than one buffer in the request and document in
>>>> the spec that this is not allowed.
>>>
>>> Well, our driver was written with the (unformal) assumption that we
>>> only deal with a pair of one output and one capture buffer. So I will
>>> add a check for this at request validation time and document it in the
>>> spec. Should that be part of the MPEG-2 PIXFMT documentation (and
>>> duplicated for future formats we add support for)?
>>
>> Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c
>> renaming it to vb2_request_buffer_cnt() and returning the number of buffers
>> in the request?
>>
>> Then you can call it here to check that you have only one buffer.
>>
>> And this has to be documented with the PIXFMT.
>>
>> Multiple buffers are certainly possible in non-codec scenarios (vim2m and
>> vivid happily accept that), so this is an exception that should be
>> documented and checked in the codec driver.
>
> Hmm, isn't it still 1 buffer per 1 queue and just multiple queues
> included in the request?

No. The request API allows multiple buffers for the same vb2_queue to be
queued for the same request (obviously when the request is committed, the
buffers are queued to the driver in the same order).

>
> If we indeed allow multiple buffers for the same queue in a request,
> we shouldn't restrict this on a per-driver basis. It's definitely not
> a hardware limitation, since the driver could just do the same as if 2
> requests with the same controls were given.

That's how it operates: for all buffers in the request the same controls
apply. But does this make sense for codecs? If the control(s) with the
codec metadata always change for every buffer, then having more than one
buffer in the request is senseless and the driver should check for this
in the validation step.

If it *does* make sense in some circumstances to have the same metadata
for multiple buffers, then it should be checked if the cedrus driver
handles this correctly.

Regards,

Hans

2018-09-07 11:25:48

by Tomasz Figa

[permalink] [raw]

Subject: Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

On Thu, Sep 6, 2018 at 4:39 PM Hans Verkuil <[email protected]> wrote:
>
> On 09/06/2018 09:25 AM, Tomasz Figa wrote:
> > On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil <[email protected]> wrote:
> >>
> >> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
> >>> Hi and thanks for the review!
> >>>
> >>> Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
> >>>> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> >>>>> +static int cedrus_request_validate(struct media_request *req)
> >>>>> +{
> >>>>> + struct media_request_object *obj, *obj_safe;
> >>>>> + struct v4l2_ctrl_handler *parent_hdl, *hdl;
> >>>>> + struct cedrus_ctx *ctx = NULL;
> >>>>> + struct v4l2_ctrl *ctrl_test;
> >>>>> + unsigned int i;
> >>>>> +
> >>>>> + list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
> >>>>
> >>>> You don't need to use the _safe variant during validation.
> >>>
> >>> Okay, I'll use the regular one then.
> >>>
> >>>>> + struct vb2_buffer *vb;
> >>>>> +
> >>>>> + if (vb2_request_object_is_buffer(obj)) {
> >>>>> + vb = container_of(obj, struct vb2_buffer, req_obj);
> >>>>> + ctx = vb2_get_drv_priv(vb->vb2_queue);
> >>>>> +
> >>>>> + break;
> >>>>> + }
> >>>>> + }
> >>>>
> >>>> Interesting question: what happens if more than one buffer is queued in the
> >>>> request? This is allowed by the request API and in that case the associated
> >>>> controls in the request apply to all queued buffers.
> >>>>
> >>>> Would this make sense at all for this driver? If not, then you need to
> >>>> check here if there is more than one buffer in the request and document in
> >>>> the spec that this is not allowed.
> >>>
> >>> Well, our driver was written with the (unformal) assumption that we
> >>> only deal with a pair of one output and one capture buffer. So I will
> >>> add a check for this at request validation time and document it in the
> >>> spec. Should that be part of the MPEG-2 PIXFMT documentation (and
> >>> duplicated for future formats we add support for)?
> >>
> >> Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c
> >> renaming it to vb2_request_buffer_cnt() and returning the number of buffers
> >> in the request?
> >>
> >> Then you can call it here to check that you have only one buffer.
> >>
> >> And this has to be documented with the PIXFMT.
> >>
> >> Multiple buffers are certainly possible in non-codec scenarios (vim2m and
> >> vivid happily accept that), so this is an exception that should be
> >> documented and checked in the codec driver.
> >
> > Hmm, isn't it still 1 buffer per 1 queue and just multiple queues
> > included in the request?
>
> No. The request API allows multiple buffers for the same vb2_queue to be
> queued for the same request (obviously when the request is committed, the
> buffers are queued to the driver in the same order).
>
> >
> > If we indeed allow multiple buffers for the same queue in a request,
> > we shouldn't restrict this on a per-driver basis. It's definitely not
> > a hardware limitation, since the driver could just do the same as if 2
> > requests with the same controls were given.
>
> That's how it operates: for all buffers in the request the same controls
> apply. But does this make sense for codecs? If the control(s) with the
> codec metadata always change for every buffer, then having more than one
> buffer in the request is senseless and the driver should check for this
> in the validation step.
>
> If it *does* make sense in some circumstances to have the same metadata
> for multiple buffers, then it should be checked if the cedrus driver
> handles this correctly.

Just FYI, we may want to move this discussion to Alex's RFC with
documentation of stateless interface:
https://patchwork.kernel.org/patch/10583233/

Best regards,
Tomasz

2018-09-07 15:52:54

by Paul Kocialkowski

[permalink] [raw]

Subject: Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

Hi,

Le jeudi 06 septembre 2018 à 09:22 +0200, Hans Verkuil a écrit :
> On 09/06/2018 09:01 AM, Hans Verkuil wrote:
> > On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
> > > Hi and thanks for the review!
> > >
> > > Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit :
> > > > On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
> > > > > +static int cedrus_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
> > > > > + unsigned int *nplanes, unsigned int sizes[],
> > > > > + struct device *alloc_devs[])
> > > > > +{
> > > > > + struct cedrus_ctx *ctx = vb2_get_drv_priv(vq);
> > > > > + struct cedrus_dev *dev = ctx->dev;
> > > > > + struct v4l2_pix_format_mplane *mplane_fmt;
> > > > > + struct cedrus_format *fmt;
> > > > > + unsigned int i;
> > > > > +
> > > > > + switch (vq->type) {
> > > > > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > > > > + mplane_fmt = &ctx->src_fmt;
> > > > > + fmt = cedrus_find_format(mplane_fmt->pixelformat,
> > > > > + CEDRUS_DECODE_SRC,
> > > > > + dev->capabilities);
> > > > > + break;
> > > > > +
> > > > > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > > > > + mplane_fmt = &ctx->dst_fmt;
> > > > > + fmt = cedrus_find_format(mplane_fmt->pixelformat,
> > > > > + CEDRUS_DECODE_DST,
> > > > > + dev->capabilities);
> > > > > + break;
> > > > > +
> > > > > + default:
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + if (!fmt)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (fmt->num_buffers == 1) {
> > > > > + sizes[0] = 0;
> > > > > +
> > > > > + for (i = 0; i < fmt->num_planes; i++)
> > > > > + sizes[0] += mplane_fmt->plane_fmt[i].sizeimage;
> > > > > + } else if (fmt->num_buffers == fmt->num_planes) {
> > > > > + for (i = 0; i < fmt->num_planes; i++)
> > > > > + sizes[i] = mplane_fmt->plane_fmt[i].sizeimage;
> > > > > + } else {
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + *nplanes = fmt->num_buffers;
> > > >
> > > > This code does not take VIDIOC_CREATE_BUFFERS into account.
> > > >
> > > > If it is called from that ioctl, then *nplanes is non-zero and you need
> > > > to check if *nplanes equals fmt->num_buffers and that sizes[n] is >=
> > > > the required size of the format. If so, then return 0, otherwise return
> > > > -EINVAL.
> > >
> > > Thanks for spotting this, I'll fix it as you suggested in the next
> > > revision.
> > >
> > > > Doesn't v4l2-compliance fail on that? Or is that test skipped because this
> > > > is a decoder for which streaming is not supported (yet)?
> > >
> > > Apparently, v4l2-compliance doesn't fail since I'm getting:
> > > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >
> > It is tested, but only with the -s option. I'll see if I can improve the
> > tests.
>
> I've improved the tests. v4l2-compliance should now fail when run (without the
> -s option) against this driver. Can you check that that is indeed the case?

I think this wasn't being tested with v8 of the driver as no default
format was provided (for the G_FMT test), which probably led to MMAP
not being picked up in valid_memorytype. Thus the subsequent
CREATE_BUFS test was reported as not failing, but it really didn't test
much of anything.

Cheers,

Paul

--
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

Attachments:

signature.asc (849.00 B)
This is a digitally signed message part