[PATCH 00/18] media: allegro: fixes and new features

[PATCH 00/18] media: allegro: fixes and new features

[Michael Tretter]

Hello,

these are a several patches for the allegro-dvt driver that have piled up over
the last few month while I was improving my understanding of the codec.

Patches 1 to 9 fix errors in the interaction with the mcu. This includes
better interpretation of return values from the firmware, wrong fields in the
mails, wrong values in the fields and an error when resetting the mcu.

Patches 10 to 14 wire up more controls and allow user space applications to
control the framerate and the quality of the codec.

Patches 15 and 16 enable the firmware to take care of the buffer scheduling
and allow more parallelism inside the firmware. Please have a close look at
patch 16, because it changes the behavior of the driver to finish the m2m_job
before the driver returns the v4l2_buffers.

Patches 17 and 18 start work to restructure how to create the mails that are
sent to the firmware, because different firmware versions expect different
mail formats and, thus, I need additional code to generate mails if I want to
support different firmware versions.

This is the v4l-compliance test result:

v4l2-compliance SHA: b62d322d4401e6b6e5cbd78cedad9eb69dac1324, 64 bits, 64-bit time_t

Compliance test for allegro device /dev/video3:

Driver Info:
	Driver name      : allegro
	Card type        : Allegro DVT Video Encoder
	Bus info         : platform:a0009000.video-codec
	Driver version   : 5.6.0
	Capabilities     : 0x84208000
		Video Memory-to-Memory
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps      : 0x04208000
		Video Memory-to-Memory
		Streaming
		Extended Pix Format
	Detected Stateful Encoder

Required ioctls:
	test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
	test second /dev/video3 open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: OK

	test invalid ioctls: OK
Debug ioctls:
	test VIDIOC_DBG_G/S_REGISTER: OK
	test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
	test VIDIOC_QUERYCTRL: OK
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 16 Private Controls: 0

Format ioctls:
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
		warn: v4l2-test-formats.cpp(1329): S_PARM is supported for buftype 2, but not for ENUM_FRAMEINTERVALS
	test VIDIOC_G/S_PARM: OK
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
	test VIDIOC_TRY_FMT: OK
	test VIDIOC_S_FMT: OK
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK (Not Supported)

Codec ioctls:
	test VIDIOC_(TRY_)ENCODER_CMD: OK
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
	test VIDIOC_EXPBUF: OK
	test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
	test read/write: OK (Not Supported)
	test blocking wait: OK
	Video Capture: Captured 59 buffers
	test MMAP (select): OK
	Video Capture: Captured 59 buffers
	test MMAP (epoll): OK
	test USERPTR (select): OK (Not Supported)
	test DMABUF: Cannot test, specify --expbuf-device

Total for allegro device /dev/video3: 50, Succeeded: 50, Failed: 0, Warnings: 1

Michael

Michael Tretter (18):
  media: allegro: print message on mcu error
  media: allegro: fail encoding only on actual errors
  media: allegro: fix type of gop_length in channel_create message
  media: allegro: remove unknown39 field from create_channel
  media: allegro: start a GOP with an IDR frame
  media: allegro: fix calculation of CPB size
  media: allegro: fix reset if WAKEUP has not been set properly
  media: allegro: extract mcu and codec address calculation
  media: allegro: warn if response message has an unexpected size
  media: allegro: skip filler data if possible
  media: allegro: make frame rate configurable
  media: allegro: make QP configurable
  media: allegro: read bitrate mode directly from control
  media: allegro: handle dependency of bitrate and bitrate_peak
  media: allegro: verify source and destination buffer in VCU response
  media: allegro: pass buffers through firmware
  media: allegro: move mail definitions to separate file
  media: allegro: create new struct for channel parameters

 drivers/staging/media/allegro-dvt/Makefile    |   2 +-
 .../staging/media/allegro-dvt/allegro-core.c  | 808 ++++++++++--------
 .../staging/media/allegro-dvt/allegro-mail.c  |  37 +
 .../staging/media/allegro-dvt/allegro-mail.h  | 267 ++++++
 4 files changed, 738 insertions(+), 376 deletions(-)
 create mode 100644 drivers/staging/media/allegro-dvt/allegro-mail.c
 create mode 100644 drivers/staging/media/allegro-dvt/allegro-mail.h

-- 
2.20.1

[PATCH 01/18] media: allegro: print message on mcu error

[Michael Tretter]

The codec firmware uses error codes to report errors during the
configuration of a channel or while encoding a frame. Translate them
into human readable strings for debugging.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../staging/media/allegro-dvt/allegro-core.c  | 62 +++++++++++++++++--
 1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 3be41698df4c..c400559637e9 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -572,6 +572,56 @@ static inline bool channel_exists(struct allegro_channel *channel)
 	return channel->mcu_channel_id != -1;
 }
 
+#define AL_ERROR			0x80
+#define AL_ERR_INIT_FAILED		0x81
+#define AL_ERR_NO_FRAME_DECODED		0x82
+#define AL_ERR_RESOLUTION_CHANGE	0x85
+#define AL_ERR_NO_MEMORY		0x87
+#define AL_ERR_STREAM_OVERFLOW		0x88
+#define AL_ERR_TOO_MANY_SLICES		0x89
+#define AL_ERR_BUF_NOT_READY		0x8c
+#define AL_ERR_NO_CHANNEL_AVAILABLE	0x8d
+#define AL_ERR_RESOURCE_UNAVAILABLE	0x8e
+#define AL_ERR_NOT_ENOUGH_CORES		0x8f
+#define AL_ERR_REQUEST_MALFORMED	0x90
+#define AL_ERR_CMD_NOT_ALLOWED		0x91
+#define AL_ERR_INVALID_CMD_VALUE	0x92
+
+static inline const char *allegro_err_to_string(unsigned int err)
+{
+	switch (err) {
+	case AL_ERR_INIT_FAILED:
+		return "initialization failed";
+	case AL_ERR_NO_FRAME_DECODED:
+		return "no frame decoded";
+	case AL_ERR_RESOLUTION_CHANGE:
+		return "resolution change";
+	case AL_ERR_NO_MEMORY:
+		return "out of memory";
+	case AL_ERR_STREAM_OVERFLOW:
+		return "stream buffer overflow";
+	case AL_ERR_TOO_MANY_SLICES:
+		return "too many slices";
+	case AL_ERR_BUF_NOT_READY:
+		return "buffer not ready";
+	case AL_ERR_NO_CHANNEL_AVAILABLE:
+		return "no channel available";
+	case AL_ERR_RESOURCE_UNAVAILABLE:
+		return "resource unavailable";
+	case AL_ERR_NOT_ENOUGH_CORES:
+		return "not enough cores";
+	case AL_ERR_REQUEST_MALFORMED:
+		return "request malformed";
+	case AL_ERR_CMD_NOT_ALLOWED:
+		return "command not allowed";
+	case AL_ERR_INVALID_CMD_VALUE:
+		return "invalid command value";
+	case AL_ERROR:
+	default:
+		return "unknown error";
+	}
+}
+
 static unsigned int estimate_stream_size(unsigned int width,
 					 unsigned int height)
 {
@@ -1488,8 +1538,10 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
 
 	if (msg->error_code) {
 		v4l2_err(&dev->v4l2_dev,
-			 "channel %d: error while encoding frame: %x\n",
-			 channel->mcu_channel_id, msg->error_code);
+			 "channel %d: failed to encode frame: %s (%x)\n",
+			 channel->mcu_channel_id,
+			 allegro_err_to_string(msg->error_code),
+			 msg->error_code);
 		goto err;
 	}
 
@@ -1632,8 +1684,10 @@ allegro_handle_create_channel(struct allegro_dev *dev,
 
 	if (msg->error_code) {
 		v4l2_err(&dev->v4l2_dev,
-			 "user %d: mcu failed to create channel: error %x\n",
-			 channel->user_id, msg->error_code);
+			 "user %d: mcu failed to create channel: %s (%x)\n",
+			 channel->user_id,
+			 allegro_err_to_string(msg->error_code),
+			 msg->error_code);
 		err = -EIO;
 		goto out;
 	}
-- 
2.20.1

[PATCH 02/18] media: allegro: fail encoding only on actual errors

[Michael Tretter]

Only negative values are actual errors and positive values are used for
warnings. Warnings should not fail the encoding process.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index c400559637e9..0aa88e489f13 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -1536,7 +1536,7 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
 	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
 	dst_buf->sequence = channel->csequence++;
 
