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

[PATCH v2 1/3] 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. the value applies to the last dequeued
capture buffer.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
v2
 - improve document description according Hans's comments
 - drop volatile flag

 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
 include/uapi/linux/v4l2-controls.h                        | 2 ++
 3 files changed, 12 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..7d82ab14b8ba 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -1653,6 +1653,11 @@ 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. The value applies to the last dequeued capture buffer
+    (VIDIOC_DQBUF). Applicable to 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..1ea52011247a 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -970,6 +970,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:			return "LTR Count";
 	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:		return "Frame LTR Index";
 	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
+	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP Value";
 	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";
 
@@ -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_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 v2 2/3] 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, the value applies to the last dequeued
capture buffer.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
v2
 - drop the volatile flag

 drivers/media/platform/amphion/venc.c        |  4 ++++
 drivers/media/platform/amphion/vpu.h         |  1 +
 drivers/media/platform/amphion/vpu_defs.h    |  1 +
 drivers/media/platform/amphion/vpu_v4l2.c    | 16 ++++++++++++++++
 drivers/media/platform/amphion/vpu_v4l2.h    |  1 +
 drivers/media/platform/amphion/vpu_windsor.c |  2 ++
 6 files changed, 25 insertions(+)

diff --git a/drivers/media/platform/amphion/venc.c b/drivers/media/platform/amphion/venc.c
index 4eb57d793a9c..cdfaba9d107b 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, NULL,
+			  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..22f0da26ccec 100644
--- a/drivers/media/platform/amphion/vpu.h
+++ b/drivers/media/platform/amphion/vpu.h
@@ -306,6 +306,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_v4l2.c b/drivers/media/platform/amphion/vpu_v4l2.c
index c88738e8fff7..83db57bc80b7 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);
@@ -539,6 +546,15 @@ static void vpu_vb2_buf_finish(struct vb2_buffer *vb)
 	struct vpu_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
 	struct vb2_queue *q = vb->vb2_queue;
 
+	if (V4L2_TYPE_IS_CAPTURE(vb->type)) {
+		struct vpu_vb2_buffer *vpu_buf = to_vpu_vb2_buffer(vbuf);
+		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(&inst->ctrl_handler,
+							V4L2_CID_MPEG_VIDEO_AVERAGE_QP);
+
+		if (ctrl)
+			v4l2_ctrl_s_ctrl(ctrl, 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

[PATCH v2 3/3] media: amphion: Remove lock in s_ctrl callback

[Ming Qian]

There is no need to add lock in s_ctrl callback, it has been
synchronized by the ctrl_handler's lock, otherwise it may led to
deadlock if driver call v4l2_ctrl_s_ctrl().

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 drivers/media/platform/amphion/vdec.c | 2 --
 drivers/media/platform/amphion/venc.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/media/platform/amphion/vdec.c b/drivers/media/platform/amphion/vdec.c
index a57f9f4f3b87..6a38a0fa0e2d 100644
--- a/drivers/media/platform/amphion/vdec.c
+++ b/drivers/media/platform/amphion/vdec.c
@@ -195,7 +195,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
 	struct vdec_t *vdec = inst->priv;
 	int ret = 0;
 
-	vpu_inst_lock(inst);
 	switch (ctrl->id) {
 	case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:
 		vdec->params.display_delay_enable = ctrl->val;
@@ -207,7 +206,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = -EINVAL;
 		break;
 	}
-	vpu_inst_unlock(inst);
 
 	return ret;
 }
diff --git a/drivers/media/platform/amphion/venc.c b/drivers/media/platform/amphion/venc.c
index cdfaba9d107b..351b4edc8742 100644
--- a/drivers/media/platform/amphion/venc.c
+++ b/drivers/media/platform/amphion/venc.c
@@ -518,7 +518,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 	struct venc_t *venc = inst->priv;
 	int ret = 0;
 
-	vpu_inst_lock(inst);
 	switch (ctrl->id) {
 	case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
 		venc->params.profile = ctrl->val;
@@ -579,7 +578,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = -EINVAL;
 		break;
 	}
-	vpu_inst_unlock(inst);
 
 	return ret;
 }
-- 
2.43.0-rc1

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

[Andrzej Pietrasiewicz]

Hi Ming Qian,

