* [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume not requested
@ 2021-11-17 15:40 Eugen Hristev
2021-11-17 16:11 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Eugen Hristev @ 2021-11-17 15:40 UTC (permalink / raw)
To: leonl, linux-media
Cc: skomatineni, sakari.ailus, luca, linux-kernel, Eugen Hristev
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@microchip.com>
---
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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume not requested
2021-11-17 15:40 [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume not requested Eugen Hristev
@ 2021-11-17 16:11 ` Sakari Ailus
2021-11-17 16:52 ` Eugen.Hristev
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2021-11-17 16:11 UTC (permalink / raw)
To: Eugen Hristev; +Cc: leonl, linux-media, skomatineni, luca, linux-kernel
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@microchip.com>
> ---
> 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.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume not requested
2021-11-17 16:11 ` Sakari Ailus
@ 2021-11-17 16:52 ` Eugen.Hristev
2021-11-17 21:03 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Eugen.Hristev @ 2021-11-17 16:52 UTC (permalink / raw)
To: sakari.ailus; +Cc: leonl, linux-media, skomatineni, luca, linux-kernel
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@microchip.com>
>> ---
>> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume not requested
2021-11-17 16:52 ` Eugen.Hristev
@ 2021-11-17 21:03 ` Sakari Ailus
2021-11-18 7:16 ` Eugen.Hristev
0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2021-11-17 21:03 UTC (permalink / raw)
To: Eugen.Hristev; +Cc: leonl, linux-media, skomatineni, luca, linux-kernel
Hi Eugen,
On Wed, Nov 17, 2021 at 04:52:40PM +0000, Eugen.Hristev@microchip.com wrote:
> 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@microchip.com>
> >> ---
> >> 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 ?
Seems reasonable, but the driver is hardly doing this in an exemplary way.
Still the rework might not worth the small gain. I'll take this one then.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume not requested
2021-11-17 21:03 ` Sakari Ailus
@ 2021-11-18 7:16 ` Eugen.Hristev
2021-11-18 9:02 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Eugen.Hristev @ 2021-11-18 7:16 UTC (permalink / raw)
To: sakari.ailus; +Cc: leonl, linux-media, skomatineni, luca, linux-kernel
On 11/17/21 11:03 PM, Sakari Ailus wrote:
> Hi Eugen,
>
> On Wed, Nov 17, 2021 at 04:52:40PM +0000, Eugen.Hristev@microchip.com wrote:
>> 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@microchip.com>
>>>> ---
>>>> 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 ?
>
> Seems reasonable, but the driver is hardly doing this in an exemplary way.
> Still the rework might not worth the small gain. I'll take this one then.
Okay, thank you.
I noticed that the fixes tag in the commit message misses the last
closing bracket ')' . Might break automated checkers and shout out a
warning. Maybe it's possible to amend it ?
Thanks again,
Eugen
>
> --
> Kind regards,
>
> Sakari Ailus
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume not requested
2021-11-18 7:16 ` Eugen.Hristev
@ 2021-11-18 9:02 ` Sakari Ailus
0 siblings, 0 replies; 6+ messages in thread
From: Sakari Ailus @ 2021-11-18 9:02 UTC (permalink / raw)
To: Eugen.Hristev; +Cc: leonl, linux-media, skomatineni, luca, linux-kernel
On Thu, Nov 18, 2021 at 07:16:16AM +0000, Eugen.Hristev@microchip.com wrote:
> On 11/17/21 11:03 PM, Sakari Ailus wrote:
> > Hi Eugen,
> >
> > On Wed, Nov 17, 2021 at 04:52:40PM +0000, Eugen.Hristev@microchip.com wrote:
> >> 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@microchip.com>
> >>>> ---
> >>>> 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 ?
> >
> > Seems reasonable, but the driver is hardly doing this in an exemplary way.
> > Still the rework might not worth the small gain. I'll take this one then.
>
>
> Okay, thank you.
> I noticed that the fixes tag in the commit message misses the last
> closing bracket ')' . Might break automated checkers and shout out a
> warning. Maybe it's possible to amend it ?
Thanks, fixed it.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-18 9:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 15:40 [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume not requested Eugen Hristev
2021-11-17 16:11 ` Sakari Ailus
2021-11-17 16:52 ` Eugen.Hristev
2021-11-17 21:03 ` Sakari Ailus
2021-11-18 7:16 ` Eugen.Hristev
2021-11-18 9:02 ` Sakari Ailus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).