-	if (msg->error_code) {
+	if (msg->error_code & AL_ERROR) {
 		v4l2_err(&dev->v4l2_dev,
 			 "channel %d: failed to encode frame: %s (%x)\n",
 			 channel->mcu_channel_id,
-- 
2.20.1

[PATCH 03/18] media: allegro: fix type of gop_length in channel_create message

[Michael Tretter]

The gop_length field is actually only u16 and there are two more u8
fields in the message:

- the number of consecutive b-frames
- frequency of golden frames

Fix the message and thus fix the configuration of the GOP length.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 0aa88e489f13..1ff58683e782 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -393,7 +393,10 @@ struct mcu_msg_create_channel {
 	u32 freq_ird;
 	u32 freq_lt;
 	u32 gdr_mode;
-	u32 gop_length;
+	u16 gop_length;
+	u8 num_b;
+	u8 freq_golden_ref;
+
 	u32 unknown39;
 
 	u32 subframe_latency;
-- 
2.20.1

[PATCH 04/18] media: allegro: remove unknown39 field from create_channel

[Michael Tretter]

The subframe_latency and lda_control_mode fields directly follow
freq_golden_ref field and there is no unknown field in between. The
unknown field it at the end of the message.

Reorder the fields accordingly. This further allows to drop the hard
coded value from the lda_control_mode field and set the mode to 0.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 1ff58683e782..c93b3ba39391 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -397,10 +397,9 @@ struct mcu_msg_create_channel {
 	u8 num_b;
 	u8 freq_golden_ref;
 
-	u32 unknown39;
-
 	u32 subframe_latency;
 	u32 lda_control_mode;
+	u32 unknown41;
 } __attribute__ ((__packed__));
 
 struct mcu_msg_create_channel_response {
@@ -1121,7 +1120,6 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 	msg.gdr_mode = 0x00000000;
 	msg.gop_length = channel->gop_size;
 	msg.subframe_latency = 0x00000000;
-	msg.lda_control_mode = 0x700d0000;
 
 	allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
 	allegro_mcu_interrupt(dev);
-- 
2.20.1

[PATCH 05/18] media: allegro: start a GOP with an IDR frame

[Michael Tretter]

When creating a channel, freq_idr defines the number of frames between
IDR frames in the coded stream. In V4L2, the period between IDR frames
shall be taken from the GOP_SIZE control.

Set the IDR frame frequency equal to the GOP size and let every GOP
start with an IDR frame.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index c93b3ba39391..b04ded8ae06b 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -390,7 +390,7 @@ struct mcu_msg_create_channel {
 
 	/* gop param */
 	u32 gop_ctrl_mode;
-	u32 freq_ird;
+	u32 freq_idr;
 	u32 freq_lt;
 	u32 gdr_mode;
 	u16 gop_length;
@@ -1115,7 +1115,7 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 	msg.rate_control_option = 0x00000000;
 
 	msg.gop_ctrl_mode = 0x00000000;
-	msg.freq_ird = 0x7fffffff;
+	msg.freq_idr = channel->gop_size;
 	msg.freq_lt = 0;
 	msg.gdr_mode = 0x00000000;
 	msg.gop_length = channel->gop_size;
-- 
2.20.1

[PATCH 06/18] media: allegro: fix calculation of CPB size

[Michael Tretter]

The cpb_size is given in kilobytes, but the bitrate is given in bits per
second. Therefore, the calculation of the initial removal delay and the
cpb size for the firmware were wrong.

Convert the bitrate to kilobytes before calculating the cpb size in 90
kHz units for sending it to the firmware. Also reuse the result for the
initial removal delay, to make it obvious that we are setting the
initial removal delay to the maximum value.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index b04ded8ae06b..383a5fe07d64 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -5,6 +5,7 @@
  * Allegro DVT video encoder driver
  */
 
+#include <linux/bits.h>
 #include <linux/firmware.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -1094,12 +1095,11 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 
 	msg.rate_control_mode =
 		v4l2_bitrate_mode_to_mcu_mode(channel->bitrate_mode);
-	/* Shall be ]0;cpb_size in 90 kHz units]. Use maximum value. */
-	msg.initial_rem_delay =
-		((channel->cpb_size * 1000) / channel->bitrate_peak) * 90000;
 	/* Encoder expects cpb_size in units of a 90 kHz clock. */
 	msg.cpb_size =
-		((channel->cpb_size * 1000) / channel->bitrate_peak) * 90000;
+		(channel->cpb_size * 90000) / (channel->bitrate_peak / 1000 / BITS_PER_BYTE);
+	/* Shall be ]0;cpb_size in 90 kHz units]. Use maximum value. */
+	msg.initial_rem_delay = msg.cpb_size;
 	msg.framerate = 25;
 	msg.clk_ratio = 1000;
 	msg.target_bitrate = channel->bitrate;
-- 
2.20.1

[PATCH 07/18] media: allegro: fix reset if WAKEUP has not been set properly

[Michael Tretter]

The Zynq UltraScale+ Devices Register Reference states that the WAKEUP
bit "should be set to 0 after the MCU sleep status bit gets back to 0."
If this is not done, the mcu is not going to sleep on reset and fail the
reset.

Set WAKEUP to 0 before triggering a reset to make sure that the reset is
successful.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 383a5fe07d64..3366e3605cf5 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -1975,6 +1975,14 @@ static int allegro_mcu_reset(struct allegro_dev *dev)
 {
 	int err;
 
+	/*
+	 * Ensure that the AL5_MCU_WAKEUP bit is set to 0 otherwise the mcu
+	 * does not go to sleep after the reset.
+	 */
+	err = regmap_write(dev->regmap, AL5_MCU_WAKEUP, 0);
+	if (err)
+		return err;
+
 	err = regmap_write(dev->regmap,
 			   AL5_MCU_RESET_MODE, AL5_MCU_RESET_MODE_SLEEP);
 	if (err < 0)
-- 
2.20.1

[PATCH 08/18] media: allegro: extract mcu and codec address calculation

[Michael Tretter]

The mcu and the codec use 32 bit addresses which are sent via the
firmware messages. Add helper functions for this address calculation to
make it obvious which address is used in the message.

As the mcu and the codec have a limited address space, print warnings if
the addresses are outside the respective address space.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../staging/media/allegro-dvt/allegro-core.c  | 44 ++++++++++++++-----
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 3366e3605cf5..c0b3a6858b27 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -532,6 +532,29 @@ union mcu_msg_response {
 	struct mcu_msg_encode_frame_response encode_frame;
 };
 
+static inline u32 to_mcu_addr(struct allegro_dev *dev, dma_addr_t phys)
+{
+	if (upper_32_bits(phys) || (lower_32_bits(phys) & MCU_CACHE_OFFSET))
+		v4l2_warn(&dev->v4l2_dev,
+			  "address %pad is outside mcu window\n", &phys);
+
+	return lower_32_bits(phys) | MCU_CACHE_OFFSET;
+}
+
+static inline u32 to_mcu_size(struct allegro_dev *dev, size_t size)
+{
+	return lower_32_bits(size);
+}
+
+static inline u32 to_codec_addr(struct allegro_dev *dev, dma_addr_t phys)
+{
+	if (upper_32_bits(phys))
+		v4l2_warn(&dev->v4l2_dev,
+			  "address %pad cannot be used by codec\n", &phys);
+
+	return lower_32_bits(phys);
+}
+
 /* Helper functions for channel and user operations */
 
 static unsigned long allegro_next_user_id(struct allegro_dev *dev)
@@ -947,8 +970,8 @@ static void allegro_mcu_send_init(struct allegro_dev *dev,
 	msg.header.type = MCU_MSG_TYPE_INIT;
 	msg.header.length = sizeof(msg) - sizeof(msg.header);
 
-	msg.suballoc_dma = lower_32_bits(suballoc_dma) | MCU_CACHE_OFFSET;
-	msg.suballoc_size = suballoc_size;
+	msg.suballoc_dma = to_mcu_addr(dev, suballoc_dma);
+	msg.suballoc_size = to_mcu_size(dev, suballoc_size);
 
 	/* disable L2 cache */
 	msg.l2_cache[0] = -1;
@@ -1158,8 +1181,8 @@ static int allegro_mcu_send_put_stream_buffer(struct allegro_dev *dev,
 	msg.header.length = sizeof(msg) - sizeof(msg.header);
 
 	msg.channel_id = channel->mcu_channel_id;
-	msg.dma_addr = paddr;
-	msg.mcu_addr = paddr | MCU_CACHE_OFFSET;
+	msg.dma_addr = to_codec_addr(dev, paddr);
+	msg.mcu_addr = to_mcu_addr(dev, paddr);
 	msg.size = size;
 	msg.offset = ENCODER_STREAM_OFFSET;
 	msg.stream_id = 0; /* copied to mcu_msg_encode_frame_response */
@@ -1186,11 +1209,11 @@ static int allegro_mcu_send_encode_frame(struct allegro_dev *dev,
 	msg.pps_qp = 26; /* qp are relative to 26 */
 	msg.user_param = 0; /* copied to mcu_msg_encode_frame_response */
 	msg.src_handle = 0; /* copied to mcu_msg_encode_frame_response */
-	msg.src_y = src_y;
-	msg.src_uv = src_uv;
+	msg.src_y = to_codec_addr(dev, src_y);
+	msg.src_uv = to_codec_addr(dev, src_uv);
 	msg.stride = channel->stride;
 	msg.ep2 = 0x0;
-	msg.ep2_v = msg.ep2 | MCU_CACHE_OFFSET;
+	msg.ep2_v = to_mcu_addr(dev, msg.ep2);
 
 	allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
 	allegro_mcu_interrupt(dev);
@@ -1249,10 +1272,9 @@ static int allegro_mcu_push_buffer_internal(struct allegro_channel *channel,
 
 	buffer = msg->buffer;
 	list_for_each_entry(al_buffer, list, head) {
-		buffer->dma_addr = lower_32_bits(al_buffer->paddr);
-		buffer->mcu_addr =
-		    lower_32_bits(al_buffer->paddr) | MCU_CACHE_OFFSET;
-		buffer->size = al_buffer->size;
+		buffer->dma_addr = to_codec_addr(dev, al_buffer->paddr);
+		buffer->mcu_addr = to_mcu_addr(dev, al_buffer->paddr);
+		buffer->size = to_mcu_size(dev, al_buffer->size);
 		buffer++;
 	}
 
-- 
2.20.1

[PATCH 09/18] media: allegro: warn if response message has an unexpected size

[Michael Tretter]

The driver uses structs to parse the responses from the VCU and expects
a certain size of the responses. However, the size and format of the
mails is not stable across firmware versions. Therefore, print a warning
if the size does not match the expected size to warn the user that
strange things might happen.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index c0b3a6858b27..868dd9b35400 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -1696,6 +1696,11 @@ allegro_handle_create_channel(struct allegro_dev *dev,
 	struct allegro_channel *channel;
 	int err = 0;
 
+	if (msg->header.length != sizeof(*msg) - sizeof(msg->header))
+		v4l2_warn(&dev->v4l2_dev,
+			  "received message has %d bytes, but expected %zu\n",
+			  msg->header.length, sizeof(*msg) - sizeof(msg->header));
+
 	channel = allegro_find_channel_by_user_id(dev, msg->user_id);
 	if (IS_ERR(channel)) {
 		v4l2_warn(&dev->v4l2_dev,
@@ -1789,6 +1794,11 @@ allegro_handle_encode_frame(struct allegro_dev *dev,
 {
 	struct allegro_channel *channel;
 
+	if (msg->header.length != sizeof(*msg) - sizeof(msg->header))
+		v4l2_warn(&dev->v4l2_dev,
+			  "received message has %d bytes, but expected %zu\n",
+			  msg->header.length, sizeof(*msg) - sizeof(msg->header));
+
 	channel = allegro_find_channel_by_channel_id(dev, msg->channel_id);
 	if (IS_ERR(channel)) {
 		v4l2_err(&dev->v4l2_dev,
-- 
2.20.1

[PATCH 10/18] media: allegro: skip filler data if possible

[Michael Tretter]

The driver instructs the firmware to leave some space space in front of
the coded frame data for SPS/PPS data. If the driver receives an IDR, it
writes the SPS/PPS into that free space and fills the rest with filler
data. However, if there is no additional data, the driver can use the
plane offset to skip this space instead of adding filler data.

As the size of the SPS/PPS is only available after writing it, keep the
filler data between the SPS/PPS and the coded frame data.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../staging/media/allegro-dvt/allegro-core.c  | 25 +++++++++++--------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 868dd9b35400..71ed3abc61f0 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -1637,17 +1637,22 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
 			 channel->mcu_channel_id, len);
 	}
 
-	len = nal_h264_write_filler(&dev->plat_dev->dev, curr, free);
-	if (len < 0) {
-		v4l2_err(&dev->v4l2_dev,
-			 "failed to write %zd filler data\n", free);
-		goto err;
+	if (msg->slice_type != AL_ENC_SLICE_TYPE_I && !msg->is_idr) {
+		dst_buf->vb2_buf.planes[0].data_offset = free;
+		free = 0;
+	} else {
+		len = nal_h264_write_filler(&dev->plat_dev->dev, curr, free);
+		if (len < 0) {
+			v4l2_err(&dev->v4l2_dev,
+					"failed to write %zd filler data\n", free);
+			goto err;
+		}
+		curr += len;
+		free -= len;
+		v4l2_dbg(2, debug, &dev->v4l2_dev,
+				"channel %d: wrote %zd bytes filler nal unit\n",
+				channel->mcu_channel_id, len);
 	}
-	curr += len;
-	free -= len;
-	v4l2_dbg(2, debug, &dev->v4l2_dev,
-		 "channel %d: wrote %zd bytes filler nal unit\n",
-		 channel->mcu_channel_id, len);
 
 	if (free != 0) {
 		v4l2_err(&dev->v4l2_dev,
-- 
2.20.1

[PATCH 11/18] media: allegro: make frame rate configurable

[Michael Tretter]

The allegro dvt codec adjust the encoding speed according to a
configured frame rate. Furthermore, the frame rate is written into the
coded stream.

Ensure that the coded video data has the correct frame rate by
implementing s_parm for setting the frame rate from user space.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../staging/media/allegro-dvt/allegro-core.c  | 60 +++++++++++++++++--
 1 file changed, 55 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 71ed3abc61f0..3ecc38047436 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -7,6 +7,7 @@
 
 #include <linux/bits.h>
 #include <linux/firmware.h>
+#include <linux/gcd.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -41,6 +42,8 @@
 #define ALLEGRO_HEIGHT_DEFAULT 1080
 #define ALLEGRO_HEIGHT_MAX 2160
 
+#define ALLEGRO_FRAMERATE_DEFAULT (struct v4l2_fract) { 30, 1 };
+
 #define ALLEGRO_GOP_SIZE_DEFAULT 25
 #define ALLEGRO_GOP_SIZE_MAX 1000
 
@@ -177,6 +180,7 @@ struct allegro_channel {
 	unsigned int width;
 	unsigned int height;
 	unsigned int stride;
+	struct v4l2_fract framerate;
 
 	enum v4l2_colorspace colorspace;
 	enum v4l2_ycbcr_encoding ycbcr_enc;
@@ -1123,8 +1127,9 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 		(channel->cpb_size * 90000) / (channel->bitrate_peak / 1000 / BITS_PER_BYTE);
 	/* Shall be ]0;cpb_size in 90 kHz units]. Use maximum value. */
 	msg.initial_rem_delay = msg.cpb_size;
-	msg.framerate = 25;
-	msg.clk_ratio = 1000;
+	msg.framerate = DIV_ROUND_UP(channel->framerate.numerator,
+				     channel->framerate.denominator);
+	msg.clk_ratio = channel->framerate.denominator == 1001 ? 1001 : 1000;
 	msg.target_bitrate = channel->bitrate;
 	msg.max_bitrate = channel->bitrate_peak;
 	msg.initial_qp = 25;
@@ -1433,9 +1438,11 @@ static ssize_t allegro_h264_write_sps(struct allegro_channel *channel,
 	sps->vui.chroma_loc_info_present_flag = 1;
 	sps->vui.chroma_sample_loc_type_top_field = 0;
 	sps->vui.chroma_sample_loc_type_bottom_field = 0;
+
 	sps->vui.timing_info_present_flag = 1;
-	sps->vui.num_units_in_tick = 1;
-	sps->vui.time_scale = 50;
+	sps->vui.num_units_in_tick = channel->framerate.denominator;
+	sps->vui.time_scale = 2 * channel->framerate.numerator;
+
 	sps->vui.fixed_frame_rate_flag = 1;
 	sps->vui.nal_hrd_parameters_present_flag = 0;
 	sps->vui.vcl_hrd_parameters_present_flag = 1;
@@ -2100,7 +2107,8 @@ static int allegro_create_channel(struct allegro_channel *channel)
 	v4l2_dbg(1, debug, &dev->v4l2_dev,
 		 "user %d: creating channel (%4.4s, %dx%d@%d)\n",
 		 channel->user_id,
-		 (char *)&channel->codec, channel->width, channel->height, 25);
+		 (char *)&channel->codec, channel->width, channel->height,
+		 DIV_ROUND_UP(channel->framerate.numerator, channel->framerate.denominator));
 
 	min_level = select_minimum_h264_level(channel->width, channel->height);
 	if (channel->level < min_level) {
@@ -2146,6 +2154,7 @@ static void allegro_set_default_params(struct allegro_channel *channel)
 	channel->width = ALLEGRO_WIDTH_DEFAULT;
 	channel->height = ALLEGRO_HEIGHT_DEFAULT;
 	channel->stride = round_up(channel->width, 32);
+	channel->framerate = ALLEGRO_FRAMERATE_DEFAULT;
 
 	channel->colorspace = V4L2_COLORSPACE_REC709;
 	channel->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
@@ -2736,6 +2745,44 @@ static int allegro_ioctl_streamon(struct file *file, void *priv,
 	return v4l2_m2m_streamon(file, fh->m2m_ctx, type);
 }
 
+static int allegro_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
+{
+	struct allegro_channel *channel = fh_to_channel(fh);
+	struct v4l2_fract *timeperframe;
+
+	if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
+		return -EINVAL;
+
+	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
+	timeperframe = &a->parm.output.timeperframe;
+	timeperframe->numerator = channel->framerate.denominator;
+	timeperframe->denominator = channel->framerate.numerator;
+
+	return 0;
+}
+
+static int allegro_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
+{
+	struct allegro_channel *channel = fh_to_channel(fh);
+	struct v4l2_fract *timeperframe;
+	int div;
+
+	if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
+		return -EINVAL;
+
+	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
+	timeperframe = &a->parm.output.timeperframe;
+
+	if (timeperframe->numerator == 0 || timeperframe->denominator == 0)
+		return allegro_g_parm(file, fh, a);
+
+	div = gcd(timeperframe->denominator, timeperframe->numerator);
+	channel->framerate.numerator = timeperframe->denominator / div;
+	channel->framerate.denominator = timeperframe->numerator / div;
+
+	return 0;
+}
+
 static int allegro_subscribe_event(struct v4l2_fh *fh,
 				   const struct v4l2_event_subscription *sub)
 {
@@ -2774,6 +2821,9 @@ static const struct v4l2_ioctl_ops allegro_ioctl_ops = {
 	.vidioc_encoder_cmd = allegro_encoder_cmd,
 	.vidioc_enum_framesizes = allegro_enum_framesizes,
 
+	.vidioc_g_parm		= allegro_g_parm,
+	.vidioc_s_parm		= allegro_s_parm,
+
 	.vidioc_subscribe_event = allegro_subscribe_event,
 	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
 };
-- 
2.20.1

[PATCH 12/18] media: allegro: make QP configurable

[Michael Tretter]

The V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE control allows to enable/disable
rate control on a channel. When rate control is disabled, the driver
shall use constant QP, which are set by the application. Also implement
the controls for configuring the QP.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../staging/media/allegro-dvt/allegro-core.c  | 105 ++++++++++++++++--
 1 file changed, 95 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 3ecc38047436..c25f76fffa5e 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -198,6 +198,7 @@ struct allegro_channel {
 	unsigned int csequence;
 
 	enum v4l2_mpeg_video_bitrate_mode bitrate_mode;
+	bool frame_rc_enable;
 	unsigned int bitrate;
 	unsigned int bitrate_peak;
 	unsigned int cpb_size;
@@ -205,6 +206,12 @@ struct allegro_channel {
 
 	struct v4l2_ctrl *mpeg_video_h264_profile;
 	struct v4l2_ctrl *mpeg_video_h264_level;
+	struct v4l2_ctrl *mpeg_video_h264_i_frame_qp;
+	struct v4l2_ctrl *mpeg_video_h264_max_qp;
+	struct v4l2_ctrl *mpeg_video_h264_min_qp;
+	struct v4l2_ctrl *mpeg_video_h264_p_frame_qp;
+	struct v4l2_ctrl *mpeg_video_h264_b_frame_qp;
+	struct v4l2_ctrl *mpeg_video_frame_rc_enable;
 	struct v4l2_ctrl *mpeg_video_bitrate_mode;
 	struct v4l2_ctrl *mpeg_video_bitrate;
 	struct v4l2_ctrl *mpeg_video_bitrate_peak;
@@ -1081,6 +1088,14 @@ v4l2_bitrate_mode_to_mcu_mode(enum v4l2_mpeg_video_bitrate_mode mode)
 	}
 }
 
+static s16 get_qp_delta(int minuend, int subtrahend)
+{
+	if (minuend == subtrahend)
+		return -1;
+	else
+		return minuend - subtrahend;
+}
+
 static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 					   struct allegro_channel *channel)
 {
@@ -1120,8 +1135,12 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 	msg.max_transfo_depth_intra = 1;
 	msg.max_transfo_depth_inter = 1;
 
-	msg.rate_control_mode =
-		v4l2_bitrate_mode_to_mcu_mode(channel->bitrate_mode);
+	if (channel->frame_rc_enable)
+		msg.rate_control_mode =
+			v4l2_bitrate_mode_to_mcu_mode(channel->bitrate_mode);
+	else
+		msg.rate_control_mode = 0;
+
 	/* Encoder expects cpb_size in units of a 90 kHz clock. */
 	msg.cpb_size =
 		(channel->cpb_size * 90000) / (channel->bitrate_peak / 1000 / BITS_PER_BYTE);
@@ -1132,11 +1151,15 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 	msg.clk_ratio = channel->framerate.denominator == 1001 ? 1001 : 1000;
 	msg.target_bitrate = channel->bitrate;
 	msg.max_bitrate = channel->bitrate_peak;
-	msg.initial_qp = 25;
-	msg.min_qp = 10;
-	msg.max_qp = 51;
-	msg.ip_delta = -1;
-	msg.pb_delta = -1;
+	msg.initial_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_i_frame_qp);
+	msg.min_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_min_qp);
+	msg.max_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_max_qp);
+	msg.ip_delta = get_qp_delta(
+			v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_i_frame_qp),
+			v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_p_frame_qp));
+	msg.pb_delta = get_qp_delta(
+			v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_p_frame_qp),
+			v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_b_frame_qp));
 	msg.golden_ref = 0;
 	msg.golden_delta = 2;
 	msg.golden_ref_frequency = 10;
@@ -1455,7 +1478,7 @@ static ssize_t allegro_h264_write_sps(struct allegro_channel *channel,
 	/* See Rec. ITU-T H.264 (04/2017) p. 410 E-54 */
 	sps->vui.vcl_hrd_parameters.cpb_size_value_minus1[0] =
 		(channel->cpb_size * 1000) / (1 << (4 + sps->vui.vcl_hrd_parameters.cpb_size_scale)) - 1;
-	sps->vui.vcl_hrd_parameters.cbr_flag[0] = 1;
+	sps->vui.vcl_hrd_parameters.cbr_flag[0] = !v4l2_ctrl_g_ctrl(channel->mpeg_video_frame_rc_enable);
 	sps->vui.vcl_hrd_parameters.initial_cpb_removal_delay_length_minus1 = 31;
 	sps->vui.vcl_hrd_parameters.cpb_removal_delay_length_minus1 = 31;
 	sps->vui.vcl_hrd_parameters.dpb_output_delay_length_minus1 = 31;
@@ -1677,13 +1700,13 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
 		dst_buf->flags |= V4L2_BUF_FLAG_PFRAME;
 
 	v4l2_dbg(1, debug, &dev->v4l2_dev,
-		 "channel %d: encoded frame #%03d (%s%s, %d bytes)\n",
+		 "channel %d: encoded frame #%03d (%s%s, QP %d, %d bytes)\n",
 		 channel->mcu_channel_id,
 		 dst_buf->sequence,
 		 msg->is_idr ? "IDR, " : "",
 		 msg->slice_type == AL_ENC_SLICE_TYPE_I ? "I slice" :
 		 msg->slice_type == AL_ENC_SLICE_TYPE_P ? "P slice" : "unknown",
-		 partition->size);
+		 msg->qp, partition->size);
 
 err:
 	v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
@@ -2062,6 +2085,12 @@ static void allegro_destroy_channel(struct allegro_channel *channel)
 
 	v4l2_ctrl_grab(channel->mpeg_video_h264_profile, false);
 	v4l2_ctrl_grab(channel->mpeg_video_h264_level, false);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_i_frame_qp, false);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_max_qp, false);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_min_qp, false);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_p_frame_qp, false);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_b_frame_qp, false);
+	v4l2_ctrl_grab(channel->mpeg_video_frame_rc_enable, false);
 	v4l2_ctrl_grab(channel->mpeg_video_bitrate_mode, false);
 	v4l2_ctrl_grab(channel->mpeg_video_bitrate, false);
 	v4l2_ctrl_grab(channel->mpeg_video_bitrate_peak, false);
@@ -2122,6 +2151,12 @@ static int allegro_create_channel(struct allegro_channel *channel)
 
 	v4l2_ctrl_grab(channel->mpeg_video_h264_profile, true);
 	v4l2_ctrl_grab(channel->mpeg_video_h264_level, true);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_i_frame_qp, true);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_max_qp, true);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_min_qp, true);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_p_frame_qp, true);
+	v4l2_ctrl_grab(channel->mpeg_video_h264_b_frame_qp, true);
+	v4l2_ctrl_grab(channel->mpeg_video_frame_rc_enable, true);
 	v4l2_ctrl_grab(channel->mpeg_video_bitrate_mode, true);
 	v4l2_ctrl_grab(channel->mpeg_video_bitrate, true);
 	v4l2_ctrl_grab(channel->mpeg_video_bitrate_peak, true);
@@ -2334,6 +2369,23 @@ static int allegro_queue_init(void *priv,
 	return 0;
 }
 
+static int allegro_clamp_qp(struct allegro_channel *channel, struct v4l2_ctrl *ctrl)
+{
+	struct v4l2_ctrl *next_ctrl;
+
+	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_I_FRAME_QP)
+		next_ctrl = channel->mpeg_video_h264_p_frame_qp;
+	else if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_P_FRAME_QP)
+		next_ctrl = channel->mpeg_video_h264_b_frame_qp;
+	else
+		return 0;
+
+	/* Modify range automatically updates the value */
+	__v4l2_ctrl_modify_range(next_ctrl, ctrl->val, 51, 1, ctrl->val);
+
+	return allegro_clamp_qp(channel, next_ctrl);
+}
+
 static int allegro_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct allegro_channel *channel = container_of(ctrl->handler,
@@ -2348,6 +2400,9 @@ static int allegro_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
 		channel->level = ctrl->val;
 		break;
+	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
+		channel->frame_rc_enable = ctrl->val;
+		break;
 	case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
 		channel->bitrate_mode = ctrl->val;
 		break;
@@ -2363,6 +2418,11 @@ static int allegro_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
 		channel->gop_size = ctrl->val;
 		break;
+	case V4L2_CID_MPEG_VIDEO_H264_I_FRAME_QP:
+	case V4L2_CID_MPEG_VIDEO_H264_P_FRAME_QP:
+	case V4L2_CID_MPEG_VIDEO_H264_B_FRAME_QP:
+		allegro_clamp_qp(channel, ctrl);
+		break;
 	}
 
 	return 0;
@@ -2407,6 +2467,31 @@ static int allegro_open(struct file *file)
 			V4L2_CID_MPEG_VIDEO_H264_LEVEL,
 			V4L2_MPEG_VIDEO_H264_LEVEL_5_1, mask,
 			V4L2_MPEG_VIDEO_H264_LEVEL_5_1);
+	channel->mpeg_video_h264_i_frame_qp = v4l2_ctrl_new_std(handler,
+			&allegro_ctrl_ops,
+			V4L2_CID_MPEG_VIDEO_H264_I_FRAME_QP,
+			0, 51, 1, 30);
+	channel->mpeg_video_h264_max_qp = v4l2_ctrl_new_std(handler,
+			&allegro_ctrl_ops,
+			V4L2_CID_MPEG_VIDEO_H264_MAX_QP,
+			0, 51, 1, 51);
+	channel->mpeg_video_h264_min_qp = v4l2_ctrl_new_std(handler,
+			&allegro_ctrl_ops,
+			V4L2_CID_MPEG_VIDEO_H264_MIN_QP,
+			0, 51, 1, 0);
+	channel->mpeg_video_h264_p_frame_qp = v4l2_ctrl_new_std(handler,
+			&allegro_ctrl_ops,
+			V4L2_CID_MPEG_VIDEO_H264_P_FRAME_QP,
+			0, 51, 1, 30);
+	channel->mpeg_video_h264_b_frame_qp = v4l2_ctrl_new_std(handler,
+			&allegro_ctrl_ops,
+			V4L2_CID_MPEG_VIDEO_H264_B_FRAME_QP,
+			0, 51, 1, 30);
+	channel->mpeg_video_frame_rc_enable = v4l2_ctrl_new_std(handler,
+			&allegro_ctrl_ops,
+			V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE,
+			false, 0x1,
+			true, false);
 	channel->mpeg_video_bitrate_mode = v4l2_ctrl_new_std_menu(handler,
 			&allegro_ctrl_ops,
 			V4L2_CID_MPEG_VIDEO_BITRATE_MODE,
-- 
2.20.1

[PATCH 13/18] media: allegro: read bitrate mode directly from control

[Michael Tretter]

There is no need to copy the bitrate mode to a field in the channel and
the value can be read directly from the control.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index c25f76fffa5e..8c26158eab93 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -197,7 +197,6 @@ struct allegro_channel {
 	unsigned int sizeimage_encoded;
 	unsigned int csequence;
 
-	enum v4l2_mpeg_video_bitrate_mode bitrate_mode;
 	bool frame_rc_enable;
 	unsigned int bitrate;
 	unsigned int bitrate_peak;
@@ -1137,7 +1136,7 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 
 	if (channel->frame_rc_enable)
 		msg.rate_control_mode =
-			v4l2_bitrate_mode_to_mcu_mode(channel->bitrate_mode);
+			v4l2_bitrate_mode_to_mcu_mode(v4l2_ctrl_g_ctrl(channel->mpeg_video_bitrate_mode));
 	else
 		msg.rate_control_mode = 0;
 
@@ -2206,7 +2205,6 @@ static void allegro_set_default_params(struct allegro_channel *channel)
 	channel->sizeimage_encoded =
 		estimate_stream_size(channel->width, channel->height);
 
-	channel->bitrate_mode = V4L2_MPEG_VIDEO_BITRATE_MODE_CBR;
 	channel->bitrate = maximum_bitrate(channel->level);
 	channel->bitrate_peak = maximum_bitrate(channel->level);
 	channel->cpb_size = maximum_cpb_size(channel->level);
@@ -2403,9 +2401,6 @@ static int allegro_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
 		channel->frame_rc_enable = ctrl->val;
 		break;
-	case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
-		channel->bitrate_mode = ctrl->val;
-		break;
 	case V4L2_CID_MPEG_VIDEO_BITRATE:
 		channel->bitrate = ctrl->val;
 		break;
@@ -2496,7 +2491,7 @@ static int allegro_open(struct file *file)
 			&allegro_ctrl_ops,
 			V4L2_CID_MPEG_VIDEO_BITRATE_MODE,
 			V4L2_MPEG_VIDEO_BITRATE_MODE_CBR, 0,
-			channel->bitrate_mode);
+			V4L2_MPEG_VIDEO_BITRATE_MODE_CBR);
 	channel->mpeg_video_bitrate = v4l2_ctrl_new_std(handler,
 			&allegro_ctrl_ops,
 			V4L2_CID_MPEG_VIDEO_BITRATE,
-- 
2.20.1

[PATCH 14/18] media: allegro: handle dependency of bitrate and bitrate_peak

[Michael Tretter]

The peak bitrate must not be smaller than the configured bitrate. Update
the other control whenever one of the controls changes to reflect this
dependency.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 8c26158eab93..cedb09ea649f 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -2403,9 +2403,15 @@ static int allegro_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_MPEG_VIDEO_BITRATE:
 		channel->bitrate = ctrl->val;
+		if (channel->bitrate > channel->bitrate_peak)
+			__v4l2_ctrl_s_ctrl(channel->mpeg_video_bitrate_peak,
+					   channel->bitrate);
 		break;
 	case V4L2_CID_MPEG_VIDEO_BITRATE_PEAK:
 		channel->bitrate_peak = ctrl->val;
+		if (channel->bitrate_peak < channel->bitrate)
+			__v4l2_ctrl_s_ctrl(channel->mpeg_video_bitrate,
+					   channel->bitrate_peak);
 		break;
 	case V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE:
 		channel->cpb_size = ctrl->val;
-- 
2.20.1

[PATCH 15/18] media: allegro: verify source and destination buffer in VCU response

[Michael Tretter]

The PUT_STREAM_BUFFER and ENCODE_FRAME request have fields that allow to
pass arbitrary 64 bit values through the firmware to the ENCODE_FRAME
response. Use these values to verify that the buffers when finishing the
frame are actually the same buffers that have been sent for encoding a
frame.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../staging/media/allegro-dvt/allegro-core.c  | 21 ++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index cedb09ea649f..4f525920c194 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -1198,7 +1198,8 @@ static int allegro_mcu_send_destroy_channel(struct allegro_dev *dev,
 static int allegro_mcu_send_put_stream_buffer(struct allegro_dev *dev,
 					      struct allegro_channel *channel,
 					      dma_addr_t paddr,
-					      unsigned long size)
+					      unsigned long size,
+					      u64 stream_id)
 {
 	struct mcu_msg_put_stream_buffer msg;
 
@@ -1212,7 +1213,7 @@ static int allegro_mcu_send_put_stream_buffer(struct allegro_dev *dev,
 	msg.mcu_addr = to_mcu_addr(dev, paddr);
 	msg.size = size;
 	msg.offset = ENCODER_STREAM_OFFSET;
-	msg.stream_id = 0; /* copied to mcu_msg_encode_frame_response */
+	msg.stream_id = stream_id; /* copied to mcu_msg_encode_frame_response */
 
 	allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
 	allegro_mcu_interrupt(dev);
@@ -1222,7 +1223,8 @@ static int allegro_mcu_send_put_stream_buffer(struct allegro_dev *dev,
 
 static int allegro_mcu_send_encode_frame(struct allegro_dev *dev,
 					 struct allegro_channel *channel,
-					 dma_addr_t src_y, dma_addr_t src_uv)
+					 dma_addr_t src_y, dma_addr_t src_uv,
+					 u64 src_handle)
 {
 	struct mcu_msg_encode_frame msg;
 
@@ -1235,7 +1237,7 @@ static int allegro_mcu_send_encode_frame(struct allegro_dev *dev,
 	msg.encoding_options = AL_OPT_FORCE_LOAD;
 	msg.pps_qp = 26; /* qp are relative to 26 */
 	msg.user_param = 0; /* copied to mcu_msg_encode_frame_response */
-	msg.src_handle = 0; /* copied to mcu_msg_encode_frame_response */
+	msg.src_handle = src_handle; /* copied to mcu_msg_encode_frame_response */
 	msg.src_y = to_codec_addr(dev, src_y);
 	msg.src_uv = to_codec_addr(dev, src_uv);
 	msg.stride = channel->stride;
@@ -1584,8 +1586,13 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
 	ssize_t free;
 
 	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
-
 	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
+
+	if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
+		v4l2_err(&dev->v4l2_dev,
+			 "channel %d: check failed\n",
+			 channel->mcu_channel_id);
+
 	dst_buf->sequence = channel->csequence++;
 
 	if (msg->error_code & AL_ERROR) {
@@ -2954,14 +2961,14 @@ static void allegro_device_run(void *priv)
 	dst_buf = v4l2_m2m_next_dst_buf(channel->fh.m2m_ctx);
 	dst_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
 	dst_size = vb2_plane_size(&dst_buf->vb2_buf, 0);
-	allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size);
+	allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size, (u64)dst_buf);
 
 	src_buf = v4l2_m2m_next_src_buf(channel->fh.m2m_ctx);
 	src_buf->sequence = channel->osequence++;
 
 	src_y = vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
 	src_uv = src_y + (channel->stride * channel->height);
-	allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv);
+	allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv, (u64)src_buf);
 }
 
 static const struct v4l2_m2m_ops allegro_m2m_ops = {
-- 
2.20.1

[PATCH 16/18] media: allegro: pass buffers through firmware

[Michael Tretter]

As we know which buffers are processed by the codec from the address in
the ENCODE_FRAME response, we can queue multiple buffers in the firmware
and retrieve the buffer from the response of the firmware. This enables
the firmware to use the internal scheduling the codec and avoids round
trips through the driver when fetching the next frame.

Remove buffers that have been passed to the firmware from the m2m buffer
queue and put them into a shadow queue for tracking the buffer in the
driver. When we receive a ENCODE_FRAME response from the firmware, get
the buffer from the shadow queue and finish the buffer.

Furthermore, it is necessary to finish the job straight after passing
the buffer to the firmware to allow the V4L2 framework to send further
buffers to the driver.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../staging/media/allegro-dvt/allegro-core.c  | 104 +++++++++++++++---
 1 file changed, 89 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 4f525920c194..80d3383b84f8 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -226,6 +226,10 @@ struct allegro_channel {
 	struct list_head buffers_reference;
 	struct list_head buffers_intermediate;
 
+	struct list_head source_shadow_list;
+	struct list_head stream_shadow_list;
+	struct mutex shadow_list_lock;
+
 	struct list_head list;
 	struct completion completion;
 
@@ -247,6 +251,14 @@ allegro_get_state(struct allegro_channel *channel)
 	return channel->state;
 }
 
+struct allegro_m2m_buffer {
+	struct v4l2_m2m_buffer buf;
+	struct list_head head;
+};
+
+#define to_allegro_m2m_buffer(__buf) \
+	container_of(__buf, struct allegro_m2m_buffer, buf)
+
 struct fw_info {
 	unsigned int id;
 	unsigned int id_codec;
@@ -1570,6 +1582,43 @@ static void allegro_channel_buf_done(struct allegro_channel *channel,
 	v4l2_m2m_buf_done(buf, state);
 }
 
+static u64 allegro_put_buffer(struct allegro_channel *channel,
+			      struct list_head *list,
+			      struct vb2_v4l2_buffer *buffer)
+{
+	struct v4l2_m2m_buffer *b = container_of(buffer,
+						 struct v4l2_m2m_buffer, vb);
+	struct allegro_m2m_buffer *shadow = to_allegro_m2m_buffer(b);
+
+	mutex_lock(&channel->shadow_list_lock);
+	list_add_tail(&shadow->head, list);
+	mutex_unlock(&channel->shadow_list_lock);
+
+	return (u64) buffer;
+}
+
+static struct vb2_v4l2_buffer *allegro_get_buffer(struct allegro_channel *channel,
+						  struct list_head *list,
+						  u64 handle)
+{
+	struct allegro_dev *dev = channel->dev;
+	struct allegro_m2m_buffer *shadow;
+	u64 found;
+
+	mutex_lock(&channel->shadow_list_lock);
+	shadow = list_first_entry(list, struct allegro_m2m_buffer, head);
+	list_del_init(&shadow->head);
+	mutex_unlock(&channel->shadow_list_lock);
+
+	found = (u64) (&shadow->buf.vb);
+	if (handle != found)
+		v4l2_warn(&dev->v4l2_dev,
+			  "channel %d: output buffer mismatch 0x%llx, expected 0x%llx\n",
+			  channel->mcu_channel_id, handle, found);
+
+	return &shadow->buf.vb;
+}
+
 static void allegro_channel_finish_frame(struct allegro_channel *channel,
 		struct mcu_msg_encode_frame_response *msg)
 {
@@ -1585,13 +1634,17 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
 	ssize_t len;
 	ssize_t free;
 
-	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
-	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
+	src_buf = allegro_get_buffer(channel, &channel->source_shadow_list, msg->src_handle);
+	if (!src_buf)
+		v4l2_warn(&dev->v4l2_dev,
+			  "channel %d: invalid source buffer\n",
+			  channel->mcu_channel_id);
 
-	if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
-		v4l2_err(&dev->v4l2_dev,
-			 "channel %d: check failed\n",
-			 channel->mcu_channel_id);
+	dst_buf = allegro_get_buffer(channel, &channel->stream_shadow_list, msg->stream_id);
+	if (!dst_buf)
+		v4l2_warn(&dev->v4l2_dev,
+			  "channel %d: invalid stream buffer\n",
+			  channel->mcu_channel_id);
 
 	dst_buf->sequence = channel->csequence++;
 
@@ -1718,8 +1771,6 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
 	v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
 
 	allegro_channel_buf_done(channel, dst_buf, state);
-
-	v4l2_m2m_job_finish(dev->m2m_dev, channel->fh.m2m_ctx);
 }
 
 static int allegro_handle_init(struct allegro_dev *dev,
@@ -2312,16 +2363,31 @@ static void allegro_stop_streaming(struct vb2_queue *q)
 	struct allegro_channel *channel = vb2_get_drv_priv(q);
 	struct allegro_dev *dev = channel->dev;
 	struct vb2_v4l2_buffer *buffer;
+	struct allegro_m2m_buffer *shadow, *tmp;
 
 	v4l2_dbg(2, debug, &dev->v4l2_dev,
 		 "%s: stop streaming\n",
 		 V4L2_TYPE_IS_OUTPUT(q->type) ? "output" : "capture");
 
 	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
+		mutex_lock(&channel->shadow_list_lock);
+		list_for_each_entry_safe(shadow, tmp, &channel->source_shadow_list, head) {
+			list_del(&shadow->head);
+			v4l2_m2m_buf_done(&shadow->buf.vb, VB2_BUF_STATE_ERROR);
+		}
+		mutex_unlock(&channel->shadow_list_lock);
+
 		allegro_set_state(channel, ALLEGRO_STATE_STOPPED);
 		while ((buffer = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx)))
 			v4l2_m2m_buf_done(buffer, VB2_BUF_STATE_ERROR);
 	} else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+		mutex_lock(&channel->shadow_list_lock);
+		list_for_each_entry_safe(shadow, tmp, &channel->stream_shadow_list, head) {
+			list_del(&shadow->head);
+			v4l2_m2m_buf_done(&shadow->buf.vb, VB2_BUF_STATE_ERROR);
+		}
+		mutex_unlock(&channel->shadow_list_lock);
+
 		allegro_destroy_channel(channel);
 		while ((buffer = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx)))
 			v4l2_m2m_buf_done(buffer, VB2_BUF_STATE_ERROR);
@@ -2352,7 +2418,7 @@ static int allegro_queue_init(void *priv,
 	src_vq->drv_priv = channel;
 	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	src_vq->ops = &allegro_queue_ops;
-	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
+	src_vq->buf_struct_size = sizeof(struct allegro_m2m_buffer);
 	src_vq->lock = &channel->dev->lock;
 	err = vb2_queue_init(src_vq);
 	if (err)
@@ -2365,7 +2431,7 @@ static int allegro_queue_init(void *priv,
 	dst_vq->drv_priv = channel;
 	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	dst_vq->ops = &allegro_queue_ops;
-	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
+	dst_vq->buf_struct_size = sizeof(struct allegro_m2m_buffer);
 	dst_vq->lock = &channel->dev->lock;
 	err = vb2_queue_init(dst_vq);
 	if (err)
@@ -2457,6 +2523,9 @@ static int allegro_open(struct file *file)
 	v4l2_fh_add(&channel->fh);
 
 	init_completion(&channel->completion);
+	INIT_LIST_HEAD(&channel->source_shadow_list);
+	INIT_LIST_HEAD(&channel->stream_shadow_list);
+	mutex_init(&channel->shadow_list_lock);
 
 	channel->dev = dev;
 
@@ -2957,18 +3026,23 @@ static void allegro_device_run(void *priv)
 	dma_addr_t src_uv;
 	dma_addr_t dst_addr;
 	unsigned long dst_size;
+	u64 src_handle;
+	u64 dst_handle;
 
-	dst_buf = v4l2_m2m_next_dst_buf(channel->fh.m2m_ctx);
+	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
 	dst_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
 	dst_size = vb2_plane_size(&dst_buf->vb2_buf, 0);
-	allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size, (u64)dst_buf);
+	dst_handle = allegro_put_buffer(channel, &channel->stream_shadow_list, dst_buf);
+	allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size, dst_handle);
 
-	src_buf = v4l2_m2m_next_src_buf(channel->fh.m2m_ctx);
+	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
 	src_buf->sequence = channel->osequence++;
-
 	src_y = vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
 	src_uv = src_y + (channel->stride * channel->height);
-	allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv, (u64)src_buf);
+	src_handle = allegro_put_buffer(channel, &channel->source_shadow_list, src_buf);
+	allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv, src_handle);
+
+	v4l2_m2m_job_finish(dev->m2m_dev, channel->fh.m2m_ctx);
 }
 
 static const struct v4l2_m2m_ops allegro_m2m_ops = {
-- 
2.20.1

[PATCH 17/18] media: allegro: move mail definitions to separate file

[Michael Tretter]

Move the mail definitions from the driver core to a dedicated file.

The mails that are exchanged between driver and firmware are not stable
across firmware versions. This is in preparation to make the driver able
to handle multiple firmware version by having dedicated code for
handling mails.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/staging/media/allegro-dvt/Makefile    |   2 +-
 .../staging/media/allegro-dvt/allegro-core.c  | 274 +-----------------
 .../staging/media/allegro-dvt/allegro-mail.c  |  37 +++
 .../staging/media/allegro-dvt/allegro-mail.h  | 263 +++++++++++++++++
 4 files changed, 302 insertions(+), 274 deletions(-)
 create mode 100644 drivers/staging/media/allegro-dvt/allegro-mail.c
 create mode 100644 drivers/staging/media/allegro-dvt/allegro-mail.h

diff --git a/drivers/staging/media/allegro-dvt/Makefile b/drivers/staging/media/allegro-dvt/Makefile
index 80817160815c..8e306dcdc55c 100644
--- a/drivers/staging/media/allegro-dvt/Makefile
+++ b/drivers/staging/media/allegro-dvt/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 
-allegro-objs := allegro-core.o nal-h264.o
+allegro-objs := allegro-core.o nal-h264.o allegro-mail.o
 
 obj-$(CONFIG_VIDEO_ALLEGRO_DVT) += allegro.o
diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 80d3383b84f8..e56256d1cdb3 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -28,6 +28,7 @@
 #include <media/videobuf2-dma-contig.h>
 #include <media/videobuf2-v4l2.h>
 
+#include "allegro-mail.h"
 #include "nal-h264.h"
 
 /*
@@ -281,279 +282,6 @@ static const struct fw_info supported_firmware[] = {
 	},
 };
 
-enum mcu_msg_type {
-	MCU_MSG_TYPE_INIT = 0x0000,
-	MCU_MSG_TYPE_CREATE_CHANNEL = 0x0005,
-	MCU_MSG_TYPE_DESTROY_CHANNEL = 0x0006,
-	MCU_MSG_TYPE_ENCODE_FRAME = 0x0007,
-	MCU_MSG_TYPE_PUT_STREAM_BUFFER = 0x0012,
-	MCU_MSG_TYPE_PUSH_BUFFER_INTERMEDIATE = 0x000e,
-	MCU_MSG_TYPE_PUSH_BUFFER_REFERENCE = 0x000f,
-};
-
-static const char *msg_type_name(enum mcu_msg_type type)
-{
-	static char buf[9];
-
-	switch (type) {
-	case MCU_MSG_TYPE_INIT:
-		return "INIT";
-	case MCU_MSG_TYPE_CREATE_CHANNEL:
-		return "CREATE_CHANNEL";
-	case MCU_MSG_TYPE_DESTROY_CHANNEL:
-		return "DESTROY_CHANNEL";
-	case MCU_MSG_TYPE_ENCODE_FRAME:
-		return "ENCODE_FRAME";
-	case MCU_MSG_TYPE_PUT_STREAM_BUFFER:
-		return "PUT_STREAM_BUFFER";
-	case MCU_MSG_TYPE_PUSH_BUFFER_INTERMEDIATE:
-		return "PUSH_BUFFER_INTERMEDIATE";
-	case MCU_MSG_TYPE_PUSH_BUFFER_REFERENCE:
-		return "PUSH_BUFFER_REFERENCE";
-	default:
-		snprintf(buf, sizeof(buf), "(0x%04x)", type);
-		return buf;
-	}
-}
-
-struct mcu_msg_header {
-	u16 length;		/* length of the body in bytes */
-	u16 type;
-} __attribute__ ((__packed__));
-
-struct mcu_msg_init_request {
-	struct mcu_msg_header header;
-	u32 reserved0;		/* maybe a unused channel id */
-	u32 suballoc_dma;
-	u32 suballoc_size;
-	s32 l2_cache[3];
-} __attribute__ ((__packed__));
-
-struct mcu_msg_init_response {
-	struct mcu_msg_header header;
-	u32 reserved0;
-} __attribute__ ((__packed__));
-
-struct mcu_msg_create_channel {
-	struct mcu_msg_header header;
-	u32 user_id;
-	u16 width;
-	u16 height;
-	u32 format;
-	u32 colorspace;
-	u32 src_mode;
-	u8 profile;
-	u16 constraint_set_flags;
-	s8 codec;
-	u16 level;
-	u16 tier;
-	u32 sps_param;
-	u32 pps_param;
-
-	u32 enc_option;
-#define AL_OPT_WPP			BIT(0)
-#define AL_OPT_TILE			BIT(1)
-#define AL_OPT_LF			BIT(2)
-#define AL_OPT_LF_X_SLICE		BIT(3)
-#define AL_OPT_LF_X_TILE		BIT(4)
-#define AL_OPT_SCL_LST			BIT(5)
-#define AL_OPT_CONST_INTRA_PRED		BIT(6)
-#define AL_OPT_QP_TAB_RELATIVE		BIT(7)
-#define AL_OPT_FIX_PREDICTOR		BIT(8)
-#define AL_OPT_CUSTOM_LDA		BIT(9)
-#define AL_OPT_ENABLE_AUTO_QP		BIT(10)
-#define AL_OPT_ADAPT_AUTO_QP		BIT(11)
-#define AL_OPT_TRANSFO_SKIP		BIT(13)
-#define AL_OPT_FORCE_REC		BIT(15)
-#define AL_OPT_FORCE_MV_OUT		BIT(16)
-#define AL_OPT_FORCE_MV_CLIP		BIT(17)
-#define AL_OPT_LOWLAT_SYNC		BIT(18)
-#define AL_OPT_LOWLAT_INT		BIT(19)
-#define AL_OPT_RDO_COST_MODE		BIT(20)
-
-	s8 beta_offset;
-	s8 tc_offset;
-	u16 reserved10;
-	u32 unknown11;
-	u32 unknown12;
-	u16 num_slices;
-	u16 prefetch_auto;
-	u32 prefetch_mem_offset;
-	u32 prefetch_mem_size;
-	u16 clip_hrz_range;
-	u16 clip_vrt_range;
-	u16 me_range[4];
-	u8 max_cu_size;
-	u8 min_cu_size;
-	u8 max_tu_size;
-	u8 min_tu_size;
-	u8 max_transfo_depth_inter;
-	u8 max_transfo_depth_intra;
-	u16 reserved20;
-	u32 entropy_mode;
-	u32 wp_mode;
-
-	/* rate control param */
-	u32 rate_control_mode;
-	u32 initial_rem_delay;
-	u32 cpb_size;
-	u16 framerate;
-	u16 clk_ratio;
-	u32 target_bitrate;
-	u32 max_bitrate;
-	u16 initial_qp;
-	u16 min_qp;
-	u16 max_qp;
-	s16 ip_delta;
-	s16 pb_delta;
-	u16 golden_ref;
-	u16 golden_delta;
-	u16 golden_ref_frequency;
-	u32 rate_control_option;
-
-	/* gop param */
-	u32 gop_ctrl_mode;
-	u32 freq_idr;
-	u32 freq_lt;
-	u32 gdr_mode;
-	u16 gop_length;
-	u8 num_b;
-	u8 freq_golden_ref;
-
-	u32 subframe_latency;
-	u32 lda_control_mode;
-	u32 unknown41;
-} __attribute__ ((__packed__));
-
-struct mcu_msg_create_channel_response {
-	struct mcu_msg_header header;
-	u32 channel_id;
-	u32 user_id;
-	u32 options;
-	u32 num_core;
-	u32 pps_param;
-	u32 int_buffers_count;
-	u32 int_buffers_size;
-	u32 rec_buffers_count;
-	u32 rec_buffers_size;
-	u32 reserved;
-	u32 error_code;
-} __attribute__ ((__packed__));
-
-struct mcu_msg_destroy_channel {
-	struct mcu_msg_header header;
-	u32 channel_id;
-} __attribute__ ((__packed__));
-
-struct mcu_msg_destroy_channel_response {
-	struct mcu_msg_header header;
-	u32 channel_id;
-} __attribute__ ((__packed__));
-
-struct mcu_msg_push_buffers_internal_buffer {
-	u32 dma_addr;
-	u32 mcu_addr;
-	u32 size;
-} __attribute__ ((__packed__));
-
-struct mcu_msg_push_buffers_internal {
-	struct mcu_msg_header header;
-	u32 channel_id;
-	struct mcu_msg_push_buffers_internal_buffer buffer[0];
-} __attribute__ ((__packed__));
-
-struct mcu_msg_put_stream_buffer {
-	struct mcu_msg_header header;
-	u32 channel_id;
-	u32 dma_addr;
-	u32 mcu_addr;
-	u32 size;
-	u32 offset;
-	u64 stream_id;
-} __attribute__ ((__packed__));
-
-struct mcu_msg_encode_frame {
-	struct mcu_msg_header header;
-	u32 channel_id;
-	u32 reserved;
-
-	u32 encoding_options;
-#define AL_OPT_USE_QP_TABLE		BIT(0)
-#define AL_OPT_FORCE_LOAD		BIT(1)
-#define AL_OPT_USE_L2			BIT(2)
-#define AL_OPT_DISABLE_INTRA		BIT(3)
-#define AL_OPT_DEPENDENT_SLICES		BIT(4)
-
-	s16 pps_qp;
-	u16 padding;
-	u64 user_param;
-	u64 src_handle;
-
-	u32 request_options;
-#define AL_OPT_SCENE_CHANGE		BIT(0)
-#define AL_OPT_RESTART_GOP		BIT(1)
-#define AL_OPT_USE_LONG_TERM		BIT(2)
-#define AL_OPT_UPDATE_PARAMS		BIT(3)
-
-	/* u32 scene_change_delay (optional) */
-	/* rate control param (optional) */
-	/* gop param (optional) */
-	u32 src_y;
-	u32 src_uv;
-	u32 stride;
-	u32 ep2;
-	u64 ep2_v;
-} __attribute__ ((__packed__));
-
-struct mcu_msg_encode_frame_response {
-	struct mcu_msg_header header;
-	u32 channel_id;
-	u64 stream_id;		/* see mcu_msg_put_stream_buffer */
-	u64 user_param;		/* see mcu_msg_encode_frame */
-	u64 src_handle;		/* see mcu_msg_encode_frame */
-	u16 skip;
-	u16 is_ref;
-	u32 initial_removal_delay;
-	u32 dpb_output_delay;
-	u32 size;
-	u32 frame_tag_size;
-	s32 stuffing;
-	s32 filler;
-	u16 num_column;
-	u16 num_row;
-	u16 qp;
-	u8 num_ref_idx_l0;
-	u8 num_ref_idx_l1;
-	u32 partition_table_offset;
-	s32 partition_table_size;
-	u32 sum_complex;
-	s32 tile_width[4];
-	s32 tile_height[22];
-	u32 error_code;
-
-	u32 slice_type;
-#define AL_ENC_SLICE_TYPE_B             0
-#define AL_ENC_SLICE_TYPE_P             1
-#define AL_ENC_SLICE_TYPE_I             2
-
-	u32 pic_struct;
-	u8 is_idr;
-	u8 is_first_slice;
-	u8 is_last_slice;
-	u8 reserved;
-	u16 pps_qp;
-	u16 reserved1;
-	u32 reserved2;
-} __attribute__ ((__packed__));
-
-union mcu_msg_response {
-	struct mcu_msg_header header;
-	struct mcu_msg_init_response init;
-	struct mcu_msg_create_channel_response create_channel;
-	struct mcu_msg_destroy_channel_response destroy_channel;
-	struct mcu_msg_encode_frame_response encode_frame;
-};
-
 static inline u32 to_mcu_addr(struct allegro_dev *dev, dma_addr_t phys)
 {
 	if (upper_32_bits(phys) || (lower_32_bits(phys) & MCU_CACHE_OFFSET))
diff --git a/drivers/staging/media/allegro-dvt/allegro-mail.c b/drivers/staging/media/allegro-dvt/allegro-mail.c
new file mode 100644
index 000000000000..df0d8d26a6fb
--- /dev/null
+++ b/drivers/staging/media/allegro-dvt/allegro-mail.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Pengutronix, Michael Tretter <kernel@pengutronix.de>
+ *
+ * Helper functions for handling messages that are send via mailbox to the
+ * Allegro VCU firmware.
+ */
+
+#include <linux/export.h>
+
+#include "allegro-mail.h"
+
+const char *msg_type_name(enum mcu_msg_type type)
+{
+	static char buf[9];
+
+	switch (type) {
+	case MCU_MSG_TYPE_INIT:
+		return "INIT";
+	case MCU_MSG_TYPE_CREATE_CHANNEL:
+		return "CREATE_CHANNEL";
+	case MCU_MSG_TYPE_DESTROY_CHANNEL:
+		return "DESTROY_CHANNEL";
+	case MCU_MSG_TYPE_ENCODE_FRAME:
+		return "ENCODE_FRAME";
+	case MCU_MSG_TYPE_PUT_STREAM_BUFFER:
+		return "PUT_STREAM_BUFFER";
+	case MCU_MSG_TYPE_PUSH_BUFFER_INTERMEDIATE:
+		return "PUSH_BUFFER_INTERMEDIATE";
+	case MCU_MSG_TYPE_PUSH_BUFFER_REFERENCE:
+		return "PUSH_BUFFER_REFERENCE";
+	default:
+		snprintf(buf, sizeof(buf), "(0x%04x)", type);
+		return buf;
+	}
+}
+EXPORT_SYMBOL(msg_type_name);
diff --git a/drivers/staging/media/allegro-dvt/allegro-mail.h b/drivers/staging/media/allegro-dvt/allegro-mail.h
new file mode 100644
index 000000000000..d9050d6b5c97
--- /dev/null
+++ b/drivers/staging/media/allegro-dvt/allegro-mail.h
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Pengutronix, Michael Tretter <kernel@pengutronix.de>
+ *
+ * Allegro VCU firmware mailbox mail definitions
+ */
+
+#ifndef ALLEGRO_MAIL_H
+#define ALLEGRO_MAIL_H
+
+#include <linux/kernel.h>
+
+enum mcu_msg_type {
+	MCU_MSG_TYPE_INIT = 0x0000,
+	MCU_MSG_TYPE_CREATE_CHANNEL = 0x0005,
+	MCU_MSG_TYPE_DESTROY_CHANNEL = 0x0006,
+	MCU_MSG_TYPE_ENCODE_FRAME = 0x0007,
+	MCU_MSG_TYPE_PUT_STREAM_BUFFER = 0x0012,
+	MCU_MSG_TYPE_PUSH_BUFFER_INTERMEDIATE = 0x000e,
+	MCU_MSG_TYPE_PUSH_BUFFER_REFERENCE = 0x000f,
+};
+
+const char *msg_type_name(enum mcu_msg_type type);
+
+struct mcu_msg_header {
+	u16 length;		/* length of the body in bytes */
+	u16 type;
+} __attribute__ ((__packed__));
+
+struct mcu_msg_init_request {
+	struct mcu_msg_header header;
+	u32 reserved0;		/* maybe a unused channel id */
+	u32 suballoc_dma;
+	u32 suballoc_size;
+	s32 l2_cache[3];
+} __attribute__ ((__packed__));
+
+struct mcu_msg_init_response {
+	struct mcu_msg_header header;
+	u32 reserved0;
+} __attribute__ ((__packed__));
+
+struct mcu_msg_create_channel {
+	struct mcu_msg_header header;
+	u32 user_id;
+	u16 width;
+	u16 height;
+	u32 format;
+	u32 colorspace;
+	u32 src_mode;
+	u8 profile;
+	u16 constraint_set_flags;
+	s8 codec;
+	u16 level;
+	u16 tier;
+	u32 sps_param;
+	u32 pps_param;
+
+	u32 enc_option;
+#define AL_OPT_WPP			BIT(0)
+#define AL_OPT_TILE			BIT(1)
+#define AL_OPT_LF			BIT(2)
+#define AL_OPT_LF_X_SLICE		BIT(3)
+#define AL_OPT_LF_X_TILE		BIT(4)
+#define AL_OPT_SCL_LST			BIT(5)
+#define AL_OPT_CONST_INTRA_PRED		BIT(6)
+#define AL_OPT_QP_TAB_RELATIVE		BIT(7)
+#define AL_OPT_FIX_PREDICTOR		BIT(8)
+#define AL_OPT_CUSTOM_LDA		BIT(9)
+#define AL_OPT_ENABLE_AUTO_QP		BIT(10)
+#define AL_OPT_ADAPT_AUTO_QP		BIT(11)
+#define AL_OPT_TRANSFO_SKIP		BIT(13)
+#define AL_OPT_FORCE_REC		BIT(15)
+#define AL_OPT_FORCE_MV_OUT		BIT(16)
+#define AL_OPT_FORCE_MV_CLIP		BIT(17)
+#define AL_OPT_LOWLAT_SYNC		BIT(18)
+#define AL_OPT_LOWLAT_INT		BIT(19)
+#define AL_OPT_RDO_COST_MODE		BIT(20)
+
+	s8 beta_offset;
+	s8 tc_offset;
+	u16 reserved10;
+	u32 unknown11;
+	u32 unknown12;
+	u16 num_slices;
+	u16 prefetch_auto;
+	u32 prefetch_mem_offset;
+	u32 prefetch_mem_size;
+	u16 clip_hrz_range;
+	u16 clip_vrt_range;
+	u16 me_range[4];
+	u8 max_cu_size;
+	u8 min_cu_size;
+	u8 max_tu_size;
+	u8 min_tu_size;
+	u8 max_transfo_depth_inter;
+	u8 max_transfo_depth_intra;
+	u16 reserved20;
+	u32 entropy_mode;
+	u32 wp_mode;
+
+	/* rate control param */
+	u32 rate_control_mode;
+	u32 initial_rem_delay;
+	u32 cpb_size;
+	u16 framerate;
+	u16 clk_ratio;
+	u32 target_bitrate;
+	u32 max_bitrate;
+	u16 initial_qp;
+	u16 min_qp;
+	u16 max_qp;
+	s16 ip_delta;
+	s16 pb_delta;
+	u16 golden_ref;
+	u16 golden_delta;
+	u16 golden_ref_frequency;
+	u32 rate_control_option;
+
+	/* gop param */
+	u32 gop_ctrl_mode;
+	u32 freq_idr;
+	u32 freq_lt;
+	u32 gdr_mode;
+	u16 gop_length;
+	u8 num_b;
+	u8 freq_golden_ref;
+
+	u32 subframe_latency;
+	u32 lda_control_mode;
+	u32 unknown41;
+} __attribute__ ((__packed__));
+
+struct mcu_msg_create_channel_response {
+	struct mcu_msg_header header;
+	u32 channel_id;
+	u32 user_id;
+	u32 options;
+	u32 num_core;
+	u32 pps_param;
+	u32 int_buffers_count;
+	u32 int_buffers_size;
+	u32 rec_buffers_count;
+	u32 rec_buffers_size;
+	u32 reserved;
+	u32 error_code;
+} __attribute__ ((__packed__));
+
+struct mcu_msg_destroy_channel {
+	struct mcu_msg_header header;
+	u32 channel_id;
+} __attribute__ ((__packed__));
+
+struct mcu_msg_destroy_channel_response {
+	struct mcu_msg_header header;
+	u32 channel_id;
+} __attribute__ ((__packed__));
+
+struct mcu_msg_push_buffers_internal_buffer {
+	u32 dma_addr;
+	u32 mcu_addr;
+	u32 size;
+} __attribute__ ((__packed__));
+
+struct mcu_msg_push_buffers_internal {
+	struct mcu_msg_header header;
+	u32 channel_id;
+	struct mcu_msg_push_buffers_internal_buffer buffer[0];
+} __attribute__ ((__packed__));
+
+struct mcu_msg_put_stream_buffer {
+	struct mcu_msg_header header;
+	u32 channel_id;
+	u32 dma_addr;
+	u32 mcu_addr;
+	u32 size;
+	u32 offset;
+	u64 stream_id;
+} __attribute__ ((__packed__));
+
+struct mcu_msg_encode_frame {
+	struct mcu_msg_header header;
+	u32 channel_id;
+	u32 reserved;
+
+	u32 encoding_options;
+#define AL_OPT_USE_QP_TABLE		BIT(0)
+#define AL_OPT_FORCE_LOAD		BIT(1)
+#define AL_OPT_USE_L2			BIT(2)
+#define AL_OPT_DISABLE_INTRA		BIT(3)
+#define AL_OPT_DEPENDENT_SLICES		BIT(4)
+
+	s16 pps_qp;
+	u16 padding;
+	u64 user_param;
+	u64 src_handle;
+
+	u32 request_options;
+#define AL_OPT_SCENE_CHANGE		BIT(0)
+#define AL_OPT_RESTART_GOP		BIT(1)
+#define AL_OPT_USE_LONG_TERM		BIT(2)
+#define AL_OPT_UPDATE_PARAMS		BIT(3)
+
+	/* u32 scene_change_delay (optional) */
+	/* rate control param (optional) */
+	/* gop param (optional) */
+	u32 src_y;
+	u32 src_uv;
+	u32 stride;
+	u32 ep2;
+	u64 ep2_v;
+} __attribute__ ((__packed__));
+
+struct mcu_msg_encode_frame_response {
+	struct mcu_msg_header header;
+	u32 channel_id;
+	u64 stream_id;		/* see mcu_msg_put_stream_buffer */
+	u64 user_param;		/* see mcu_msg_encode_frame */
+	u64 src_handle;		/* see mcu_msg_encode_frame */
+	u16 skip;
+	u16 is_ref;
+	u32 initial_removal_delay;
+	u32 dpb_output_delay;
+	u32 size;
+	u32 frame_tag_size;
+	s32 stuffing;
+	s32 filler;
+	u16 num_column;
+	u16 num_row;
+	u16 qp;
+	u8 num_ref_idx_l0;
+	u8 num_ref_idx_l1;
+	u32 partition_table_offset;
+	s32 partition_table_size;
+	u32 sum_complex;
+	s32 tile_width[4];
+	s32 tile_height[22];
+	u32 error_code;
+
+	u32 slice_type;
+#define AL_ENC_SLICE_TYPE_B             0
+#define AL_ENC_SLICE_TYPE_P             1
+#define AL_ENC_SLICE_TYPE_I             2
+
+	u32 pic_struct;
+	u8 is_idr;
+	u8 is_first_slice;
+	u8 is_last_slice;
+	u8 reserved;
+	u16 pps_qp;
+	u16 reserved1;
+	u32 reserved2;
+} __attribute__ ((__packed__));
+
+union mcu_msg_response {
+	struct mcu_msg_header header;
+	struct mcu_msg_init_response init;
+	struct mcu_msg_create_channel_response create_channel;
+	struct mcu_msg_destroy_channel_response destroy_channel;
+	struct mcu_msg_encode_frame_response encode_frame;
+};
+
+#endif
-- 
2.20.1

[PATCH 18/18] media: allegro: create new struct for channel parameters

[Michael Tretter]

Add a new struct for the channel parameters that is contained in the
CREATE_CHANNEL message. This is in preparation for newer firmwares that
pass the channel parameters in a dedicated buffer instead of embedding
the parameters into the CREATE_CHANNEL message.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../staging/media/allegro-dvt/allegro-core.c  | 139 ++++++++++--------
 .../staging/media/allegro-dvt/allegro-mail.h  |  10 +-
 2 files changed, 83 insertions(+), 66 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index e56256d1cdb3..a7aa85ba5391 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -835,81 +835,94 @@ static s16 get_qp_delta(int minuend, int subtrahend)
 		return minuend - subtrahend;
 }
 
-static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
-					   struct allegro_channel *channel)
-{
-	struct mcu_msg_create_channel msg;
-
-	memset(&msg, 0, sizeof(msg));
-
-	msg.header.type = MCU_MSG_TYPE_CREATE_CHANNEL;
-	msg.header.length = sizeof(msg) - sizeof(msg.header);
-
-	msg.user_id = channel->user_id;
-	msg.width = channel->width;
-	msg.height = channel->height;
-	msg.format = v4l2_pixelformat_to_mcu_format(channel->pixelformat);
-	msg.colorspace = v4l2_colorspace_to_mcu_colorspace(channel->colorspace);
-	msg.src_mode = 0x0;
-	msg.profile = v4l2_profile_to_mcu_profile(channel->profile);
-	msg.constraint_set_flags = BIT(1);
-	msg.codec = v4l2_pixelformat_to_mcu_codec(channel->codec);
-	msg.level = v4l2_level_to_mcu_level(channel->level);
-	msg.tier = 0;
-	msg.sps_param = BIT(20) | 0x4a;
-	msg.pps_param = BIT(2);
-	msg.enc_option = AL_OPT_RDO_COST_MODE | AL_OPT_LF_X_TILE |
-			 AL_OPT_LF_X_SLICE | AL_OPT_LF;
-	msg.beta_offset = -1;
-	msg.tc_offset = -1;
-	msg.num_slices = 1;
-	msg.me_range[0] = 8;
-	msg.me_range[1] = 8;
-	msg.me_range[2] = 16;
-	msg.me_range[3] = 16;
-	msg.max_cu_size = ilog2(SIZE_MACROBLOCK);
-	msg.min_cu_size = ilog2(8);
-	msg.max_tu_size = 2;
-	msg.min_tu_size = 2;
-	msg.max_transfo_depth_intra = 1;
-	msg.max_transfo_depth_inter = 1;
+static int fill_create_channel_param(struct allegro_channel *channel,
+				     struct create_channel_param *param)
+{
+	param->width = channel->width;
+	param->height = channel->height;
+	param->format = v4l2_pixelformat_to_mcu_format(channel->pixelformat);
+	param->colorspace = v4l2_colorspace_to_mcu_colorspace(channel->colorspace);
+	param->src_mode = 0x0;
+	param->profile = v4l2_profile_to_mcu_profile(channel->profile);
+	param->constraint_set_flags = BIT(1);
+	param->codec = v4l2_pixelformat_to_mcu_codec(channel->codec);
+	param->level = v4l2_level_to_mcu_level(channel->level);
+	param->tier = 0;
+	param->sps_param = BIT(20) | 0x4a;
+	param->pps_param = BIT(2);
+	param->enc_option = AL_OPT_RDO_COST_MODE | AL_OPT_LF_X_TILE |
+		AL_OPT_LF_X_SLICE | AL_OPT_LF;
+	param->beta_offset = -1;
+	param->tc_offset = -1;
+	param->num_slices = 1;
+	param->me_range[0] = 8;
+	param->me_range[1] = 8;
+	param->me_range[2] = 16;
+	param->me_range[3] = 16;
+	param->max_cu_size = ilog2(SIZE_MACROBLOCK);
+	param->min_cu_size = ilog2(8);
+	param->max_tu_size = 2;
+	param->min_tu_size = 2;
+	param->max_transfo_depth_intra = 1;
+	param->max_transfo_depth_inter = 1;
+
+	param->prefetch_auto = 0;
+	param->prefetch_mem_offset = 0;
+	param->prefetch_mem_size = 0;
 
 	if (channel->frame_rc_enable)
-		msg.rate_control_mode =
+		param->rate_control_mode =
 			v4l2_bitrate_mode_to_mcu_mode(v4l2_ctrl_g_ctrl(channel->mpeg_video_bitrate_mode));
 	else
-		msg.rate_control_mode = 0;
+		param->rate_control_mode = 0;
 
 	/* Encoder expects cpb_size in units of a 90 kHz clock. */
-	msg.cpb_size =
+	param->cpb_size =
 		(channel->cpb_size * 90000) / (channel->bitrate_peak / 1000 / BITS_PER_BYTE);
 	/* Shall be ]0;cpb_size in 90 kHz units]. Use maximum value. */
-	msg.initial_rem_delay = msg.cpb_size;
-	msg.framerate = DIV_ROUND_UP(channel->framerate.numerator,
-				     channel->framerate.denominator);
-	msg.clk_ratio = channel->framerate.denominator == 1001 ? 1001 : 1000;
-	msg.target_bitrate = channel->bitrate;
-	msg.max_bitrate = channel->bitrate_peak;
-	msg.initial_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_i_frame_qp);
-	msg.min_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_min_qp);
-	msg.max_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_max_qp);
-	msg.ip_delta = get_qp_delta(
+	param->initial_rem_delay = param->cpb_size;
+	param->framerate = DIV_ROUND_UP(channel->framerate.numerator,
+					channel->framerate.denominator);
+	param->clk_ratio = channel->framerate.denominator == 1001 ? 1001 : 1000;
+	param->target_bitrate = channel->bitrate;
+	param->max_bitrate = channel->bitrate_peak;
+	param->initial_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_i_frame_qp);
+	param->min_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_min_qp);
+	param->max_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_max_qp);
+	param->ip_delta = get_qp_delta(
 			v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_i_frame_qp),
 			v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_p_frame_qp));
