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

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

[Michael Tretter]

Hello,

This is v2 of the series containing various patches for the allegro-dvt
driver.

I fixed the various errors/warnings/checks reported by checkpatch.pl. Except
for a warning about a potential update in MAINTAINERS, which is not required,
because I am already the maintainer for that directory, and a check for
the alignment with an open parenthesis, which I ignored, because readability
gets worse if I fix it, checkpatch.pl does not report any problems anymore.

I combined the BITRATE_MODE, BITRATE and BITRATE_PEAK controls into a control
cluster, which ensures that BITRATE_PEAK is always at least BITRATE.

I fixed a build warning due to casting pointers to u64. Since I need the u64
in the messages that are passed to the firmware, I properly convert the
pointers to u64 now.

Finally, I changed the functions to retrieve buffers from the shadows lists to
return NULL if a buffer cannot be found to be able to handle errors in the
allegro_channel_finish_frame() function. I also fixed a bug, which resulted in
the last but one buffer to be marked as the last buffer, because the driver
missed a check if there are any buffers still in the source shadow list.

v4l2-compliance still produces a warning, because I implemented s_parm, but
don't support enum_frameintervals. However, once I implement
enum_frameintervals, it produces a failure, because I don't implement s_parm
on OUTPUT _and_ CAPTURE, but I am understanding, that s_parm for setting the
framerate should be only implemented on OUTPUT. I am not sure what to do with
that.

Michael

This is the v4l-compliance test result:

v4l2-compliance SHA: 0b239af2ca93726b63dfa9c64a8622644f898a25, 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 60 buffers
	test MMAP (select): OK
	Video Capture: Captured 60 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

Changelog:

v1 -> v2:
- Fix checkpatch.pl errors/warnings/checks
- Use control cluster for bitrate controls
- Fix build warnings of ptr casts to u64
- Fix handling of errors on shadow lists

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  | 905 ++++++++++--------
 .../staging/media/allegro-dvt/allegro-mail.c  |  37 +
 .../staging/media/allegro-dvt/allegro-mail.h  | 267 ++++++
 4 files changed, 824 insertions(+), 387 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 v2 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>
---
Changelog:

v1 -> v2: None
---
 .../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 3c949090e8d2..74e84395db4e 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 v2 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>
---
Changelog:

v1 -> v2: None
---
 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 74e84395db4e..c605dcdc5656 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 v2 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>
---
Changelog:

v1 -> v2: None
---
 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 c605dcdc5656..6e3f28e637af 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 v2 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>
---
Changelog:

v1 -> v2: None
---
 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 6e3f28e637af..d67f84bf38d3 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 v2 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>
---
Changelog:

v1 -> v2: None
---
 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 d67f84bf38d3..cbe79bc216fd 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 v2 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>
---
Changelog:

v1 -> v2:
- Add separate function for calculating CPB for mcu
---
 .../staging/media/allegro-dvt/allegro-core.c  | 25 +++++++++++++++----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index cbe79bc216fd..87592419ec84 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>
@@ -1053,6 +1054,22 @@ v4l2_bitrate_mode_to_mcu_mode(enum v4l2_mpeg_video_bitrate_mode mode)
 	}
 }
 
+static u32 v4l2_cpb_size_to_mcu(unsigned int cpb_size, unsigned int bitrate)
+{
+	unsigned int cpb_size_kbit;
+	unsigned int bitrate_kbps;
+
+	/*
+	 * The mcu expects the CPB size in units of a 90 kHz clock, but the
+	 * channel follows the V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE and stores
+	 * the CPB size in kilobytes.
+	 */
+	cpb_size_kbit = cpb_size * BITS_PER_BYTE;
+	bitrate_kbps = bitrate / 1000;
+
+	return (cpb_size_kbit * 90000) / bitrate_kbps;
+}
+
 static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 					   struct allegro_channel *channel)
 {
@@ -1094,12 +1111,10 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 
 	msg.rate_control_mode =
 		v4l2_bitrate_mode_to_mcu_mode(channel->bitrate_mode);
+	msg.cpb_size = v4l2_cpb_size_to_mcu(channel->cpb_size,
+					    channel->bitrate_peak);
 	/* 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;
+	msg.initial_rem_delay = msg.cpb_size;
 	msg.framerate = 25;
 	msg.clk_ratio = 1000;
 	msg.target_bitrate = channel->bitrate;
-- 
2.20.1

[PATCH v2 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>
---
Changelog:

v1 -> v2: None
---
 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 87592419ec84..de50405ba907 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -1990,6 +1990,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 v2 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>
---
Changelog:

v1 -> v2: None
---
 .../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 de50405ba907..6c2237d4a674 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;
@@ -1173,8 +1196,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 */
@@ -1201,11 +1224,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);
@@ -1264,10 +1287,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 v2 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>
---
Changelog:

