Re: [PATCH 6/8] media: qcom: camss: refcount streaming on the shared CSIPHY and CSID

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 14/09/2026 14:34, Hitesh Patel wrote:
> video_start_streaming() walks the pipeline from the video node
> upstream and calls video.s_stream on every subdev it finds;
> video_stop_streaming() does the same to stop. The core allows one
> transition per subdev: call_s_stream() keeps a single
> sd->s_stream_enabled flag and warns about, and drops, a start of a
> subdev that is already started or a stop of one already stopped.
> 
> That is correct for a subdev with a single user, but the CSIPHY and
> the CSID are shared when a CSI-2 transmitter aggregates several
> cameras onto one port. A MAX9296A GMSL deserializer sends two
> cameras on one CSI-2 output as two virtual channels; the CSID
> demultiplexes them to RDI0 and RDI1, each of which is its own VFE
> line, video node and thus pipeline, and both pipelines traverse the
> same CSIPHY and CSID. Starting the second camera hits the core check:
> the CSIPHY and CSID s_stream(1) are dropped with a WARN, and while
> the hardware happens to be already running, stopping the first
> camera then calls s_stream(0) on both and tears the CSIPHY and CSID
> down underneath the second camera, which stops receiving frames.
> 
> Count the pipelines streaming through each CSIPHY and CSID and only
> forward the first start and the last stop to the subdev. The count
> is updated under the media graph mutex, which serialises the two
> pipelines' start/stop against each other. All other subdevs of the
> pipeline are driven exactly as before, so the ordinary one camera
> per port case does not change.
> 
> csid_set_stream() programs every virtual channel of the en_vc mask
> in one go, so a single start already covers all demultiplexed RDIs;
> nothing needs to change on the CSID or CSIPHY side.
> 
> Signed-off-by: Hitesh Patel <hitesh@ebytelogic.com>
> ---
>   .../media/platform/qcom/camss/camss-csid.h    |  2 +
>   .../media/platform/qcom/camss/camss-csiphy.h  |  2 +
>   .../media/platform/qcom/camss/camss-video.c   | 48 +++++++++++++++++--
>   drivers/media/platform/qcom/camss/camss.c     | 28 +++++++----
>   drivers/media/platform/qcom/camss/camss.h     |  2 +
>   5 files changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index 5296b10f6..9e612ae99 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -167,6 +167,8 @@ struct csid_device {
>   	struct v4l2_ctrl_handler ctrls;
>   	struct v4l2_ctrl *testgen_mode;
>   	const struct csid_subdev_resources *res;
> +	/* Number of pipelines streaming through this CSID */
> +	unsigned int stream_users;
>   };
>   
>   struct camss_subdev_resources;
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
> index 9d9657b82..b920fe670 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
> @@ -114,6 +114,8 @@ struct csiphy_device {
>   	struct v4l2_mbus_framefmt fmt[MSM_CSIPHY_PADS_NUM];
>   	const struct csiphy_subdev_resources *res;
>   	struct csiphy_device_regs *regs;
> +	/* Number of pipelines streaming through this CSIPHY */
> +	unsigned int stream_users;
>   };
>   
>   struct camss_subdev_resources;
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 0852eb6f1..16c5f3748 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -249,6 +249,49 @@ static int video_prepare_streaming(struct vb2_queue *q)
>   	return ret;
>   }
>   
> +/*
> + * video_subdev_set_stream - Start or stop a subdev of the pipeline
> + * @video: CAMSS video device
> + * @subdev: Subdevice to start or stop
> + * @enable: Start when true, stop when false
> + *
> + * CSIPHY and CSID are shared between pipelines when a transmitter aggregates
> + * several cameras onto one CSI-2 port. The core allows a single s_stream
> + * transition per subdev, so only forward the first start and the last stop
> + * to them. Every other subdev is driven unconditionally as before.
> + */
> +static int video_subdev_set_stream(struct camss_video *video,
> +				   struct v4l2_subdev *subdev, bool enable)
> +{
> +	struct media_device *mdev = &video->camss->media_dev;
> +	unsigned int *users;
> +	bool forward;
> +	int ret;
> +
> +	users = camss_subdev_stream_users(video->camss, subdev);
> +	if (!users)
> +		return v4l2_subdev_call(subdev, video, s_stream, enable);

This logic is mad and genuinely hurts my head a bit to read.

> +	mutex_lock(&mdev->graph_mutex);
> +	if (enable)
> +		forward = (*users)++ == 0;
> +	else
> +		forward = !WARN_ON(!*users) && --(*users) == 0;
> +	mutex_unlock(&mdev->graph_mutex);

You can radically simplify the logic here to

if (enable) {
	forward = *users == 0;
	*users++;
} else {
	forward = *users == 1;
	*users--;
}

I checked ;)

[] ~ gcc -o zed zed.c
[] ~ ./zed
Enable users 0 = 1
Enable users simple 0 = 1
Disable users 0 = 0
Disable users simple 0 = 0
Enable users 1 = 0
Enable users simple 1 = 0
Disable users 1 = 1
Disable users simple 1 = 1
Enable users 2 = 0
Enable users simple 2 = 0
Disable users 2 = 0
Disable users simple 2 = 0
Enable users 3 = 0
Enable users simple 3 = 0
Disable users 3 = 0
Disable users simple 3 = 0
[] ~ cat zed.c
#include <stdio.h>

bool fw(unsigned int users, bool enable)
{
	bool forward;

	if (enable)
		forward = (users)++ == 0;
	else
		forward = !(!users) && --(users) == 0;

	return forward;
}