-	msg.pb_delta = get_qp_delta(
+	param->pb_delta = get_qp_delta(
 			v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_p_frame_qp),
 			v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_b_frame_qp));
-	msg.golden_ref = 0;
-	msg.golden_delta = 2;
-	msg.golden_ref_frequency = 10;
-	msg.rate_control_option = 0x00000000;
-
-	msg.gop_ctrl_mode = 0x00000000;
-	msg.freq_idr = channel->gop_size;
-	msg.freq_lt = 0;
-	msg.gdr_mode = 0x00000000;
-	msg.gop_length = channel->gop_size;
-	msg.subframe_latency = 0x00000000;
+	param->golden_ref = 0;
+	param->golden_delta = 2;
+	param->golden_ref_frequency = 10;
+	param->rate_control_option = 0x00000000;
+
+	param->gop_ctrl_mode = 0x00000000;
+	param->freq_idr = channel->gop_size;
+	param->freq_lt = 0;
+	param->gdr_mode = 0x00000000;
+	param->gop_length = channel->gop_size;
+	param->subframe_latency = 0x00000000;
+
+	return 0;
+}
+
+static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
+					   struct allegro_channel *channel)
+{
+	struct mcu_msg_create_channel msg;
+
+	memset(&msg, 0, sizeof(msg));
+
+	msg.header.type = MCU_MSG_TYPE_CREATE_CHANNEL;
+	msg.header.length = sizeof(msg) - sizeof(msg.header);
+
+	msg.user_id = channel->user_id;
+
+	fill_create_channel_param(channel, &msg.param);
 
 	allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
 	allegro_mcu_interrupt(dev);
diff --git a/drivers/staging/media/allegro-dvt/allegro-mail.h b/drivers/staging/media/allegro-dvt/allegro-mail.h
index d9050d6b5c97..1a80320db4da 100644
--- a/drivers/staging/media/allegro-dvt/allegro-mail.h
+++ b/drivers/staging/media/allegro-dvt/allegro-mail.h
@@ -40,9 +40,7 @@ struct mcu_msg_init_response {
 	u32 reserved0;
 } __attribute__ ((__packed__));
 
-struct mcu_msg_create_channel {
-	struct mcu_msg_header header;
-	u32 user_id;
+struct create_channel_param {
 	u16 width;
 	u16 height;
 	u32 format;
@@ -131,6 +129,12 @@ struct mcu_msg_create_channel {
 	u32 unknown41;
 } __attribute__ ((__packed__));
 
+struct mcu_msg_create_channel {
+	struct mcu_msg_header header;
+	u32 user_id;
+	struct create_channel_param param;
+} __attribute__ ((__packed__));
+
 struct mcu_msg_create_channel_response {
 	struct mcu_msg_header header;
 	u32 channel_id;
-- 
2.20.1

Re: [PATCH 15/18] media: allegro: verify source and destination buffer in VCU response

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7855 bytes --]

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.6-rc2 next-20200219]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Michael-Tretter/media-allegro-fixes-and-new-features/20200219-202330
base:   git://linuxtv.org/media_tree.git master
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/staging/media/allegro-dvt/allegro-core.c: In function 'allegro_channel_finish_frame':
>> drivers/staging/media/allegro-dvt/allegro-core.c:1591:6: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
         ^
   drivers/staging/media/allegro-dvt/allegro-core.c:1591:41: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
                                            ^
   drivers/staging/media/allegro-dvt/allegro-core.c: In function 'allegro_device_run':
   drivers/staging/media/allegro-dvt/allegro-core.c:2964:71: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size, (u64)dst_buf);
                                                                          ^
   drivers/staging/media/allegro-dvt/allegro-core.c:2971:61: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv, (u64)src_buf);
                                                                ^

