On Thu, 2023-07-27 at 15:15 +0800, bingbu.cao@xxxxxxxxx wrote:
> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>
> Register v4l2 video device and setup the vb2 queue to
> support basic video capture. Video streaming callback
> will trigger the input system driver to construct a
> input system stream configuration for firmware based on
> data type and stream ID and then queue buffers to firmware
> to do capture.
...
> +static int start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> + struct ipu6_isys_queue *aq = vb2_queue_to_ipu6_isys_queue(q);
> + struct ipu6_isys_video *av = ipu6_isys_queue_to_video(aq);
> + struct device *dev = &av->isys->adev->auxdev.dev;
> + struct ipu6_isys_buffer_list __bl, *bl = NULL;
> + struct ipu6_isys_stream *stream;
> + struct media_entity *source_entity = NULL;
> + int nr_queues, ret;
> +
> + dev_dbg(dev, "stream: %s: width %u, height %u, css
> pixelformat %u\n",
> + av->vdev.name, av->mpix.width, av->mpix.height,
> + av->pfmt->css_pixelformat);
> +
> + ret = ipu6_isys_setup_video(av, &source_entity, &nr_queues);
> + if (ret < 0) {
> + dev_err(dev, "failed to setup video\n");
> + goto out_return_buffers;
> + }
> +
> + ret = aq->link_fmt_validate(aq);
> + if (ret) {
> + dev_err(dev,
> + "%s: link format validation failed (%d)\n",
> + av->vdev.name, ret);
> + goto out_pipeline_stop;
> + }
> +
> + ret = ipu6_isys_fw_open(av->isys);
> + if (ret)
> + goto out_pipeline_stop;
> +
> + stream = av->stream;
...
> +out_pipeline_stop:
> + video_device_pipeline_stop(&av->vdev);
> + ipu6_isys_put_stream(stream);
> + av->stream = NULL;
if ipu6_isys_fw_open fails, stream is uninitialized here. Should be av-
>stream.
I think it would be cleaner if you wrapped the above three lines in a
cleanup function that cleans up whatever ipu6_isys_setup_video does.
That function could probably also be used in stop_streaming, where the
same three lines are duplicated (although with other code in between,
so may not be trivial). See below:
...
> +static void stop_streaming(struct vb2_queue *q)
> +{
> + struct ipu6_isys_queue *aq = vb2_queue_to_ipu6_isys_queue(q);
> + struct ipu6_isys_video *av = ipu6_isys_queue_to_video(aq);
> + struct ipu6_isys_stream *stream = av->stream;
> +
> + ipu6_isys_set_csi2_streams_status(av, false);
> +
> + mutex_lock(&stream->mutex);
> +
> + ipu6_isys_update_stream_watermark(av, false);
> +
> + mutex_lock(&av->isys->stream_mutex);
> + if (stream->nr_streaming == stream->nr_queues && stream-
> >streaming)
> + ipu6_isys_video_set_streaming(av, 0, NULL);
> + mutex_unlock(&av->isys->stream_mutex);
> +
> + video_device_pipeline_stop(&av->vdev);
> + av->stream = NULL;
> +
> + stream->nr_streaming--;
> + list_del(&aq->node);
> + stream->streaming = 0;
> +
> + mutex_unlock(&stream->mutex);
> + ipu6_isys_put_stream(stream);
> +
> + return_buffers(aq, VB2_BUF_STATE_ERROR);
> +
> + ipu6_isys_fw_close(av->isys);
> +}
/Andreas