W dniu 25.04.2024 o 08:50, Ming Qian pisze:
> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
> value of current encoded frame. the value applies to the last dequeued
> capture buffer.
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
> v2
>   - improve document description according Hans's comments
>   - drop volatile flag
> 
>   Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>   drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
>   include/uapi/linux/v4l2-controls.h                        | 2 ++
>   3 files changed, 12 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..7d82ab14b8ba 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -1653,6 +1653,11 @@ 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. The value applies to the last dequeued capture buffer
> +    (VIDIOC_DQBUF). Applicable to encoders.
> +

What is the intended range of the values? And why? Is your intention to make it
a hardware-agnostic control or a hardware-specific control?

Regrds,

Andrzej

>   .. 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..1ea52011247a 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -970,6 +970,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>   	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:			return "LTR Count";
>   	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:		return "Frame LTR Index";
>   	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:		return "Use LTR Frames";
> +	case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:			return "Average QP Value";
>   	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";
>   
> @@ -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_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 v2 3/3] media: amphion: Remove lock in s_ctrl callback

[Andrzej Pietrasiewicz]

Hi Ming Qian,

W dniu 25.04.2024 o 08:50, Ming Qian pisze:
> There is no need to add lock in s_ctrl callback, it has been
> synchronized by the ctrl_handler's lock, otherwise it may led to
> deadlock if driver call v4l2_ctrl_s_ctrl().
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
>   drivers/media/platform/amphion/vdec.c | 2 --
>   drivers/media/platform/amphion/venc.c | 2 --
>   2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/media/platform/amphion/vdec.c b/drivers/media/platform/amphion/vdec.c
> index a57f9f4f3b87..6a38a0fa0e2d 100644
> --- a/drivers/media/platform/amphion/vdec.c
> +++ b/drivers/media/platform/amphion/vdec.c
> @@ -195,7 +195,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>   	struct vdec_t *vdec = inst->priv;
>   	int ret = 0;
>   
> -	vpu_inst_lock(inst);

I assume that PATCH v2 2/3 might cause the said deadlock to happen?
If so, maybe it would make more sense to make the current patch preceed
  PATCH v2 2/3? Otherwise the kernel at PATCH v2 2/3 introduces a potential
deadlock.

Regards,

Andrzej

>   	switch (ctrl->id) {
>   	case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:
>   		vdec->params.display_delay_enable = ctrl->val;
> @@ -207,7 +206,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>   		ret = -EINVAL;
>   		break;
>   	}
> -	vpu_inst_unlock(inst);
>   
>   	return ret;
>   }
> diff --git a/drivers/media/platform/amphion/venc.c b/drivers/media/platform/amphion/venc.c
> index cdfaba9d107b..351b4edc8742 100644
> --- a/drivers/media/platform/amphion/venc.c
> +++ b/drivers/media/platform/amphion/venc.c
> @@ -518,7 +518,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>   	struct venc_t *venc = inst->priv;
>   	int ret = 0;
>   
> -	vpu_inst_lock(inst);
>   	switch (ctrl->id) {
>   	case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>   		venc->params.profile = ctrl->val;
> @@ -579,7 +578,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>   		ret = -EINVAL;
>   		break;
>   	}
> -	vpu_inst_unlock(inst);
>   
>   	return ret;
>   }

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

[ming qian]

Hi Andrzej,

> Hi Ming Qian,
> 
> W dniu 25.04.2024 o 08:50, Ming Qian pisze:
>> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
>> value of current encoded frame. the value applies to the last dequeued
>> capture buffer.
>>
>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> ---
>> v2
>>   - improve document description according Hans's comments
>>   - drop volatile flag
>>
>>   Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
>>   include/uapi/linux/v4l2-controls.h                        | 2 ++
>>   3 files changed, 12 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..7d82ab14b8ba 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -1653,6 +1653,11 @@ 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. The value applies to the last dequeued capture buffer
>> +    (VIDIOC_DQBUF). Applicable to encoders.
>> +
> 
> What is the intended range of the values? And why? Is your intention to 
> make it
> a hardware-agnostic control or a hardware-specific control?
> 
> Regrds,
> 
> Andrzej
> 

The value range depends on the format,
For H264, it's [V4L2_CID_MPEG_VIDEO_H264_MIN_QP, 
V4L2_CID_MPEG_VIDEO_H264_MIN_QP],
usually this is [0, 51].
For HEVC, it's [V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP, 
V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP],
usually it's from 0 to 51 for 8 bit and from 0 to 63 for 10 bit.
For H263 and MPEG4, it may be from 1 to 31.
For VPX, it may be from 1 to 128.

