Sakari,
On 6/3/24 5:43 PM, Sakari Ailus wrote:
> CSI-2 sub-device streaming control should use {enable,disable}_streams pad
> ops and not s_stream video ops as the sub-device supports streams. Fix
> this by removing driver-implemented stream management and moving sensor
> streaming control to the CSI-2 sub-device sub-driver.
>
> Fixes: a11a5570a09d ("media: intel/ipu6: add IPU6 CSI2 receiver v4l2 sub-device")
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> since v1:
>
> - Removed unused variables.
>
> drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 98 ++++++++-----------
> drivers/media/pci/intel/ipu6/ipu6-isys-csi2.h | 2 -
> .../media/pci/intel/ipu6/ipu6-isys-queue.c | 3 -
> .../media/pci/intel/ipu6/ipu6-isys-video.c | 43 ++------
> 4 files changed, 49 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> index b9ce4324996d..051898ce53f4 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> @@ -345,42 +345,61 @@ static int ipu6_isys_csi2_set_stream(struct v4l2_subdev *sd,
> return ret;
> }
>
> -static int set_stream(struct v4l2_subdev *sd, int enable)
> +static int ipu6_isys_csi2_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + u32 pad, u64 streams_mask)
> {
> struct ipu6_isys_subdev *asd = to_ipu6_isys_subdev(sd);
> struct ipu6_isys_csi2 *csi2 = to_ipu6_isys_csi2(asd);
> - struct device *dev = &csi2->isys->adev->auxdev.dev;
> struct ipu6_isys_csi2_timing timing = { };
> - unsigned int nlanes;
> + struct v4l2_subdev *remote_sd;
> + struct media_pad *remote_pad;
> + u64 sink_streams;
> int ret;
>
> - dev_dbg(dev, "csi2 stream %s callback\n", enable ? "on" : "off");
> -
> - if (!enable) {
> - csi2->stream_count--;
> - if (csi2->stream_count)
> - return 0;
> -
> - ipu6_isys_csi2_set_stream(sd, &timing, 0, enable);
> - return 0;
> - }
> -
> - if (csi2->stream_count) {
> - csi2->stream_count++;
> - return 0;
> - }
> + remote_pad = media_pad_remote_pad_first(&sd->entity.pads[CSI2_PAD_SINK]);
> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
>
> - nlanes = csi2->nlanes;
> + sink_streams = v4l2_subdev_state_xlate_streams(state, CSI2_PAD_SRC,
> + CSI2_PAD_SINK,
> + &streams_mask);
>
> ret = ipu6_isys_csi2_calc_timing(csi2, &timing, CSI2_ACCINV);
> if (ret)
> return ret;
>
> - ret = ipu6_isys_csi2_set_stream(sd, &timing, nlanes, enable);
> + ret = ipu6_isys_csi2_set_stream(sd, &timing, csi2->nlanes, true);
In theory, ipu6_isys_csi2_set_stream is only called once per CSI2 port
streaming, so this patch looks breaking the frame+meta capture case
Hongju did some test along with this patch and see some regressions.
Do we need script update for this patch to capture metadata?
> if (ret)
> return ret;
>
> - csi2->stream_count++;
> + ret = v4l2_subdev_enable_streams(remote_sd, remote_pad->index,
> + sink_streams);
> + if (ret) {
> + ipu6_isys_csi2_set_stream(sd, NULL, 0, false);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ipu6_isys_csi2_disable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + u32 pad, u64 streams_mask)
> +{
> + struct v4l2_subdev *remote_sd;
> + struct media_pad *remote_pad;
> + u64 sink_streams;
> +
> + sink_streams = v4l2_subdev_state_xlate_streams(state, CSI2_PAD_SRC,
> + CSI2_PAD_SINK,
> + &streams_mask);
> +
> + remote_pad = media_pad_remote_pad_first(&sd->entity.pads[CSI2_PAD_SINK]);
> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
> +
> + ipu6_isys_csi2_set_stream(sd, NULL, 0, false);
> +
> + v4l2_subdev_disable_streams(remote_sd, remote_pad->index, sink_streams);
>
> return 0;
> }
> @@ -475,10 +494,6 @@ static int ipu6_isys_csi2_get_sel(struct v4l2_subdev *sd,
> return ret;
> }
>
> -static const struct v4l2_subdev_video_ops csi2_sd_video_ops = {
> - .s_stream = set_stream,
> -};
> -
> static const struct v4l2_subdev_pad_ops csi2_sd_pad_ops = {
> .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = ipu6_isys_subdev_set_fmt,
> @@ -486,11 +501,12 @@ static const struct v4l2_subdev_pad_ops csi2_sd_pad_ops = {
> .set_selection = ipu6_isys_csi2_set_sel,
> .enum_mbus_code = ipu6_isys_subdev_enum_mbus_code,
> .set_routing = ipu6_isys_subdev_set_routing,
> + .enable_streams = ipu6_isys_csi2_enable_streams,
> + .disable_streams = ipu6_isys_csi2_disable_streams,
> };
>
> static const struct v4l2_subdev_ops csi2_sd_ops = {
> .core = &csi2_sd_core_ops,
> - .video = &csi2_sd_video_ops,
> .pad = &csi2_sd_pad_ops,
> };
>
> @@ -631,33 +647,3 @@ int ipu6_isys_csi2_get_remote_desc(u32 source_stream,
>
> return 0;
> }
> -
> -void ipu6_isys_set_csi2_streams_status(struct ipu6_isys_video *av, bool status)
> -{
> - struct ipu6_isys_stream *stream = av->stream;
> - struct v4l2_subdev *sd = &stream->asd->sd;
> - struct v4l2_subdev_state *state;
> - struct media_pad *r_pad;
> - unsigned int i;
> - u32 r_stream;
> -
> - r_pad = media_pad_remote_pad_first(&av->pad);
> - r_stream = ipu6_isys_get_src_stream_by_src_pad(sd, r_pad->index);
> -
> - state = v4l2_subdev_lock_and_get_active_state(sd);
> -
> - for (i = 0; i < state->stream_configs.num_configs; i++) {
> - struct v4l2_subdev_stream_config *cfg =
> - &state->stream_configs.configs[i];
> -
> - if (cfg->pad == r_pad->index && r_stream == cfg->stream) {
> - dev_dbg(&av->isys->adev->auxdev.dev,
> - "%s: pad:%u, stream:%u, status:%u\n",
> - sd->entity.name, r_pad->index, r_stream,
> - status);
> - cfg->enabled = status;
> - }
> - }
> -
> - v4l2_subdev_unlock_state(state);
> -}
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.h b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.h
> index eba6b29386ea..bc8594c94f99 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.h
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.h
> @@ -45,7 +45,6 @@ struct ipu6_isys_csi2 {
> u32 receiver_errors;
> unsigned int nlanes;
> unsigned int port;
> - unsigned int stream_count;
> };
>
> struct ipu6_isys_csi2_timing {
> @@ -77,6 +76,5 @@ int ipu6_isys_csi2_get_remote_desc(u32 source_stream,
> struct ipu6_isys_csi2 *csi2,
> struct media_entity *source_entity,
> struct v4l2_mbus_frame_desc_entry *entry);
> -void ipu6_isys_set_csi2_streams_status(struct ipu6_isys_video *av, bool status);
>
> #endif /* IPU6_ISYS_CSI2_H */
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> index 40a8ebfcfce2..6bea7754875d 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> @@ -551,7 +551,6 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
> stream->nr_queues);
>
> list_add(&aq->node, &stream->queues);
> - ipu6_isys_set_csi2_streams_status(av, true);
> ipu6_isys_configure_stream_watermark(av, true);
> ipu6_isys_update_stream_watermark(av, true);
>
> @@ -598,8 +597,6 @@ static void stop_streaming(struct vb2_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);
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> index c8a33e1e910c..e41d40243abd 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -990,9 +990,7 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> struct v4l2_subdev_state *subdev_state;
> struct device *dev = &av->isys->adev->auxdev.dev;
> struct v4l2_subdev *sd;
> - struct v4l2_subdev *ssd;
> struct media_pad *r_pad;
> - struct media_pad *s_pad;
> u32 sink_pad, sink_stream;
> u64 r_stream;
> u64 stream_mask = 0;
> @@ -1003,7 +1001,6 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> if (WARN(!stream->source_entity, "No source entity for stream\n"))
> return -ENODEV;
>
> - ssd = media_entity_to_v4l2_subdev(stream->source_entity);
> sd = &stream->asd->sd;
> r_pad = media_pad_remote_pad_first(&av->pad);
> r_stream = ipu6_isys_get_src_stream_by_src_pad(sd, r_pad->index);
> @@ -1017,27 +1014,15 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> if (ret)
> return ret;
>
> - s_pad = media_pad_remote_pad_first(&stream->asd->pad[sink_pad]);
> -
> stream_mask = get_stream_mask_by_pipeline(av);
> if (!state) {
> stop_streaming_firmware(av);
>
> - /* stop external sub-device now. */
> - dev_dbg(dev, "disable streams 0x%llx of %s\n", stream_mask,
> - ssd->name);
> - ret = v4l2_subdev_disable_streams(ssd, s_pad->index,
> - stream_mask);
> - if (ret) {
> - dev_err(dev, "disable streams of %s failed with %d\n",
> - ssd->name, ret);
> - return ret;
> - }
> -
> /* stop sub-device which connects with video */
> - dev_dbg(dev, "stream off entity %s pad:%d\n", sd->name,
> - r_pad->index);
> - ret = v4l2_subdev_call(sd, video, s_stream, state);
> + dev_dbg(dev, "stream off entity %s pad:%d mask:0x%llx\n",
> + sd->name, r_pad->index, stream_mask);
> + ret = v4l2_subdev_disable_streams(sd, r_pad->index,
> + stream_mask);
> if (ret) {
> dev_err(dev, "stream off %s failed with %d\n", sd->name,
> ret);
> @@ -1052,34 +1037,20 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> }
>
> /* start sub-device which connects with video */
> - dev_dbg(dev, "stream on %s pad %d\n", sd->name, r_pad->index);
> - ret = v4l2_subdev_call(sd, video, s_stream, state);
> + dev_dbg(dev, "stream on %s pad %d mask 0x%llx\n", sd->name,
> + r_pad->index, stream_mask);
> + ret = v4l2_subdev_enable_streams(sd, r_pad->index, stream_mask);
> if (ret) {
> dev_err(dev, "stream on %s failed with %d\n", sd->name,
> ret);
> goto out_media_entity_stop_streaming_firmware;
> }
> -
> - /* start external sub-device now. */
> - dev_dbg(dev, "enable streams 0x%llx of %s\n", stream_mask,
> - ssd->name);
> - ret = v4l2_subdev_enable_streams(ssd, s_pad->index,
> - stream_mask);
> - if (ret) {
> - dev_err(dev,
> - "enable streams 0x%llx of %s failed with %d\n",
> - stream_mask, stream->source_entity->name, ret);
> - goto out_media_entity_stop_streaming;
> - }
> }
>
> av->streaming = state;
>
> return 0;
>
> -out_media_entity_stop_streaming:
> - v4l2_subdev_disable_streams(sd, r_pad->index, BIT(r_stream));
> -
> out_media_entity_stop_streaming_firmware:
> stop_streaming_firmware(av);
>
>
--
Best regards,
Bingbu Cao