bool simple_fw(unsigned int users, bool enable)
{
	if (enable)
		return users == 0;
	else
		return users == 1;
}

int main(int argc, char *argv[])
{
	unsigned int users[] = {0, 1, 2, 3};
	int i;

	for (i = 0; i < sizeof(users)/sizeof(unsigned int); i++) {
		printf("Enable users %d = %d\n", users[i], fw(users[i], true));
		printf("Enable users simple %d = %d\n", users[i], simple_fw(users[i], 
true));
		printf("Disable users %d = %d\n", users[i], fw(users[i], false));
		printf("Disable users simple %d = %d\n", users[i], simple_fw(users[i], 
false));
	}

	return 0;
}

In other words please make the code less cryptographic and more human 
readable.

> +
> +	if (!forward)
> +		return 0;
> +
> +	ret = v4l2_subdev_call(subdev, video, s_stream, enable);
> +	if (enable && ret < 0 && ret != -ENOIOCTLCMD) {
> +		mutex_lock(&mdev->graph_mutex);
> +		(*users)--;
> +		mutex_unlock(&mdev->graph_mutex);
> +	}

So taking the graph_mutex here is to stop re-entrancy of two parallel 
streams and only in this function too.

I have to say I distrust locking like this and its racy.

thread0:              thread1:
mutex_lock();
*users = 1
forward = 1;
mutex_unlock();
sleep
                       mutex_lock();
                       *users == 1;
                       forward = 0;
                       mutex_unlock();

                       if (!forward)
                           return 0;

/* sometime later */
v4l2_subdev_call();

Thread1 has returned from set_stream in a non-error state but nothing 
has been set to stream.

The set_stream() call associated with thread0 is reporting a truthful 
state, the set_stream() call from thread1 is not.

Since this is the only ? place you want to modify *users you can add a 
new mutex for that and hold it for the duration of the critical section.

> +
> +	return ret;
> +}
> +
>   static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>   {
>   	struct camss_video *video = vb2_get_drv_priv(q);
> @@ -281,7 +324,7 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>   		entity = pad->entity;
>   		subdev = media_entity_to_v4l2_subdev(entity);
>   
> -		ret = v4l2_subdev_call(subdev, video, s_stream, 1);
> +		ret = video_subdev_set_stream(video, subdev, true);
>   		if (ret < 0 && ret != -ENOIOCTLCMD)
>   			goto error;
>   	}
> @@ -319,8 +362,7 @@ static void video_stop_streaming(struct vb2_queue *q)
>   		entity = pad->entity;
>   		subdev = media_entity_to_v4l2_subdev(entity);
>   
> -		ret = v4l2_subdev_call(subdev, video, s_stream, 0);
> -
> +		ret = video_subdev_set_stream(video, subdev, false);
>   		if (ret) {
>   			dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
>   			return;
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 4cf736d80..ca8101c2d 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -4620,27 +4620,39 @@ struct media_pad *camss_find_sensor_pad(struct media_entity *entity)
>   }
>   
>   /*
> - * camss_is_receiver_subdev - Test whether a subdev is a CAMSS CSI-2 receiver
> + * camss_subdev_stream_users - Streaming user count of a CAMSS receiver subdev
>    * @camss: CAMSS device
>    * @sd: Subdevice to test
>    *
> - * Return true for a CSIPHY or CSID belonging to @camss, false for anything
> - * else, in particular for the external subdev transmitting to them.
> + * CSIPHY and CSID are traversed by several pipelines at once when a CSI-2
> + * transmitter aggregates several cameras onto one port: every virtual channel
> + * is demultiplexed to its own RDI and forms its own pipeline. The hardware
> + * must only be started by the first of them and stopped by the last.
> + *
> + * Return a pointer to the user count of @sd if it is a CSIPHY or CSID of
> + * @camss, NULL for any other subdev, in particular for the external subdev
> + * transmitting to them.
>    */
> -static bool camss_is_receiver_subdev(struct camss *camss,
> -				     struct v4l2_subdev *sd)
> +unsigned int *camss_subdev_stream_users(struct camss *camss,
> +					struct v4l2_subdev *sd)

I'm all for progressive changes but I wonder if you could have dispensed 
with introducing camss_is_receiver_subdev() given you only added it ~ 2 
patches ago.

>   {
>   	unsigned int i;

You need to assert lockdep held for the new mutex here.
>   	for (i = 0; i < camss->res->csiphy_num; i++)
>   		if (sd == &camss->csiphy[i].subdev)
> -			return true;
> +			return &camss->csiphy[i].stream_users;
>   
>   	for (i = 0; i < camss->res->csid_num; i++)
>   		if (sd == &camss->csid[i].subdev)
> -			return true;
> +			return &camss->csid[i].stream_users;
>   
> -	return false;
> +	return NULL;
> +}
> +
> +static bool camss_is_receiver_subdev(struct camss *camss,
> +				     struct v4l2_subdev *sd)
> +{
> +	return camss_subdev_stream_users(camss, sd);
>   }
>   
>   /*
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 39ea33e61..00b8d5304 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -167,6 +167,8 @@ void camss_add_clock_margin(u64 *rate);
>   int camss_enable_clocks(int nclocks, struct camss_clock *clock,
>   			struct device *dev);
>   void camss_disable_clocks(int nclocks, struct camss_clock *clock);
> +unsigned int *camss_subdev_stream_users(struct camss *camss,
> +					struct v4l2_subdev *sd);
>   struct media_pad *camss_find_sensor_pad(struct media_entity *entity);
>   s64 camss_get_link_freq(struct camss *camss, struct media_entity *entity,
>   			unsigned int bpp, unsigned int lanes);