Re: [PATCH v7 10/16] media: cadence: csi2rx: add multistream support

[Tomi Valkeinen]

Hi,

On 11/09/2025 13:28, Rishikesh Donadkar wrote:
> From: Jai Luthra <j-luthra@xxxxxx>
> 
> Cadence CSI-2 bridge IP supports capturing multiple virtual "streams"
> of data over the same physical interface using MIPI Virtual Channels.
> 
> While the hardware IP supports usecases where streams coming in the sink
> pad can be broadcasted to multiple source pads, the driver will need
> significant re-architecture to make that possible. The two users of this
> IP in mainline linux are TI Shim and StarFive JH7110 CAMSS, and both
> have only integrated the first source pad i.e stream0 of this IP. So for
> now keep it simple and only allow 1-to-1 mapping of streams from sink to
> source, without any broadcasting.
> 
> The enable_streams() API in v4l2 supports passing a bitmask to enable
> each pad/stream combination individually on any media subdev. Use this
> API instead of  s_stream() API.
> 
> Implement the enable_stream and disable_stream hooks in place of the
> stream-unaware s_stream hook.
> 
> Implement a fallback s_stream hook that internally calls enable_stream
> on each source pad, for consumer drivers that don't use multi-stream
> APIs to still work. The helper function v4l2_subdev_s_stream_helper()
> form the v4l2 framework is not used here as it is meant only for the
> subedvs that have a single source pad and this hardware IP supports
> having multiple source pads.

<snip>

> +static int csi2rx_enable_streams(struct v4l2_subdev *subdev,
> +				 struct v4l2_subdev_state *state, u32 pad,
> +				 u64 streams_mask)
> +{
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +	u64 sink_streams;
> +	int ret;
> +
> +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad,
> +						       CSI2RX_PAD_SINK,
> +						       &streams_mask);
> +
> +	guard(mutex)(&csi2rx->lock);

This looks a bit odd too. With enable/disable_streams, the state is
already locked. What is the mutex protecting?

j721e-csi2rx also has mutexes, and it's very unclear what they protect.
This should be described in the code.

I think in csi2rx the whole mutex can be just dropped.

j721e-csi2rx is a bit more complex, but there also I would consider if
and when the state lock protects the relevant parts already, and when
another lock is needed, what is the sequence to lock/unlock (e.g. always
csi->mutex first, then csi->subdev state lock), and make sure the code
follows that.

 Tomi