* [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
* 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: [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
* [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: [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
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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.