Re: [PATCH 2/2] media: chips-media: wave5: Fix Reports from Kernel Lock Validator

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

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

Le jeudi 02 avril 2026 à 13:45 -0500, Brandon Brnich a écrit :
> handle_dynamic_resolution change requires that the state_lock be acquired
> based on the lockdep_assert_held. However, the
> handle_dynamic_resolution_change call in initialize_sequence does not
> properly obtain the lock before calling.
> 
> Since the v4l2_ctrl_find and s_ctrl can sleep, they should not be called
> while a lock is already held. Store off the fbc_buf_count then properly
> update control once lock has been freed.
> 
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
>  .../chips-media/wave5/wave5-vpu-dec.c         | 50 ++++++++++++++-----
>  1 file changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> index 80e1831a42e0..62b21b2c5e29 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> @@ -283,10 +283,25 @@ static void send_eos_event(struct vpu_instance *inst)
>  	inst->sent_eos = true;
>  }
>  
> +static void wave5_update_min_bufs_ctrl(struct vpu_instance *inst, u32 fbc_buf_count)
> +{
> +	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
> +	struct v4l2_ctrl *ctrl;
> +
> +	if (!fbc_buf_count ||
> +			fbc_buf_count == v4l2_m2m_num_dst_bufs_ready(m2m_ctx))
> +		return;
> +
> +	ctrl = v4l2_ctrl_find(&inst->v4l2_ctrl_hdl,
> +			V4L2_CID_MIN_BUFFERS_FOR_CAPTURE);
> +	if (ctrl)
> +		v4l2_ctrl_s_ctrl(ctrl, fbc_buf_count);
> +}
> +
> +

I might have to drop that extra line, but I'll take care.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

>  static int handle_dynamic_resolution_change(struct vpu_instance *inst)
>  {
>  	struct v4l2_fh *fh = &inst->v4l2_fh;
> -	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
>  
>  	static const struct v4l2_event vpu_event_src_ch = {
>  		.type = V4L2_EVENT_SOURCE_CHANGE,
> @@ -305,14 +320,6 @@ static int handle_dynamic_resolution_change(struct vpu_instance *inst)
>  
>  	inst->needs_reallocation = true;
>  	inst->fbc_buf_count = initial_info->min_frame_buffer_count + 1;
> -	if (inst->fbc_buf_count != v4l2_m2m_num_dst_bufs_ready(m2m_ctx)) {
> -		struct v4l2_ctrl *ctrl;
> -
> -		ctrl = v4l2_ctrl_find(&inst->v4l2_ctrl_hdl,
> -				      V4L2_CID_MIN_BUFFERS_FOR_CAPTURE);
> -		if (ctrl)
> -			v4l2_ctrl_s_ctrl(ctrl, inst->fbc_buf_count);
> -	}
>  
>  	if (p_dec_info->initial_info_obtained) {
>  		const struct vpu_format *vpu_fmt;
> @@ -439,19 +446,24 @@ static void wave5_vpu_dec_finish_decode(struct vpu_instance *inst)
>  	if ((dec_info.index_frame_display == DISPLAY_IDX_FLAG_SEQ_END ||
>  	     dec_info.sequence_changed)) {
>  		unsigned long flags;
> +		u32 fbc_buf_count = 0;
>  
>  		spin_lock_irqsave(&inst->state_spinlock, flags);
>  		if (!v4l2_m2m_has_stopped(m2m_ctx)) {
>  			switch_state(inst, VPU_INST_STATE_STOP);
>  
> -			if (dec_info.sequence_changed)
> +			if (dec_info.sequence_changed) {
>  				handle_dynamic_resolution_change(inst);
> -			else
> +				fbc_buf_count = inst->fbc_buf_count;
> +			} else {
>  				send_eos_event(inst);
> +			}
>  
>  			flag_last_buffer_done(inst);
>  		}
>  		spin_unlock_irqrestore(&inst->state_spinlock, flags);
> +
> +		wave5_update_min_bufs_ctrl(inst, fbc_buf_count);
>  	}
>  
>  	if (inst->sent_eos &&
> @@ -1583,6 +1595,8 @@ static const struct vpu_instance_ops wave5_vpu_dec_inst_ops = {
>  static int initialize_sequence(struct vpu_instance *inst)
>  {
>  	struct dec_initial_info initial_info;
> +	unsigned long flags;
> +	u32 fbc_buf_count;
>  	int ret = 0;
>  
>  	memset(&initial_info, 0, sizeof(struct dec_initial_info));
> @@ -1605,7 +1619,12 @@ static int initialize_sequence(struct vpu_instance *inst)
>  		return ret;
>  	}
>  
> +	spin_lock_irqsave(&inst->state_spinlock, flags);
>  	handle_dynamic_resolution_change(inst);
> +	fbc_buf_count = inst->fbc_buf_count;
> +	spin_unlock_irqrestore(&inst->state_spinlock, flags);
> +
> +	wave5_update_min_bufs_ctrl(inst, fbc_buf_count);
>  
>  	return 0;
>  }
> @@ -1647,6 +1666,7 @@ static void wave5_vpu_dec_device_run(void *priv)
>  		ret = initialize_sequence(inst);
>  		if (ret) {
>  			unsigned long flags;
> +			u32 fbc_buf_count = 0;
>  
>  			spin_lock_irqsave(&inst->state_spinlock, flags);
>  			if (wave5_is_draining_or_eos(inst) &&
> @@ -1655,14 +1675,18 @@ static void wave5_vpu_dec_device_run(void *priv)
>  
>  				switch_state(inst, VPU_INST_STATE_STOP);
>  
> -				if (vb2_is_streaming(dst_vq))
> +				if (vb2_is_streaming(dst_vq)) {
>  					send_eos_event(inst);
> -				else
> +				} else {
>  					handle_dynamic_resolution_change(inst);
> +					fbc_buf_count = inst->fbc_buf_count;
> +				}
>  
>  				flag_last_buffer_done(inst);
>  			}
>  			spin_unlock_irqrestore(&inst->state_spinlock, flags);
> +
> +			wave5_update_min_bufs_ctrl(inst, fbc_buf_count);
>  		} else {
>  			set_instance_state(inst, VPU_INST_STATE_INIT_SEQ);
>  		}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]