v1 -> v2:
- Fix checkpatch warnings about lines over 80 characters
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 6c2237d4a674..2392867b4270 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -1711,6 +1711,12 @@ 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,
@@ -1804,6 +1810,12 @@ 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 v2 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>
---
Changelog:

v1 -> v2:
- Fix indentation of messages
---
 .../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 2392867b4270..4a87647749f3 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -1652,17 +1652,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 v2 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>
---
Changelog:

v1 -> v2:
- Fix indentation
- Fix trailing semicolon and complex value in macro
---
 .../staging/media/allegro-dvt/allegro-core.c  | 63 +++++++++++++++++--
 1 file changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 4a87647749f3..be4cc957439e 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;
@@ -1138,8 +1142,9 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 					    channel->bitrate_peak);
 	/* 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;
@@ -1448,9 +1453,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;
@@ -2117,7 +2124,9 @@ 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) {
@@ -2163,6 +2172,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;
@@ -2769,6 +2779,46 @@ 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)
 {
@@ -2807,6 +2857,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 v2 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>
---
Changelog:

v1 -> v2:
- Fix indentation
---
 .../staging/media/allegro-dvt/allegro-core.c  | 112 ++++++++++++++++--
 1 file changed, 102 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index be4cc957439e..4d83a9f44e7e 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;
@@ -1097,10 +1104,21 @@ static u32 v4l2_cpb_size_to_mcu(unsigned int cpb_size, unsigned int bitrate)
 	return (cpb_size_kbit * 90000) / bitrate_kbps;
 }
 
+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)
 {
 	struct mcu_msg_create_channel msg;
+	int i_frame_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_i_frame_qp);
+	int p_frame_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_p_frame_qp);
+	int b_frame_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_b_frame_qp);
 
 	memset(&msg, 0, sizeof(msg));
 
@@ -1136,8 +1154,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;
+
 	msg.cpb_size = v4l2_cpb_size_to_mcu(channel->cpb_size,
 					    channel->bitrate_peak);
 	/* Shall be ]0;cpb_size in 90 kHz units]. Use maximum value. */
@@ -1147,11 +1169,11 @@ 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 = 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(i_frame_qp, p_frame_qp);
+	msg.pb_delta = get_qp_delta(p_frame_qp, b_frame_qp);
 	msg.golden_ref = 0;
 	msg.golden_delta = 2;
 	msg.golden_ref_frequency = 10;
@@ -1470,7 +1492,8 @@ 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;
@@ -1692,13 +1715,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);
@@ -2079,6 +2102,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);
@@ -2140,6 +2169,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);
@@ -2352,6 +2387,24 @@ 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,
@@ -2366,6 +2419,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;
@@ -2381,6 +2437,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;
@@ -2422,6 +2483,37 @@ 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 v2 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>
---
Changelog:

v1 -> v2:
- Fix indentation
---
 drivers/staging/media/allegro-dvt/allegro-core.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 4d83a9f44e7e..6a492de52987 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;
@@ -1119,6 +1118,7 @@ static int allegro_mcu_send_create_channel(struct allegro_dev *dev,
 	int i_frame_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_i_frame_qp);
 	int p_frame_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_p_frame_qp);
 	int b_frame_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_b_frame_qp);
