Received-SPF: pass (google.com: domain of linux-kernel+bounces-173543-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-v2-1-2e665f072f8f@linaro.org>
 <dvyed4grpazqk7a3tz6dqwpkd76ghtrt4euinxt3kycdeh63ez@ljgfjsfhypix>
 <20a0300a-ac16-456c-840a-e272f49050a8@linaro.org> <bppn2qglcya3xbfy7uey5cgybyanxthhweqv7foojwi5rvqwmk@temzdedvecfe>
In-Reply-To: <bppn2qglcya3xbfy7uey5cgybyanxthhweqv7foojwi5rvqwmk@temzdedvecfe>
From: Dave Stevenson <dave.stevenson@raspberrypi.com>
Date: Wed, 8 May 2024 17:31:39 +0100
Message-ID: <CAPY8ntAJJu8RM66xFr4dGWtZJVhsjjXEecT5=YKBVr+0hVL9+w@mail.gmail.com>
Subject: Re: [PATCH v2] media: i2c: Fix imx412 exposure control
To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Cc: "Bryan O'Donoghue" <bryan.odonoghue@linaro.org>, Sakari Ailus <sakari.ailus@linux.intel.com>, 
	Mauro Carvalho Chehab <mchehab@kernel.org>, "Paul J. Murphy" <paul.j.murphy@intel.com>, 
	Martina Krasteva <quic_mkrastev@quicinc.com>, 
	Daniele Alessandrelli <daniele.alessandrelli@intel.com>, 
	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, linux-media@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Hi Jacopo and Bryan

On Wed, 8 May 2024 at 13:43, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Bryan
>
> On Wed, May 08, 2024 at 01:30:31PM GMT, Bryan O'Donoghue wrote:
> > On 08/05/2024 09:02, Jacopo Mondi wrote:
> > > Hi Bryan
> > >
> > > On Mon, May 06, 2024 at 11:38:26PM GMT, Bryan O'Donoghue wrote:
> > > > Currently we have the following algorithm to calculate what value should be
> > > > written to the exposure control of imx412.
> > > >
> > > > lpfr = imx412->vblank + imx412->cur_mode->height;
> > > > shutter = lpfr - exposure;
> > > >
> > > > The 'shutter' value is given to IMX412_REG_EXPOSURE_CIT however, the above
> > > > algorithm will result in the value given to IMX412_REG_EXPOSURE_CIT
> > > > decreasing as the requested exposure value from user-space goes up.
> > > >
> > > > e.g.
> > > > [ 2255.713989] imx412 20-001a: Received exp 1608, analog gain 0
> > > > [ 2255.714002] imx412 20-001a: Set exp 1608, analog gain 0, shutter 1938, lpfr 3546
> > > > [ 2256.302770] imx412 20-001a: Received exp 2586, analog gain 100
> > > > [ 2256.302800] imx412 20-001a: Set exp 2586, analog gain 100, shutter 960, lpfr 3546
> > > > [ 2256.753755] imx412 20-001a: Received exp 3524, analog gain 110
> > > > [ 2256.753772] imx412 20-001a: Set exp 3524, analog gain 110, shutter 22, lpfr 3546
> > > >
> > > > This behaviour results in the image having less exposure as the requested
> > > > exposure value from user-space increases.
> > > >
> > > > Other sensor drivers such as ov5675, imx218, hid556 and others take the
> > > > requested exposure value and directly.
> > >
> > > has the phrase been truncated or is it me reading it wrong ?
> >
> > Sod's law says no matter how many times you send yourself a patch before
> > sending it to LKML you'll find a typo ~ 2 seconds after reading your patch
> > on LKML.
> >
>
> Sounds familiar enough
>
> >
> > > > Looking at the range of imx sensors, it appears this particular error has
> > > > been replicated a number of times but, I haven't so far really drilled into
> > > > each sensor.
> > >
> > > Ouch, what other driver have the same issue ?
> >
> > So without data sheet or sensor its hard to say if these are correct or
> > incorrect, it's the same basic calculation though.
> >
> > drivers/media/i2c/imx334.c::imx334_update_exp_gain()
> >
> >         lpfr = imx334->vblank + imx334->cur_mode->height;
> >         shutter = lpfr - exposure;
> >
> >         ret = imx334_write_reg(imx334, IMX334_REG_SHUTTER, 3, shutter);
> >
> >
> > drivers/media/i2c/imx335.c::imx335_update_exp_gain()
> >
> >         lpfr = imx335->vblank + imx335->cur_mode->height;
> >         shutter = lpfr - exposure;
> >
> >         ret = imx335_write_reg(imx335, IMX334_REG_SHUTTER, 3, shutter);
> >
> >
> > Looking again I'm inclined to believe the imx334/imx335 stuff is probably
> > correct for those sensors, got copied to imx412/imx577 and misapplied to the
> > EXPOSURE control in imx412.
> >
>
> Without datasheet/devices it really is hard to tell. Cargo cult at
> play most probably.

For reference certainly imx327/290/462 which are all siblings in the
Sony Starvis family do calculate exposure as
exposure = 1 frame period - (SHS1 + 1) * (1H period)
So 0 = max exposure and bigger values are shorter exposure time.

I'm not saying that the imx412 driver is right in doing this as well,
but it seems there is a trend with the Sony Starvis family to program
exposure in this different manner. Don't discount it as wrong for all
drivers!

  Dave

> >
> > > > - ret = imx412_write_reg(imx412, IMX412_REG_EXPOSURE_CIT, 2, shutter);
> > > > + ret = imx412_write_reg(imx412, IMX412_REG_EXPOSURE_CIT, 2, exposure);
> > >
> > > No datasheet here, can you confirm the IMX412_REG_EXPOSURE_CIT
> > > register is actually in lines ?
> >
> >
> > Looks like.
> >
> > From downstream "coarseIntgTimeAddr"
> >
> > imx577_sensor.xml
> >     <coarseIntgTimeAddr>0x0202</coarseIntgTimeAddr>
> >
> > imx586/imx586_sensor.cpp
> > pRegSettingsInfo->regSetting[regCount].registerAddr  =
> > pExposureData->pRegInfo->coarseIntgTimeAddr + 1;
> >
> > pRegSettingsInfo->regSetting[regCount].registerData  = (lineCount & 0xFF);
> >
> > > Apart from that, as the CID_EXPOSURE control limit are correctly
> > > updated when a new VBLANK is set by taking into account the exposure
> > > margins, I think writing the control value to the register is the
> > > right thing to do (if the register is in lines of course)
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >
> > > Thanks
> > >    j
> > >
> >
> > If that's good enough I'll fix the typo and apply your RB.
>
> Sure
>
> Thanks
>   j
>
> >
> > ---
> > bod
> >
>