[PATCH 1/2] media: v4l2-ctrls: Add average qp control

[PATCH 1/2] media: v4l2-ctrls: Add average qp control

[Ming Qian]

Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
value of current encoded frame.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 ++++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
 include/uapi/linux/v4l2-controls.h                        | 2 ++
 3 files changed, 11 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 2a165ae063fb..cef20b3f54ca 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1653,6 +1653,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     Quantization parameter for a P frame for FWHT. Valid range: from 1
     to 31.
 
+``V4L2_CID_MPEG_VIDEO_AVERAGE_QP (integer)``
+    This read-only control returns the average qp value of the currently
+    encoded frame. Applicable to the H264 and HEVC encoders.
+
 .. raw:: latex
 
     \normalsize
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 8696eb1cdd61..88e86e4e539d 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -972,6 +972,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
 	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
 	case V4L2_CID_FWHT_P_FRAME_QP:				return "FWHT P-Frame QP Value";
+	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP value";
 
 	/* VPX controls */
 	case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS:		return "VPX Number of Partitions";
@@ -1507,6 +1508,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		*max = 0xffffffffffffLL;
 		*step = 1;
 		break;
+	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:
+		*type = V4L2_CTRL_TYPE_INTEGER;
+		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
+		break;
 	case V4L2_CID_PIXEL_RATE:
 		*type = V4L2_CTRL_TYPE_INTEGER64;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 99c3f5e99da7..974fd254e573 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -898,6 +898,8 @@ enum v4l2_mpeg_video_av1_level {
 	V4L2_MPEG_VIDEO_AV1_LEVEL_7_3 = 23
 };
 
+#define V4L2_CID_MPEG_VIDEO_AVERAGE_QP  (V4L2_CID_CODEC_BASE + 657)
+
 /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
 #define V4L2_CID_CODEC_CX2341X_BASE				(V4L2_CTRL_CLASS_CODEC | 0x1000)
 #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_CODEC_CX2341X_BASE+0)
-- 
2.43.0-rc1

[PATCH 2/2] media: amphion: Report the average qp of current encoded frame

[Ming Qian]

Report the average qp value of current encoded frame via the control
V4L2_CID_MPEG_VIDEO_AVERAGE_QP

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 drivers/media/platform/amphion/venc.c        | 4 ++++
 drivers/media/platform/amphion/vpu.h         | 2 ++
 drivers/media/platform/amphion/vpu_defs.h    | 1 +
 drivers/media/platform/amphion/vpu_helpers.c | 3 +++
 drivers/media/platform/amphion/vpu_v4l2.c    | 9 +++++++++
 drivers/media/platform/amphion/vpu_v4l2.h    | 1 +
 drivers/media/platform/amphion/vpu_windsor.c | 2 ++
 7 files changed, 22 insertions(+)

diff --git a/drivers/media/platform/amphion/venc.c b/drivers/media/platform/amphion/venc.c
index 4eb57d793a9c..12cebafeaf3b 100644
--- a/drivers/media/platform/amphion/venc.c
+++ b/drivers/media/platform/amphion/venc.c
@@ -680,6 +680,9 @@ static int venc_ctrl_init(struct vpu_inst *inst)
 			       ~(1 << V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME),
 			       V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME);
 
