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

[Rishikesh Donadkar]

On 25/09/25 18:14, Tomi Valkeinen wrote:
> Hi,


Hi Tomi,

Thank you for the review.


> 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?

We call the csi2rx_start() function in this function, which is 
responsible for configuring and starting the streams, and operates on 
shared resources like the HW IP registers. This function is supposed to 
be called only once (done on the first enable_stream() call), for the 
rest enable/disable_stream() calls, we just increment/decrement a common 
variable(csi2rx->count).


> 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.


Sure, If the core itself serializes the enable/disable_streams() calls 
than this mutex can be dropped. I will make this change in the patch 
adding .enable/disable_stream().


Regards,

Rishikesh


> 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