* [PATCH 0/2] IMX258 driver fixes
@ 2021-07-23 11:22 Umang Jain
2021-07-23 11:22 ` [PATCH 1/2] media: imx258: Rectify mismatch of VTS value Umang Jain
2021-07-23 11:22 ` [PATCH 2/2] media: imx258: Limit the max analogue gain to 480 Umang Jain
0 siblings, 2 replies; 9+ messages in thread
From: Umang Jain @ 2021-07-23 11:22 UTC (permalink / raw)
To: linux-media; +Cc: libcamera-devel, Umang Jain
While working on libcamera with a IMX258 sensor attached
to IPU3 ISP, we noticed a few niggles that seem to be driver
related. After running a series of investigations based on
libcamera and v4l2-ctl tools, we could pin-point and address
a couple of them for release immediate pain points.
Laurent Pinchart (1):
media: imx258: Rectify mismatch of VTS value
Umang Jain (1):
media: imx258: Limit the max analogue gain to 480
drivers/media/i2c/imx258.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] media: imx258: Rectify mismatch of VTS value
2021-07-23 11:22 [PATCH 0/2] IMX258 driver fixes Umang Jain
@ 2021-07-23 11:22 ` Umang Jain
2021-07-23 11:50 ` Laurent Pinchart
2021-07-23 11:22 ` [PATCH 2/2] media: imx258: Limit the max analogue gain to 480 Umang Jain
1 sibling, 1 reply; 9+ messages in thread
From: Umang Jain @ 2021-07-23 11:22 UTC (permalink / raw)
To: linux-media; +Cc: libcamera-devel, Laurent Pinchart, Umang Jain
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The frame_length_lines (0x0340) registers are hard-coded as follows:
- 4208x3118
frame_length_lines = 0x0c50
- 2104x1560
frame_length_lines = 0x0638
- 1048x780
frame_length_lines = 0x034c
The driver exposes the V4L2_CID_VBLANK control in read-only mode and
sets its value to vts_def - height, where vts_def is a mode-dependent
value coming from the supported_modes array. It is set using one of
the following macros defined in the driver:
#define IMX258_VTS_30FPS 0x0c98
#define IMX258_VTS_30FPS_2K 0x0638
#define IMX258_VTS_30FPS_VGA 0x034c
There's a clear mismatch in the value for the full resolution mode i.e.
IMX258_VTS_30FPS. Fix it by rectifying the macro with the value set for
the frame_length_lines register as stated above.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
drivers/media/i2c/imx258.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 7ab9e5f9f267..4e695096e5d0 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -23,7 +23,7 @@
#define IMX258_CHIP_ID 0x0258
/* V_TIMING internal */
-#define IMX258_VTS_30FPS 0x0c98
+#define IMX258_VTS_30FPS 0x0c50
#define IMX258_VTS_30FPS_2K 0x0638
#define IMX258_VTS_30FPS_VGA 0x034c
#define IMX258_VTS_MAX 0xffff
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] media: imx258: Limit the max analogue gain to 480
2021-07-23 11:22 [PATCH 0/2] IMX258 driver fixes Umang Jain
2021-07-23 11:22 ` [PATCH 1/2] media: imx258: Rectify mismatch of VTS value Umang Jain
@ 2021-07-23 11:22 ` Umang Jain
2021-07-23 11:44 ` [libcamera-devel] " Laurent Pinchart
1 sibling, 1 reply; 9+ messages in thread
From: Umang Jain @ 2021-07-23 11:22 UTC (permalink / raw)
To: linux-media; +Cc: libcamera-devel, Umang Jain
The range for analog gain mentioned in the datasheet is [0, 480].
The real gain formula mentioned in the datasheet is:
Gain = 512 / (512 – X)
Hence, values larger than 511 clearly makes no sense. The gain
register field is also documented to be of 9-bits in the datasheet.
Certainly, it is enough to infer that, the kernel driver currently
advertises an arbitrary analog gain max. Fix it by rectifying the
value as per the data sheet i.e. 480.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
drivers/media/i2c/imx258.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 4e695096e5d0..81cdf37216ca 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -47,7 +47,7 @@
/* Analog gain control */
#define IMX258_REG_ANALOG_GAIN 0x0204
#define IMX258_ANA_GAIN_MIN 0
-#define IMX258_ANA_GAIN_MAX 0x1fff
+#define IMX258_ANA_GAIN_MAX 480
#define IMX258_ANA_GAIN_STEP 1
#define IMX258_ANA_GAIN_DEFAULT 0x0
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [libcamera-devel] [PATCH 2/2] media: imx258: Limit the max analogue gain to 480
2021-07-23 11:22 ` [PATCH 2/2] media: imx258: Limit the max analogue gain to 480 Umang Jain
@ 2021-07-23 11:44 ` Laurent Pinchart
2021-07-23 11:52 ` Laurent Pinchart
0 siblings, 1 reply; 9+ messages in thread
From: Laurent Pinchart @ 2021-07-23 11:44 UTC (permalink / raw)
To: Umang Jain; +Cc: linux-media, libcamera-devel, Sakari Ailus
Hi Umang,
Thank you for the patch.
CC'ing Sakari. For future kernel patches, you can use the
./scripts/get_maintainer.pl script in the kernel sources to get a list
of appropriate recipients. The list should be taken with a grain of salt
though, it has a tendency to return too many recipients. For this
particular patch, for instance, it also recommends Mauro and LKML.
Whether to CC the subsystem maintainer on every patch depends on the
subsystem (it's more common for small ones than big ones) and on the
maintainer's preferences. LKML is a catch-all mailing list with very
high traffic, and when more appropriate venues exist for patches, I
usually recommend skipping LKML.
On Fri, Jul 23, 2021 at 04:52:33PM +0530, Umang Jain wrote:
> The range for analog gain mentioned in the datasheet is [0, 480].
> The real gain formula mentioned in the datasheet is:
>
> Gain = 512 / (512 – X)
>
> Hence, values larger than 511 clearly makes no sense. The gain
> register field is also documented to be of 9-bits in the datasheet.
>
> Certainly, it is enough to infer that, the kernel driver currently
> advertises an arbitrary analog gain max. Fix it by rectifying the
> value as per the data sheet i.e. 480.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/i2c/imx258.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 4e695096e5d0..81cdf37216ca 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -47,7 +47,7 @@
> /* Analog gain control */
> #define IMX258_REG_ANALOG_GAIN 0x0204
> #define IMX258_ANA_GAIN_MIN 0
> -#define IMX258_ANA_GAIN_MAX 0x1fff
> +#define IMX258_ANA_GAIN_MAX 480
> #define IMX258_ANA_GAIN_STEP 1
> #define IMX258_ANA_GAIN_DEFAULT 0x0
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: imx258: Rectify mismatch of VTS value
2021-07-23 11:22 ` [PATCH 1/2] media: imx258: Rectify mismatch of VTS value Umang Jain
@ 2021-07-23 11:50 ` Laurent Pinchart
2021-07-26 2:20 ` Bingbu Cao
0 siblings, 1 reply; 9+ messages in thread
From: Laurent Pinchart @ 2021-07-23 11:50 UTC (permalink / raw)
To: Umang Jain
Cc: linux-media, libcamera-devel, Sakari Ailus, Dave Stevenson,
Krzysztof Kozlowski, Bingbu Cao
Hi Umang,
Thank you for the patch.
(Expanding the CC list)
On Fri, Jul 23, 2021 at 04:52:32PM +0530, Umang Jain wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The frame_length_lines (0x0340) registers are hard-coded as follows:
>
> - 4208x3118
> frame_length_lines = 0x0c50
>
> - 2104x1560
> frame_length_lines = 0x0638
>
> - 1048x780
> frame_length_lines = 0x034c
>
> The driver exposes the V4L2_CID_VBLANK control in read-only mode and
> sets its value to vts_def - height, where vts_def is a mode-dependent
> value coming from the supported_modes array. It is set using one of
> the following macros defined in the driver:
>
> #define IMX258_VTS_30FPS 0x0c98
> #define IMX258_VTS_30FPS_2K 0x0638
> #define IMX258_VTS_30FPS_VGA 0x034c
>
> There's a clear mismatch in the value for the full resolution mode i.e.
> IMX258_VTS_30FPS. Fix it by rectifying the macro with the value set for
> the frame_length_lines register as stated above.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/i2c/imx258.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 7ab9e5f9f267..4e695096e5d0 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -23,7 +23,7 @@
> #define IMX258_CHIP_ID 0x0258
>
> /* V_TIMING internal */
> -#define IMX258_VTS_30FPS 0x0c98
> +#define IMX258_VTS_30FPS 0x0c50
> #define IMX258_VTS_30FPS_2K 0x0638
> #define IMX258_VTS_30FPS_VGA 0x034c
> #define IMX258_VTS_MAX 0xffff
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [libcamera-devel] [PATCH 2/2] media: imx258: Limit the max analogue gain to 480
2021-07-23 11:44 ` [libcamera-devel] " Laurent Pinchart
@ 2021-07-23 11:52 ` Laurent Pinchart
2021-07-23 12:29 ` Krzysztof Kozlowski
2021-07-23 12:36 ` Dave Stevenson
0 siblings, 2 replies; 9+ messages in thread
From: Laurent Pinchart @ 2021-07-23 11:52 UTC (permalink / raw)
To: Umang Jain
Cc: linux-media, libcamera-devel, Sakari Ailus, Dave Stevenson,
Krzysztof Kozlowski, Bingbu Cao
On Fri, Jul 23, 2021 at 02:44:40PM +0300, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> CC'ing Sakari. For future kernel patches, you can use the
> ./scripts/get_maintainer.pl script in the kernel sources to get a list
> of appropriate recipients. The list should be taken with a grain of salt
> though, it has a tendency to return too many recipients. For this
> particular patch, for instance, it also recommends Mauro and LKML.
> Whether to CC the subsystem maintainer on every patch depends on the
> subsystem (it's more common for small ones than big ones) and on the
> maintainer's preferences. LKML is a catch-all mailing list with very
> high traffic, and when more appropriate venues exist for patches, I
> usually recommend skipping LKML.
And expanding the CC list further to include Dave (for his contribution
to the discussion), and Krzysztof and Bingbu (for their contributions to
the driver, as reported by git log).
> On Fri, Jul 23, 2021 at 04:52:33PM +0530, Umang Jain wrote:
> > The range for analog gain mentioned in the datasheet is [0, 480].
> > The real gain formula mentioned in the datasheet is:
> >
> > Gain = 512 / (512 – X)
> >
> > Hence, values larger than 511 clearly makes no sense. The gain
> > register field is also documented to be of 9-bits in the datasheet.
> >
> > Certainly, it is enough to infer that, the kernel driver currently
> > advertises an arbitrary analog gain max. Fix it by rectifying the
> > value as per the data sheet i.e. 480.
> >
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > ---
> > drivers/media/i2c/imx258.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 4e695096e5d0..81cdf37216ca 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -47,7 +47,7 @@
> > /* Analog gain control */
> > #define IMX258_REG_ANALOG_GAIN 0x0204
> > #define IMX258_ANA_GAIN_MIN 0
> > -#define IMX258_ANA_GAIN_MAX 0x1fff
> > +#define IMX258_ANA_GAIN_MAX 480
> > #define IMX258_ANA_GAIN_STEP 1
> > #define IMX258_ANA_GAIN_DEFAULT 0x0
> >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [libcamera-devel] [PATCH 2/2] media: imx258: Limit the max analogue gain to 480
2021-07-23 11:52 ` Laurent Pinchart
@ 2021-07-23 12:29 ` Krzysztof Kozlowski
2021-07-23 12:36 ` Dave Stevenson
1 sibling, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2021-07-23 12:29 UTC (permalink / raw)
To: Laurent Pinchart, Umang Jain
Cc: linux-media, libcamera-devel, Sakari Ailus, Dave Stevenson, Bingbu Cao
On 23/07/2021 13:52, Laurent Pinchart wrote:
> On Fri, Jul 23, 2021 at 02:44:40PM +0300, Laurent Pinchart wrote:
>> Hi Umang,
>>
>> Thank you for the patch.
>>
>> CC'ing Sakari. For future kernel patches, you can use the
>> ./scripts/get_maintainer.pl script in the kernel sources to get a list
>> of appropriate recipients. The list should be taken with a grain of salt
>> though, it has a tendency to return too many recipients. For this
>> particular patch, for instance, it also recommends Mauro and LKML.
>> Whether to CC the subsystem maintainer on every patch depends on the
>> subsystem (it's more common for small ones than big ones) and on the
>> maintainer's preferences. LKML is a catch-all mailing list with very
>> high traffic, and when more appropriate venues exist for patches, I
>> usually recommend skipping LKML.
>
> And expanding the CC list further to include Dave (for his contribution
> to the discussion), and Krzysztof and Bingbu (for their contributions to
> the driver, as reported by git log).
>
I don't work with this sensor anymore and I don't have it. I trust the
change is reasonable, but I did not check it in documentation.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [libcamera-devel] [PATCH 2/2] media: imx258: Limit the max analogue gain to 480
2021-07-23 11:52 ` Laurent Pinchart
2021-07-23 12:29 ` Krzysztof Kozlowski
@ 2021-07-23 12:36 ` Dave Stevenson
1 sibling, 0 replies; 9+ messages in thread
From: Dave Stevenson @ 2021-07-23 12:36 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Umang Jain, Linux Media Mailing List, libcamera devel,
Sakari Ailus, Krzysztof Kozlowski, Bingbu Cao
Hi Umang and Laurent
On Fri, 23 Jul 2021 at 12:52, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Fri, Jul 23, 2021 at 02:44:40PM +0300, Laurent Pinchart wrote:
> > Hi Umang,
> >
> > Thank you for the patch.
> >
> > CC'ing Sakari. For future kernel patches, you can use the
> > ./scripts/get_maintainer.pl script in the kernel sources to get a list
> > of appropriate recipients. The list should be taken with a grain of salt
> > though, it has a tendency to return too many recipients. For this
> > particular patch, for instance, it also recommends Mauro and LKML.
> > Whether to CC the subsystem maintainer on every patch depends on the
> > subsystem (it's more common for small ones than big ones) and on the
> > maintainer's preferences. LKML is a catch-all mailing list with very
> > high traffic, and when more appropriate venues exist for patches, I
> > usually recommend skipping LKML.
>
> And expanding the CC list further to include Dave (for his contribution
> to the discussion), and Krzysztof and Bingbu (for their contributions to
> the driver, as reported by git log).
>
> > On Fri, Jul 23, 2021 at 04:52:33PM +0530, Umang Jain wrote:
> > > The range for analog gain mentioned in the datasheet is [0, 480].
> > > The real gain formula mentioned in the datasheet is:
> > >
> > > Gain = 512 / (512 – X)
> > >
> > > Hence, values larger than 511 clearly makes no sense. The gain
> > > register field is also documented to be of 9-bits in the datasheet.
> > >
> > > Certainly, it is enough to infer that, the kernel driver currently
> > > advertises an arbitrary analog gain max. Fix it by rectifying the
> > > value as per the data sheet i.e. 480.
> > >
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
That certainly follows the datasheet that I have.
Gains up to code 480 work as expected. Up to 496 seems to work, but
going beyond that causes issues. Adopting the documented maximum value
is the safest approach.
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > > ---
> > > drivers/media/i2c/imx258.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > > index 4e695096e5d0..81cdf37216ca 100644
> > > --- a/drivers/media/i2c/imx258.c
> > > +++ b/drivers/media/i2c/imx258.c
> > > @@ -47,7 +47,7 @@
> > > /* Analog gain control */
> > > #define IMX258_REG_ANALOG_GAIN 0x0204
> > > #define IMX258_ANA_GAIN_MIN 0
> > > -#define IMX258_ANA_GAIN_MAX 0x1fff
> > > +#define IMX258_ANA_GAIN_MAX 480
> > > #define IMX258_ANA_GAIN_STEP 1
> > > #define IMX258_ANA_GAIN_DEFAULT 0x0
> > >
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: imx258: Rectify mismatch of VTS value
2021-07-23 11:50 ` Laurent Pinchart
@ 2021-07-26 2:20 ` Bingbu Cao
0 siblings, 0 replies; 9+ messages in thread
From: Bingbu Cao @ 2021-07-26 2:20 UTC (permalink / raw)
To: Laurent Pinchart, Umang Jain
Cc: linux-media, libcamera-devel, Sakari Ailus, Dave Stevenson,
Krzysztof Kozlowski, Bingbu Cao
Umang and Laurent,
I just checked the spec, 0xc98 should be the initial value not for 4208x3118, the patch
looks good to me though I am not working on that anymore.
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
On 7/23/21 7:50 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> (Expanding the CC list)
>
> On Fri, Jul 23, 2021 at 04:52:32PM +0530, Umang Jain wrote:
>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> The frame_length_lines (0x0340) registers are hard-coded as follows:
>>
>> - 4208x3118
>> frame_length_lines = 0x0c50
>>
>> - 2104x1560
>> frame_length_lines = 0x0638
>>
>> - 1048x780
>> frame_length_lines = 0x034c
>>
>> The driver exposes the V4L2_CID_VBLANK control in read-only mode and
>> sets its value to vts_def - height, where vts_def is a mode-dependent
>> value coming from the supported_modes array. It is set using one of
>> the following macros defined in the driver:
>>
>> #define IMX258_VTS_30FPS 0x0c98
>> #define IMX258_VTS_30FPS_2K 0x0638
>> #define IMX258_VTS_30FPS_VGA 0x034c
>>
>> There's a clear mismatch in the value for the full resolution mode i.e.
>> IMX258_VTS_30FPS. Fix it by rectifying the macro with the value set for
>> the frame_length_lines register as stated above.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> ---
>> drivers/media/i2c/imx258.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> index 7ab9e5f9f267..4e695096e5d0 100644
>> --- a/drivers/media/i2c/imx258.c
>> +++ b/drivers/media/i2c/imx258.c
>> @@ -23,7 +23,7 @@
>> #define IMX258_CHIP_ID 0x0258
>>
>> /* V_TIMING internal */
>> -#define IMX258_VTS_30FPS 0x0c98
>> +#define IMX258_VTS_30FPS 0x0c50
>> #define IMX258_VTS_30FPS_2K 0x0638
>> #define IMX258_VTS_30FPS_VGA 0x034c
>> #define IMX258_VTS_MAX 0xffff
>
--
Best regards,
Bingbu Cao
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-26 2:22 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 11:22 [PATCH 0/2] IMX258 driver fixes Umang Jain
2021-07-23 11:22 ` [PATCH 1/2] media: imx258: Rectify mismatch of VTS value Umang Jain
2021-07-23 11:50 ` Laurent Pinchart
2021-07-26 2:20 ` Bingbu Cao
2021-07-23 11:22 ` [PATCH 2/2] media: imx258: Limit the max analogue gain to 480 Umang Jain
2021-07-23 11:44 ` [libcamera-devel] " Laurent Pinchart
2021-07-23 11:52 ` Laurent Pinchart
2021-07-23 12:29 ` Krzysztof Kozlowski
2021-07-23 12:36 ` Dave Stevenson
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).