I think the driver that supports this ctrl can set an appropriate value
range based on the format it supports.

Users also need to read the value of this ctrl according to the format
they set.

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..1ea52011247a 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -970,6 +970,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>       case V4L2_CID_MPEG_VIDEO_LTR_COUNT:            return "LTR Count";
>>       case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:        return "Frame 
>> LTR Index";
>>       case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:        return "Use LTR 
>> Frames";
>> +    case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:            return "Average 
>> QP Value";
>>       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";
>> @@ -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_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 v2 3/3] media: amphion: Remove lock in s_ctrl callback

[ming qian]

Hi Andrzej,

> Hi Ming Qian,
> 
> W dniu 25.04.2024 o 08:50, Ming Qian pisze:
>> There is no need to add lock in s_ctrl callback, it has been
>> synchronized by the ctrl_handler's lock, otherwise it may led to
>> deadlock if driver call v4l2_ctrl_s_ctrl().
>>
>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>> ---
>>   drivers/media/platform/amphion/vdec.c | 2 --
>>   drivers/media/platform/amphion/venc.c | 2 --
>>   2 files changed, 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/amphion/vdec.c 
>> b/drivers/media/platform/amphion/vdec.c
>> index a57f9f4f3b87..6a38a0fa0e2d 100644
>> --- a/drivers/media/platform/amphion/vdec.c
>> +++ b/drivers/media/platform/amphion/vdec.c
>> @@ -195,7 +195,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>       struct vdec_t *vdec = inst->priv;
>>       int ret = 0;
>> -    vpu_inst_lock(inst);
> 
> I assume that PATCH v2 2/3 might cause the said deadlock to happen?
> If so, maybe it would make more sense to make the current patch preceed
>   PATCH v2 2/3? Otherwise the kernel at PATCH v2 2/3 introduces a potential
> deadlock.
> 
> Regards,
> 
> Andrzej
> 

I actually discovered this problem when I was preparing the v2 2/3 patch.

But in the v2 2/3 patch, it tried to add a read-only ctrl, then I just
unset the s_ctrl callback for the new added ctrl, the potential deadlock
is caused by call the s_ctrl back in a locked environment, so after unset
the s_ctrl callback, the 2/3 patch won't trigger the deadlock even if
this patch is missing.

In order to avoid encountering similar problems in the future, that
driver may set or get some ctrl, I added this patch.

Best regards,
Ming

>>       switch (ctrl->id) {
>>       case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:
>>           vdec->params.display_delay_enable = ctrl->val;
>> @@ -207,7 +206,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>           ret = -EINVAL;
>>           break;
>>       }
>> -    vpu_inst_unlock(inst);
>>       return ret;
>>   }
>> diff --git a/drivers/media/platform/amphion/venc.c 
>> b/drivers/media/platform/amphion/venc.c
>> index cdfaba9d107b..351b4edc8742 100644
>> --- a/drivers/media/platform/amphion/venc.c
>> +++ b/drivers/media/platform/amphion/venc.c
>> @@ -518,7 +518,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>       struct venc_t *venc = inst->priv;
>>       int ret = 0;
>> -    vpu_inst_lock(inst);
>>       switch (ctrl->id) {
>>       case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>>           venc->params.profile = ctrl->val;
>> @@ -579,7 +578,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>           ret = -EINVAL;
>>           break;
>>       }
>> -    vpu_inst_unlock(inst);
>>       return ret;
>>   }
> 

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

[Andrzej Pietrasiewicz]

Hi Ming,

