Re: [PATCH 1/8] media: qcom: camss: Add streams API support for CSIPHY

[Gjorgji Rosikopulos (Consultant)]

Hi Bryan,

Thanks for the review.

On 9/11/2026 1:37 PM, Bryan O'Donoghue wrote:
> On 11/09/2026 07:22, 
> Gjorgji.Rosikopulos.gjorgji.rosikopulos@xxxxxxxxxxxxxxxx wrote:
>> From: Gjorgji Rosikopulos <gjorgji.rosikopulos@xxxxxxxxxxxxxxxx>
>>
>> Add the V4L2 subdev streams API to the CSIPHY driver: a passthrough
>> routing table (one VC per stream) and NO_STREAM_MIX routing validation.
>>
>> enable/disable_streams pad ops track active streams per pad via a
>> per-pad enabled_streams[] bitmask, so that multiple source-pad
>> consumers can share a single propagated sink stream without redundant
>> or colliding propagation to the sensor. The shared D-PHY lanes are
>> enabled once, on the transition from no active streams to at least
>> one, and disabled once all streams have gone idle.
>>
>> This is opt-in per CSIPHY instance via the new streams_enable resource
>> flag; no platform sets it yet, so CSIPHYs continue to use the legacy
>> non-streams subdev ops unchanged.
>>
>> Signed-off-by: Gjorgji Rosikopulos <gjorgji.rosikopulos@xxxxxxxxxxxxxxxx>
>> ---
>>   .../media/platform/qcom/camss/camss-csiphy.c  | 223 +++++++++++++++++-
>>   .../media/platform/qcom/camss/camss-csiphy.h  |   2 +
>>   2 files changed, 222 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
>> index 539ac4888b60..7e9748e92ab8 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
>> @@ -332,6 +332,112 @@ static int csiphy_set_stream(struct v4l2_subdev *sd, int enable)
>>   	return ret;
>>   }
>>   
>> +/*
>> + * csiphy_pad_enable_streams - Enable one or more streams on the source pad
>> + * @sd: CSIPHY V4L2 subdevice
>> + * @state: V4L2 subdevice state
>> + * @pad: Pad number
>> + * @streams_mask: Bitmask of streams to enable
>> + *
>> + * The shared D-PHY lanes are enabled once, on the transition from no active
>> + * sink streams to at least one. 
> on the transition to an active state.
> 
> The sink stream(s) are propagated upstream to
>> + * the sensor only for the subset that isn't already active,
> only for streams which are inactive
> 
>   so a stream

Thanks that will be incorporated in next patchset.

>> + * that's already running is never redundantly re-propagated.
>> + *
>> + * CSIPHY is only ever linked to a single entity on its source pad, and that
>> + * entity is responsible for only enabling a stream on this pad while it
>> + * itself still needs it, so no cross-consumer refcounting is needed here.
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +static int csiphy_pad_enable_streams(struct v4l2_subdev *sd,
>> +				     struct v4l2_subdev_state *state,
>> +				     u32 pad, u64 streams_mask)
>> +{
>> +	struct csiphy_device *csiphy = v4l2_get_subdevdata(sd);
>> +	struct media_pad *remote_pad =
>> +		media_pad_remote_pad_first(&csiphy->pads[MSM_CSIPHY_PAD_SINK]);
>> +	bool first_arrival = !csiphy->enabled_streams[MSM_CSIPHY_PAD_SINK];
>> +	u64 sink_streams, propagate_mask;
>> +	int ret;
>> +
>> +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad, MSM_CSIPHY_PAD_SINK,
>> +						       &streams_mask);
>> +
>> +	propagate_mask = sink_streams & ~csiphy->enabled_streams[MSM_CSIPHY_PAD_SINK];
>> +
>> +	if (first_arrival) {
>> +		ret = csiphy_set_stream(sd, 1);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	csiphy->enabled_streams[MSM_CSIPHY_PAD_SINK] |= sink_streams;
>> +	csiphy->enabled_streams[pad] |= streams_mask;
>> +
>> +	if (propagate_mask && remote_pad) {
> 
> When is remote pad false ?

Never, but i get some internal AI bot review comments so i have added that check, i agree it can be removed.

> 
>> +		ret = v4l2_subdev_enable_streams(media_entity_to_v4l2_subdev(remote_pad->entity),
>> +						 remote_pad->index, propagate_mask);
>> +		if (ret) {
>> +			csiphy->enabled_streams[MSM_CSIPHY_PAD_SINK] &= ~propagate_mask;
>> +			csiphy->enabled_streams[pad] &= ~streams_mask;
>> +
>> +			if (first_arrival)
>> +				csiphy_set_stream(sd, 0);
>> +
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * csiphy_pad_disable_streams - Disable one or more streams on the source pad
>> + * @sd: CSIPHY V4L2 subdevice
>> + * @state: V4L2 subdevice state
>> + * @pad: Pad number
>> + * @streams_mask: Bitmask of streams to disable
>> + *
>> + * The shared D-PHY lanes, and the propagation to the sensor, are only torn
>> + * down once no sink stream is referenced by any source pad any more.
>> + *
>> + * CSIPHY is only ever linked to a single entity on its source pad, and that
>> + * entity is responsible for only disabling a stream on this pad once it no
>> + * longer needs it, so no cross-consumer refcounting is needed here.
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +static int csiphy_pad_disable_streams(struct v4l2_subdev *sd,
>> +				      struct v4l2_subdev_state *state,
>> +				      u32 pad, u64 streams_mask)
>> +{
>> +	struct csiphy_device *csiphy = v4l2_get_subdevdata(sd);
>> +	struct media_pad *remote_pad =
>> +		media_pad_remote_pad_first(&csiphy->pads[MSM_CSIPHY_PAD_SINK]);
>> +	u64 sink_streams;
>> +	int ret = 0;
>> +
>> +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad, MSM_CSIPHY_PAD_SINK,
>> +						       &streams_mask);
>> +
>> +	csiphy->enabled_streams[pad] &= ~streams_mask;
>> +	csiphy->enabled_streams[MSM_CSIPHY_PAD_SINK] &= ~sink_streams;
>> +
>> +	if (sink_streams && remote_pad)
>> +		ret = v4l2_subdev_disable_streams(media_entity_to_v4l2_subdev(remote_pad->entity),
>> +						  remote_pad->index, sink_streams);
> 
> I'm again questioning when the remote_pad is false i.e. can a PHY have a 
> sink_stream without a remote ?

Never, it will be removed...

>> +
>> +	if (!csiphy->enabled_streams[MSM_CSIPHY_PAD_SINK]) {
>> +		int stream_ret = csiphy_set_stream(sd, 0);
>> +
>> +		if (!ret)
>> +			ret = stream_ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   /*
>>    * __csiphy_get_format - Get pointer to format structure
>>    * @csiphy: CSIPHY device
>> @@ -743,6 +849,71 @@ static int csiphy_link_setup(struct media_entity *entity,
>>   	return 0;
>>   }
>>   
>> +static int csiphy_init_state(struct v4l2_subdev *sd,
>> +			     struct v4l2_subdev_state *state)
>> +{
>> +	struct v4l2_subdev_route routes[] = {
>> +		{
>> +			.sink_pad = MSM_CSIPHY_PAD_SINK,
>> +			.sink_stream = 0,
>> +			.source_pad = MSM_CSIPHY_PAD_SRC,
>> +			.source_stream = 0,
>> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
>> +		},
>> +	};
>> +	struct v4l2_subdev_krouting routing = {
>> +		.num_routes = ARRAY_SIZE(routes),
>> +		.routes = routes,
>> +	};
>> +
>> +	/*
>> +	 * CSIPHY is a transparent D-PHY with no per-VC demux, so every sink
>> +	 * stream (VC) a multi-stream sensor may drive must pass straight
>> +	 * through as the same source stream, or downstream link validation
>> +	 * (e.g. against CSID's multi-pad sink) will flag it as dangling. A
>> +	 * multi-VC sensor is supported by userspace adding further routes via
>> +	 * .set_routing; this default covers the common single-VC case.
>> +	 */
> 
> Drop the references to DPHY - people are actively working on CPHY support.