vim +1591 drivers/staging/media/allegro-dvt/allegro-core.c

  1572	
  1573	static void allegro_channel_finish_frame(struct allegro_channel *channel,
  1574			struct mcu_msg_encode_frame_response *msg)
  1575	{
  1576		struct allegro_dev *dev = channel->dev;
  1577		struct vb2_v4l2_buffer *src_buf;
  1578		struct vb2_v4l2_buffer *dst_buf;
  1579		struct {
  1580			u32 offset;
  1581			u32 size;
  1582		} *partition;
  1583		enum vb2_buffer_state state = VB2_BUF_STATE_ERROR;
  1584		char *curr;
  1585		ssize_t len;
  1586		ssize_t free;
  1587	
  1588		src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
  1589		dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
  1590	
> 1591		if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
  1592			v4l2_err(&dev->v4l2_dev,
  1593				 "channel %d: check failed\n",
  1594				 channel->mcu_channel_id);
  1595	
  1596		dst_buf->sequence = channel->csequence++;
  1597	
  1598		if (msg->error_code & AL_ERROR) {
  1599			v4l2_err(&dev->v4l2_dev,
  1600				 "channel %d: failed to encode frame: %s (%x)\n",
  1601				 channel->mcu_channel_id,
  1602				 allegro_err_to_string(msg->error_code),
  1603				 msg->error_code);
  1604			goto err;
  1605		}
  1606	
  1607		if (msg->partition_table_size != 1) {
  1608			v4l2_warn(&dev->v4l2_dev,
  1609				  "channel %d: only handling first partition table entry (%d entries)\n",
  1610				  channel->mcu_channel_id, msg->partition_table_size);
  1611		}
  1612	
  1613		if (msg->partition_table_offset +
  1614		    msg->partition_table_size * sizeof(*partition) >
  1615		    vb2_plane_size(&dst_buf->vb2_buf, 0)) {
  1616			v4l2_err(&dev->v4l2_dev,
  1617				 "channel %d: partition table outside of dst_buf\n",
  1618				 channel->mcu_channel_id);
  1619			goto err;
  1620		}
  1621	
  1622		partition =
  1623		    vb2_plane_vaddr(&dst_buf->vb2_buf, 0) + msg->partition_table_offset;
  1624		if (partition->offset + partition->size >
  1625		    vb2_plane_size(&dst_buf->vb2_buf, 0)) {
  1626			v4l2_err(&dev->v4l2_dev,
  1627				 "channel %d: encoded frame is outside of dst_buf (offset 0x%x, size 0x%x)\n",
  1628				 channel->mcu_channel_id, partition->offset,
  1629				 partition->size);
  1630			goto err;
  1631		}
  1632	
  1633		v4l2_dbg(2, debug, &dev->v4l2_dev,
  1634			 "channel %d: encoded frame of size %d is at offset 0x%x\n",
  1635			 channel->mcu_channel_id, partition->size, partition->offset);
  1636	
  1637		/*
  1638		 * The payload must include the data before the partition offset,
  1639		 * because we will put the sps and pps data there.
  1640		 */
  1641		vb2_set_plane_payload(&dst_buf->vb2_buf, 0,
  1642				      partition->offset + partition->size);
  1643	
  1644		curr = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
  1645		free = partition->offset;
  1646		if (msg->is_idr) {
  1647			len = allegro_h264_write_sps(channel, curr, free);
  1648			if (len < 0) {
  1649				v4l2_err(&dev->v4l2_dev,
  1650					 "not enough space for sequence parameter set: %zd left\n",
  1651					 free);
  1652				goto err;
  1653			}
  1654			curr += len;
  1655			free -= len;
  1656			v4l2_dbg(1, debug, &dev->v4l2_dev,
  1657				 "channel %d: wrote %zd byte SPS nal unit\n",
  1658				 channel->mcu_channel_id, len);
  1659		}
  1660	
  1661		if (msg->slice_type == AL_ENC_SLICE_TYPE_I) {
  1662			len = allegro_h264_write_pps(channel, curr, free);
  1663			if (len < 0) {
  1664				v4l2_err(&dev->v4l2_dev,
  1665					 "not enough space for picture parameter set: %zd left\n",
  1666					 free);
  1667				goto err;
  1668			}
  1669			curr += len;
  1670			free -= len;
  1671			v4l2_dbg(1, debug, &dev->v4l2_dev,
  1672				 "channel %d: wrote %zd byte PPS nal unit\n",
  1673				 channel->mcu_channel_id, len);
  1674		}
  1675	
  1676		if (msg->slice_type != AL_ENC_SLICE_TYPE_I && !msg->is_idr) {
  1677			dst_buf->vb2_buf.planes[0].data_offset = free;
  1678			free = 0;
  1679		} else {
  1680			len = nal_h264_write_filler(&dev->plat_dev->dev, curr, free);
  1681			if (len < 0) {
  1682				v4l2_err(&dev->v4l2_dev,
  1683						"failed to write %zd filler data\n", free);
  1684				goto err;
  1685			}
  1686			curr += len;
  1687			free -= len;
  1688			v4l2_dbg(2, debug, &dev->v4l2_dev,
  1689					"channel %d: wrote %zd bytes filler nal unit\n",
  1690					channel->mcu_channel_id, len);
  1691		}
  1692	
  1693		if (free != 0) {
  1694			v4l2_err(&dev->v4l2_dev,
  1695				 "non-VCL NAL units do not fill space until VCL NAL unit: %zd bytes left\n",
  1696				 free);
  1697			goto err;
  1698		}
  1699	
  1700		state = VB2_BUF_STATE_DONE;
  1701	
  1702		v4l2_m2m_buf_copy_metadata(src_buf, dst_buf, false);
  1703		if (msg->is_idr)
  1704			dst_buf->flags |= V4L2_BUF_FLAG_KEYFRAME;
  1705		else
  1706			dst_buf->flags |= V4L2_BUF_FLAG_PFRAME;
  1707	
  1708		v4l2_dbg(1, debug, &dev->v4l2_dev,
  1709			 "channel %d: encoded frame #%03d (%s%s, QP %d, %d bytes)\n",
  1710			 channel->mcu_channel_id,
  1711			 dst_buf->sequence,
  1712			 msg->is_idr ? "IDR, " : "",
  1713			 msg->slice_type == AL_ENC_SLICE_TYPE_I ? "I slice" :
  1714			 msg->slice_type == AL_ENC_SLICE_TYPE_P ? "P slice" : "unknown",
  1715			 msg->qp, partition->size);
  1716	
  1717	err:
  1718		v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
  1719	
  1720		allegro_channel_buf_done(channel, dst_buf, state);
  1721	
  1722		v4l2_m2m_job_finish(dev->m2m_dev, channel->fh.m2m_ctx);
  1723	}
  1724	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51587 bytes --]