W dniu 30.04.2024 o 04:20, ming qian pisze:
> 
> Hi Andrzej,
> 
>> Hi Ming Qian,
>>
>> W dniu 25.04.2024 o 08:50, Ming Qian pisze:
>>> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
>>> value of current encoded frame. the value applies to the last dequeued
>>> capture buffer.
>>>
>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>> ---
>>> v2
>>>   - improve document description according Hans's comments
>>>   - drop volatile flag
>>>
>>>   Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
>>>   include/uapi/linux/v4l2-controls.h                        | 2 ++
>>>   3 files changed, 12 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..7d82ab14b8ba 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>> @@ -1653,6 +1653,11 @@ 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. The value applies to the last dequeued capture buffer
>>> +    (VIDIOC_DQBUF). Applicable to encoders.
>>> +
>>
>> What is the intended range of the values? And why? Is your intention to make it
>> a hardware-agnostic control or a hardware-specific control?
>>
>> Regrds,
>>
>> Andrzej
>>
> 
> The value range depends on the format,
> For H264, it's [V4L2_CID_MPEG_VIDEO_H264_MIN_QP, V4L2_CID_MPEG_VIDEO_H264_MIN_QP],
> usually this is [0, 51].
> For HEVC, it's [V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP, V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP],
> usually it's from 0 to 51 for 8 bit and from 0 to 63 for 10 bit.
> For H263 and MPEG4, it may be from 1 to 31.
> For VPX, it may be from 1 to 128.
> 
> I think the driver that supports this ctrl can set an appropriate value
> range based on the format it supports.
> 
> Users also need to read the value of this ctrl according to the format
> they set.

What happens if the user does not set the format or until they set it?

This looks like a contextual control, where the context is a cross
product between setting or not setting the format and which particular
format is actually set.

I don't want to voice strong opinions about whether this should or
should not be accepted, but this kind of behavior should definitely
be documented so that driver writers know how to implement this control.
Right now reading the documentation in the *.rst file as a driver writer
I would be puzzled. So, from a driver writer's perspective I'd expect
these additions to the doc:

- the range of returned values
- the expected behavior when format has not been set
- the expected behavior when no frames have been processed yet (so there's no QP
to be read back from the hardware)

Regards,

Andrzej

> 
> 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..1ea52011247a 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> @@ -970,6 +970,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>       case V4L2_CID_MPEG_VIDEO_LTR_COUNT:            return "LTR Count";
>>>       case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:        return "Frame LTR Index";
>>>       case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:        return "Use LTR Frames";
>>> +    case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:            return "Average QP Value";
>>>       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";
>>> @@ -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_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 v2 3/3] media: amphion: Remove lock in s_ctrl callback

[Andrzej Pietrasiewicz]

Hi Ming,

W dniu 30.04.2024 o 04:32, ming qian pisze:
> Hi Andrzej,
> 
>> Hi Ming Qian,
>>
>> W dniu 25.04.2024 o 08:50, Ming Qian pisze:
>>> There is no need to add lock in s_ctrl callback, it has been
>>> synchronized by the ctrl_handler's lock, otherwise it may led to
>>> deadlock if driver call v4l2_ctrl_s_ctrl().
>>>
>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>> ---
>>>   drivers/media/platform/amphion/vdec.c | 2 --
>>>   drivers/media/platform/amphion/venc.c | 2 --
>>>   2 files changed, 4 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/amphion/vdec.c 
>>> b/drivers/media/platform/amphion/vdec.c
>>> index a57f9f4f3b87..6a38a0fa0e2d 100644
>>> --- a/drivers/media/platform/amphion/vdec.c
>>> +++ b/drivers/media/platform/amphion/vdec.c
>>> @@ -195,7 +195,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>       struct vdec_t *vdec = inst->priv;
>>>       int ret = 0;
>>> -    vpu_inst_lock(inst);
>>
>> I assume that PATCH v2 2/3 might cause the said deadlock to happen?
>> If so, maybe it would make more sense to make the current patch preceed
>>   PATCH v2 2/3? Otherwise the kernel at PATCH v2 2/3 introduces a potential
>> deadlock.
>>
>> Regards,
>>
>> Andrzej
>>
> 
> I actually discovered this problem when I was preparing the v2 2/3 patch.
> 
> But in the v2 2/3 patch, it tried to add a read-only ctrl, then I just
> unset the s_ctrl callback for the new added ctrl, the potential deadlock
> is caused by call the s_ctrl back in a locked environment, so after unset
> the s_ctrl callback, the 2/3 patch won't trigger the deadlock even if
> this patch is missing.
> 
> In order to avoid encountering similar problems in the future, that
> driver may set or get some ctrl, I added this patch.

Do I understand you correctly that patch 2/3 is written in such a way that
it does not introduce a deadlock, and you add patch 3/3 only to prevent future
problems? If so, it seems to me that patch 3/3 could/should be separate from
this series, as it does not quite match "Add average qp control".

Regards,

Andrzej

