Re: [PATCH v1 07/15] media: i2c: imx290: Factor out black level setting to a function

Dave Stevenson
2022-11-24T15:10:22Z

Hi Laurent

On Wed, 23 Nov 2022 at 11:00, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> 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.

> > 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.

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.

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.

As it stands the code retains the existing (weird) behaviour, and
drops the duplication, so I'll add:

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 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