Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
From:   Ivan Bornyakov <brnkv.i1@gmail.com>
To:     Sebastian Fricke <sebastian.fricke@collabora.com>
Cc:     Ivan Bornyakov <brnkv.i1@gmail.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        NXP Linux Team <linux-imx@nxp.com>,
        Conor Dooley <conor+dt@kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Jackson Lee <jackson.lee@chipsnmedia.com>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Rob Herring <robh+dt@kernel.org>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Shawn Guo <shawnguo@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Nas Chung <nas.chung@chipsnmedia.com>,
        Fabio Estevam <festevam@gmail.com>,
        linux-media@vger.kernel.org, Tomasz Figa <tfiga@chromium.org>,
        linux-kernel@vger.kernel.org,
        Nicolas Dufresne <nicolas.dufresne@collabora.com>,
        kernel@collabora.com, Robert Beckett <bob.beckett@collabora.com>,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Darren Etheridge <detheridge@ti.com>
Subject: Re: [PATCH v13 5/8] media: chips-media: wave5: Add the v4l2 layer
Date:   Wed, 18 Oct 2023 01:13:52 +0300
Message-ID: <20231017221359.20164-1-brnkv.i1@gmail.com>
In-Reply-To: <20230929-wave5_v13_media_master-v13-5-5ac60ccbf2ce@collabora.com>
References: 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi!

On Thu, 12 Oct 2023 13:01:03 +0200, Sebastian Fricke wrote:
> Add the decoder and encoder implementing the v4l2
> API. This patch also adds the Makefile and the VIDEO_WAVE_VPU config
> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> ---
>  drivers/media/platform/chips-media/Kconfig         |    1 +
>  drivers/media/platform/chips-media/Makefile        |    1 +
>  drivers/media/platform/chips-media/wave5/Kconfig   |   12 +
>  drivers/media/platform/chips-media/wave5/Makefile  |   10 +
>  .../platform/chips-media/wave5/wave5-helper.c      |  213 +++
>  .../platform/chips-media/wave5/wave5-helper.h      |   31 +
>  .../platform/chips-media/wave5/wave5-vpu-dec.c     | 1953 ++++++++++++++++++++
>  .../platform/chips-media/wave5/wave5-vpu-enc.c     | 1794 ++++++++++++++++++
>  .../media/platform/chips-media/wave5/wave5-vpu.c   |  291 +++
>  .../media/platform/chips-media/wave5/wave5-vpu.h   |   83 +
>  .../platform/chips-media/wave5/wave5-vpuapi.h      |    2 -
>  11 files changed, 4389 insertions(+), 2 deletions(-)

[...]

> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> new file mode 100644
> index 000000000000..74d1fae64fa4
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c

[...]

> +static int wave5_vpu_dec_queue_setup(struct vb2_queue *q, unsigned int *num_buffers,
> +				     unsigned int *num_planes, unsigned int sizes[],
> +				     struct device *alloc_devs[])
> +{
> +	struct vpu_instance *inst = vb2_get_drv_priv(q);
> +	struct v4l2_pix_format_mplane inst_format =
> +		(q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) ? inst->src_fmt : inst->dst_fmt;
> +	unsigned int i;
> +
> +	dev_dbg(inst->dev->dev, "%s: num_buffers: %u | num_planes: %u | type: %u\n", __func__,
> +		*num_buffers, *num_planes, q->type);
> +
> +	/* the CREATE_BUFS case */
> +	if (*num_planes) {
> +		if (inst_format.num_planes != *num_planes)
> +			return -EINVAL;
> +
> +		for (i = 0; i < *num_planes; i++) {
> +			if (sizes[i] < inst_format.plane_fmt[i].sizeimage)
> +				return -EINVAL;
> +		}
> +	/* the REQBUFS case */
> +	} else {
> +		*num_planes = inst_format.num_planes;
> +
> +		if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> +			sizes[0] = inst_format.plane_fmt[0].sizeimage;
> +			dev_dbg(inst->dev->dev, "%s: size[0]: %u\n", __func__, sizes[0]);
> +		} else if (*num_planes == 1) {

I think, you should also set *num_buffers to be inst->fbc_buf_count for
V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, like this:

		} else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
			if (*num_buffers < inst->fbc_buf_count)
				*num_buffers = inst->fbc_buf_count;

			switch (*num_planes) {
			case 1:
				...
			case 2:
				...
			case 3:
				...
			}
		}

The reason for that is if fbc_buf_count is greater than initial num_buffers,
wave5_vpu_dec_job_ready() wouldn't allow to invoke wave5_vpu_dec_device_run()