Noted it will be removed.

> 
>> +	return v4l2_subdev_set_routing(sd, state, &routing);
>> +}
>> +
>> +/*
>> + * csiphy_set_routing - Set routing for the CSIPHY subdev
>> + * @sd: CSIPHY V4L2 subdevice
>> + * @state: V4L2 subdevice state
>> + * @which: Type of format state (V4L2_SUBDEV_FORMAT_ACTIVE or TRY)
>> + * @routing: Routing table to set
>> + *
>> + * CSIPHY is a transparent D-PHY with no per-VC demux, so every route must
>> + * pass a sink stream straight through as the same source stream.
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +static int csiphy_set_routing(struct v4l2_subdev *sd,
>> +			      struct v4l2_subdev_state *state,
>> +			      enum v4l2_subdev_format_whence which,
>> +			      struct v4l2_subdev_krouting *routing)
>> +{
>> +	struct csiphy_device *csiphy = v4l2_get_subdevdata(sd);
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	if (which == V4L2_SUBDEV_FORMAT_ACTIVE && csiphy->enabled_streams[MSM_CSIPHY_PAD_SINK])
>> +		return -EBUSY;
> 
> Can this happen ?
> 
> dev_err() ?

I need to check the core but there should be protection to not be able to set the routing while stream is active.
If that is the case i will remove this check.

> 
>> +
>> +	for (i = 0; i < routing->num_routes; i++)
>> +		if (routing->routes[i].sink_stream != routing->routes[i].source_stream)
>> +			return -EINVAL;
>> +
>> +	ret = v4l2_subdev_routing_validate(sd, routing,
>> +					   V4L2_SUBDEV_ROUTING_NO_STREAM_MIX |
>> +					   V4L2_SUBDEV_ROUTING_NO_N_TO_1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return v4l2_subdev_set_routing(sd, state, routing);
>> +}
>> +
>>   static const struct v4l2_subdev_core_ops csiphy_core_ops = {
>>   	.s_power = csiphy_set_power,
>>   };
>> @@ -764,10 +935,35 @@ static const struct v4l2_subdev_ops csiphy_v4l2_ops = {
>>   	.pad = &csiphy_pad_ops,
>>   };
>>   
>> +static const struct v4l2_subdev_pad_ops csiphy_streams_pad_ops = {
>> +	.enum_mbus_code = csiphy_enum_mbus_code,
>> +	.enum_frame_size = csiphy_enum_frame_size,
>> +	.get_fmt = csiphy_get_format,
>> +	.set_fmt = csiphy_set_format,
>> +	.get_frame_desc = v4l2_subdev_get_frame_desc_passthrough,
>> +	.set_routing = csiphy_set_routing,
>> +	.enable_streams = csiphy_pad_enable_streams,
>> +	.disable_streams = csiphy_pad_disable_streams,
>> +};
>> +
>> +static const struct v4l2_subdev_video_ops csiphy_streams_video_ops = {
>> +	.s_stream = v4l2_subdev_s_stream_helper,
>> +};
>> +
>> +static const struct v4l2_subdev_ops csiphy_streams_v4l2_ops = {
>> +	.core = &csiphy_core_ops,
>> +	.pad = &csiphy_streams_pad_ops,
>> +	.video = &csiphy_streams_video_ops,
>> +};
>> +
>>   static const struct v4l2_subdev_internal_ops csiphy_v4l2_internal_ops = {
>>   	.open = csiphy_init_formats,
>>   };
>>   
>> +static const struct v4l2_subdev_internal_ops csiphy_streams_internal_ops = {
>> +	.init_state = csiphy_init_state,
>> +};
>> +
>>   static const struct media_entity_operations csiphy_media_ops = {
>>   	.link_setup = csiphy_link_setup,
>>   	.link_validate = v4l2_subdev_link_validate,
>> @@ -786,11 +982,16 @@ int msm_csiphy_register_entity(struct csiphy_device *csiphy,
>>   	struct v4l2_subdev *sd = &csiphy->subdev;
>>   	struct media_pad *pads = csiphy->pads;
>>   	struct device *dev = csiphy->camss->dev;
>> +	bool streams_api = csiphy->res->streams_enable;
> 
> Is this really a feature of the CSIPHY though ?
> 
> Its the CSI decoder that has to program registers in itself to trap the 
> appropriate VC and route it.
> 
> Its a supported feature for a particular SoC.
> 
> This flag should live in struct camss_resources and you have a pointer o 
> that structure in this routine anyway so..

Hmm yes there should not be separate flag for each sub-device becouse either the whole topolgy
supports multistream or not. I will move the flag in camss_resourcess. Thanks 

> 
>>   	int ret;
>>   
>> -	v4l2_subdev_init(sd, &csiphy_v4l2_ops);
>> -	sd->internal_ops = &csiphy_v4l2_internal_ops;
>> +	v4l2_subdev_init(sd, streams_api ? &csiphy_streams_v4l2_ops
>> +					      : &csiphy_v4l2_ops);
>> +	sd->internal_ops = streams_api ? &csiphy_streams_internal_ops
>> +					: &csiphy_v4l2_internal_ops;
>>   	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	if (streams_api)
>> +		sd->flags |= V4L2_SUBDEV_FL_STREAMS;
>>   	snprintf(sd->name, ARRAY_SIZE(sd->name), "%s%d",
>>   		 MSM_CSIPHY_NAME, csiphy->id);
>>   	sd->grp_id = CSIPHY_GRP_ID;
>> @@ -813,12 +1014,27 @@ int msm_csiphy_register_entity(struct csiphy_device *csiphy,
>>   		return ret;
>>   	}
>>   
>> +	if (streams_api) {
>> +		ret = v4l2_subdev_init_finalize(sd);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to finalize subdev: %d\n", ret);
>> +			goto err_media_entity_cleanup;
>> +		}
>> +	}
>> +
>>   	ret = v4l2_device_register_subdev(v4l2_dev, sd);
>>   	if (ret < 0) {
>>   		dev_err(dev, "Failed to register subdev: %d\n", ret);
>> -		media_entity_cleanup(&sd->entity);
>> +		goto err_v4l2_subdev_cleanup;
>>   	}
>>   
>> +	return 0;
>> +
>> +err_v4l2_subdev_cleanup:
>> +	v4l2_subdev_cleanup(sd);
>> +err_media_entity_cleanup:
>> +	media_entity_cleanup(&sd->entity);
>> +
>>   	return ret;
>>   }
>>   
>> @@ -829,5 +1045,6 @@ int msm_csiphy_register_entity(struct csiphy_device *csiphy,
>>   void msm_csiphy_unregister_entity(struct csiphy_device *csiphy)
>>   {
>>   	v4l2_device_unregister_subdev(&csiphy->subdev);
>> +	v4l2_subdev_cleanup(&csiphy->subdev);
>>   	media_entity_cleanup(&csiphy->subdev.entity);
>>   }
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
>> index 9d9657b82f74..e55c098dbe67 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
>> @@ -84,6 +84,7 @@ struct csiphy_hw_ops {
>>   
>>   struct csiphy_subdev_resources {
>>   	u8 id;
>> +	bool streams_enable;
>>   	const struct csiphy_hw_ops *hw_ops;
>>   	const struct csiphy_formats *formats;
>>   };
>> @@ -114,6 +115,7 @@ struct csiphy_device {
>>   	struct v4l2_mbus_framefmt fmt[MSM_CSIPHY_PADS_NUM];
>>   	const struct csiphy_subdev_resources *res;
>>   	struct csiphy_device_regs *regs;
>> +	u64 enabled_streams[MSM_CSIPHY_PADS_NUM];
>>   };
>>   
>>   struct camss_subdev_resources;
>