Re: [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume not requested

[Eugen.Hristev]

On 11/17/21 6:11 PM, Sakari Ailus wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Eugen,
> 
> On Wed, Nov 17, 2021 at 05:40:09PM +0200, Eugen Hristev wrote:
>> pm_runtime_resume_and_get should be called when the s_frame_interval
>> is called.
>>
>> The driver will try to access device registers to configure VMAX, coarse
>> time and exposure.
>>
>> Currently if the runtime is not resumed, this fails:
>>   # media-ctl -d /dev/media0 --set-v4l2 '"IMX274 1-001a":0[fmt:SRGGB10_1X10/3840x2
>> 160@1/10]'
>>
>> IMX274 1-001a: imx274_binning_goodness: ask 3840x2160, size 3840x2160, goodness 0
>> IMX274 1-001a: imx274_binning_goodness: ask 3840x2160, size 1920x1080, goodness -3000
>> IMX274 1-001a: imx274_binning_goodness: ask 3840x2160, size 1280x720, goodness -4000
>> IMX274 1-001a: imx274_binning_goodness: ask 3840x2160, size 1280x540, goodness -4180
>> IMX274 1-001a: __imx274_change_compose: selected 1x1 binning
>> IMX274 1-001a: imx274_set_frame_interval: input frame interval = 1 / 10
>> IMX274 1-001a: imx274_read_mbreg : addr 0x300e, val=0x1 (2 bytes)
>> IMX274 1-001a: imx274_set_frame_interval : register SVR = 1
>> IMX274 1-001a: imx274_read_mbreg : addr 0x30f6, val=0x6a8 (2 bytes)
>> IMX274 1-001a: imx274_set_frame_interval : register HMAX = 1704
>> IMX274 1-001a: imx274_set_frame_length : input length = 2112
>> IMX274 1-001a: imx274_write_mbreg : i2c bulk write failed, 30f8 = 884 (3 bytes)
>> IMX274 1-001a: imx274_set_frame_length error = -121
>> IMX274 1-001a: imx274_set_frame_interval error = -121
>> Unable to setup formats: Remote I/O error (121)
>>
>> The device is not resumed thus the remote I/O error.
>>
>> Setting the frame interval works at streaming time, because
>> pm_runtime_resume_and_get is called at s_stream time before sensor setup.
>> The failure happens when only the s_frame_interval is called separately
>> independently on streaming time.
>>
>> Fixes: ad97bc37426c ("media: i2c: imx274: Add IMX274 power on and off sequence"
>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>> ---
>>   drivers/media/i2c/imx274.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index e89ef35a71c5..6e63fdcc5e46 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -1420,6 +1420,10 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>>        int min, max, def;
>>        int ret;
>>
>> +     ret = pm_runtime_resume_and_get(&imx274->client->dev);
>> +     if (ret < 0)
>> +             return ret;
>> +
>>        mutex_lock(&imx274->lock);
>>        ret = imx274_set_frame_interval(imx274, fi->interval);
>>
>> @@ -1451,6 +1455,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>>
>>   unlock:
>>        mutex_unlock(&imx274->lock);
>> +     pm_runtime_put(&imx274->client->dev);
>>
>>        return ret;
>>   }
> 
> If the device is powered off in the end, could you instead not power it on
> in the first place? I.e. see how this works for the s_ctrl() callback.


Hi Sakari,

I tried this initially, as in s_ctrl,

         if (!pm_runtime_get_if_in_use(&imx274->client->dev)) 

                 return 0;


However, if the device is powered off, the s_frame_interval does not do 
anything (return 0), and the frame interval is not changed. Not even the 
internal structure frame_interval is updated (as this is updated after 
configuring the actual device).
And in consequence media-ctl -p will still print the old frame interval.

So either we power on the device to set everything, or, things have to 
be set in the software struct and written once streaming starts.
I am in favor of the first option (hence the patch), to avoid having 
configuration that was requested but not written to the device itself.
The second option would require some rework to move the software part 
before the hardware part, and to assume that the hardware part never 
fails in bounds or by other reason (or the software part would be no 
longer consistent)

What do you think ?

Eugen

> 
> --
> Kind regards,
> 
> Sakari Ailus
>