+	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+			  V4L2_CID_MPEG_VIDEO_AVERAGE_QP, 0, 51, 1, 0);
+
 	if (inst->ctrl_handler.error) {
 		ret = inst->ctrl_handler.error;
 		v4l2_ctrl_handler_free(&inst->ctrl_handler);
@@ -819,6 +822,7 @@ static int venc_get_one_encoded_frame(struct vpu_inst *inst,
 	vbuf->field = inst->cap_format.field;
 	vbuf->flags |= frame->info.pic_type;
 	vpu_set_buffer_state(vbuf, VPU_BUF_STATE_IDLE);
+	vpu_set_buffer_average_qp(vbuf, frame->info.average_qp);
 	dev_dbg(inst->dev, "[%d][OUTPUT TS]%32lld\n", inst->id, vbuf->vb2_buf.timestamp);
 	v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE);
 	venc->ready_count++;
diff --git a/drivers/media/platform/amphion/vpu.h b/drivers/media/platform/amphion/vpu.h
index 0246cf0ac3a8..d21ca6bf2459 100644
--- a/drivers/media/platform/amphion/vpu.h
+++ b/drivers/media/platform/amphion/vpu.h
@@ -270,6 +270,7 @@ struct vpu_inst {
 	u8 xfer_func;
 	u32 sequence;
 	u32 extra_size;
+	u32 current_average_qp;
 
 	u32 flows[16];
 	u32 flow_idx;
@@ -306,6 +307,7 @@ struct vpu_vb2_buffer {
 	dma_addr_t chroma_v;
 	unsigned int state;
 	u32 tag;
+	u32 average_qp;
 };
 
 void vpu_writel(struct vpu_dev *vpu, u32 reg, u32 val);
diff --git a/drivers/media/platform/amphion/vpu_defs.h b/drivers/media/platform/amphion/vpu_defs.h
index 7320852668d6..428d988cf2f7 100644
--- a/drivers/media/platform/amphion/vpu_defs.h
+++ b/drivers/media/platform/amphion/vpu_defs.h
@@ -114,6 +114,7 @@ struct vpu_enc_pic_info {
 	u32 wptr;
 	u32 crc;
 	s64 timestamp;
+	u32 average_qp;
 };
 
 struct vpu_dec_codec_info {
diff --git a/drivers/media/platform/amphion/vpu_helpers.c b/drivers/media/platform/amphion/vpu_helpers.c
index d12310af9ebc..59139302cb1d 100644
--- a/drivers/media/platform/amphion/vpu_helpers.c
+++ b/drivers/media/platform/amphion/vpu_helpers.c
@@ -378,6 +378,9 @@ int vpu_helper_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
 		ctrl->val = inst->min_buffer_out;
 		break;
+	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:
+		ctrl->val = inst->current_average_qp;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/platform/amphion/vpu_v4l2.c b/drivers/media/platform/amphion/vpu_v4l2.c
index c88738e8fff7..893f494ffb0b 100644
--- a/drivers/media/platform/amphion/vpu_v4l2.c
+++ b/drivers/media/platform/amphion/vpu_v4l2.c
@@ -63,6 +63,13 @@ unsigned int vpu_get_buffer_state(struct vb2_v4l2_buffer *vbuf)
 	return vpu_buf->state;
 }
 
+void vpu_set_buffer_average_qp(struct vb2_v4l2_buffer *vbuf, u32 qp)
+{
+	struct vpu_vb2_buffer *vpu_buf = to_vpu_vb2_buffer(vbuf);
+
+	vpu_buf->average_qp = qp;
+}
+
 void vpu_v4l2_set_error(struct vpu_inst *inst)
 {
 	vpu_inst_lock(inst);
@@ -536,9 +543,11 @@ static int vpu_vb2_buf_prepare(struct vb2_buffer *vb)
 static void vpu_vb2_buf_finish(struct vb2_buffer *vb)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct vpu_vb2_buffer *vpu_buf = to_vpu_vb2_buffer(vbuf);
 	struct vpu_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
 	struct vb2_queue *q = vb->vb2_queue;
 
+	inst->current_average_qp = vpu_buf->average_qp;
 	if (vbuf->flags & V4L2_BUF_FLAG_LAST)
 		vpu_notify_eos(inst);
 
diff --git a/drivers/media/platform/amphion/vpu_v4l2.h b/drivers/media/platform/amphion/vpu_v4l2.h
index 60f43056a7a2..56f2939fa84d 100644
--- a/drivers/media/platform/amphion/vpu_v4l2.h
+++ b/drivers/media/platform/amphion/vpu_v4l2.h
@@ -12,6 +12,7 @@ void vpu_inst_lock(struct vpu_inst *inst);
 void vpu_inst_unlock(struct vpu_inst *inst);
 void vpu_set_buffer_state(struct vb2_v4l2_buffer *vbuf, unsigned int state);
 unsigned int vpu_get_buffer_state(struct vb2_v4l2_buffer *vbuf);
+void vpu_set_buffer_average_qp(struct vb2_v4l2_buffer *vbuf, u32 qp);
 
 int vpu_v4l2_open(struct file *file, struct vpu_inst *inst);
 int vpu_v4l2_close(struct file *file);
diff --git a/drivers/media/platform/amphion/vpu_windsor.c b/drivers/media/platform/amphion/vpu_windsor.c
index 5f1101d7cf9e..e7d37aa4b826 100644
--- a/drivers/media/platform/amphion/vpu_windsor.c
+++ b/drivers/media/platform/amphion/vpu_windsor.c
@@ -499,6 +499,7 @@ struct windsor_pic_info {
 	u32 proc_dacc_rng_wr_cnt;
 	s32 tv_s;
 	u32 tv_ns;
+	u32 average_qp;
 };
 
 u32 vpu_windsor_get_data_size(void)
@@ -734,6 +735,7 @@ static void vpu_windsor_unpack_pic_info(struct vpu_rpc_event *pkt, void *data)
 	info->wptr = get_ptr(windsor->str_buff_wptr);
 	info->crc = windsor->frame_crc;
 	info->timestamp = timespec64_to_ns(&ts);
+	info->average_qp = windsor->average_qp;
 }
 
 static void vpu_windsor_unpack_mem_req(struct vpu_rpc_event *pkt, void *data)
-- 
2.43.0-rc1

Re: [PATCH 1/2] media: v4l2-ctrls: Add average qp control

[Nicolas Dufresne]

Hi,

Le vendredi 29 mars 2024 à 18:23 +0900, Ming Qian a écrit :
> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
> value of current encoded frame.
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 ++++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
>  include/uapi/linux/v4l2-controls.h                        | 2 ++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 2a165ae063fb..cef20b3f54ca 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1653,6 +1653,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      Quantization parameter for a P frame for FWHT. Valid range: from 1
>      to 31.
>  
> +``V4L2_CID_MPEG_VIDEO_AVERAGE_QP (integer)``
> +    This read-only control returns the average qp value of the currently
> +    encoded frame. Applicable to the H264 and HEVC encoders.

That seems ambiguous at best. What does it mean the "currently encoded frame" ?
The OUTPUT and CAPTURE queue can be holding multiple frames. For "per frame"
accurate reporting, I feel like we'd need something like Hans' read-only
requests proposal [0]. Its basically a mechanism that let you attach request FD
to capture buffer, so that supported controls can be saved per v4l2-buffer and
read later on.

https://patches.linaro.org/project/linux-media/patch/20210610113615.785359-12-hverkuil-cisco@xs4all.nl/

If this isn't what you wanted, we'll need a better definition. It might be
helpful to explain what this is used for.

Nicolas

> +
>  .. raw:: latex
>  
>      \normalsize
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 8696eb1cdd61..88e86e4e539d 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -972,6 +972,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>  	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
>  	case V4L2_CID_FWHT_P_FRAME_QP:				return "FWHT P-Frame QP Value";
> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP value";
>  
>  	/* VPX controls */
>  	case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS:		return "VPX Number of Partitions";
> @@ -1507,6 +1508,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  		*max = 0xffffffffffffLL;
>  		*step = 1;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
> +		break;
>  	case V4L2_CID_PIXEL_RATE:
>  		*type = V4L2_CTRL_TYPE_INTEGER64;
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 99c3f5e99da7..974fd254e573 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -898,6 +898,8 @@ enum v4l2_mpeg_video_av1_level {
>  	V4L2_MPEG_VIDEO_AV1_LEVEL_7_3 = 23
>  };
>  
> +#define V4L2_CID_MPEG_VIDEO_AVERAGE_QP  (V4L2_CID_CODEC_BASE + 657)
> +
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_CODEC_CX2341X_BASE				(V4L2_CTRL_CLASS_CODEC | 0x1000)
>  #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_CODEC_CX2341X_BASE+0)

Re: [PATCH 1/2] media: v4l2-ctrls: Add average qp control

[ming qian]

Hi Nicolas,

On 4/5/24 02:14, Nicolas Dufresne wrote:
> Hi,
> 
> Le vendredi 29 mars 2024 à 18:23 +0900, Ming Qian a écrit :
>> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
>> value of current encoded frame.
>>
>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> ---
>>   Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 ++++
>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
>>   include/uapi/linux/v4l2-controls.h                        | 2 ++
>>   3 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index 2a165ae063fb..cef20b3f54ca 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -1653,6 +1653,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>       Quantization parameter for a P frame for FWHT. Valid range: from 1
>>       to 31.
>>   
>> +``V4L2_CID_MPEG_VIDEO_AVERAGE_QP (integer)``
>> +    This read-only control returns the average qp value of the currently
>> +    encoded frame. Applicable to the H264 and HEVC encoders.
> 
> That seems ambiguous at best. What does it mean the "currently encoded frame" ?
> The OUTPUT and CAPTURE queue can be holding multiple frames. For "per frame"
> accurate reporting, I feel like we'd need something like Hans' read-only
> requests proposal [0]. Its basically a mechanism that let you attach request FD
> to capture buffer, so that supported controls can be saved per v4l2-buffer and
> read later on.
> 
> https://patches.linaro.org/project/linux-media/patch/20210610113615.785359-12-hverkuil-cisco@xs4all.nl/
> 
> If this isn't what you wanted, we'll need a better definition. It might be
> helpful to explain what this is used for.
> 
> Nicolas
> 

Yes, I want to report the qp value for every frame.
I thought the request FD is only used for stateless decoder, but I want
to add a read-only ctrl for the stateful encoder. So I checked the
defined read-only ctrls, I think it's similar with
V4L2_CID_MPEG_VIDEO_DEC_PTS.
(https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/ext-ctrls-codec.html?highlight=v4l2_cid_mpeg_video_dec_pts)

then back to your question about the "currently encoded frame", it's the
last dequeued capture buffer of the encoder, the capture queue can hold
multiple frames, each frame will have a qp value in this case, and this
ctrl only report the qp value of the last dequeued frame, when user has
dequeued an encoded frame from the capture queue, he can get the ctrl
value of V4L2_CID_MPEG_VIDEO_AVERAGE_QP immediately to get the qp value
of the currently dequeued frame. If user doesn't care about this
parameter, he doesn't need to do anything, it's just the same as before.
so I think this ctrl is backward compatible.

Maybe the request FD is a better and more intuitive way to suggest a
one-to-one correspondence between ctrl and frame. I'm just not sure if
it just applies to the stateless decoder. I did find any stateful
decoder or encoder to use them.
If we use the request FD for this stateful encoder, I'm not sure if it
will break the original flow.

best regards,
Ming

>> +
>>   .. raw:: latex
>>   
>>       \normalsize
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index 8696eb1cdd61..88e86e4e539d 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -972,6 +972,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>   	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>>   	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
>>   	case V4L2_CID_FWHT_P_FRAME_QP:				return "FWHT P-Frame QP Value";
>> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP value";
>>   
>>   	/* VPX controls */
>>   	case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS:		return "VPX Number of Partitions";
>> @@ -1507,6 +1508,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>   		*max = 0xffffffffffffLL;
>>   		*step = 1;
>>   		break;
>> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
>> +		break;
>>   	case V4L2_CID_PIXEL_RATE:
>>   		*type = V4L2_CTRL_TYPE_INTEGER64;
>>   		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 99c3f5e99da7..974fd254e573 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -898,6 +898,8 @@ enum v4l2_mpeg_video_av1_level {
>>   	V4L2_MPEG_VIDEO_AV1_LEVEL_7_3 = 23
>>   };
>>   
>> +#define V4L2_CID_MPEG_VIDEO_AVERAGE_QP  (V4L2_CID_CODEC_BASE + 657)
>> +
>>   /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>>   #define V4L2_CID_CODEC_CX2341X_BASE				(V4L2_CTRL_CLASS_CODEC | 0x1000)
>>   #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_CODEC_CX2341X_BASE+0)
> 

Re: [PATCH 1/2] media: v4l2-ctrls: Add average qp control

[Hans Verkuil]

On 29/03/2024 10:23, Ming Qian wrote:
> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
> value of current encoded frame.
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 ++++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
>  include/uapi/linux/v4l2-controls.h                        | 2 ++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 2a165ae063fb..cef20b3f54ca 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1653,6 +1653,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>      Quantization parameter for a P frame for FWHT. Valid range: from 1
>      to 31.
>  
> +``V4L2_CID_MPEG_VIDEO_AVERAGE_QP (integer)``
> +    This read-only control returns the average qp value of the currently
> +    encoded frame. Applicable to the H264 and HEVC encoders.

qp -> QP

Why is this applicable to H264/HEVC only? I think it is fine for any codec.

This needs to document that the value applies to the last dequeued buffer
(VIDIOC_DQBUF).

> +
>  .. raw:: latex
>  
>      \normalsize
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 8696eb1cdd61..88e86e4e539d 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -972,6 +972,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>  	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
>  	case V4L2_CID_FWHT_P_FRAME_QP:				return "FWHT P-Frame QP Value";
> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP value";

value -> Value

Also move it up two lines so that it follows V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES
rather than FWHT controls.

>  
>  	/* VPX controls */
>  	case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS:		return "VPX Number of Partitions";
> @@ -1507,6 +1508,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  		*max = 0xffffffffffffLL;
>  		*step = 1;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;

Drop the volatile flag, this isn't a volatile value.

> +		break;
>  	case V4L2_CID_PIXEL_RATE:
>  		*type = V4L2_CTRL_TYPE_INTEGER64;
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 99c3f5e99da7..974fd254e573 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -898,6 +898,8 @@ enum v4l2_mpeg_video_av1_level {
>  	V4L2_MPEG_VIDEO_AV1_LEVEL_7_3 = 23
>  };
>  
> +#define V4L2_CID_MPEG_VIDEO_AVERAGE_QP  (V4L2_CID_CODEC_BASE + 657)
> +
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_CODEC_CX2341X_BASE				(V4L2_CTRL_CLASS_CODEC | 0x1000)
>  #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_CODEC_CX2341X_BASE+0)

Regards,

	Hans

Re: [PATCH 2/2] media: amphion: Report the average qp of current encoded frame

[Hans Verkuil]

On 29/03/2024 10:23, Ming Qian wrote:
> Report the average qp value of current encoded frame via the control
> V4L2_CID_MPEG_VIDEO_AVERAGE_QP
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
>  drivers/media/platform/amphion/venc.c        | 4 ++++
>  drivers/media/platform/amphion/vpu.h         | 2 ++
>  drivers/media/platform/amphion/vpu_defs.h    | 1 +
>  drivers/media/platform/amphion/vpu_helpers.c | 3 +++
>  drivers/media/platform/amphion/vpu_v4l2.c    | 9 +++++++++
>  drivers/media/platform/amphion/vpu_v4l2.h    | 1 +
>  drivers/media/platform/amphion/vpu_windsor.c | 2 ++
>  7 files changed, 22 insertions(+)
> 
> diff --git a/drivers/media/platform/amphion/venc.c b/drivers/media/platform/amphion/venc.c
> index 4eb57d793a9c..12cebafeaf3b 100644
> --- a/drivers/media/platform/amphion/venc.c
> +++ b/drivers/media/platform/amphion/venc.c
> @@ -680,6 +680,9 @@ static int venc_ctrl_init(struct vpu_inst *inst)
>  			       ~(1 << V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME),
>  			       V4L2_MPEG_VIDEO_HEADER_MODE_JOINED_WITH_1ST_FRAME);
>  
> +	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
> +			  V4L2_CID_MPEG_VIDEO_AVERAGE_QP, 0, 51, 1, 0);
> +
>  	if (inst->ctrl_handler.error) {
>  		ret = inst->ctrl_handler.error;
>  		v4l2_ctrl_handler_free(&inst->ctrl_handler);
> @@ -819,6 +822,7 @@ static int venc_get_one_encoded_frame(struct vpu_inst *inst,
>  	vbuf->field = inst->cap_format.field;
>  	vbuf->flags |= frame->info.pic_type;
>  	vpu_set_buffer_state(vbuf, VPU_BUF_STATE_IDLE);
> +	vpu_set_buffer_average_qp(vbuf, frame->info.average_qp);
>  	dev_dbg(inst->dev, "[%d][OUTPUT TS]%32lld\n", inst->id, vbuf->vb2_buf.timestamp);
>  	v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE);
>  	venc->ready_count++;
> diff --git a/drivers/media/platform/amphion/vpu.h b/drivers/media/platform/amphion/vpu.h
> index 0246cf0ac3a8..d21ca6bf2459 100644
> --- a/drivers/media/platform/amphion/vpu.h
> +++ b/drivers/media/platform/amphion/vpu.h
> @@ -270,6 +270,7 @@ struct vpu_inst {
>  	u8 xfer_func;
>  	u32 sequence;
>  	u32 extra_size;
> +	u32 current_average_qp;
>  
>  	u32 flows[16];
>  	u32 flow_idx;
> @@ -306,6 +307,7 @@ struct vpu_vb2_buffer {
>  	dma_addr_t chroma_v;
>  	unsigned int state;
>  	u32 tag;
> +	u32 average_qp;
>  };
>  
>  void vpu_writel(struct vpu_dev *vpu, u32 reg, u32 val);
> diff --git a/drivers/media/platform/amphion/vpu_defs.h b/drivers/media/platform/amphion/vpu_defs.h
> index 7320852668d6..428d988cf2f7 100644
> --- a/drivers/media/platform/amphion/vpu_defs.h
> +++ b/drivers/media/platform/amphion/vpu_defs.h
> @@ -114,6 +114,7 @@ struct vpu_enc_pic_info {
>  	u32 wptr;
>  	u32 crc;
>  	s64 timestamp;
> +	u32 average_qp;
>  };
>  
>  struct vpu_dec_codec_info {
> diff --git a/drivers/media/platform/amphion/vpu_helpers.c b/drivers/media/platform/amphion/vpu_helpers.c
> index d12310af9ebc..59139302cb1d 100644
> --- a/drivers/media/platform/amphion/vpu_helpers.c
> +++ b/drivers/media/platform/amphion/vpu_helpers.c
> @@ -378,6 +378,9 @@ int vpu_helper_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
>  		ctrl->val = inst->min_buffer_out;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:
> +		ctrl->val = inst->current_average_qp;
> +		break;

This is not a volatile control. Instead, the control should be updated
with the new average_qp in the vpu_vb2_buf_finish function.

Regards,

	Hans

>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/media/platform/amphion/vpu_v4l2.c b/drivers/media/platform/amphion/vpu_v4l2.c
> index c88738e8fff7..893f494ffb0b 100644
> --- a/drivers/media/platform/amphion/vpu_v4l2.c
> +++ b/drivers/media/platform/amphion/vpu_v4l2.c
> @@ -63,6 +63,13 @@ unsigned int vpu_get_buffer_state(struct vb2_v4l2_buffer *vbuf)
>  	return vpu_buf->state;
>  }
>  
> +void vpu_set_buffer_average_qp(struct vb2_v4l2_buffer *vbuf, u32 qp)
> +{
> +	struct vpu_vb2_buffer *vpu_buf = to_vpu_vb2_buffer(vbuf);
> +
> +	vpu_buf->average_qp = qp;
> +}
> +
>  void vpu_v4l2_set_error(struct vpu_inst *inst)
>  {
>  	vpu_inst_lock(inst);
> @@ -536,9 +543,11 @@ static int vpu_vb2_buf_prepare(struct vb2_buffer *vb)
>  static void vpu_vb2_buf_finish(struct vb2_buffer *vb)
>  {
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct vpu_vb2_buffer *vpu_buf = to_vpu_vb2_buffer(vbuf);
>  	struct vpu_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
>  	struct vb2_queue *q = vb->vb2_queue;
>  
> +	inst->current_average_qp = vpu_buf->average_qp;
>  	if (vbuf->flags & V4L2_BUF_FLAG_LAST)
>  		vpu_notify_eos(inst);
>  
> diff --git a/drivers/media/platform/amphion/vpu_v4l2.h b/drivers/media/platform/amphion/vpu_v4l2.h
> index 60f43056a7a2..56f2939fa84d 100644
> --- a/drivers/media/platform/amphion/vpu_v4l2.h
> +++ b/drivers/media/platform/amphion/vpu_v4l2.h
> @@ -12,6 +12,7 @@ void vpu_inst_lock(struct vpu_inst *inst);
>  void vpu_inst_unlock(struct vpu_inst *inst);
>  void vpu_set_buffer_state(struct vb2_v4l2_buffer *vbuf, unsigned int state);
>  unsigned int vpu_get_buffer_state(struct vb2_v4l2_buffer *vbuf);
> +void vpu_set_buffer_average_qp(struct vb2_v4l2_buffer *vbuf, u32 qp);
>  
>  int vpu_v4l2_open(struct file *file, struct vpu_inst *inst);
>  int vpu_v4l2_close(struct file *file);
> diff --git a/drivers/media/platform/amphion/vpu_windsor.c b/drivers/media/platform/amphion/vpu_windsor.c
> index 5f1101d7cf9e..e7d37aa4b826 100644
> --- a/drivers/media/platform/amphion/vpu_windsor.c
> +++ b/drivers/media/platform/amphion/vpu_windsor.c
> @@ -499,6 +499,7 @@ struct windsor_pic_info {
>  	u32 proc_dacc_rng_wr_cnt;
>  	s32 tv_s;
>  	u32 tv_ns;
> +	u32 average_qp;
>  };
>  
>  u32 vpu_windsor_get_data_size(void)
> @@ -734,6 +735,7 @@ static void vpu_windsor_unpack_pic_info(struct vpu_rpc_event *pkt, void *data)
>  	info->wptr = get_ptr(windsor->str_buff_wptr);
>  	info->crc = windsor->frame_crc;
>  	info->timestamp = timespec64_to_ns(&ts);
> +	info->average_qp = windsor->average_qp;
>  }
>  
>  static void vpu_windsor_unpack_mem_req(struct vpu_rpc_event *pkt, void *data)

Re: [PATCH 1/2] media: v4l2-ctrls: Add average qp control

[Nicolas Dufresne]

Le mercredi 24 avril 2024 à 12:03 +0200, Hans Verkuil a écrit :
> On 29/03/2024 10:23, Ming Qian wrote:
> > Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
> > value of current encoded frame.
> > 
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > ---
> >  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 ++++
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
> >  include/uapi/linux/v4l2-controls.h                        | 2 ++
> >  3 files changed, 11 insertions(+)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 2a165ae063fb..cef20b3f54ca 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -1653,6 +1653,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
> >      Quantization parameter for a P frame for FWHT. Valid range: from 1
> >      to 31.
> >  
> > +``V4L2_CID_MPEG_VIDEO_AVERAGE_QP (integer)``
> > +    This read-only control returns the average qp value of the currently
> > +    encoded frame. Applicable to the H264 and HEVC encoders.
> 
> qp -> QP
> 
> Why is this applicable to H264/HEVC only? I think it is fine for any codec.

Same question, though we should document the range, which will differ per-
codecs. We should also document to always use the specified range, rather then a
HW custom range. This way we don't endup with issues we have hit with other ill-
defined controls.

Nicolas

> 
> This needs to document that the value applies to the last dequeued buffer
> (VIDIOC_DQBUF).
> 
> > +
> >  .. raw:: latex
> >  
> >      \normalsize
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index 8696eb1cdd61..88e86e4e539d 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > @@ -972,6 +972,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
> >  	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
> >  	case V4L2_CID_FWHT_P_FRAME_QP:				return "FWHT P-Frame QP Value";
> > +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP value";
> 
> value -> Value
> 
> Also move it up two lines so that it follows V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES
> rather than FWHT controls.
> 
> >  
> >  	/* VPX controls */
> >  	case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS:		return "VPX Number of Partitions";
> > @@ -1507,6 +1508,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >  		*max = 0xffffffffffffLL;
> >  		*step = 1;
> >  		break;
> > +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:
> > +		*type = V4L2_CTRL_TYPE_INTEGER;
> > +		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
> 
> Drop the volatile flag, this isn't a volatile value.
> 
> > +		break;
> >  	case V4L2_CID_PIXEL_RATE:
> >  		*type = V4L2_CTRL_TYPE_INTEGER64;
> >  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index 99c3f5e99da7..974fd254e573 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -898,6 +898,8 @@ enum v4l2_mpeg_video_av1_level {
> >  	V4L2_MPEG_VIDEO_AV1_LEVEL_7_3 = 23
> >  };
> >  
> > +#define V4L2_CID_MPEG_VIDEO_AVERAGE_QP  (V4L2_CID_CODEC_BASE + 657)
> > +
> >  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
> >  #define V4L2_CID_CODEC_CX2341X_BASE				(V4L2_CTRL_CLASS_CODEC | 0x1000)
> >  #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_CODEC_CX2341X_BASE+0)
> 
> Regards,
> 
> 	Hans
> 

Re: [PATCH 1/2] media: v4l2-ctrls: Add average qp control

[ming qian]

Hi Nicolas,

> Le mercredi 24 avril 2024 à 12:03 +0200, Hans Verkuil a écrit :
>> On 29/03/2024 10:23, Ming Qian wrote:
>>> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
>>> value of current encoded frame.
>>>
>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>> ---
>>>   Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 4 ++++
>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
>>>   include/uapi/linux/v4l2-controls.h                        | 2 ++
>>>   3 files changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> index 2a165ae063fb..cef20b3f54ca 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -1653,6 +1653,10 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>>       Quantization parameter for a P frame for FWHT. Valid range: from 1
>>>       to 31.
>>>   
>>> +``V4L2_CID_MPEG_VIDEO_AVERAGE_QP (integer)``
>>> +    This read-only control returns the average qp value of the currently
>>> +    encoded frame. Applicable to the H264 and HEVC encoders.
>>
>> qp -> QP
>>
>> Why is this applicable to H264/HEVC only? I think it is fine for any codec.
> 
> Same question, though we should document the range, which will differ per-
> codecs. We should also document to always use the specified range, rather then a
> HW custom range. This way we don't endup with issues we have hit with other ill-
> defined controls.
> 
> Nicolas

Thanks for the tip, I will add a description of the value range in V3
patch.


> 
>>
>> This needs to document that the value applies to the last dequeued buffer
>> (VIDIOC_DQBUF).
>>
>>> +
>>>   .. raw:: latex
>>>   
>>>       \normalsize
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> index 8696eb1cdd61..88e86e4e539d 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> @@ -972,6 +972,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>   	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
>>>   	case V4L2_CID_FWHT_I_FRAME_QP:				return "FWHT I-Frame QP Value";
>>>   	case V4L2_CID_FWHT_P_FRAME_QP:				return "FWHT P-Frame QP Value";
>>> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP value";
>>
>> value -> Value
>>
>> Also move it up two lines so that it follows V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES
>> rather than FWHT controls.
>>
>>>   
>>>   	/* VPX controls */
>>>   	case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS:		return "VPX Number of Partitions";
>>> @@ -1507,6 +1508,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>   		*max = 0xffffffffffffLL;
>>>   		*step = 1;
>>>   		break;
>>> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:
>>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>>> +		*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
>>
>> Drop the volatile flag, this isn't a volatile value.
>>
>>> +		break;
>>>   	case V4L2_CID_PIXEL_RATE:
>>>   		*type = V4L2_CTRL_TYPE_INTEGER64;
>>>   		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>> index 99c3f5e99da7..974fd254e573 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -898,6 +898,8 @@ enum v4l2_mpeg_video_av1_level {
>>>   	V4L2_MPEG_VIDEO_AV1_LEVEL_7_3 = 23
>>>   };
>>>   
>>> +#define V4L2_CID_MPEG_VIDEO_AVERAGE_QP  (V4L2_CID_CODEC_BASE + 657)
>>> +
>>>   /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>>>   #define V4L2_CID_CODEC_CX2341X_BASE				(V4L2_CTRL_CLASS_CODEC | 0x1000)
>>>   #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE		(V4L2_CID_CODEC_CX2341X_BASE+0)
>>
>> Regards,
>>
>> 	Hans
>>
>