Re: [PATCH v3 03/15] media: rcar-isp: Move {enable|disable}_streams() calls

[Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>]

Hi,

On 02/06/2025 12:43, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, May 30, 2025 at 04:50:32PM +0300, Tomi Valkeinen wrote:
>> With multiple streams the operation to enable the ISP hardware and to
>> call {enable|disable}_streams() on upstream subdev will need to be
>> handled separately.
>>
>> Prepare for that by moving {enable|disable}_streams() calls out from
>> risp_start() and risp_stop().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/renesas/rcar-isp/csisp.c | 18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
>> index 8fb2cc3b5650..2337c5d44c40 100644
>> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
>> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
>> @@ -268,18 +268,11 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>>  	/* Start ISP. */
>>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
>>  
>> -	ret = v4l2_subdev_enable_streams(isp->remote, isp->remote_pad,
>> -					 BIT_ULL(0));
>> -	if (ret)
>> -		risp_power_off(isp);
>> -
>> -	return ret;
>> +	return 0;
>>  }
>>  
>>  static void risp_stop(struct rcar_isp *isp)
>>  {
>> -	v4l2_subdev_disable_streams(isp->remote, isp->remote_pad, BIT_ULL(0));
>> -
>>  	/* Stop ISP. */
>>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_STOP);
>>  
>> @@ -305,6 +298,13 @@ static int risp_enable_streams(struct v4l2_subdev *sd,
>>  			return ret;
>>  	}
>>  
>> +	ret = v4l2_subdev_enable_streams(isp->remote, isp->remote_pad,
>> +					 BIT_ULL(0));
> 
> You're now potentially calling v4l2_subdev_disable_streams() multiple
> times on the same pad and stream, as this call isn't covered by the
> stream_count check anymore. Is that correct ? Maybe because
> risp_enable_streams() is guaranteed to never be called multiple times,
> with stream_count never becoming larger than 1 ? If so that should be
> explained in the commit message, and stream_count should probably be
> dropped.

At this point in the series risp_enable_streams() is called just once,
from a single VIN. That is, assuming only a single stream is supported.
In the cover letter I mention that there seems to be some kind of
attempts for multistreaming in upstream, which breaks during this
series. My understanding from Niklas was that the custom multistreaming
doesn't work or at least can be dropped. Niklas, correct me if I'm wrong
(and if I am wrong, someone else needs to take care of the custom
multistreaming =).

Later in the series risp_enable_streams will be called multiple times.
Each VIN video node will call it once when enabling.

 Tomi

> 
> Same when stopping.
> 
>> +	if (ret) {
>> +		risp_stop(isp);
> 
> This is also not covered by the stream_count, while risp_start() is.
> 
>> +		return ret;
>> +	}
>> +
>>  	isp->stream_count += 1;
>>  
>>  	return ret;
>> @@ -322,6 +322,8 @@ static int risp_disable_streams(struct v4l2_subdev *sd,
>>  	if (!isp->remote)
>>  		return -ENODEV;
>>  
>> +	v4l2_subdev_disable_streams(isp->remote, isp->remote_pad, BIT_ULL(0));
>> +
>>  	if (isp->stream_count == 1)
>>  		risp_stop(isp);
>>  
>