+	int bitrate_mode = v4l2_ctrl_g_ctrl(channel->mpeg_video_bitrate_mode);
 
 	memset(&msg, 0, sizeof(msg));
 
@@ -1156,7 +1156,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(bitrate_mode);
 	else
 		msg.rate_control_mode = 0;
 
@@ -2224,7 +2224,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);
@@ -2422,9 +2421,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;
@@ -2518,7 +2514,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 v2 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>
---
Changelog:

v1 -> v2:
- Use control cluster
---
 .../staging/media/allegro-dvt/allegro-core.c  | 49 ++++++++++++++++---
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 6a492de52987..820fb21ab0f1 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -211,9 +211,11 @@ struct allegro_channel {
 	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;
+	struct { /* video bitrate mode control cluster */
+		struct v4l2_ctrl *mpeg_video_bitrate_mode;
+		struct v4l2_ctrl *mpeg_video_bitrate;
+		struct v4l2_ctrl *mpeg_video_bitrate_peak;
+	};
 	struct v4l2_ctrl *mpeg_video_cpb_size;
 	struct v4l2_ctrl *mpeg_video_gop_size;
 
@@ -2404,6 +2406,34 @@ static int allegro_clamp_qp(struct allegro_channel *channel,
 	return allegro_clamp_qp(channel, next_ctrl);
 }
 
+static int allegro_clamp_bitrate(struct allegro_channel *channel,
+				 struct v4l2_ctrl *ctrl)
+{
+	struct v4l2_ctrl *ctrl_bitrate = channel->mpeg_video_bitrate;
+	struct v4l2_ctrl *ctrl_bitrate_peak = channel->mpeg_video_bitrate_peak;
+
+	if (ctrl->val == V4L2_MPEG_VIDEO_BITRATE_MODE_VBR &&
+	    ctrl_bitrate_peak->val < ctrl_bitrate->val)
+		ctrl_bitrate_peak->val = ctrl_bitrate->val;
+
+	return 0;
+}
+
+static int allegro_try_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct allegro_channel *channel = container_of(ctrl->handler,
+						       struct allegro_channel,
+						       ctrl_handler);
+
+	switch (ctrl->id) {
+	case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
+		allegro_clamp_bitrate(channel, ctrl);
+		break;
+	}
+
+	return 0;
+}
+
 static int allegro_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct allegro_channel *channel = container_of(ctrl->handler,
@@ -2421,11 +2451,11 @@ 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:
-		channel->bitrate = ctrl->val;
-		break;
-	case V4L2_CID_MPEG_VIDEO_BITRATE_PEAK:
-		channel->bitrate_peak = ctrl->val;
+	case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
+		channel->bitrate = channel->mpeg_video_bitrate->val;
+		channel->bitrate_peak = channel->mpeg_video_bitrate_peak->val;
+		v4l2_ctrl_activate(channel->mpeg_video_bitrate_peak,
+				ctrl->val == V4L2_MPEG_VIDEO_BITRATE_MODE_VBR);
 		break;
 	case V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE:
 		channel->cpb_size = ctrl->val;
@@ -2444,6 +2474,7 @@ static int allegro_s_ctrl(struct v4l2_ctrl *ctrl)
 }
 
 static const struct v4l2_ctrl_ops allegro_ctrl_ops = {
+	.try_ctrl = allegro_try_ctrl,
 	.s_ctrl = allegro_s_ctrl,
 };
 
@@ -2547,6 +2578,8 @@ static int allegro_open(struct file *file)
 
 	channel->fh.ctrl_handler = handler;
 
+	v4l2_ctrl_cluster(3, &channel->mpeg_video_bitrate_mode);
+
 	channel->mcu_channel_id = -1;
 	channel->user_id = -1;
 
-- 
2.20.1

[PATCH v2 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>
---
Changelog:

v1 -> v2:
- Fix warnings about lines over 80 characters
- Fix warnings about cast from ptr to u64
---
 .../staging/media/allegro-dvt/allegro-core.c  | 32 +++++++++++++++----
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 820fb21ab0f1..45e0d2c2fc44 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -567,6 +567,11 @@ static inline u32 to_codec_addr(struct allegro_dev *dev, dma_addr_t phys)
 	return lower_32_bits(phys);
 }
 
