* [PATCH] media: ov5675: use group write to update digital gain
@ 2021-12-29 8:57 Bingbu Cao
2021-12-29 9:41 ` Sakari Ailus
0 siblings, 1 reply; 8+ messages in thread
From: Bingbu Cao @ 2021-12-29 8:57 UTC (permalink / raw)
To: linux-media, sakari.ailus
Cc: shawnx.tu, senozhatsky, tfiga, bingbu.cao, bingbu.cao, andy.yeh
MWB gain register are used to set gain for each mwb channel mannually.
However, it will involve some artifacts at low light environment as gain
cannot be applied to each channel synchronously. Update the driver to use
group write for digital gain to make the sure RGB digital gain be applied
together at frame boundary.
Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
drivers/media/i2c/ov5675.c | 32 +++++++++++++++++++++++++++++---
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 00925850fa7c..82ba9f56baec 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -50,14 +50,21 @@
#define OV5675_ANAL_GAIN_STEP 1
/* Digital gain controls from sensor */
+#define OV5675_REG_DIGITAL_GAIN 0x350a
#define OV5675_REG_MWB_R_GAIN 0x5019
#define OV5675_REG_MWB_G_GAIN 0x501b
#define OV5675_REG_MWB_B_GAIN 0x501d
-#define OV5675_DGTL_GAIN_MIN 0
+#define OV5675_DGTL_GAIN_MIN 1024
#define OV5675_DGTL_GAIN_MAX 4095
#define OV5675_DGTL_GAIN_STEP 1
#define OV5675_DGTL_GAIN_DEFAULT 1024
+/* Group Access */
+#define OV5675_REG_GROUP_ACCESS 0x3208
+#define OV5675_GROUP_HOLD_START 0x0
+#define OV5675_GROUP_HOLD_END 0x10
+#define OV5675_GROUP_HOLD_LAUNCH 0xa0
+
/* Test Pattern Control */
#define OV5675_REG_TEST_PATTERN 0x4503
#define OV5675_TEST_PATTERN_ENABLE BIT(7)
@@ -587,6 +594,12 @@ static int ov5675_update_digital_gain(struct ov5675 *ov5675, u32 d_gain)
{
int ret;
+ ret = ov5675_write_reg(ov5675, OV5675_REG_GROUP_ACCESS,
+ OV5675_REG_VALUE_08BIT,
+ OV5675_GROUP_HOLD_START);
+ if (ret)
+ return ret;
+
ret = ov5675_write_reg(ov5675, OV5675_REG_MWB_R_GAIN,
OV5675_REG_VALUE_16BIT, d_gain);
if (ret)
@@ -597,8 +610,21 @@ static int ov5675_update_digital_gain(struct ov5675 *ov5675, u32 d_gain)
if (ret)
return ret;
- return ov5675_write_reg(ov5675, OV5675_REG_MWB_B_GAIN,
- OV5675_REG_VALUE_16BIT, d_gain);
+ ret = ov5675_write_reg(ov5675, OV5675_REG_MWB_B_GAIN,
+ OV5675_REG_VALUE_16BIT, d_gain);
+ if (ret)
+ return ret;
+
+ ret = ov5675_write_reg(ov5675, OV5675_REG_GROUP_ACCESS,
+ OV5675_REG_VALUE_08BIT,
+ OV5675_GROUP_HOLD_END);
+ if (ret)
+ return ret;
+
+ ret = ov5675_write_reg(ov5675, OV5675_REG_GROUP_ACCESS,
+ OV5675_REG_VALUE_08BIT,
+ OV5675_GROUP_HOLD_LAUNCH);
+ return ret;
}
static int ov5675_test_pattern(struct ov5675 *ov5675, u32 pattern)
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] media: ov5675: use group write to update digital gain
2021-12-29 8:57 [PATCH] media: ov5675: use group write to update digital gain Bingbu Cao
@ 2021-12-29 9:41 ` Sakari Ailus
2021-12-29 10:00 ` Cao, Bingbu
0 siblings, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2021-12-29 9:41 UTC (permalink / raw)
To: Bingbu Cao
Cc: linux-media, shawnx.tu, senozhatsky, tfiga, bingbu.cao, andy.yeh
Hi Bingbu,
On Wed, Dec 29, 2021 at 04:57:39PM +0800, Bingbu Cao wrote:
> MWB gain register are used to set gain for each mwb channel mannually.
> However, it will involve some artifacts at low light environment as gain
> cannot be applied to each channel synchronously. Update the driver to use
> group write for digital gain to make the sure RGB digital gain be applied
> together at frame boundary.
How about the analogue gain and exposure time?
Shouldn't they be applied similarly as well? Adding two more writes
increases the probability of missing a frame there.
This is of course a trick since the control framework doesn't really
support this, but I think this support should be added.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] media: ov5675: use group write to update digital gain
2021-12-29 9:41 ` Sakari Ailus
@ 2021-12-29 10:00 ` Cao, Bingbu
2022-01-10 11:32 ` Sakari Ailus
0 siblings, 1 reply; 8+ messages in thread
From: Cao, Bingbu @ 2021-12-29 10:00 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Tu, ShawnX, senozhatsky, tfiga, bingbu.cao, Yeh, Andy
Hi Sakari,
Thanks for your review.
________________________
BRs,
Bingbu Cao
> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> Sent: Wednesday, December 29, 2021 5:42 PM
> To: Cao, Bingbu <bingbu.cao@intel.com>
> Cc: linux-media@vger.kernel.org; Tu, ShawnX <shawnx.tu@intel.com>;
> senozhatsky@chromium.org; tfiga@chromium.org; bingbu.cao@linux.intel.com;
> Yeh, Andy <andy.yeh@intel.com>
> Subject: Re: [PATCH] media: ov5675: use group write to update digital
> gain
>
> Hi Bingbu,
>
> On Wed, Dec 29, 2021 at 04:57:39PM +0800, Bingbu Cao wrote:
> > MWB gain register are used to set gain for each mwb channel mannually.
> > However, it will involve some artifacts at low light environment as
> > gain cannot be applied to each channel synchronously. Update the
> > driver to use group write for digital gain to make the sure RGB
> > digital gain be applied together at frame boundary.
>
> How about the analogue gain and exposure time?
>
> Shouldn't they be applied similarly as well? Adding two more writes
> increases the probability of missing a frame there.
We did not meet issue related to analog gain as the it was applied by only
1 reg write, it looks like same issue we found on ov8856, changing to set
digital gain by only 1 global gain write will fix the problem.
>
> This is of course a trick since the control framework doesn't really
> support this, but I think this support should be added.
>
> --
> Regards,
>
> Sakari Ailus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: ov5675: use group write to update digital gain
2021-12-29 10:00 ` Cao, Bingbu
@ 2022-01-10 11:32 ` Sakari Ailus
2022-01-11 3:22 ` Bingbu Cao
2022-01-11 4:27 ` Tomasz Figa
0 siblings, 2 replies; 8+ messages in thread
From: Sakari Ailus @ 2022-01-10 11:32 UTC (permalink / raw)
To: Cao, Bingbu
Cc: linux-media, Tu, ShawnX, senozhatsky, tfiga, bingbu.cao, Yeh, Andy
On Wed, Dec 29, 2021 at 10:00:43AM +0000, Cao, Bingbu wrote:
> Hi Sakari,
>
> Thanks for your review.
>
> ________________________
> BRs,
> Bingbu Cao
>
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Sent: Wednesday, December 29, 2021 5:42 PM
> > To: Cao, Bingbu <bingbu.cao@intel.com>
> > Cc: linux-media@vger.kernel.org; Tu, ShawnX <shawnx.tu@intel.com>;
> > senozhatsky@chromium.org; tfiga@chromium.org; bingbu.cao@linux.intel.com;
> > Yeh, Andy <andy.yeh@intel.com>
> > Subject: Re: [PATCH] media: ov5675: use group write to update digital
> > gain
> >
> > Hi Bingbu,
> >
> > On Wed, Dec 29, 2021 at 04:57:39PM +0800, Bingbu Cao wrote:
> > > MWB gain register are used to set gain for each mwb channel mannually.
> > > However, it will involve some artifacts at low light environment as
> > > gain cannot be applied to each channel synchronously. Update the
> > > driver to use group write for digital gain to make the sure RGB
> > > digital gain be applied together at frame boundary.
> >
> > How about the analogue gain and exposure time?
> >
> > Shouldn't they be applied similarly as well? Adding two more writes
> > increases the probability of missing a frame there.
>
> We did not meet issue related to analog gain as the it was applied by only
> 1 reg write, it looks like same issue we found on ov8856, changing to set
> digital gain by only 1 global gain write will fix the problem.
That device is different in its support for global digital gain. This patch
sets the gain for each component separately.
Adding more writes on a given frame increases the probability of slipping
to the following frame. Doing the exposure and gain updates in the same
group write would alleviate that a little.
>
> >
> > This is of course a trick since the control framework doesn't really
> > support this, but I think this support should be added.
> >
> > --
> > Regards,
> >
> > Sakari Ailus
--
Sakari Ailus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: ov5675: use group write to update digital gain
2022-01-10 11:32 ` Sakari Ailus
@ 2022-01-11 3:22 ` Bingbu Cao
2022-01-11 4:27 ` Tomasz Figa
1 sibling, 0 replies; 8+ messages in thread
From: Bingbu Cao @ 2022-01-11 3:22 UTC (permalink / raw)
To: Sakari Ailus, Cao, Bingbu
Cc: linux-media, Tu, ShawnX, senozhatsky, tfiga, Yeh, Andy
On 1/10/22 7:32 PM, Sakari Ailus wrote:
> On Wed, Dec 29, 2021 at 10:00:43AM +0000, Cao, Bingbu wrote:
>> Hi Sakari,
>>
>> Thanks for your review.
>>
>> ________________________
>> BRs,
>> Bingbu Cao
>>
>>> -----Original Message-----
>>> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> Sent: Wednesday, December 29, 2021 5:42 PM
>>> To: Cao, Bingbu <bingbu.cao@intel.com>
>>> Cc: linux-media@vger.kernel.org; Tu, ShawnX <shawnx.tu@intel.com>;
>>> senozhatsky@chromium.org; tfiga@chromium.org; bingbu.cao@linux.intel.com;
>>> Yeh, Andy <andy.yeh@intel.com>
>>> Subject: Re: [PATCH] media: ov5675: use group write to update digital
>>> gain
>>>
>>> Hi Bingbu,
>>>
>>> On Wed, Dec 29, 2021 at 04:57:39PM +0800, Bingbu Cao wrote:
>>>> MWB gain register are used to set gain for each mwb channel mannually.
>>>> However, it will involve some artifacts at low light environment as
>>>> gain cannot be applied to each channel synchronously. Update the
>>>> driver to use group write for digital gain to make the sure RGB
>>>> digital gain be applied together at frame boundary.
>>>
>>> How about the analogue gain and exposure time?
>>>
>>> Shouldn't they be applied similarly as well? Adding two more writes
>>> increases the probability of missing a frame there.
>>
>> We did not meet issue related to analog gain as the it was applied by only
>> 1 reg write, it looks like same issue we found on ov8856, changing to set
>> digital gain by only 1 global gain write will fix the problem.
>
> That device is different in its support for global digital gain. This patch
> sets the gain for each component separately.
>
> Adding more writes on a given frame increases the probability of slipping
> to the following frame. Doing the exposure and gain updates in the same
> group write would alleviate that a little.
Sakari,
It is feasible to put the exposure and gain update in a same group, however, lunching
the group at different time may increase the probability of slipping as well.
What do you think?
>
>>
>>>
>>> This is of course a trick since the control framework doesn't really
>>> support this, but I think this support should be added.
>>>
>>> --
>>> Regards,
>>>
>>> Sakari Ailus
>
--
Best regards,
Bingbu Cao
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: ov5675: use group write to update digital gain
2022-01-10 11:32 ` Sakari Ailus
2022-01-11 3:22 ` Bingbu Cao
@ 2022-01-11 4:27 ` Tomasz Figa
2022-01-27 3:14 ` Bingbu Cao
1 sibling, 1 reply; 8+ messages in thread
From: Tomasz Figa @ 2022-01-11 4:27 UTC (permalink / raw)
To: Sakari Ailus
Cc: Cao, Bingbu, linux-media, Tu, ShawnX, senozhatsky, bingbu.cao, Yeh, Andy
Hi Sakari,
On Mon, Jan 10, 2022 at 8:32 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Wed, Dec 29, 2021 at 10:00:43AM +0000, Cao, Bingbu wrote:
> > Hi Sakari,
> >
> > Thanks for your review.
> >
> > ________________________
> > BRs,
> > Bingbu Cao
> >
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Sent: Wednesday, December 29, 2021 5:42 PM
> > > To: Cao, Bingbu <bingbu.cao@intel.com>
> > > Cc: linux-media@vger.kernel.org; Tu, ShawnX <shawnx.tu@intel.com>;
> > > senozhatsky@chromium.org; tfiga@chromium.org; bingbu.cao@linux.intel.com;
> > > Yeh, Andy <andy.yeh@intel.com>
> > > Subject: Re: [PATCH] media: ov5675: use group write to update digital
> > > gain
> > >
> > > Hi Bingbu,
> > >
> > > On Wed, Dec 29, 2021 at 04:57:39PM +0800, Bingbu Cao wrote:
> > > > MWB gain register are used to set gain for each mwb channel mannually.
> > > > However, it will involve some artifacts at low light environment as
> > > > gain cannot be applied to each channel synchronously. Update the
> > > > driver to use group write for digital gain to make the sure RGB
> > > > digital gain be applied together at frame boundary.
> > >
> > > How about the analogue gain and exposure time?
> > >
> > > Shouldn't they be applied similarly as well? Adding two more writes
> > > increases the probability of missing a frame there.
> >
> > We did not meet issue related to analog gain as the it was applied by only
> > 1 reg write, it looks like same issue we found on ov8856, changing to set
> > digital gain by only 1 global gain write will fix the problem.
>
> That device is different in its support for global digital gain. This patch
> sets the gain for each component separately.
That's not what the patch does. The existing code programs the 3
per-component registers separately. This patch made it happen under
one write group. It doesn't increase the likelihood of the frame
having wrong parameters - given the same timeline, before this patch,
the frame would just have an even worse, partial gain setting, while
with this patch it can either have the old or new gain.
Best regards,
Tomasz
>
> Adding more writes on a given frame increases the probability of slipping
> to the following frame. Doing the exposure and gain updates in the same
> group write would alleviate that a little.
>
> >
> > >
> > > This is of course a trick since the control framework doesn't really
> > > support this, but I think this support should be added.
> > >
> > > --
> > > Regards,
> > >
> > > Sakari Ailus
>
> --
> Sakari Ailus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: ov5675: use group write to update digital gain
2022-01-11 4:27 ` Tomasz Figa
@ 2022-01-27 3:14 ` Bingbu Cao
2022-01-28 9:42 ` Sakari Ailus
0 siblings, 1 reply; 8+ messages in thread
From: Bingbu Cao @ 2022-01-27 3:14 UTC (permalink / raw)
To: Tomasz Figa, Sakari Ailus
Cc: Cao, Bingbu, linux-media, Tu, ShawnX, senozhatsky, Yeh, Andy
Sakari,
I agree with Tomasz, the group write will not cause timing issues, instead it will help
on that. So we did not need to group hold exposure and digital gain along with analog
gain. Driver can not make the policy that the exposure, a-gain and d-gain are applied
together.
On 1/11/22 12:27 PM, Tomasz Figa wrote:
> Hi Sakari,
>
> On Mon, Jan 10, 2022 at 8:32 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
>>
>> On Wed, Dec 29, 2021 at 10:00:43AM +0000, Cao, Bingbu wrote:
>>> Hi Sakari,
>>>
>>> Thanks for your review.
>>>
>>> ________________________
>>> BRs,
>>> Bingbu Cao
>>>
>>>> -----Original Message-----
>>>> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> Sent: Wednesday, December 29, 2021 5:42 PM
>>>> To: Cao, Bingbu <bingbu.cao@intel.com>
>>>> Cc: linux-media@vger.kernel.org; Tu, ShawnX <shawnx.tu@intel.com>;
>>>> senozhatsky@chromium.org; tfiga@chromium.org; bingbu.cao@linux.intel.com;
>>>> Yeh, Andy <andy.yeh@intel.com>
>>>> Subject: Re: [PATCH] media: ov5675: use group write to update digital
>>>> gain
>>>>
>>>> Hi Bingbu,
>>>>
>>>> On Wed, Dec 29, 2021 at 04:57:39PM +0800, Bingbu Cao wrote:
>>>>> MWB gain register are used to set gain for each mwb channel mannually.
>>>>> However, it will involve some artifacts at low light environment as
>>>>> gain cannot be applied to each channel synchronously. Update the
>>>>> driver to use group write for digital gain to make the sure RGB
>>>>> digital gain be applied together at frame boundary.
>>>>
>>>> How about the analogue gain and exposure time?
>>>>
>>>> Shouldn't they be applied similarly as well? Adding two more writes
>>>> increases the probability of missing a frame there.
>>>
>>> We did not meet issue related to analog gain as the it was applied by only
>>> 1 reg write, it looks like same issue we found on ov8856, changing to set
>>> digital gain by only 1 global gain write will fix the problem.
>>
>> That device is different in its support for global digital gain. This patch
>> sets the gain for each component separately.
>
> That's not what the patch does. The existing code programs the 3
> per-component registers separately. This patch made it happen under
> one write group. It doesn't increase the likelihood of the frame
> having wrong parameters - given the same timeline, before this patch,
> the frame would just have an even worse, partial gain setting, while
> with this patch it can either have the old or new gain.
>
> Best regards,
> Tomasz
>
>>
>> Adding more writes on a given frame increases the probability of slipping
>> to the following frame. Doing the exposure and gain updates in the same
>> group write would alleviate that a little.
>>
>>>
>>>>
>>>> This is of course a trick since the control framework doesn't really
>>>> support this, but I think this support should be added.
>>>>
>>>> --
>>>> Regards,
>>>>
>>>> Sakari Ailus
>>
>> --
>> Sakari Ailus
--
Best regards,
Bingbu Cao
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: ov5675: use group write to update digital gain
2022-01-27 3:14 ` Bingbu Cao
@ 2022-01-28 9:42 ` Sakari Ailus
0 siblings, 0 replies; 8+ messages in thread
From: Sakari Ailus @ 2022-01-28 9:42 UTC (permalink / raw)
To: Bingbu Cao
Cc: Tomasz Figa, Cao, Bingbu, linux-media, Tu, ShawnX, senozhatsky,
Yeh, Andy
Hi Bingbu,
On Thu, Jan 27, 2022 at 11:14:56AM +0800, Bingbu Cao wrote:
> Sakari,
>
> I agree with Tomasz, the group write will not cause timing issues, instead it will help
> on that. So we did not need to group hold exposure and digital gain along with analog
> gain. Driver can not make the policy that the exposure, a-gain and d-gain are applied
> together.
I'll take the patch but we're leaving some technical debt here, admittedly
much of which was there to begin with.
Please wrap before 80.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-01-28 9:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-29 8:57 [PATCH] media: ov5675: use group write to update digital gain Bingbu Cao
2021-12-29 9:41 ` Sakari Ailus
2021-12-29 10:00 ` Cao, Bingbu
2022-01-10 11:32 ` Sakari Ailus
2022-01-11 3:22 ` Bingbu Cao
2022-01-11 4:27 ` Tomasz Figa
2022-01-27 3:14 ` Bingbu Cao
2022-01-28 9:42 ` Sakari Ailus
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.