Hi David,
On Thu, Nov 24, 2022 at 05:16:17PM +0000, David Plowman wrote:
> On Thu, 24 Nov 2022 at 16:58, Laurent Pinchart wrote:
> > On Thu, Nov 24, 2022 at 03:10:02PM +0000, Dave Stevenson wrote:
> > > On Wed, 23 Nov 2022 at 11:00, Laurent Pinchart wrote:
> > > > On Wed, Nov 23, 2022 at 10:08:19AM +0000, Dave Stevenson wrote:
> > > > > On Tue, 22 Nov 2022 at 22:34, Laurent Pinchart wrote:
> > > > > >
> > > > > > The black level programmed in the BLKLEVEL register depends on the
> > > > > > output format. The black level value computation is currently performed
> > > > > > in imx290_set_ctrl(), in addition to having different black level values
> > > > > > in the output-specific register value tables. Move it to a separate
> > > > > > function to simplify the imx290_set_ctrl() code.
> > > > >
> > > > > Does this actually help?
> > > > > The sensor only supports 10 and 12 bpp readout, so making it generic
> > > > > for any bpp is rather redundant.
> > > > > If you are looking at having different on-sensor black level values
> > > > > kicking around, then tuning data goes out the window.
> > > >
> > > > This may be needed for some use cases, but that's not what I'm after
> > > > here. By the way, could you explain why we can't just set it to 0 in the
> > > > sensor and avoid subtracting it in the ISP (at least for the data
> > > > pedestal value that is added on purpose in the sensor, there can also be
> > > > an additional black level due to other reasons) ? I'm sure there's a
> > > > reason :-)
> > >
> > > A quick play does imply that it works with 0 being defined in both
> > > places, but personally I'd prefer following the manufacturer's
> > > guidance on this sort of setting. David P may have some bright ideas
> > > as to why a pedestal is desired.
> > >
> > > There is a little bit of me thinking that if it were 0, then the mono
> > > variant of the sensor with a lens exhibiting minimal vignetting could
> > > be used directly for 10 or 12bit data under controlled lighting.
> > > However that is a very specific and unusual use case, so not worth
> > > potentially compromising IQ for normal use cases.
> >
> > One reason I can think of is to get the dark areas away from
> > non-linearities in the analogue path, but I lack visibility (no pun
> > intended) on the real issues this could cause. Some insight from David
> > would indeed be very appreciated.
>
> Not sure I can add very much! If you don't have a black level it's
> certainly true that your "zero" pixels will have a very odd
> distribution because the noise can't take them negative. Perhaps
> there's a 50% chance of getting zero, and a 50% chance of positive
> noise? So that would be like a 50% chance of zero, and 50% of coming
> from a "half Normal" distribution? Which would give an expected value
> of something like 0.4 * standard deviation. Or something like that!
Thanks for the feedback. I wonder if a good rule would be to set the
pedestal to N * sigma, for some value of N, to avoid messing up the
noise distribution which could impact denoising negatively. Or maybe
that's not something that will happen in practice and the denoising
algorithms will be totally fine, maybe the only practical issue with no
pedestal would be the non-linearity of the analogue path close to 0.
> > > > > OK we do appear to have duplication between imx290_[10|12]bit_settings
> > > > > and imx290_set_ctrl, but I don't get the need to compute it.
> > > >
> > > > The idea was to centralize black level setting in a single place, to
> > > > prepare for the removal of he imx290->bpp field. It's really in
> > > > imx290_set_ctrl() that it matters, I could keep the value in the
> > > > imx290_10bit_settings and imx290_12bit_settings arrays, but I thought I
> > > > could as well do it the same way everywhere.
> > >
> > > I'd missed that it was reprogramming the black level for the test patterns.
> > >
> > > Changing the black level when choosing a test pattern seems a very odd
> > > thing to do. You're wanting to test the pipeline as configured for the
> > > sensor, but you're artificially changing the incoming data and
> > > therefore any processing steps are misconfigured.
> >
> > Now that you mention it, it's indeed very clear that there's a problem
> > there. We shouldn't have to change ISP parameters when switching to a
> > test pattern (some things may need to change in the algorithms though,
> > AGC and AWB won't necessarily work nicely).
> >
> > > Looking at the data it's weird. Viewing the data as
> > > V4L2_PIX_FMT_SRGGB12 Pattern 4 as is gives a 12 pixel pattern of
> > > (bytes reversed):
> > > 0105 0105 c204 c204 3e01 3e01 0000 0000 0000 0000 0000 0105
> > > repeated multiple times along each line, and then that line repeated
> > > vertically. So we definitely have pixels below the black level.
> > > Alter the black level back to the default 0xf0 and you get:
> > > f105 f105 b205 b205 2e02 2e02 0000 0000 9c00 9c00 0000 f105
> > > So it's added the offset of 0xf0 to the non-zero pixels, but only some
> > > of the zero ones and then not by the configured amount.
> >
> > What the... :-S
> >
> > > My datasheets have no reference to a PGCTRL register or 0x308c, so I
> > > can't say if Sony has provided any guidance on needing to change black level.
> > > Perhaps Manivannan has some further information on this as he added
> > > the test pattern support.
> >
> > I thought I saw documentation about that, but it was for the IMX296, not
> > the IMX290.
> >
> > > As it stands the code retains the existing (weird) behaviour, and
> > > drops the duplication, so I'll add:
> > >
> > > Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> >
> > Thanks. We can fix the test pattern on top.
> >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/media/i2c/imx290.c | 53 +++++++++++++++++++++-----------------
> > > > > > 1 file changed, 29 insertions(+), 24 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > > > index 135ed55592a1..d9fc6c44b0f0 100644
> > > > > > --- a/drivers/media/i2c/imx290.c
> > > > > > +++ b/drivers/media/i2c/imx290.c
> > > > > > @@ -152,6 +152,9 @@
> > > > > > #define IMX290_PIXEL_ARRAY_RECORDING_WIDTH 1920
> > > > > > #define IMX290_PIXEL_ARRAY_RECORDING_HEIGHT 1080
> > > > > >
> > > > > > +/* Equivalent value for 16bpp */
> > > > > > +#define IMX290_BLACK_LEVEL_DEFAULT 3840
> > > > > > +
> > > > > > #define IMX290_NUM_SUPPLIES 3
> > > > > >
> > > > > > struct imx290_regval {
> > > > > > @@ -315,7 +318,6 @@ static const struct imx290_regval imx290_10bit_settings[] = {
> > > > > > { IMX290_ADBIT2, IMX290_ADBIT2_10BIT },
> > > > > > { IMX290_ADBIT3, IMX290_ADBIT3_10BIT },
> > > > > > { IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW10 },
> > > > > > - { IMX290_BLKLEVEL, 60 },
> > > > > > };
> > > > > >
> > > > > > static const struct imx290_regval imx290_12bit_settings[] = {
> > > > > > @@ -325,7 +327,6 @@ static const struct imx290_regval imx290_12bit_settings[] = {
> > > > > > { IMX290_ADBIT2, IMX290_ADBIT2_12BIT },
> > > > > > { IMX290_ADBIT3, IMX290_ADBIT3_12BIT },
> > > > > > { IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 },
> > > > > > - { IMX290_BLKLEVEL, 240 },
> > > > > > };
> > > > > >
> > > > > > /* supported link frequencies */
> > > > > > @@ -516,35 +517,43 @@ static int imx290_set_data_lanes(struct imx290 *imx290)
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > +static int imx290_set_black_level(struct imx290 *imx290,
> > > > > > + unsigned int black_level, int *err)
> > > > > > +{
> > > > > > + return imx290_write(imx290, IMX290_BLKLEVEL,
> > > > > > + black_level >> (16 - imx290->bpp), err);
> > > > > > +}
> > > > > > +
> > > > > > static int imx290_write_current_format(struct imx290 *imx290)
> > > > > > {
> > > > > > + const struct imx290_regval *regs;
> > > > > > + unsigned int num_regs;
> > > > > > + unsigned int bpp;
> > > > > > int ret;
> > > > > >
> > > > > > switch (imx290->current_format.code) {
> > > > > > case MEDIA_BUS_FMT_SRGGB10_1X10:
> > > > > > - ret = imx290_set_register_array(imx290, imx290_10bit_settings,
> > > > > > - ARRAY_SIZE(
> > > > > > - imx290_10bit_settings));
> > > > > > - if (ret < 0) {
> > > > > > - dev_err(imx290->dev, "Could not set format registers\n");
> > > > > > - return ret;
> > > > > > - }
> > > > > > + regs = imx290_10bit_settings;
> > > > > > + num_regs = ARRAY_SIZE(imx290_10bit_settings);
> > > > > > + bpp = 10;
> > > > > > break;
> > > > > > case MEDIA_BUS_FMT_SRGGB12_1X12:
> > > > > > - ret = imx290_set_register_array(imx290, imx290_12bit_settings,
> > > > > > - ARRAY_SIZE(
> > > > > > - imx290_12bit_settings));
> > > > > > - if (ret < 0) {
> > > > > > - dev_err(imx290->dev, "Could not set format registers\n");
> > > > > > - return ret;
> > > > > > - }
> > > > > > + regs = imx290_12bit_settings;
> > > > > > + num_regs = ARRAY_SIZE(imx290_12bit_settings);
> > > > > > + bpp = 12;
> > > > > > break;
> > > > > > default:
> > > > > > dev_err(imx290->dev, "Unknown pixel format\n");
> > > > > > return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > - return 0;
> > > > > > + ret = imx290_set_register_array(imx290, regs, num_regs);
> > > > > > + if (ret < 0) {
> > > > > > + dev_err(imx290->dev, "Could not set format registers\n");
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + return imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT, &ret);
> > > > > > }
> > > > > >
> > > > > > /* ----------------------------------------------------------------------------
> > > > > > @@ -573,7 +582,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > >
> > > > > > case V4L2_CID_TEST_PATTERN:
> > > > > > if (ctrl->val) {
> > > > > > - imx290_write(imx290, IMX290_BLKLEVEL, 0, &ret);
> > > > > > + imx290_set_black_level(imx290, 0, &ret);
> > > > > > usleep_range(10000, 11000);
> > > > > > imx290_write(imx290, IMX290_PGCTRL,
> > > > > > (u8)(IMX290_PGCTRL_REGEN |
> > > > > > @@ -582,12 +591,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > } else {
> > > > > > imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
> > > > > > usleep_range(10000, 11000);
> > > > > > - if (imx290->bpp == 10)
> > > > > > - imx290_write(imx290, IMX290_BLKLEVEL, 0x3c,
> > > > > > - &ret);
> > > > > > - else /* 12 bits per pixel */
> > > > > > - imx290_write(imx290, IMX290_BLKLEVEL, 0xf0,
> > > > > > - &ret);
> > > > > > + imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT,
> > > > > > + &ret);
> > > > > > }
> > > > > > break;
> > > > > > default:
--
Regards,
Laurent Pinchart