> 
> Best regards,
> Ming
> 
>>>       switch (ctrl->id) {
>>>       case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:
>>>           vdec->params.display_delay_enable = ctrl->val;
>>> @@ -207,7 +206,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>           ret = -EINVAL;
>>>           break;
>>>       }
>>> -    vpu_inst_unlock(inst);
>>>       return ret;
>>>   }
>>> diff --git a/drivers/media/platform/amphion/venc.c 
>>> b/drivers/media/platform/amphion/venc.c
>>> index cdfaba9d107b..351b4edc8742 100644
>>> --- a/drivers/media/platform/amphion/venc.c
>>> +++ b/drivers/media/platform/amphion/venc.c
>>> @@ -518,7 +518,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>       struct venc_t *venc = inst->priv;
>>>       int ret = 0;
>>> -    vpu_inst_lock(inst);
>>>       switch (ctrl->id) {
>>>       case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>>>           venc->params.profile = ctrl->val;
>>> @@ -579,7 +578,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>           ret = -EINVAL;
>>>           break;
>>>       }
>>> -    vpu_inst_unlock(inst);
>>>       return ret;
>>>   }
>>

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

[ming qian]

Hi Andrzej,

> Hi Ming,
> 
> W dniu 30.04.2024 o 04:20, ming qian pisze:
>>
>> Hi Andrzej,
>>
>>> Hi Ming Qian,
>>>
>>> W dniu 25.04.2024 o 08:50, Ming Qian pisze:
>>>> Add a control V4L2_CID_MPEG_VIDEO_AVERAGE_QP to report the average qp
>>>> value of current encoded frame. the value applies to the last dequeued
>>>> capture buffer.
>>>>
>>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>>> ---
>>>> v2
>>>>   - improve document description according Hans's comments
>>>>   - drop volatile flag
>>>>
>>>>   Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 5 +++++
>>>>   include/uapi/linux/v4l2-controls.h                        | 2 ++
>>>>   3 files changed, 12 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..7d82ab14b8ba 100644
>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> @@ -1653,6 +1653,11 @@ 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. The value applies to the last dequeued capture 
>>>> buffer
>>>> +    (VIDIOC_DQBUF). Applicable to encoders.
>>>> +
>>>
>>> What is the intended range of the values? And why? Is your intention 
>>> to make it
>>> a hardware-agnostic control or a hardware-specific control?
>>>
>>> Regrds,
>>>
>>> Andrzej
>>>
>>
>> The value range depends on the format,
>> For H264, it's [V4L2_CID_MPEG_VIDEO_H264_MIN_QP, 
>> V4L2_CID_MPEG_VIDEO_H264_MIN_QP],
>> usually this is [0, 51].
>> For HEVC, it's [V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP, 
>> V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP],
>> usually it's from 0 to 51 for 8 bit and from 0 to 63 for 10 bit.
>> For H263 and MPEG4, it may be from 1 to 31.
>> For VPX, it may be from 1 to 128.
>>
>> I think the driver that supports this ctrl can set an appropriate value
>> range based on the format it supports.
>>
>> Users also need to read the value of this ctrl according to the format
>> they set.
> 
> What happens if the user does not set the format or until they set it?
> 
> This looks like a contextual control, where the context is a cross
> product between setting or not setting the format and which particular
> format is actually set.
> 
> I don't want to voice strong opinions about whether this should or
> should not be accepted, but this kind of behavior should definitely
> be documented so that driver writers know how to implement this control.
> Right now reading the documentation in the *.rst file as a driver writer
> I would be puzzled. So, from a driver writer's perspective I'd expect
> these additions to the doc:
> 
> - the range of returned values
> - the expected behavior when format has not been set
> - the expected behavior when no frames have been processed yet (so 
> there's no QP
> to be read back from the hardware)
> 
> Regards,
> 
> Andrzej
> 

The ctrl value only applies to the last dequeued capture buffer. It
requires that the format has been set and the streaming has been
started. Before that, the value is meaningless, and the driver can set a
default value by itself.

I agree that it's better to add the value range description in the
document, but as discussed above, it's not a fixed value, maybe I can
add some description according to format.

Best regards,
Ming

>>
>> 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..1ea52011247a 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>> @@ -970,6 +970,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>>       case V4L2_CID_MPEG_VIDEO_LTR_COUNT:            return "LTR 
>>>> Count";
>>>>       case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:        return "Frame 
>>>> LTR Index";
>>>>       case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:        return "Use 
>>>> LTR Frames";
>>>> +    case V4L2_CID_MPEG_VIDEO_AVERAGE_QP:            return "Average 
>>>> QP Value";
>>>>       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";
>>>> @@ -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_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 v2 3/3] media: amphion: Remove lock in s_ctrl callback