+static inline u64 ptr_to_u64(const void *ptr)
+{
+	return (uintptr_t)ptr;
+}
+
 /* Helper functions for channel and user operations */
 
 static unsigned long allegro_next_user_id(struct allegro_dev *dev)
@@ -1215,7 +1220,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;
 
@@ -1229,7 +1235,8 @@ 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 */
+	/* copied to mcu_msg_encode_frame_response */
+	msg.stream_id = stream_id;
 
 	allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
 	allegro_mcu_interrupt(dev);
@@ -1239,7 +1246,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;
 
@@ -1252,7 +1260,8 @@ 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 */
+	/* src_handle is copied to mcu_msg_encode_frame_response */
+	msg.src_handle = src_handle;
 	msg.src_y = to_codec_addr(dev, src_y);
 	msg.src_uv = to_codec_addr(dev, src_uv);
 	msg.stride = channel->stride;
@@ -1602,8 +1611,17 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
 	ssize_t free;
 
 	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
+	if (ptr_to_u64(src_buf) != msg->src_handle)
+		v4l2_warn(&dev->v4l2_dev,
+			  "channel %d: invalid source buffer (0x%llx)\n",
+			  channel->mcu_channel_id, msg->src_handle);
 
 	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
+	if (ptr_to_u64(dst_buf) != msg->stream_id)
+		v4l2_warn(&dev->v4l2_dev,
+			  "channel %d: invalid stream buffer (0x%llx)\n",
+			  channel->mcu_channel_id, msg->stream_id);
+
 	dst_buf->sequence = channel->csequence++;
 
 	if (msg->error_code & AL_ERROR) {
@@ -3025,14 +3043,16 @@ 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,
+					   ptr_to_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,
+				      ptr_to_u64(src_buf));
 }
 
 static const struct v4l2_m2m_ops allegro_m2m_ops = {
-- 
2.20.1

[PATCH v2 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>
---
Changelog

v1 -> v2:
- Search buffer in entire shadow buffer queue
- Fix error handling if firmware reports wrong buffer handles
- Fix warnings about lines longer than 80 characters
- Set LAST flag on last buffer instead of last but one buffer
---
 .../staging/media/allegro-dvt/allegro-core.c  | 124 +++++++++++++++---
 1 file changed, 103 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index 45e0d2c2fc44..1554beebea88 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -228,6 +228,11 @@ 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;
+	/* protect shadow lists of buffers passed to firmware */
+	struct mutex shadow_list_lock;
+
 	struct list_head list;
 	struct completion completion;
 
@@ -249,6 +254,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;
@@ -1567,8 +1580,11 @@ static bool allegro_channel_is_at_eos(struct allegro_channel *channel)
 		break;
 	case ALLEGRO_STATE_DRAIN:
 	case ALLEGRO_STATE_WAIT_FOR_BUFFER:
-		if (v4l2_m2m_num_src_bufs_ready(channel->fh.m2m_ctx) == 0)
+		mutex_lock(&channel->shadow_list_lock);
+		if (v4l2_m2m_num_src_bufs_ready(channel->fh.m2m_ctx) == 0 &&
+		    list_empty(&channel->source_shadow_list))
 			is_at_eos = true;
+		mutex_unlock(&channel->shadow_list_lock);
 		break;
 	default:
 		break;
@@ -1595,6 +1611,41 @@ 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 ptr_to_u64(buffer);
+}
+
+static struct vb2_v4l2_buffer *
+allegro_get_buffer(struct allegro_channel *channel,
+		   struct list_head *list, u64 handle)
+{
+	struct allegro_m2m_buffer *shadow, *tmp;
+	struct vb2_v4l2_buffer *buffer = NULL;
+
+	mutex_lock(&channel->shadow_list_lock);
+	list_for_each_entry_safe(shadow, tmp, list, head) {
+		if (handle == ptr_to_u64(&shadow->buf.vb)) {
+			buffer = &shadow->buf.vb;
+			list_del_init(&shadow->head);
+			break;
+		}
+	}
+	mutex_unlock(&channel->shadow_list_lock);
+
+	return buffer;
+}
+
 static void allegro_channel_finish_frame(struct allegro_channel *channel,
 		struct mcu_msg_encode_frame_response *msg)
 {
@@ -1610,17 +1661,22 @@ 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);
-	if (ptr_to_u64(src_buf) != msg->src_handle)
+	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 (0x%llx)\n",
-			  channel->mcu_channel_id, msg->src_handle);
+			  "channel %d: invalid source buffer\n",
+			  channel->mcu_channel_id);
 
