Skip to content

drivers: media: imx708: Tweak broken line correction parameter #5701

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 9, 2023
+42 −8
Merged

drivers: media: imx708: Tweak broken line correction parameter #5701

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 42 additions & 8 deletions drivers/media/i2c/imx708.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@
#include <media/v4l2-fwnode.h>
#include <media/v4l2-mediabus.h>

/*
* Parameter to adjust Quad Bayer re-mosaic broken line correction
* strength, used in full-resolution mode only. Set zero to disable.
*/
static int qbc_adjust = 2;
module_param(qbc_adjust, int, 0644);
MODULE_PARM_DESC(qbc_adjust, "Quad Bayer broken line correction strength [0,2-5]");

#define IMX708_REG_VALUE_08BIT 1
#define IMX708_REG_VALUE_16BIT 2

Expand Down Expand Up @@ -99,11 +107,17 @@

/* HDR exposure ratio (long:med == med:short) */
#define IMX708_HDR_EXPOSURE_RATIO 4
#define IMX708_REG_MID_EXPOSURE 0x3116
#define IMX708_REG_SHT_EXPOSURE 0x0224
#define IMX708_REG_MID_EXPOSURE 0x3116
#define IMX708_REG_SHT_EXPOSURE 0x0224
#define IMX708_REG_MID_ANALOG_GAIN 0x3118
#define IMX708_REG_SHT_ANALOG_GAIN 0x0216

/* QBC Re-mosaic broken line correction registers */
#define IMX708_LPF_INTENSITY_EN 0xC428
#define IMX708_LPF_INTENSITY_ENABLED 0x00
#define IMX708_LPF_INTENSITY_DISABLED 0x01
#define IMX708_LPF_INTENSITY 0xC429

/*
* Metadata buffer holds a variety of data, all sent with the same VC/DT (0x12).
* It comprises two scanlines (of up to 5760 bytes each, for 4608 pixels)
Expand Down Expand Up @@ -171,6 +185,9 @@ struct imx708_mode {

/* HDR flag, used for checking if the current mode is HDR */
bool hdr;

/* Quad Bayer Re-mosaic flag */
bool remosaic;
};

/* Default PDAF pixel correction gains */
Expand Down Expand Up @@ -363,8 +380,6 @@ static const struct imx708_reg mode_4608x2592_regs[] = {
{0x341f, 0x20},
{0x3420, 0x00},
{0x3421, 0xd8},
{0xC428, 0x00},
{0xC429, 0x04},
{0x3366, 0x00},
{0x3367, 0x00},
{0x3368, 0x00},
Expand Down Expand Up @@ -677,7 +692,8 @@ static const struct imx708_mode supported_modes_10bit_no_hdr[] = {
.pixel_rate = 595200000,
.exposure_lines_min = 8,
.exposure_lines_step = 1,
.hdr = false
.hdr = false,
.remosaic = true
},
{
/* regular 2x2 binned. */
Expand All @@ -699,7 +715,8 @@ static const struct imx708_mode supported_modes_10bit_no_hdr[] = {
.pixel_rate = 585600000,
.exposure_lines_min = 4,
.exposure_lines_step = 2,
.hdr = false
.hdr = false,
.remosaic = false
},
{
/* 2x2 binned and cropped for 720p. */
Expand All @@ -721,7 +738,8 @@ static const struct imx708_mode supported_modes_10bit_no_hdr[] = {
.pixel_rate = 566400000,
.exposure_lines_min = 4,
.exposure_lines_step = 2,
.hdr = false
.hdr = false,
.remosaic = false
},
};

Expand All @@ -746,7 +764,8 @@ static const struct imx708_mode supported_modes_10bit_hdr[] = {
.pixel_rate = 777600000,
.exposure_lines_min = 8 * IMX708_HDR_EXPOSURE_RATIO * IMX708_HDR_EXPOSURE_RATIO,
.exposure_lines_step = 2 * IMX708_HDR_EXPOSURE_RATIO * IMX708_HDR_EXPOSURE_RATIO,
.hdr = true
.hdr = true,
.remosaic = false
}
};

Expand Down Expand Up @@ -1515,6 +1534,21 @@ static int imx708_start_streaming(struct imx708 *imx708)
return ret;
}

/* Quad Bayer re-mosaic adjustments (for full-resolution mode only) */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does the sensor handle this only affecting full res mode? The registers were in imx708_reg mode_4608x2592_regs so inherently only affected full res mode, but now this is common for all modes.

Copy link
Contributor Author

@njhollinghurst njhollinghurst Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presumed there were no side-effects from redundantly writing them, so it seemed unnecessary to spend a line of code checking the mode.

Previously they would have been left unchanged when switching from full-res to another mode (not that that happened often). More commonly they would keep their reset values (0x01,0x01). Yes I suppose it would make sense to explicitly set the "disable" flag in other modes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously they would have been left unchanged when switching from full-res to another mode (not that that happened often).

Did they actually persist? imx708_set_stream calls pm_runtime_get_sync on enable and pm_runtime_put on disable, which in theory enables and disables regulators (may be delayed, I've never checked). This is why Laurent is pushing for using pm_runtime_set_autosuspend_delay / pm_runtime_use_autosuspend / pm_runtime_put_autosuspend to definitely avoid dropping regulators on a mode switch.

If tested then I'm happy for it to remain unconditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A running switch can happen in libcamera-still when taking a timelapse, or some Python cases -- not common ones. Anyway, it now sets the enable conditionally.

Yes, tested. I'd forgotten about HDR mode but have now tested that too.

(I swapped the order of the writes in the hope it might do a tail merge and save a few bytes of code...)

if (imx708->mode->remosaic && qbc_adjust > 0) {
imx708_write_reg(imx708, IMX708_LPF_INTENSITY,
IMX708_REG_VALUE_08BIT, qbc_adjust);
imx708_write_reg(imx708,
IMX708_LPF_INTENSITY_EN,
IMX708_REG_VALUE_08BIT,
IMX708_LPF_INTENSITY_ENABLED);
} else {
imx708_write_reg(imx708,
IMX708_LPF_INTENSITY_EN,
IMX708_REG_VALUE_08BIT,
IMX708_LPF_INTENSITY_DISABLED);
}

/* Apply customized values from user */
ret = __v4l2_ctrl_handler_setup(imx708->sd.ctrl_handler);
if (ret)
Expand Down