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

njhollinghurst · 2023-11-08T11:43:08Z

In QBC full-resolution mode, reduce QBC_LPF_INTENSITY parameter to reduce the occurrence of re-mosaic artefacts in colourful scenes, at the slight expense of black-and-white ones.

See IMX708_IQ_Tuning_Operation_Manual_v1.0.0 section 4.

Higher values reduce broken-line artifacts in black-and-white gratings with perfect white balance gains, but can introduce "ladder" artefacts in coloured gratings and textures. It is a trade-off, but existing test images and user feedback suggest the latter are more important. Reducing the register value reduces the frequency (but not the severity) of these artifacts in many cases.

njhollinghurst · 2023-11-08T11:50:34Z

Sony don't specify definitive values for these QBC parameters. I also experimented with QBC Re-mosaic noise-reduction and false-colour curves but those had a less obvious effect, so have left them at the defaults (for minimal filtering).

njhollinghurst · 2023-11-08T13:46:52Z

Another thought: Since the change can't be proven to be monotonically beneficial, maybe it should be a module parameter?
(I see we have a couple of those already in imx477, e.g. dpc_enable which is very much in the same ballpark.)
@6by9 @naushir opinions?

6by9 · 2023-11-08T14:17:06Z

No particularly strong views from me.
If you can come up with an appropriate module name then you could do that.

njhollinghurst · 2023-11-08T14:38:41Z

Hmm. The register name LPF_INTENSITY makes it easily confused with a different sharpness parameter, also documented as "LPF intensity", and doesn't seem to capture its purpose. Might just call it qbc_adjust.

6by9 · 2023-11-08T14:47:32Z

Hmm. The register name LPF_INTENSITY makes it easily confused with a different sharpness parameter, also documented as "LPF intensity", and doesn't seem to capture its purpose. Might just call it qbc_adjust.

If you assign specific values rather than allowing the user to write any arbitrary rubbish, then you can describe what those values are in the parameter text, eg modinfo drm

pi@bookworm64:~ $ modinfo drm
filename:       /lib/modules/6.6.0-v8+/kernel/drivers/gpu/drm/drm.ko.xz
...
parm:           debug:Enable debug output, where each bit enables a debug category.
		Bit 0 (0x01)  will enable CORE messages (drm core code)
		Bit 1 (0x02)  will enable DRIVER messages (drm controller code)
		Bit 2 (0x04)  will enable KMS messages (modesetting code)
		Bit 3 (0x08)  will enable PRIME messages (prime code)
		Bit 4 (0x10)  will enable ATOMIC messages (atomic code)
		Bit 5 (0x20)  will enable VBL messages (vblank code)
		Bit 7 (0x80)  will enable LEASE messages (leasing code)
		Bit 8 (0x100) will enable DP messages (displayport code) (ulong)
parm:           edid_fixup:Minimum number of valid EDID header bytes (0-8, default 6) (int)

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_print.c#L45

naushir · 2023-11-09T09:53:44Z

qbc_adjust seems a reasonable name to me.

6by9 · 2023-11-09T10:16:20Z

drivers/media/i2c/imx708.c

@@ -1515,6 +1521,10 @@ static int imx708_start_streaming(struct imx708 *imx708)
 		return ret;
 	}

+	/* Quad Bayer re-mosaic adjustments (for full-resolution mode only) */


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.

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.

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.

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

6by9 · 2023-11-09T10:23:08Z

drivers/media/i2c/imx708.c

@@ -1515,6 +1521,10 @@ static int imx708_start_streaming(struct imx708 *imx708)
 		return ret;
 	}

+	/* Quad Bayer re-mosaic adjustments (for full-resolution mode only) */
+	imx708_write_reg(imx708, 0xC428, IMX708_REG_VALUE_08BIT, !qbc_adjust);
+	imx708_write_reg(imx708, 0xC429, IMX708_REG_VALUE_08BIT, qbc_adjust);


0xc428 is an enable and 0xc429 is a strength?

Checking datasheet it is, but with 0xc428 = 0 being enabled, and = 1 being disabled. Quirky!