-	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
-	if (ptr_to_u64(dst_buf) != msg->stream_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 (0x%llx)\n",
-			  channel->mcu_channel_id, msg->stream_id);
+			  "channel %d: invalid stream buffer\n",
+			  channel->mcu_channel_id);
+
+	if (!src_buf || !dst_buf)
+		goto err;
 
 	dst_buf->sequence = channel->csequence++;
 
@@ -1744,11 +1800,11 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
 		 msg->qp, partition->size);
 
 err:
-	v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
+	if (src_buf)
+		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);
+	if (dst_buf)
+		allegro_channel_buf_done(channel, dst_buf, state);
 }
 
 static int allegro_handle_init(struct allegro_dev *dev,
@@ -2344,16 +2400,33 @@ 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);
@@ -2384,7 +2457,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)
@@ -2397,7 +2470,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)
@@ -2510,6 +2583,9 @@ static int allegro_open(struct file *file)
 		return -ENOMEM;
 
 	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;
 
@@ -3039,20 +3115,26 @@ 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);
+	dst_handle = allegro_put_buffer(channel, &channel->stream_shadow_list,
+					dst_buf);
 	allegro_mcu_send_put_stream_buffer(dev, channel, dst_addr, dst_size,
-					   ptr_to_u64(dst_buf));
+					   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,
-				      ptr_to_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 v2 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>
---
Changelog:

v1 -> v2:
- Fix SPDX-License-Identifier
---
 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 1554beebea88..ed5e9a33b7f9 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"
 
 /*
@@ -284,279 +285,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..4a50c595720a
--- /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 v2 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>
---
Changelog:

v1 -> v2:
- Fix checkpatch warnings
---
 .../staging/media/allegro-dvt/allegro-core.c  | 134 ++++++++++--------
 .../staging/media/allegro-dvt/allegro-mail.h  |  10 +-
 2 files changed, 80 insertions(+), 64 deletions(-)

diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
index ed5e9a33b7f9..6bba296dfc40 100644
--- a/drivers/staging/media/allegro-dvt/allegro-core.c
+++ b/drivers/staging/media/allegro-dvt/allegro-core.c
@@ -859,80 +859,92 @@ 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)
+static int fill_create_channel_param(struct allegro_channel *channel,
+				     struct create_channel_param *param)
 {
-	struct mcu_msg_create_channel msg;
 	int i_frame_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_i_frame_qp);
 	int p_frame_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_p_frame_qp);
 	int b_frame_qp = v4l2_ctrl_g_ctrl(channel->mpeg_video_h264_b_frame_qp);
 	int bitrate_mode = v4l2_ctrl_g_ctrl(channel->mpeg_video_bitrate_mode);
 
+	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;
+
+	param->rate_control_mode = channel->frame_rc_enable ?
+		v4l2_bitrate_mode_to_mcu_mode(bitrate_mode) : 0;
+
+	param->cpb_size = v4l2_cpb_size_to_mcu(channel->cpb_size,
+					       channel->bitrate_peak);
+	/* Shall be ]0;cpb_size in 90 kHz units]. Use maximum value. */
+	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 = 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(i_frame_qp, p_frame_qp);
+	param->pb_delta = get_qp_delta(p_frame_qp, b_frame_qp);
+	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;
-	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;
-
-	if (channel->frame_rc_enable)
-		msg.rate_control_mode =
-			v4l2_bitrate_mode_to_mcu_mode(bitrate_mode);
-	else
-		msg.rate_control_mode = 0;
 