Re: [PATCH 15/18] media: allegro: verify source and destination buffer in VCU response

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8056 bytes --]

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.6-rc2 next-20200219]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Michael-Tretter/media-allegro-fixes-and-new-features/20200219-202330
base:   git://linuxtv.org/media_tree.git master
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/staging/media/allegro-dvt/allegro-core.c: In function 'allegro_channel_finish_frame':
>> drivers/staging/media/allegro-dvt/allegro-core.c:1591:6: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
         ^
   drivers/staging/media/allegro-dvt/allegro-core.c:1591:41: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
                                            ^
   drivers/staging/media/allegro-dvt/allegro-core.c: In function 'allegro_device_run':
   drivers/staging/media/allegro-dvt/allegro-core.c:2964:71: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size, (u64)dst_buf);
                                                                          ^
   drivers/staging/media/allegro-dvt/allegro-core.c:2971:61: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv, (u64)src_buf);
                                                                ^

vim +1591 drivers/staging/media/allegro-dvt/allegro-core.c

  1572	
  1573	static void allegro_channel_finish_frame(struct allegro_channel *channel,
  1574			struct mcu_msg_encode_frame_response *msg)
  1575	{
  1576		struct allegro_dev *dev = channel->dev;
  1577		struct vb2_v4l2_buffer *src_buf;
  1578		struct vb2_v4l2_buffer *dst_buf;
  1579		struct {
  1580			u32 offset;
  1581			u32 size;
  1582		} *partition;
  1583		enum vb2_buffer_state state = VB2_BUF_STATE_ERROR;
  1584		char *curr;
  1585		ssize_t len;
  1586		ssize_t free;
  1587	
  1588		src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
  1589		dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
  1590	
> 1591		if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
  1592			v4l2_err(&dev->v4l2_dev,
  1593				 "channel %d: check failed\n",
  1594				 channel->mcu_channel_id);
  1595	
  1596		dst_buf->sequence = channel->csequence++;
  1597	
  1598		if (msg->error_code & AL_ERROR) {
  1599			v4l2_err(&dev->v4l2_dev,
  1600				 "channel %d: failed to encode frame: %s (%x)\n",
  1601				 channel->mcu_channel_id,
  1602				 allegro_err_to_string(msg->error_code),
  1603				 msg->error_code);
  1604			goto err;
  1605		}
  1606	
  1607		if (msg->partition_table_size != 1) {
  1608			v4l2_warn(&dev->v4l2_dev,
  1609				  "channel %d: only handling first partition table entry (%d entries)\n",
  1610				  channel->mcu_channel_id, msg->partition_table_size);
  1611		}
  1612	
  1613		if (msg->partition_table_offset +
  1614		    msg->partition_table_size * sizeof(*partition) >
  1615		    vb2_plane_size(&dst_buf->vb2_buf, 0)) {
  1616			v4l2_err(&dev->v4l2_dev,
  1617				 "channel %d: partition table outside of dst_buf\n",
  1618				 channel->mcu_channel_id);
  1619			goto err;
  1620		}
  1621	
  1622		partition =
  1623		    vb2_plane_vaddr(&dst_buf->vb2_buf, 0) + msg->partition_table_offset;
  1624		if (partition->offset + partition->size >
  1625		    vb2_plane_size(&dst_buf->vb2_buf, 0)) {
  1626			v4l2_err(&dev->v4l2_dev,
  1627				 "channel %d: encoded frame is outside of dst_buf (offset 0x%x, size 0x%x)\n",
  1628				 channel->mcu_channel_id, partition->offset,
  1629				 partition->size);
  1630			goto err;
  1631		}
  1632	
  1633		v4l2_dbg(2, debug, &dev->v4l2_dev,
  1634			 "channel %d: encoded frame of size %d is at offset 0x%x\n",
  1635			 channel->mcu_channel_id, partition->size, partition->offset);
  1636	
  1637		/*
  1638		 * The payload must include the data before the partition offset,
  1639		 * because we will put the sps and pps data there.
  1640		 */
  1641		vb2_set_plane_payload(&dst_buf->vb2_buf, 0,
  1642				      partition->offset + partition->size);
  1643	
  1644		curr = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
  1645		free = partition->offset;
  1646		if (msg->is_idr) {
  1647			len = allegro_h264_write_sps(channel, curr, free);
  1648			if (len < 0) {
  1649				v4l2_err(&dev->v4l2_dev,
  1650					 "not enough space for sequence parameter set: %zd left\n",
  1651					 free);
  1652				goto err;
  1653			}
  1654			curr += len;
  1655			free -= len;
  1656			v4l2_dbg(1, debug, &dev->v4l2_dev,
  1657				 "channel %d: wrote %zd byte SPS nal unit\n",
  1658				 channel->mcu_channel_id, len);
  1659		}
  1660	
  1661		if (msg->slice_type == AL_ENC_SLICE_TYPE_I) {
  1662			len = allegro_h264_write_pps(channel, curr, free);
  1663			if (len < 0) {
  1664				v4l2_err(&dev->v4l2_dev,
  1665					 "not enough space for picture parameter set: %zd left\n",
  1666					 free);
  1667				goto err;
  1668			}
  1669			curr += len;
  1670			free -= len;
  1671			v4l2_dbg(1, debug, &dev->v4l2_dev,
  1672				 "channel %d: wrote %zd byte PPS nal unit\n",
  1673				 channel->mcu_channel_id, len);
  1674		}
  1675	
  1676		if (msg->slice_type != AL_ENC_SLICE_TYPE_I && !msg->is_idr) {
  1677			dst_buf->vb2_buf.planes[0].data_offset = free;
  1678			free = 0;
  1679		} else {
  1680			len = nal_h264_write_filler(&dev->plat_dev->dev, curr, free);
  1681			if (len < 0) {
  1682				v4l2_err(&dev->v4l2_dev,
  1683						"failed to write %zd filler data\n", free);
  1684				goto err;
  1685			}
  1686			curr += len;
  1687			free -= len;
  1688			v4l2_dbg(2, debug, &dev->v4l2_dev,
  1689					"channel %d: wrote %zd bytes filler nal unit\n",
  1690					channel->mcu_channel_id, len);
  1691		}
  1692	
  1693		if (free != 0) {
  1694			v4l2_err(&dev->v4l2_dev,
  1695				 "non-VCL NAL units do not fill space until VCL NAL unit: %zd bytes left\n",
  1696				 free);
  1697			goto err;
  1698		}
  1699	
  1700		state = VB2_BUF_STATE_DONE;
  1701	
  1702		v4l2_m2m_buf_copy_metadata(src_buf, dst_buf, false);
  1703		if (msg->is_idr)
  1704			dst_buf->flags |= V4L2_BUF_FLAG_KEYFRAME;
  1705		else
  1706			dst_buf->flags |= V4L2_BUF_FLAG_PFRAME;
  1707	
  1708		v4l2_dbg(1, debug, &dev->v4l2_dev,
  1709			 "channel %d: encoded frame #%03d (%s%s, QP %d, %d bytes)\n",
  1710			 channel->mcu_channel_id,
  1711			 dst_buf->sequence,
  1712			 msg->is_idr ? "IDR, " : "",
  1713			 msg->slice_type == AL_ENC_SLICE_TYPE_I ? "I slice" :
  1714			 msg->slice_type == AL_ENC_SLICE_TYPE_P ? "P slice" : "unknown",
  1715			 msg->qp, partition->size);
  1716	
  1717	err:
  1718		v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
  1719	
  1720		allegro_channel_buf_done(channel, dst_buf, state);
  1721	
  1722		v4l2_m2m_job_finish(dev->m2m_dev, channel->fh.m2m_ctx);
  1723	}
  1724	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 51587 bytes --]

