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