[ming qian]

Hi Andrzej,

> Hi Ming,
> 
> W dniu 30.04.2024 o 04:32, ming qian pisze:
>> Hi Andrzej,
>>
>>> Hi Ming Qian,
>>>
>>> W dniu 25.04.2024 o 08:50, Ming Qian pisze:
>>>> There is no need to add lock in s_ctrl callback, it has been
>>>> synchronized by the ctrl_handler's lock, otherwise it may led to
>>>> deadlock if driver call v4l2_ctrl_s_ctrl().
>>>>
>>>> Signed-off-by: Ming Qian <ming.qian@nxp.com>
>>>> ---
>>>>   drivers/media/platform/amphion/vdec.c | 2 --
>>>>   drivers/media/platform/amphion/venc.c | 2 --
>>>>   2 files changed, 4 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/amphion/vdec.c 
>>>> b/drivers/media/platform/amphion/vdec.c
>>>> index a57f9f4f3b87..6a38a0fa0e2d 100644
>>>> --- a/drivers/media/platform/amphion/vdec.c
>>>> +++ b/drivers/media/platform/amphion/vdec.c
>>>> @@ -195,7 +195,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>       struct vdec_t *vdec = inst->priv;
>>>>       int ret = 0;
>>>> -    vpu_inst_lock(inst);
>>>
>>> I assume that PATCH v2 2/3 might cause the said deadlock to happen?
>>> If so, maybe it would make more sense to make the current patch preceed
>>>   PATCH v2 2/3? Otherwise the kernel at PATCH v2 2/3 introduces a 
>>> potential
>>> deadlock.
>>>
>>> Regards,
>>>
>>> Andrzej
>>>
>>
>> I actually discovered this problem when I was preparing the v2 2/3 patch.
>>
>> But in the v2 2/3 patch, it tried to add a read-only ctrl, then I just
>> unset the s_ctrl callback for the new added ctrl, the potential deadlock
>> is caused by call the s_ctrl back in a locked environment, so after unset
>> the s_ctrl callback, the 2/3 patch won't trigger the deadlock even if
>> this patch is missing.
>>
>> In order to avoid encountering similar problems in the future, that
>> driver may set or get some ctrl, I added this patch.
> 
> Do I understand you correctly that patch 2/3 is written in such a way that
> it does not introduce a deadlock, and you add patch 3/3 only to prevent 
> future
> problems? If so, it seems to me that patch 3/3 could/should be separate 
> from
> this series, as it does not quite match "Add average qp control".
> 
> Regards,
> 
> Andrzej
> 

You're right, when I prepare the 2/3 patch, I add the new ctrl just like
the previous ctrl, but I got a deadlock. then I tried to fix a deadlock
in 2/3 patch first, and make this patch again to avoid similar errors.

I put this patch to this series, as I think this better explains why I
made this change.

Regards,
Ming

>>
>> Best regards,
>> Ming
>>
>>>>       switch (ctrl->id) {
>>>>       case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:
>>>>           vdec->params.display_delay_enable = ctrl->val;
>>>> @@ -207,7 +206,6 @@ static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>           ret = -EINVAL;
>>>>           break;
>>>>       }
>>>> -    vpu_inst_unlock(inst);
>>>>       return ret;
>>>>   }
>>>> diff --git a/drivers/media/platform/amphion/venc.c 
>>>> b/drivers/media/platform/amphion/venc.c
>>>> index cdfaba9d107b..351b4edc8742 100644
>>>> --- a/drivers/media/platform/amphion/venc.c
>>>> +++ b/drivers/media/platform/amphion/venc.c
>>>> @@ -518,7 +518,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>       struct venc_t *venc = inst->priv;
>>>>       int ret = 0;
>>>> -    vpu_inst_lock(inst);
>>>>       switch (ctrl->id) {
>>>>       case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>>>>           venc->params.profile = ctrl->val;
>>>> @@ -579,7 +578,6 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>           ret = -EINVAL;
>>>>           break;
>>>>       }
>>>> -    vpu_inst_unlock(inst);
>>>>       return ret;
>>>>   }
>>>
>