Re: [PATCH 16/18] media: allegro: pass buffers through firmware

[Hans Verkuil]

On 2/17/20 4:13 PM, Michael Tretter wrote:
> As we know which buffers are processed by the codec from the address in
> the ENCODE_FRAME response, we can queue multiple buffers in the firmware
> and retrieve the buffer from the response of the firmware. This enables
> the firmware to use the internal scheduling the codec and avoids round
> trips through the driver when fetching the next frame.
> 
> Remove buffers that have been passed to the firmware from the m2m buffer
> queue and put them into a shadow queue for tracking the buffer in the
> driver. When we receive a ENCODE_FRAME response from the firmware, get
> the buffer from the shadow queue and finish the buffer.
> 
> Furthermore, it is necessary to finish the job straight after passing
> the buffer to the firmware to allow the V4L2 framework to send further
> buffers to the driver.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  .../staging/media/allegro-dvt/allegro-core.c  | 104 +++++++++++++++---
>  1 file changed, 89 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
> index 4f525920c194..80d3383b84f8 100644
> --- a/drivers/staging/media/allegro-dvt/allegro-core.c
> +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
> @@ -226,6 +226,10 @@ struct allegro_channel {
>  	struct list_head buffers_reference;
>  	struct list_head buffers_intermediate;
>  
> +	struct list_head source_shadow_list;
> +	struct list_head stream_shadow_list;
> +	struct mutex shadow_list_lock;

This lock is never used in interrupt context, right? Just checking.

Also add a comment explaining what the lock protects.

> +
>  	struct list_head list;
>  	struct completion completion;
>  
> @@ -247,6 +251,14 @@ allegro_get_state(struct allegro_channel *channel)
>  	return channel->state;
>  }
>  
> +struct allegro_m2m_buffer {
> +	struct v4l2_m2m_buffer buf;
> +	struct list_head head;
> +};
> +
> +#define to_allegro_m2m_buffer(__buf) \
> +	container_of(__buf, struct allegro_m2m_buffer, buf)
> +
>  struct fw_info {
>  	unsigned int id;
>  	unsigned int id_codec;
> @@ -1570,6 +1582,43 @@ static void allegro_channel_buf_done(struct allegro_channel *channel,
>  	v4l2_m2m_buf_done(buf, state);
>  }
>  
> +static u64 allegro_put_buffer(struct allegro_channel *channel,
> +			      struct list_head *list,
> +			      struct vb2_v4l2_buffer *buffer)
> +{
> +	struct v4l2_m2m_buffer *b = container_of(buffer,
> +						 struct v4l2_m2m_buffer, vb);
> +	struct allegro_m2m_buffer *shadow = to_allegro_m2m_buffer(b);
> +
> +	mutex_lock(&channel->shadow_list_lock);
> +	list_add_tail(&shadow->head, list);
> +	mutex_unlock(&channel->shadow_list_lock);
> +
> +	return (u64) buffer;
> +}
> +
> +static struct vb2_v4l2_buffer *allegro_get_buffer(struct allegro_channel *channel,
> +						  struct list_head *list,
> +						  u64 handle)
> +{
> +	struct allegro_dev *dev = channel->dev;
> +	struct allegro_m2m_buffer *shadow;
> +	u64 found;
> +
> +	mutex_lock(&channel->shadow_list_lock);
> +	shadow = list_first_entry(list, struct allegro_m2m_buffer, head);
> +	list_del_init(&shadow->head);
> +	mutex_unlock(&channel->shadow_list_lock);
> +
> +	found = (u64) (&shadow->buf.vb);
> +	if (handle != found)
> +		v4l2_warn(&dev->v4l2_dev,
> +			  "channel %d: output buffer mismatch 0x%llx, expected 0x%llx\n",
> +			  channel->mcu_channel_id, handle, found);
> +
> +	return &shadow->buf.vb;

This function never returns NULL...

> +}
> +
>  static void allegro_channel_finish_frame(struct allegro_channel *channel,
>  		struct mcu_msg_encode_frame_response *msg)
>  {
> @@ -1585,13 +1634,17 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
>  	ssize_t len;
>  	ssize_t free;
>  
> -	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
> -	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
> +	src_buf = allegro_get_buffer(channel, &channel->source_shadow_list, msg->src_handle);
> +	if (!src_buf)

...but this it checked here...

> +		v4l2_warn(&dev->v4l2_dev,
> +			  "channel %d: invalid source buffer\n",
> +			  channel->mcu_channel_id);
>  
> -	if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
> -		v4l2_err(&dev->v4l2_dev,
> -			 "channel %d: check failed\n",
> -			 channel->mcu_channel_id);
> +	dst_buf = allegro_get_buffer(channel, &channel->stream_shadow_list, msg->stream_id);
> +	if (!dst_buf)

...and here. That doesn't look right.

> +		v4l2_warn(&dev->v4l2_dev,
> +			  "channel %d: invalid stream buffer\n",
> +			  channel->mcu_channel_id);
>  
>  	dst_buf->sequence = channel->csequence++;
>  
> @@ -1718,8 +1771,6 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
>  	v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
>  
>  	allegro_channel_buf_done(channel, dst_buf, state);
> -
> -	v4l2_m2m_job_finish(dev->m2m_dev, channel->fh.m2m_ctx);
>  }
>  
>  static int allegro_handle_init(struct allegro_dev *dev,
> @@ -2312,16 +2363,31 @@ static void allegro_stop_streaming(struct vb2_queue *q)
>  	struct allegro_channel *channel = vb2_get_drv_priv(q);
>  	struct allegro_dev *dev = channel->dev;
>  	struct vb2_v4l2_buffer *buffer;
> +	struct allegro_m2m_buffer *shadow, *tmp;
>  
>  	v4l2_dbg(2, debug, &dev->v4l2_dev,
>  		 "%s: stop streaming\n",
>  		 V4L2_TYPE_IS_OUTPUT(q->type) ? "output" : "capture");
>  
>  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
> +		mutex_lock(&channel->shadow_list_lock);
> +		list_for_each_entry_safe(shadow, tmp, &channel->source_shadow_list, head) {
> +			list_del(&shadow->head);
> +			v4l2_m2m_buf_done(&shadow->buf.vb, VB2_BUF_STATE_ERROR);
> +		}
> +		mutex_unlock(&channel->shadow_list_lock);
> +
>  		allegro_set_state(channel, ALLEGRO_STATE_STOPPED);
>  		while ((buffer = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx)))
>  			v4l2_m2m_buf_done(buffer, VB2_BUF_STATE_ERROR);
>  	} else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> +		mutex_lock(&channel->shadow_list_lock);
> +		list_for_each_entry_safe(shadow, tmp, &channel->stream_shadow_list, head) {
> +			list_del(&shadow->head);
> +			v4l2_m2m_buf_done(&shadow->buf.vb, VB2_BUF_STATE_ERROR);
> +		}
> +		mutex_unlock(&channel->shadow_list_lock);
> +
>  		allegro_destroy_channel(channel);
>  		while ((buffer = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx)))
>  			v4l2_m2m_buf_done(buffer, VB2_BUF_STATE_ERROR);
> @@ -2352,7 +2418,7 @@ static int allegro_queue_init(void *priv,
>  	src_vq->drv_priv = channel;
>  	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>  	src_vq->ops = &allegro_queue_ops;
> -	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> +	src_vq->buf_struct_size = sizeof(struct allegro_m2m_buffer);
>  	src_vq->lock = &channel->dev->lock;
>  	err = vb2_queue_init(src_vq);
>  	if (err)
> @@ -2365,7 +2431,7 @@ static int allegro_queue_init(void *priv,
>  	dst_vq->drv_priv = channel;
>  	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>  	dst_vq->ops = &allegro_queue_ops;
> -	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> +	dst_vq->buf_struct_size = sizeof(struct allegro_m2m_buffer);
>  	dst_vq->lock = &channel->dev->lock;
>  	err = vb2_queue_init(dst_vq);
>  	if (err)
> @@ -2457,6 +2523,9 @@ static int allegro_open(struct file *file)
>  	v4l2_fh_add(&channel->fh);
>  
>  	init_completion(&channel->completion);
> +	INIT_LIST_HEAD(&channel->source_shadow_list);
> +	INIT_LIST_HEAD(&channel->stream_shadow_list);
> +	mutex_init(&channel->shadow_list_lock);
>  
>  	channel->dev = dev;
>  
> @@ -2957,18 +3026,23 @@ static void allegro_device_run(void *priv)
>  	dma_addr_t src_uv;
>  	dma_addr_t dst_addr;
>  	unsigned long dst_size;
> +	u64 src_handle;
> +	u64 dst_handle;
>  
> -	dst_buf = v4l2_m2m_next_dst_buf(channel->fh.m2m_ctx);
> +	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
>  	dst_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
>  	dst_size = vb2_plane_size(&dst_buf->vb2_buf, 0);
> -	allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size, (u64)dst_buf);
> +	dst_handle = allegro_put_buffer(channel, &channel->stream_shadow_list, dst_buf);
> +	allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size, dst_handle);
>  
> -	src_buf = v4l2_m2m_next_src_buf(channel->fh.m2m_ctx);
> +	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
>  	src_buf->sequence = channel->osequence++;
> -
>  	src_y = vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
>  	src_uv = src_y + (channel->stride * channel->height);
> -	allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv, (u64)src_buf);
> +	src_handle = allegro_put_buffer(channel, &channel->source_shadow_list, src_buf);
> +	allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv, src_handle);
> +
> +	v4l2_m2m_job_finish(dev->m2m_dev, channel->fh.m2m_ctx);
>  }
>  
>  static const struct v4l2_m2m_ops allegro_m2m_ops = {
> 

Regards,

	Hans

Re: [PATCH 14/18] media: allegro: handle dependency of bitrate and bitrate_peak

[Hans Verkuil]

On 2/17/20 4:13 PM, Michael Tretter wrote:
> The peak bitrate must not be smaller than the configured bitrate. Update
> the other control whenever one of the controls changes to reflect this
> dependency.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/staging/media/allegro-dvt/allegro-core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
> index 8c26158eab93..cedb09ea649f 100644
> --- a/drivers/staging/media/allegro-dvt/allegro-core.c
> +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
> @@ -2403,9 +2403,15 @@ static int allegro_s_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	case V4L2_CID_MPEG_VIDEO_BITRATE:
>  		channel->bitrate = ctrl->val;
> +		if (channel->bitrate > channel->bitrate_peak)
> +			__v4l2_ctrl_s_ctrl(channel->mpeg_video_bitrate_peak,
> +					   channel->bitrate);
>  		break;
>  	case V4L2_CID_MPEG_VIDEO_BITRATE_PEAK:
>  		channel->bitrate_peak = ctrl->val;
> +		if (channel->bitrate_peak < channel->bitrate)
> +			__v4l2_ctrl_s_ctrl(channel->mpeg_video_bitrate,
> +					   channel->bitrate_peak);
>  		break;
>  	case V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE:
>  		channel->cpb_size = ctrl->val;
> 

In the case of controls that are depend on one another you use a
control cluster and implement try_ctrl. See e.g. drivers/media/usb/hdpvr/hdpvr-video.c
which does the same thing.

Documentation on control clusters is here:

https://hverkuil.home.xs4all.nl/spec/kapi/v4l2-controls.html#control-clusters

Regards,

	Hans

Re: [PATCH 00/18] media: allegro: fixes and new features

[Hans Verkuil]

On 2/17/20 4:13 PM, Michael Tretter wrote:
> Hello,
> 
> these are a several patches for the allegro-dvt driver that have piled up over
> the last few month while I was improving my understanding of the codec.
> 
> Patches 1 to 9 fix errors in the interaction with the mcu. This includes
> better interpretation of return values from the firmware, wrong fields in the
> mails, wrong values in the fields and an error when resetting the mcu.
> 
> Patches 10 to 14 wire up more controls and allow user space applications to
> control the framerate and the quality of the codec.
> 
> Patches 15 and 16 enable the firmware to take care of the buffer scheduling
> and allow more parallelism inside the firmware. Please have a close look at
> patch 16, because it changes the behavior of the driver to finish the m2m_job
> before the driver returns the v4l2_buffers.
> 
> Patches 17 and 18 start work to restructure how to create the mails that are
> sent to the firmware, because different firmware versions expect different
> mail formats and, thus, I need additional code to generate mails if I want to
> support different firmware versions.

Posted comments for patches 14 and 16. Also note the 'kbuild test robot' post for
patch 15.

I also get a number of warnings/checks when checkpatch.pl --strict over the
patch series (and even one ERROR). Some can be ignored, but there are others
that can be easily fixed with some reformatting.

Regards,

	Hans

> 
> This is the v4l-compliance test result:
> 
> v4l2-compliance SHA: b62d322d4401e6b6e5cbd78cedad9eb69dac1324, 64 bits, 64-bit time_t
> 
> Compliance test for allegro device /dev/video3:
> 
> Driver Info:
> 	Driver name      : allegro
> 	Card type        : Allegro DVT Video Encoder
> 	Bus info         : platform:a0009000.video-codec
> 	Driver version   : 5.6.0
> 	Capabilities     : 0x84208000
> 		Video Memory-to-Memory
> 		Streaming
> 		Extended Pix Format
> 		Device Capabilities
> 	Device Caps      : 0x04208000
> 		Video Memory-to-Memory
> 		Streaming
> 		Extended Pix Format
> 	Detected Stateful Encoder
> 
> Required ioctls:
> 	test VIDIOC_QUERYCAP: OK
> 
> Allow for multiple opens:
> 	test second /dev/video3 open: OK
> 	test VIDIOC_QUERYCAP: OK
> 	test VIDIOC_G/S_PRIORITY: OK
> 	test for unlimited opens: OK
> 
> 	test invalid ioctls: OK
> Debug ioctls:
> 	test VIDIOC_DBG_G/S_REGISTER: OK
> 	test VIDIOC_LOG_STATUS: OK (Not Supported)
> 
> Input ioctls:
> 	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> 	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> 	test VIDIOC_ENUMAUDIO: OK (Not Supported)
> 	test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
> 	test VIDIOC_G/S_AUDIO: OK (Not Supported)
> 	Inputs: 0 Audio Inputs: 0 Tuners: 0
> 
> Output ioctls:
> 	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> 	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> 	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> 	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> 	Outputs: 0 Audio Outputs: 0 Modulators: 0
> 
> Input/Output configuration ioctls:
> 	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> 	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> 	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> 	test VIDIOC_G/S_EDID: OK (Not Supported)
> 
> Control ioctls:
> 	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
> 	test VIDIOC_QUERYCTRL: OK
> 	test VIDIOC_G/S_CTRL: OK
> 	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> 	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
> 	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> 	Standard Controls: 16 Private Controls: 0
> 
> Format ioctls:
> 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> 		warn: v4l2-test-formats.cpp(1329): S_PARM is supported for buftype 2, but not for ENUM_FRAMEINTERVALS
> 	test VIDIOC_G/S_PARM: OK
> 	test VIDIOC_G_FBUF: OK (Not Supported)
> 	test VIDIOC_G_FMT: OK
> 	test VIDIOC_TRY_FMT: OK
> 	test VIDIOC_S_FMT: OK
> 	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> 	test Cropping: OK (Not Supported)
> 	test Composing: OK (Not Supported)
> 	test Scaling: OK (Not Supported)
> 
> Codec ioctls:
> 	test VIDIOC_(TRY_)ENCODER_CMD: OK
> 	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> 	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
> 
> Buffer ioctls:
> 	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> 	test VIDIOC_EXPBUF: OK
> 	test Requests: OK (Not Supported)
> 
> Test input 0:
> 
> Streaming ioctls:
> 	test read/write: OK (Not Supported)
> 	test blocking wait: OK
> 	Video Capture: Captured 59 buffers
> 	test MMAP (select): OK
> 	Video Capture: Captured 59 buffers
> 	test MMAP (epoll): OK
> 	test USERPTR (select): OK (Not Supported)
> 	test DMABUF: Cannot test, specify --expbuf-device
> 
> Total for allegro device /dev/video3: 50, Succeeded: 50, Failed: 0, Warnings: 1
> 
> Michael
> 
> Michael Tretter (18):
>   media: allegro: print message on mcu error
>   media: allegro: fail encoding only on actual errors
>   media: allegro: fix type of gop_length in channel_create message
>   media: allegro: remove unknown39 field from create_channel
>   media: allegro: start a GOP with an IDR frame
>   media: allegro: fix calculation of CPB size
>   media: allegro: fix reset if WAKEUP has not been set properly
>   media: allegro: extract mcu and codec address calculation
>   media: allegro: warn if response message has an unexpected size
>   media: allegro: skip filler data if possible
>   media: allegro: make frame rate configurable
>   media: allegro: make QP configurable
>   media: allegro: read bitrate mode directly from control
>   media: allegro: handle dependency of bitrate and bitrate_peak
>   media: allegro: verify source and destination buffer in VCU response
>   media: allegro: pass buffers through firmware
>   media: allegro: move mail definitions to separate file
>   media: allegro: create new struct for channel parameters
> 
>  drivers/staging/media/allegro-dvt/Makefile    |   2 +-
>  .../staging/media/allegro-dvt/allegro-core.c  | 808 ++++++++++--------
>  .../staging/media/allegro-dvt/allegro-mail.c  |  37 +
>  .../staging/media/allegro-dvt/allegro-mail.h  | 267 ++++++
>  4 files changed, 738 insertions(+), 376 deletions(-)
>  create mode 100644 drivers/staging/media/allegro-dvt/allegro-mail.c
>  create mode 100644 drivers/staging/media/allegro-dvt/allegro-mail.h
> 

Re: [PATCH 16/18] media: allegro: pass buffers through firmware

[Michael Tretter]

On Tue, 25 Feb 2020 15:09:37 +0100, Hans Verkuil wrote:
> On 2/17/20 4:13 PM, Michael Tretter wrote:
> > As we know which buffers are processed by the codec from the address in
> > the ENCODE_FRAME response, we can queue multiple buffers in the firmware
> > and retrieve the buffer from the response of the firmware. This enables
> > the firmware to use the internal scheduling the codec and avoids round
> > trips through the driver when fetching the next frame.
> > 
> > Remove buffers that have been passed to the firmware from the m2m buffer
> > queue and put them into a shadow queue for tracking the buffer in the
> > driver. When we receive a ENCODE_FRAME response from the firmware, get
> > the buffer from the shadow queue and finish the buffer.
> > 
> > Furthermore, it is necessary to finish the job straight after passing
> > the buffer to the firmware to allow the V4L2 framework to send further
> > buffers to the driver.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  .../staging/media/allegro-dvt/allegro-core.c  | 104 +++++++++++++++---
> >  1 file changed, 89 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
> > index 4f525920c194..80d3383b84f8 100644
> > --- a/drivers/staging/media/allegro-dvt/allegro-core.c
> > +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
> > @@ -226,6 +226,10 @@ struct allegro_channel {
> >  	struct list_head buffers_reference;
> >  	struct list_head buffers_intermediate;
> >  
> > +	struct list_head source_shadow_list;
> > +	struct list_head stream_shadow_list;
> > +	struct mutex shadow_list_lock;  
> 
> This lock is never used in interrupt context, right? Just checking.

The lock is used from user threads and the threaded irq handler, but
not in an interrupt context.

> 
> Also add a comment explaining what the lock protects.

Will do.

> 
> > +
> >  	struct list_head list;
> >  	struct completion completion;
> >  
> > @@ -247,6 +251,14 @@ allegro_get_state(struct allegro_channel *channel)
> >  	return channel->state;
> >  }
> >  
> > +struct allegro_m2m_buffer {
> > +	struct v4l2_m2m_buffer buf;
> > +	struct list_head head;
> > +};
> > +
> > +#define to_allegro_m2m_buffer(__buf) \
> > +	container_of(__buf, struct allegro_m2m_buffer, buf)
> > +
> >  struct fw_info {
> >  	unsigned int id;
> >  	unsigned int id_codec;
> > @@ -1570,6 +1582,43 @@ static void allegro_channel_buf_done(struct allegro_channel *channel,
> >  	v4l2_m2m_buf_done(buf, state);
> >  }
> >  
> > +static u64 allegro_put_buffer(struct allegro_channel *channel,
> > +			      struct list_head *list,
> > +			      struct vb2_v4l2_buffer *buffer)
> > +{
> > +	struct v4l2_m2m_buffer *b = container_of(buffer,
> > +						 struct v4l2_m2m_buffer, vb);
> > +	struct allegro_m2m_buffer *shadow = to_allegro_m2m_buffer(b);
> > +
> > +	mutex_lock(&channel->shadow_list_lock);
> > +	list_add_tail(&shadow->head, list);
> > +	mutex_unlock(&channel->shadow_list_lock);
> > +
> > +	return (u64) buffer;
> > +}
> > +
> > +static struct vb2_v4l2_buffer *allegro_get_buffer(struct allegro_channel *channel,
> > +						  struct list_head *list,
> > +						  u64 handle)
> > +{
> > +	struct allegro_dev *dev = channel->dev;
> > +	struct allegro_m2m_buffer *shadow;
> > +	u64 found;
> > +
> > +	mutex_lock(&channel->shadow_list_lock);
> > +	shadow = list_first_entry(list, struct allegro_m2m_buffer, head);
> > +	list_del_init(&shadow->head);
> > +	mutex_unlock(&channel->shadow_list_lock);
> > +
> > +	found = (u64) (&shadow->buf.vb);
> > +	if (handle != found)
> > +		v4l2_warn(&dev->v4l2_dev,
> > +			  "channel %d: output buffer mismatch 0x%llx, expected 0x%llx\n",
> > +			  channel->mcu_channel_id, handle, found);
> > +
> > +	return &shadow->buf.vb;  
> 
> This function never returns NULL...
> 
> > +}
> > +
> >  static void allegro_channel_finish_frame(struct allegro_channel *channel,
> >  		struct mcu_msg_encode_frame_response *msg)
> >  {
> > @@ -1585,13 +1634,17 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
> >  	ssize_t len;
> >  	ssize_t free;
> >  
> > -	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
> > -	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
> > +	src_buf = allegro_get_buffer(channel, &channel->source_shadow_list, msg->src_handle);
> > +	if (!src_buf)  
> 
> ...but this it checked here...
> 
> > +		v4l2_warn(&dev->v4l2_dev,
> > +			  "channel %d: invalid source buffer\n",
> > +			  channel->mcu_channel_id);
> >  
> > -	if ((u64)src_buf != msg->src_handle || (u64)dst_buf != msg->stream_id)
> > -		v4l2_err(&dev->v4l2_dev,
> > -			 "channel %d: check failed\n",
> > -			 channel->mcu_channel_id);
> > +	dst_buf = allegro_get_buffer(channel, &channel->stream_shadow_list, msg->stream_id);
> > +	if (!dst_buf)  
> 
> ...and here. That doesn't look right.

I started to implement error handling in case the firmware returns
wrong buffers, but I stopped at printing warnings and using the next
buffers anyway. I didn't really know what to do, if that happens,
because I don't have v4l2_buffers that I could return to user space to
report errors. Maybe it is best to keep buffers on the shadow queues,
print a warning, and ignore the message from the firmware.

I will fix allegro_get_buffer() to return NULL and properly fail in
these cases.

Thanks,

Michael

> 
> > +		v4l2_warn(&dev->v4l2_dev,
> > +			  "channel %d: invalid stream buffer\n",
> > +			  channel->mcu_channel_id);
> >  
> >  	dst_buf->sequence = channel->csequence++;
> >  
> > @@ -1718,8 +1771,6 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
> >  	v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
> >  
> >  	allegro_channel_buf_done(channel, dst_buf, state);
> > -
> > -	v4l2_m2m_job_finish(dev->m2m_dev, channel->fh.m2m_ctx);
> >  }
> >  
> >  static int allegro_handle_init(struct allegro_dev *dev,
> > @@ -2312,16 +2363,31 @@ static void allegro_stop_streaming(struct vb2_queue *q)
> >  	struct allegro_channel *channel = vb2_get_drv_priv(q);
> >  	struct allegro_dev *dev = channel->dev;
> >  	struct vb2_v4l2_buffer *buffer;
> > +	struct allegro_m2m_buffer *shadow, *tmp;
> >  
> >  	v4l2_dbg(2, debug, &dev->v4l2_dev,
> >  		 "%s: stop streaming\n",
> >  		 V4L2_TYPE_IS_OUTPUT(q->type) ? "output" : "capture");
> >  
> >  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
> > +		mutex_lock(&channel->shadow_list_lock);
> > +		list_for_each_entry_safe(shadow, tmp, &channel->source_shadow_list, head) {
> > +			list_del(&shadow->head);
> > +			v4l2_m2m_buf_done(&shadow->buf.vb, VB2_BUF_STATE_ERROR);
> > +		}
> > +		mutex_unlock(&channel->shadow_list_lock);
> > +
> >  		allegro_set_state(channel, ALLEGRO_STATE_STOPPED);
> >  		while ((buffer = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx)))
> >  			v4l2_m2m_buf_done(buffer, VB2_BUF_STATE_ERROR);
> >  	} else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> > +		mutex_lock(&channel->shadow_list_lock);
> > +		list_for_each_entry_safe(shadow, tmp, &channel->stream_shadow_list, head) {
> > +			list_del(&shadow->head);
> > +			v4l2_m2m_buf_done(&shadow->buf.vb, VB2_BUF_STATE_ERROR);
> > +		}
> > +		mutex_unlock(&channel->shadow_list_lock);
> > +
> >  		allegro_destroy_channel(channel);
> >  		while ((buffer = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx)))
> >  			v4l2_m2m_buf_done(buffer, VB2_BUF_STATE_ERROR);
> > @@ -2352,7 +2418,7 @@ static int allegro_queue_init(void *priv,
> >  	src_vq->drv_priv = channel;
> >  	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> >  	src_vq->ops = &allegro_queue_ops;
> > -	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > +	src_vq->buf_struct_size = sizeof(struct allegro_m2m_buffer);
> >  	src_vq->lock = &channel->dev->lock;
> >  	err = vb2_queue_init(src_vq);
> >  	if (err)
> > @@ -2365,7 +2431,7 @@ static int allegro_queue_init(void *priv,
> >  	dst_vq->drv_priv = channel;
> >  	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> >  	dst_vq->ops = &allegro_queue_ops;
> > -	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > +	dst_vq->buf_struct_size = sizeof(struct allegro_m2m_buffer);
> >  	dst_vq->lock = &channel->dev->lock;
> >  	err = vb2_queue_init(dst_vq);
> >  	if (err)
> > @@ -2457,6 +2523,9 @@ static int allegro_open(struct file *file)
> >  	v4l2_fh_add(&channel->fh);
> >  
> >  	init_completion(&channel->completion);
> > +	INIT_LIST_HEAD(&channel->source_shadow_list);
> > +	INIT_LIST_HEAD(&channel->stream_shadow_list);
> > +	mutex_init(&channel->shadow_list_lock);
> >  
> >  	channel->dev = dev;
> >  
> > @@ -2957,18 +3026,23 @@ static void allegro_device_run(void *priv)
> >  	dma_addr_t src_uv;
> >  	dma_addr_t dst_addr;
> >  	unsigned long dst_size;
> > +	u64 src_handle;
> > +	u64 dst_handle;
> >  
> > -	dst_buf = v4l2_m2m_next_dst_buf(channel->fh.m2m_ctx);
> > +	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
> >  	dst_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> >  	dst_size = vb2_plane_size(&dst_buf->vb2_buf, 0);
> > -	allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size, (u64)dst_buf);
> > +	dst_handle = allegro_put_buffer(channel, &channel->stream_shadow_list, dst_buf);
> > +	allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size, dst_handle);
> >  
> > -	src_buf = v4l2_m2m_next_src_buf(channel->fh.m2m_ctx);
> > +	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
> >  	src_buf->sequence = channel->osequence++;
> > -
> >  	src_y = vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
> >  	src_uv = src_y + (channel->stride * channel->height);
> > -	allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv, (u64)src_buf);
> > +	src_handle = allegro_put_buffer(channel, &channel->source_shadow_list, src_buf);
> > +	allegro_mcu_send_encode_frame(dev, channel, src_y, src_uv, src_handle);
> > +
> > +	v4l2_m2m_job_finish(dev->m2m_dev, channel->fh.m2m_ctx);
> >  }
> >  
> >  static const struct v4l2_m2m_ops allegro_m2m_ops = {
> >   
> 
> Regards,
> 
> 	Hans
> 

Re: [PATCH 00/18] media: allegro: fixes and new features

[Michael Tretter]

On Tue, 25 Feb 2020 15:20:41 +0100, Hans Verkuil wrote:
> On 2/17/20 4:13 PM, Michael Tretter wrote:
> > Hello,
> > 
> > these are a several patches for the allegro-dvt driver that have piled up over
> > the last few month while I was improving my understanding of the codec.
> > 
> > Patches 1 to 9 fix errors in the interaction with the mcu. This includes
> > better interpretation of return values from the firmware, wrong fields in the
> > mails, wrong values in the fields and an error when resetting the mcu.
> > 
> > Patches 10 to 14 wire up more controls and allow user space applications to
> > control the framerate and the quality of the codec.
> > 
> > Patches 15 and 16 enable the firmware to take care of the buffer scheduling
> > and allow more parallelism inside the firmware. Please have a close look at
> > patch 16, because it changes the behavior of the driver to finish the m2m_job
> > before the driver returns the v4l2_buffers.
> > 
> > Patches 17 and 18 start work to restructure how to create the mails that are
> > sent to the firmware, because different firmware versions expect different
> > mail formats and, thus, I need additional code to generate mails if I want to
> > support different firmware versions.  
> 
> Posted comments for patches 14 and 16. Also note the 'kbuild test robot' post for
> patch 15.
> 
> I also get a number of warnings/checks when checkpatch.pl --strict over the
> patch series (and even one ERROR). Some can be ignored, but there are others
> that can be easily fixed with some reformatting.

Thanks. I totally forgot about checkpatch... I will fix the
warnings/checks in v2.

Michael

> 
> Regards,
> 
> 	Hans
> 
> > 
> > This is the v4l-compliance test result:
> > 
> > v4l2-compliance SHA: b62d322d4401e6b6e5cbd78cedad9eb69dac1324, 64 bits, 64-bit time_t
> > 
> > Compliance test for allegro device /dev/video3:
> > 
> > Driver Info:
> > 	Driver name      : allegro
> > 	Card type        : Allegro DVT Video Encoder
> > 	Bus info         : platform:a0009000.video-codec
> > 	Driver version   : 5.6.0
> > 	Capabilities     : 0x84208000
> > 		Video Memory-to-Memory
> > 		Streaming
> > 		Extended Pix Format
> > 		Device Capabilities
> > 	Device Caps      : 0x04208000
> > 		Video Memory-to-Memory
> > 		Streaming
> > 		Extended Pix Format
> > 	Detected Stateful Encoder
> > 
> > Required ioctls:
> > 	test VIDIOC_QUERYCAP: OK
> > 
> > Allow for multiple opens:
> > 	test second /dev/video3 open: OK
> > 	test VIDIOC_QUERYCAP: OK
> > 	test VIDIOC_G/S_PRIORITY: OK
> > 	test for unlimited opens: OK
> > 
> > 	test invalid ioctls: OK
> > Debug ioctls:
> > 	test VIDIOC_DBG_G/S_REGISTER: OK
> > 	test VIDIOC_LOG_STATUS: OK (Not Supported)
> > 
> > Input ioctls:
> > 	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> > 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> > 	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> > 	test VIDIOC_ENUMAUDIO: OK (Not Supported)
> > 	test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
> > 	test VIDIOC_G/S_AUDIO: OK (Not Supported)
> > 	Inputs: 0 Audio Inputs: 0 Tuners: 0
> > 
> > Output ioctls:
> > 	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> > 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> > 	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> > 	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> > 	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> > 	Outputs: 0 Audio Outputs: 0 Modulators: 0
> > 
> > Input/Output configuration ioctls:
> > 	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> > 	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> > 	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> > 	test VIDIOC_G/S_EDID: OK (Not Supported)
> > 
> > Control ioctls:
> > 	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
> > 	test VIDIOC_QUERYCTRL: OK
> > 	test VIDIOC_G/S_CTRL: OK
> > 	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> > 	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
> > 	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> > 	Standard Controls: 16 Private Controls: 0
> > 
> > Format ioctls:
> > 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> > 		warn: v4l2-test-formats.cpp(1329): S_PARM is supported for buftype 2, but not for ENUM_FRAMEINTERVALS
> > 	test VIDIOC_G/S_PARM: OK
> > 	test VIDIOC_G_FBUF: OK (Not Supported)
> > 	test VIDIOC_G_FMT: OK
> > 	test VIDIOC_TRY_FMT: OK
> > 	test VIDIOC_S_FMT: OK
> > 	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> > 	test Cropping: OK (Not Supported)
> > 	test Composing: OK (Not Supported)
> > 	test Scaling: OK (Not Supported)
> > 
> > Codec ioctls:
> > 	test VIDIOC_(TRY_)ENCODER_CMD: OK
> > 	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> > 	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
> > 
> > Buffer ioctls:
> > 	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> > 	test VIDIOC_EXPBUF: OK
> > 	test Requests: OK (Not Supported)
> > 
> > Test input 0:
> > 
> > Streaming ioctls:
> > 	test read/write: OK (Not Supported)
> > 	test blocking wait: OK
> > 	Video Capture: Captured 59 buffers
> > 	test MMAP (select): OK
> > 	Video Capture: Captured 59 buffers
> > 	test MMAP (epoll): OK
> > 	test USERPTR (select): OK (Not Supported)
> > 	test DMABUF: Cannot test, specify --expbuf-device
> > 
> > Total for allegro device /dev/video3: 50, Succeeded: 50, Failed: 0, Warnings: 1
> > 
> > Michael
> > 
> > Michael Tretter (18):
> >   media: allegro: print message on mcu error
> >   media: allegro: fail encoding only on actual errors
> >   media: allegro: fix type of gop_length in channel_create message
> >   media: allegro: remove unknown39 field from create_channel
> >   media: allegro: start a GOP with an IDR frame
> >   media: allegro: fix calculation of CPB size
> >   media: allegro: fix reset if WAKEUP has not been set properly
> >   media: allegro: extract mcu and codec address calculation
> >   media: allegro: warn if response message has an unexpected size
> >   media: allegro: skip filler data if possible
> >   media: allegro: make frame rate configurable
> >   media: allegro: make QP configurable
> >   media: allegro: read bitrate mode directly from control
> >   media: allegro: handle dependency of bitrate and bitrate_peak
> >   media: allegro: verify source and destination buffer in VCU response
> >   media: allegro: pass buffers through firmware
> >   media: allegro: move mail definitions to separate file
> >   media: allegro: create new struct for channel parameters
> > 
> >  drivers/staging/media/allegro-dvt/Makefile    |   2 +-
> >  .../staging/media/allegro-dvt/allegro-core.c  | 808 ++++++++++--------
> >  .../staging/media/allegro-dvt/allegro-mail.c  |  37 +
> >  .../staging/media/allegro-dvt/allegro-mail.h  | 267 ++++++
> >  4 files changed, 738 insertions(+), 376 deletions(-)
> >  create mode 100644 drivers/staging/media/allegro-dvt/allegro-mail.c
> >  create mode 100644 drivers/staging/media/allegro-dvt/allegro-mail.h
> >   
> 
>