Received-SPF: pass (google.com: domain of linux-kernel+bounces-173330-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <a506816a-cbcb-467e-83b8-11ba9a1b1a90@quicinc.com>
Date: Wed, 8 May 2024 16:38:16 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] media: i2c: Fix imx42 exposure control
To: 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>
CC: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        <linux-media@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-v1-1-4b3a9426bde8@linaro.org>
Content-Language: en-US
From: "Gjorgji Rosikopulos (Consultant)" <quic_grosikop@quicinc.com>
In-Reply-To: <20240506-b4-linux-next-camss-x13s-mmsol-integration-in-test-imx577-fix-v1-1-4b3a9426bde8@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

Hi Bryan,

Thanks for fixing this.

We have also found this issue during our internal testing but somehow
was not posted upstream...

On 5/7/2024 1:33 AM, 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.
> 
> 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.

As far as i know this issue is only for this imx sensor driver.
The sensor driver for imx412 was submitted along with imx335 and imx334,
maybe after all the reworking this was missed.
Our apologies for that.
imx334 and imx335 are using shutter for setting the exposure from where
this calculation is taken.

However imx412 uses number of lines for exposure.

Reviewed-by: Gjorgji Rosikopulos <quic_grosikop@quicinc.com>

~Gjorgji