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

[Rishikesh Donadkar <r-donadkar@ti.com>]

On 25/09/25 18:14, Tomi Valkeinen wrote:
> Hi,
>
> On 11/09/2025 13:28, Rishikesh Donadkar wrote:
>> From: Jai Luthra <j-luthra@ti.com>
>>
>> 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.


Thanks for pointing out, Since enable/disable_streams() are protected by 
the v4l2-core by taking the state lock, all the mutex protection inside 
these functions in the j721e-csi2rx driver can also be removed.

I will make sure that this order (csi->mutex followed by the 
state->lock) is followed through out this driver.


Regards,

Rishikesh

>
>   Tomi
>