Re: [PATCH v2 10/12] media: mt9m114: Drop start-, stop-streaming sequence from initialize

[Hans de Goede <hansg@kernel.org>]

Hi Laurent,

On 3-Jun-25 1:33 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sat, May 31, 2025 at 06:31:45PM +0200, Hans de Goede wrote:
>> Drop the start-, stop-streaming sequence from initialize.
>>
>> When streaming is started with a runtime-suspended sensor,
>> mt9m114_start_streaming() will runtime-resume the sensor which calls
>> mt9m114_initialize() immediately followed by calling
>> mt9m114_set_state(ENTER_CONFIG_CHANGE).
>>
>> This results in the following state changes in quick succession:
>>
>> mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING
>> mt9m114_set_state(ENTER_SUSPEND)       -> transitions to SUSPENDED
>> mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING
>>
>> these quick state changes confuses the CSI receiver on atomisp devices
>> causing streaming to not work.
>>
>> Drop the state changes from mt9m114_initialize() so that only
>> a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call is made
>> when streaming is started with a runtime-suspend sensor.
>>
>> This means that the sensor may have config changes pending when
>> mt9m114_runtime_suspend() gets called the first time after mt9m114_probe(),
>> when streaming was not started within the 1 second runtime-pm timeout.
>> Keep track of this and do the ENTER_CONFIG_CHANGE + ENTER suspend from
>> mt9m114_runtime_suspend() if necessary.
>>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>>  drivers/media/i2c/mt9m114.c | 19 +++++++++++--------
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
>> index d954f2be8f0d..c4d3122d698e 100644
>> --- a/drivers/media/i2c/mt9m114.c
>> +++ b/drivers/media/i2c/mt9m114.c
>> @@ -389,6 +389,7 @@ struct mt9m114 {
>>  
>>  	unsigned int pixrate;
>>  	bool streaming;
>> +	bool config_change_pending;
>>  	u32 clk_freq;
>>  
>>  	/* Pixel Array */
>> @@ -782,14 +783,7 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
>> -	if (ret < 0)
>> -		return ret;
> 
> Entering Config-Change here was meant to ensure the PLL and output mode
> settings get applied. The PLL settings can probably wait until we start
> streaming. For the output mode, I'm slightly concerned that incorrect
> settings could lead to hardware issues, as the sensor starts in parallel
> output mode. I suppose it shouldn't cause any hardware damage, so I
> think we could try to just delay Config-Change until we start streaming.
> 
>> -
>> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_SUSPEND);
>> -	if (ret < 0)
>> -		return ret;
> 
> This bothers me a bit more. As far as I understand, the sensor starts
> streaming right after power on reset, so I'd like to disable streaming
> as quickly as possible.

I tried doing the MT9M114_SYS_STATE_ENTER_SUSPEND without a prior
MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE but that does not work, then
mt9m114_poll_command() times out.

Also I believe that the start/stop/start streaming vs just a
single start streaming is actually causing the issues on atomisp
devices.

Normally we only come out of runtime-suspend from
mt9m114_start_streaming() so in this case there is no issue
with not disabling streaming from mt9m114_initialize().

The only case where the sensor would be streaming when
we do not want it would be after the power-up +
mt9m114_initialize() from probe().

And we can get the desired "disable streaming
as quickly as possible." by forcing a runtime-suspend
from probe() directly after mt9m114_initialize() instead
of waiting for the auto runtimesuspend.

I'll make probe() immediately runtime-suspend the sensor
after mt9m114_initialize() in the next version.

Regards,

Hans



> 
>> -
>> +	sensor->config_change_pending = true;
>>  	return 0;
>>  }
>>  
>> @@ -976,6 +970,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
>>  	if (ret)
>>  		goto error;
>>  
>> +	sensor->config_change_pending = false;
>>  	sensor->streaming = true;
>>  
>>  	return 0;
>> @@ -2267,6 +2262,14 @@ static int __maybe_unused mt9m114_runtime_suspend(struct device *dev)
>>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
>>  
>> +	if (sensor->config_change_pending) {
>> +		/* mt9m114_set_state() prints errors itself, no need to check */
>> +		mt9m114_set_state(sensor,
>> +				  MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
>> +		mt9m114_set_state(sensor,
>> +				  MT9M114_SYS_STATE_ENTER_SUSPEND);
>> +	}
>> +
>>  	mt9m114_power_off(sensor);
>>  
>>  	return 0;
>