-	msg.cpb_size = v4l2_cpb_size_to_mcu(channel->cpb_size,
-					    channel->bitrate_peak);
-	/* 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 = 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(i_frame_qp, p_frame_qp);
-	msg.pb_delta = get_qp_delta(p_frame_qp, 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;
+	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 4a50c595720a..1fd36f65be78 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 v2 15/18] media: allegro: verify source and destination buffer in VCU response

[Hans Verkuil]

Hi Michael,

I'm going to accept this patch, but I do have a suggestion for a future patch:

On 3/16/20 4:26 PM, Michael Tretter wrote:
> 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>
> ---
> Changelog:
> 
> v1 -> v2:
> - Fix warnings about lines over 80 characters
> - Fix warnings about cast from ptr to u64
> ---
>  .../staging/media/allegro-dvt/allegro-core.c  | 32 +++++++++++++++----
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
> index 820fb21ab0f1..45e0d2c2fc44 100644
> --- a/drivers/staging/media/allegro-dvt/allegro-core.c
> +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
> @@ -567,6 +567,11 @@ static inline u32 to_codec_addr(struct allegro_dev *dev, dma_addr_t phys)
>  	return lower_32_bits(phys);
>  }
>  
> +static inline u64 ptr_to_u64(const void *ptr)
> +{
> +	return (uintptr_t)ptr;
> +}
> +
>  /* Helper functions for channel and user operations */
>  
>  static unsigned long allegro_next_user_id(struct allegro_dev *dev)
> @@ -1215,7 +1220,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)

stream_id,

>  {
>  	struct mcu_msg_put_stream_buffer msg;
>  
> @@ -1229,7 +1235,8 @@ 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 */
> +	/* copied to mcu_msg_encode_frame_response */
> +	msg.stream_id = stream_id;
>  
>  	allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
>  	allegro_mcu_interrupt(dev);
> @@ -1239,7 +1246,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)

src_handle,