Here is a part of the log of described situation:

  vdec 20410000.wave515: Switch state from NONE to OPEN.
  [...]
  vdec 20410000.wave515: wave5_vpu_dec_init_seq: init seq sent (queue 1 : 1)
  vdec 20410000.wave515: wave5_vpu_dec_get_seq_info: init seq complete (queue 0 : 0)
  [...]
  vdec 20410000.wave515: handle_dynamic_resolution_change: width: 4112 height: 3008 profile: 1 | minbuffer: 6
  ^^^^^^^^ note that minbuffer is 6

  vdec 20410000.wave515: Switch state from OPEN to INIT_SEQ.
  [...]
  vdec 20410000.wave515: decoder command: 1
  [...]
  vdec 20410000.wave515: wave5_vpu_dec_queue_setup: num_buffers: 4 | num_planes: 0 | type: 9
  ^^^^^^^^ note that num_buffers is 4

  vdec 20410000.wave515: wave5_vpu_dec_queue_setup: size[0]: 18625536
  vdec 20410000.wave515: CAPTURE queue must be streaming to queue jobs!
  vdec 20410000.wave515: CAPTURE queue must be streaming to queue jobs!
  vdec 20410000.wave515: CAPTURE queue must be streaming to queue jobs!
  vdec 20410000.wave515: CAPTURE queue must be streaming to queue jobs!
  vdec 20410000.wave515: wave5_vpu_dec_buf_queue: type:    9 index:    0 size: ([0]=18625536, [1]=   0, [2]=   0)
  vdec 20410000.wave515: wave5_vpu_dec_buf_queue: type:    9 index:    1 size: ([0]=18625536, [1]=   0, [2]=   0)
  vdec 20410000.wave515: wave5_vpu_dec_buf_queue: type:    9 index:    2 size: ([0]=18625536, [1]=   0, [2]=   0)
  vdec 20410000.wave515: wave5_vpu_dec_buf_queue: type:    9 index:    3 size: ([0]=18625536, [1]=   0, [2]=   0)
  vdec 20410000.wave515: wave5_vpu_dec_start_streaming: type: 9
  vdec 20410000.wave515: No capture buffer ready to decode!
  ^^^^^^^^ here v4l2_m2m_num_dst_bufs_ready(m2m_ctx) < (inst->fbc_buf_count - 1), namely 4 < 6
  
  vdec 20410000.wave515: wave5_vpu_dec_stop_streaming: type: 9
  vdec 20410000.wave515: streamoff_capture: Setting display flag of buf index: 0, fail: -22
  vdec 20410000.wave515: streamoff_capture: Setting display flag of buf index: 1, fail: -22
  vdec 20410000.wave515: streamoff_capture: Setting display flag of buf index: 2, fail: -22
  vdec 20410000.wave515: streamoff_capture: Setting display flag of buf index: 3, fail: -22
  [...]
  vdec 20410000.wave515: wave5_vpu_dec_close: dec_finish_seq complete

Altering num_buffers solves the issue for me.

> +			if (inst->output_format == FORMAT_422)
> +				sizes[0] = inst_format.width * inst_format.height * 2;
> +			else
> +				sizes[0] = inst_format.width * inst_format.height * 3 / 2;
> +			dev_dbg(inst->dev->dev, "%s: size[0]: %u\n", __func__, sizes[0]);
> +		} else if (*num_planes == 2) {
> +			sizes[0] = inst_format.width * inst_format.height;
> +			if (inst->output_format == FORMAT_422)
> +				sizes[1] = inst_format.width * inst_format.height;
> +			else
> +				sizes[1] = inst_format.width * inst_format.height / 2;
> +			dev_dbg(inst->dev->dev, "%s: size[0]: %u | size[1]: %u\n",
> +				__func__, sizes[0], sizes[1]);
> +		} else if (*num_planes == 3) {
> +			sizes[0] = inst_format.width * inst_format.height;
> +			if (inst->output_format == FORMAT_422) {
> +				sizes[1] = inst_format.width * inst_format.height / 2;
> +				sizes[2] = inst_format.width * inst_format.height / 2;
> +			} else {
> +				sizes[1] = inst_format.width * inst_format.height / 4;
> +				sizes[2] = inst_format.width * inst_format.height / 4;
> +			}
> +			dev_dbg(inst->dev->dev, "%s: size[0]: %u | size[1]: %u | size[2]: %u\n",
> +				__func__, sizes[0], sizes[1], sizes[2]);
> +		}
> +	}
> +
> +	return 0;
> +}

BTW I'm currently tinkering with your patchset on another C&M IP and would be
gratefull if you Cc me in the future versions of the patchset, if any.

Thanks.