Received-SPF: pass (google.com: domain of linux-kernel+bounces-172873-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 8 May 2024 10:02:03 +0200
From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: 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
Subject: Re: [PATCH v2] media: i2c: Fix imx412 exposure control
Message-ID: <dvyed4grpazqk7a3tz6dqwpkd76ghtrt4euinxt3kycdeh63ez@ljgfjsfhypix>
References: <20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-v2-1-2e665f072f8f@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-v2-1-2e665f072f8f@linaro.org>

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 ?

>
> Take the example of the above cited sensor drivers and directly apply the
> requested exposure value from user-space. The 'lpfr' variable still
> functions as before but the 'shutter' variable can be dispensed with as a
> result.
>
> Once done a similar run of the test application requesting higher exposure
> looks like this, with 'exp' written directly to the sensor.
>
> [  133.207884] imx412 20-001a: Received exp 1608, analog gain 0
> [  133.207899] imx412 20-001a: Set exp 1608, analog gain 0, lpfr 3546
> [  133.905309] imx412 20-001a: Received exp 2844, analog gain 100
> [  133.905344] imx412 20-001a: Set exp 2844, analog gain 100, lpfr 3546
> [  134.241705] imx412 20-001a: Received exp 3524, analog gain 110
> [  134.241775] imx412 20-001a: Set exp 3524, analog gain 110, lpfr 3546
>
> The result is then setting the sensor exposure to lower values results in
> darker, less exposure images and vice versa with higher exposure values.
>
> Fixes: 9214e86c0cc1 ("media: i2c: Add imx412 camera sensor driver")
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # qrb5165-rb5/imx577
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
> Using libcamera/SoftISP on a Qualcomm RB5 with the imx577 sensor I found
> that unlike on other platforms such as the Lenovo x13s/ov5675 the image was
> constantly getting darker and darker.
>
> At first I assumed a bug in SoftISP but, looking into the code it appeared
> SoftISP was requesting higher and higher exposure values which resulted in
> the image getting progressively darker.
>
> To my mind the software contract between user-space and kernel should be
> increasing exposure requests always meant higher exposure but, to be
> certain I asked around on IRC.
>
> Those polled agreed in principle that the software contract was consistent
> across sensors.
>
> 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 ?

>
> As a first pass I'm submitting the fix for the sensor I have but, I expect
> if this fix is acceptable upstream it should be pushed to most of the imx
> sensors with what seems to be copy/paste code for the exposure.
> ---
> Changes in v2:
> - Fix typo in patch 42 -> 412
> - Link to v1: https://lore.kernel.org/r/20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-v1-1-4b3a9426bde8@linaro.org
> ---
>  drivers/media/i2c/imx412.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index 0efce329525e4..7d1f7af0a9dff 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -542,14 +542,13 @@ static int imx412_update_controls(struct imx412 *imx412,
>   */
>  static int imx412_update_exp_gain(struct imx412 *imx412, u32 exposure, u32 gain)
>  {
> -	u32 lpfr, shutter;
> +	u32 lpfr;
>  	int ret;
>
>  	lpfr = imx412->vblank + imx412->cur_mode->height;
> -	shutter = lpfr - exposure;
>
> -	dev_dbg(imx412->dev, "Set exp %u, analog gain %u, shutter %u, lpfr %u",
> -		exposure, gain, shutter, lpfr);
> +	dev_dbg(imx412->dev, "Set exp %u, analog gain %u, lpfr %u",
> +		exposure, gain, lpfr);
>
>  	ret = imx412_write_reg(imx412, IMX412_REG_HOLD, 1, 1);
>  	if (ret)
> @@ -559,7 +558,7 @@ static int imx412_update_exp_gain(struct imx412 *imx412, u32 exposure, u32 gain)
>  	if (ret)
>  		goto error_release_group_hold;
>
> -	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 ?

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 (ret)
>  		goto error_release_group_hold;
>
>
> ---
> base-commit: ff3959189f1b97e99497183d76ab9b007bec4c88
> change-id: 20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-f1fee6070641
>
> Best regards,
> --
> Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>
>