I'd be tempted to use qbc_adjust ? 0x01 : 0x00 instead of !qbc_adjust to make it more obvious the values being programmed, otherwise you end up checking the C specification to confirm that !0 = 1. Even better would be macros defining the bits in the register.

I've seen many examples of ! and !! for this elsewhere but I agree ?: is clearer.

Just make sure you get them the right way round....

Now reworked and hopefully clearer, if a bit less terse.

In full-resolution mode, the LPF_INTENSITY_EN and LPF_INTENSITY registers control Quad Bayer Re-mosaic broken line correction. Expose this as a module parameter "qbc_adjust": zero disables the correction and values in the range 2 to 5 set its strength. There is a trade-off between coloured and monochrome patterns. The previous fixed value 4 could produce ladder/spots artefacts in coloured textures. The new default value 2 may suit a wider range of scenes. Signed-off-by: Nick Hollinghurst <[email protected]>

6by9 · 2023-11-09T14:34:10Z

LGTM (I'll ignore the unrelated whitespace change).

naushir · 2023-11-09T14:42:40Z

LGTM

kernel: KMS stale dlist alloc changes See: raspberrypi/linux#5684 kernel: drm/vc4: crtc: Support odd horizontal timings on BCM2712 See: raspberrypi/linux#5679 kernel: spi: dw-dma: Get the last DMA scoop out of the FIFO See: raspberrypi/linux#5699 kernel: defconfigs: Drop FB_UDL from all Pi defconfigs See: raspberrypi/linux#5702 kernel: RP1 GPIO header SDIO support See: raspberrypi/linux#5704 kernel: ASoC: bcm: audioinjector_octo: Add soundcard owner See: raspberrypi/linux#5705 kernel: drivers: media: imx708: Tweak broken line correction parameter See: raspberrypi/linux#5701

njhollinghurst force-pushed the imx708_broken_lines branch from c2d22e6 to ae1547c Compare November 8, 2023 17:16

6by9 reviewed Nov 9, 2023

View reviewed changes

njhollinghurst force-pushed the imx708_broken_lines branch from ae1547c to 5cae9cc Compare November 9, 2023 11:46

pelwell merged commit f364e0e into rpi-6.1.y Nov 9, 2023
12 checks passed

njhollinghurst deleted the imx708_broken_lines branch January 24, 2024 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

njhollinghurst commented Nov 8, 2023

njhollinghurst commented Nov 8, 2023

njhollinghurst commented Nov 8, 2023

6by9 commented Nov 8, 2023

njhollinghurst commented Nov 8, 2023

6by9 commented Nov 8, 2023

naushir commented Nov 9, 2023

6by9 Nov 9, 2023

njhollinghurst Nov 9, 2023 •

edited

Loading

6by9 Nov 9, 2023

njhollinghurst Nov 9, 2023

6by9 Nov 9, 2023

njhollinghurst Nov 9, 2023

pelwell Nov 9, 2023

njhollinghurst Nov 9, 2023

6by9 commented Nov 9, 2023

naushir commented Nov 9, 2023

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

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

Conversation

njhollinghurst commented Nov 8, 2023

njhollinghurst commented Nov 8, 2023

njhollinghurst commented Nov 8, 2023

6by9 commented Nov 8, 2023

njhollinghurst commented Nov 8, 2023

6by9 commented Nov 8, 2023

naushir commented Nov 9, 2023

6by9 Nov 9, 2023

Choose a reason for hiding this comment

njhollinghurst Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

6by9 Nov 9, 2023

Choose a reason for hiding this comment

njhollinghurst Nov 9, 2023

Choose a reason for hiding this comment

6by9 Nov 9, 2023

Choose a reason for hiding this comment

njhollinghurst Nov 9, 2023

Choose a reason for hiding this comment

pelwell Nov 9, 2023

Choose a reason for hiding this comment

njhollinghurst Nov 9, 2023

Choose a reason for hiding this comment

6by9 commented Nov 9, 2023

naushir commented Nov 9, 2023

njhollinghurst Nov 9, 2023 •

edited

Loading