>  {
>  	struct mcu_msg_encode_frame msg;
>  
> @@ -1252,7 +1260,8 @@ 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 */
> +	/* src_handle is copied to mcu_msg_encode_frame_response */
> +	msg.src_handle = src_handle;
>  	msg.src_y = to_codec_addr(dev, src_y);
>  	msg.src_uv = to_codec_addr(dev, src_uv);
>  	msg.stride = channel->stride;
> @@ -1602,8 +1611,17 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
>  	ssize_t free;
>  
>  	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
> +	if (ptr_to_u64(src_buf) != msg->src_handle)
> +		v4l2_warn(&dev->v4l2_dev,
> +			  "channel %d: invalid source buffer (0x%llx)\n",
> +			  channel->mcu_channel_id, msg->src_handle);
>  
>  	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
> +	if (ptr_to_u64(dst_buf) != msg->stream_id)

dst_buf, and elsewhere in this code 'dst_handle' and 'handle' are all really
just buffer pointers, but the variable names are all over the place.

It's very confusing, and a patch that cleans this up by giving better names
and probably adding some comments would help a lot.

Regards,

	Hans

> +		v4l2_warn(&dev->v4l2_dev,
> +			  "channel %d: invalid stream buffer (0x%llx)\n",
> +			  channel->mcu_channel_id, msg->stream_id);
> +
>  	dst_buf->sequence = channel->csequence++;
>  
>  	if (msg->error_code & AL_ERROR) {
> @@ -3025,14 +3043,16 @@ 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,
> +					   ptr_to_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,
> +				      ptr_to_u64(src_buf));
>  }
>  
>  static const struct v4l2_m2m_ops allegro_m2m_ops = {
> 

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

[Michael Tretter]

Hi Hans,

On Thu, Mar 19, 2020 at 02:16:24PM +0100, Hans Verkuil wrote:
> Hi Michael,
> 
> I'm going to accept this patch, but I do have a suggestion for a future patch:
> 
> On 3/16/20 4:26 PM, Michael Tretter wrote:
> > 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>
> > ---
> > Changelog:
> > 
> > v1 -> v2:
> > - Fix warnings about lines over 80 characters
> > - Fix warnings about cast from ptr to u64
> > ---
> >  .../staging/media/allegro-dvt/allegro-core.c  | 32 +++++++++++++++----
> >  1 file changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/media/allegro-dvt/allegro-core.c b/drivers/staging/media/allegro-dvt/allegro-core.c
> > index 820fb21ab0f1..45e0d2c2fc44 100644
> > --- a/drivers/staging/media/allegro-dvt/allegro-core.c
> > +++ b/drivers/staging/media/allegro-dvt/allegro-core.c
> > @@ -567,6 +567,11 @@ static inline u32 to_codec_addr(struct allegro_dev *dev, dma_addr_t phys)
> >  	return lower_32_bits(phys);
> >  }
> >  
> > +static inline u64 ptr_to_u64(const void *ptr)
> > +{
> > +	return (uintptr_t)ptr;
> > +}
> > +
> >  /* Helper functions for channel and user operations */
> >  
> >  static unsigned long allegro_next_user_id(struct allegro_dev *dev)
> > @@ -1215,7 +1220,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)
> 
> stream_id,
> 
> >  {
> >  	struct mcu_msg_put_stream_buffer msg;
> >  
> > @@ -1229,7 +1235,8 @@ 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 */
> > +	/* copied to mcu_msg_encode_frame_response */
> > +	msg.stream_id = stream_id;
> >  
> >  	allegro_mbox_write(dev, &dev->mbox_command, &msg, sizeof(msg));
> >  	allegro_mcu_interrupt(dev);
> > @@ -1239,7 +1246,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)
> 
> src_handle,
> 
> >  {
> >  	struct mcu_msg_encode_frame msg;
> >  
> > @@ -1252,7 +1260,8 @@ 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 */
> > +	/* src_handle is copied to mcu_msg_encode_frame_response */
> > +	msg.src_handle = src_handle;
> >  	msg.src_y = to_codec_addr(dev, src_y);
> >  	msg.src_uv = to_codec_addr(dev, src_uv);
> >  	msg.stride = channel->stride;
> > @@ -1602,8 +1611,17 @@ static void allegro_channel_finish_frame(struct allegro_channel *channel,
> >  	ssize_t free;
> >  
> >  	src_buf = v4l2_m2m_src_buf_remove(channel->fh.m2m_ctx);
> > +	if (ptr_to_u64(src_buf) != msg->src_handle)
> > +		v4l2_warn(&dev->v4l2_dev,
> > +			  "channel %d: invalid source buffer (0x%llx)\n",
> > +			  channel->mcu_channel_id, msg->src_handle);
> >  
> >  	dst_buf = v4l2_m2m_dst_buf_remove(channel->fh.m2m_ctx);
> > +	if (ptr_to_u64(dst_buf) != msg->stream_id)
> 
> dst_buf, and elsewhere in this code 'dst_handle' and 'handle' are all really
> just buffer pointers, but the variable names are all over the place.
> 
> It's very confusing, and a patch that cleans this up by giving better names
> and probably adding some comments would help a lot.

Thanks for the suggestion. I have plans to make the driver compatible with
more recent firmware versions, which will require changes to how the messages
are handled. I will keep your suggestion in mind and choose names for the
fields in the messages that fit to their use in the driver.

Michael

> 
> Regards,
> 
> 	Hans
> 
> > +		v4l2_warn(&dev->v4l2_dev,
> > +			  "channel %d: invalid stream buffer (0x%llx)\n",
> > +			  channel->mcu_channel_id, msg->stream_id);
> > +
> >  	dst_buf->sequence = channel->csequence++;
> >  
> >  	if (msg->error_code & AL_ERROR) {
> > @@ -3025,14 +3043,16 @@ 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,
> > +					   ptr_to_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,
> > +				      ptr_to_u64(src_buf));
> >  }
> >  
> >  static const struct v4l2_m2m_ops allegro_m2m_ops = {
> > 
> 
>