Re: [PATCH 5/9] media: camss: csid: report frame starts from CSID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: camss: vfe_buf_done() never resets frame_started for next frame
- [Medium] media: camss: vfe: inconsistent event ioctls when frame_start is unset
--

commit 7457e598955f906dac687d34057f24b9b8d1d505
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

media: camss: csid: report frame starts from CSID

This commit handles CSID generation 980 start-of-frame interrupts and
forwards them to the paired VFE via camss_frame_start(). The VFE queues a
V4L2_EVENT_FRAME_SYNC event on the subdev node, tracks frame sequence numbers
at frame start, and checks whether a frame started before marking a completed
buffer as done.

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index d1b66033a096d..f42fc56e7dfc9 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -444,6 +444,34 @@ u32 vfe_hw_version(struct vfe_device *vfe)
>  	return hw_version;
>  }
>  
> +void vfe_frame_start(struct vfe_device *vfe, int wm)
> +{
> +	struct v4l2_event event = { .type = V4L2_EVENT_FRAME_SYNC };
> +	struct vfe_output *output;
> +	struct vfe_line *line;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vfe->output_lock, flags);
> +	if (vfe->wm_output_map[wm] == VFE_LINE_NONE)
> +		goto out_unlock;
> +
> +	line = &vfe->line[vfe->wm_output_map[wm]];
> +	output = &line->output;
> +	if (output->state != VFE_OUTPUT_ON)
> +		goto out_unlock;
> +
> +	/* Count frames even when there is no buffer available for capture. */
> +	event.u.frame_sync.frame_sequence = output->sequence++;
> +	output->frame_started = true;
> +	spin_unlock_irqrestore(&vfe->output_lock, flags);
> +
> +	v4l2_event_queue(line->subdev.devnode, &event);
> +	return;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&vfe->output_lock, flags);
> +}
> +
>  /*
>   * vfe_buf_done - Process write master done interrupt
>   * @vfe: VFE Device
> @@ -478,7 +506,13 @@ void vfe_buf_done(struct vfe_device *vfe, int wm)
>  	}
>  
>  	ready_buf->vb.vb2_buf.timestamp = ts;
> -	ready_buf->vb.sequence = output->sequence++;
> +	if (ops->frame_start) {
> +		ready_buf->vb.sequence = output->sequence - 1;
> +		if (!output->frame_started)
> +			state = VB2_BUF_STATE_ERROR;
> +	} else {
> +		ready_buf->vb.sequence = output->sequence++;
> +	}

[Severity: Medium]
Should vfe_buf_done() reset output->frame_started to false after consuming
it for the current buffer?

In vfe_enable_output_v2(), output->sequence is initialized to 0 and
output->frame_started is set to false. When csid_isr() -> camss_frame_start()
-> vfe_frame_start() runs on the first start-of-frame interrupt, it sets
output->frame_started = true.

Because vfe_buf_done() never clears output->frame_started back to false,
output->frame_started remains true for the rest of the streaming session. On
subsequent frames, if a buffer completion interrupt arrives before that
frame's start-of-frame interrupt, !output->frame_started will not trigger,
leaving state as VB2_BUF_STATE_DONE with a duplicate sequence number
(output->sequence - 1).

Also, if a buffer completes before the very first start-of-frame interrupt
while output->sequence is still 0, does ready_buf->vb.sequence =
output->sequence - 1 underflow unsigned int to UINT_MAX?

[ ... ]

> @@ -1972,8 +2007,23 @@ static int vfe_link_setup(struct media_entity *entity,
>  	return 0;
>  }
>  
> +static int vfe_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> +			       struct v4l2_event_subscription *sub)
> +{
> +	struct vfe_line *line = v4l2_get_subdevdata(sd);
> +	struct vfe_device *vfe = to_vfe(line);
> +
> +	if (!vfe->res->hw_ops->frame_start ||
> +	    sub->type != V4L2_EVENT_FRAME_SYNC || sub->id)
> +		return -EINVAL;
> +
> +	return v4l2_event_subscribe(fh, sub, 16, NULL);
> +}
> +
>  static const struct vfe_subdev_core_ops vfe_core_ops = {
>  	.s_power = vfe_set_power,
> +	.subscribe_event = vfe_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  };
>  
>  static const struct v4l2_subdev_video_ops vfe_video_ops = {
> @@ -2080,6 +2130,8 @@ int msm_vfe_register_entities(struct vfe_device *vfe,
>  		v4l2_subdev_init(sd, &vfe_v4l2_ops);
>  		sd->internal_ops = &vfe_v4l2_internal_ops;
>  		sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +		if (vfe->res->hw_ops->frame_start)
> +			sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;

[Severity: Medium]
On VFE hardware generations where vfe->res->hw_ops->frame_start is NULL,
msm_vfe_register_entities() does not set V4L2_SUBDEV_FL_HAS_EVENTS on the
subdevice, but vfe_core_ops still wires up .subscribe_event and
.unsubscribe_event unconditionally.

In drivers/media/v4l2-core/v4l2-subdev.c:subdev_do_ioctl(), the core checks
for those callbacks before checking V4L2_SUBDEV_FL_HAS_EVENTS:

	case VIDIOC_DQEVENT:
		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
			return -ENOIOCTLCMD;

		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);

	case VIDIOC_SUBSCRIBE_EVENT:
		if (v4l2_subdev_has_op(sd, core, subscribe_event))
			return v4l2_subdev_call(sd, core, subscribe_event,
						vfh, arg);
		...
		return -ENOIOCTLCMD;

	case VIDIOC_UNSUBSCRIBE_EVENT:
		if (v4l2_subdev_has_op(sd, core, unsubscribe_event))
			return v4l2_subdev_call(sd, core, unsubscribe_event,
						vfh, arg);

		if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)
			return v4l2_event_subdev_unsubscribe(sd, vfh, arg);

		return -ENOIOCTLCMD;

When vfe->res->hw_ops->frame_start is NULL, VIDIOC_DQEVENT returns
-ENOIOCTLCMD (-ENOTTY), whereas VIDIOC_SUBSCRIBE_EVENT calls
vfe_subscribe_event() and returns -EINVAL, and VIDIOC_UNSUBSCRIBE_EVENT calls
v4l2_event_subdev_unsubscribe() and returns 0.

Should vfe_subscribe_event() return -ENOIOCTLCMD when
!vfe->res->hw_ops->frame_start, and could .unsubscribe_event be omitted from
vfe_core_ops so subdev_do_ioctl() falls back to
v4l2_event_subdev_unsubscribe() only when V4L2_SUBDEV_FL_HAS_EVENTS is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028034105.326535-1-xiaqinghuan@mail.ustc.edu.cn?part=5