RP1 DPI: Add interlaced modes by njhollinghurst · Pull Request #6529 · raspberrypi/linux

njhollinghurst · 2024-12-10T13:53:36Z

Add support for interlaced DPI modes on Raspberry Pi 5. (Draft for testing, based on rpi-6.6.y)

In order for this to work correctly, DPI's DE output must be enabled on GPIO1 (whether used or not). PIO can then snoop on DE and HSYNC signals to generate a fixed-up VSYNC on GPIO2.

This is the cut-down version of the patch which omits CSYNC support, but it should still be possible to implement CSYNC (and equalizing pulses) using an external PIO program; I will need to update the example in utils/piolib/examples.

6by9 · 2024-12-10T14:18:44Z

+				BITS(DPI_DMA_FRONT_PORCH_COLSM1, mode->hsync_start - mode->hdisplay - 1));
+
+		vctrl = BITS(DPI_DMA_CONTROL_VSYNC_POL, !!(mode->flags & DRM_MODE_FLAG_NVSYNC)) |
+			BITS(DPI_DMA_CONTROL_VBP_EN,    (mode->vtotal != mode->vsync_start))    |


Remove the excess white space. Normal coding style wouldn't align this type, only in tables.

6by9 · 2024-12-10T14:19:51Z

+		 * based on HSYNC, DE (which *must* both be mapped to GPIOs 1, 3 respectively).
+		 * This driver includes a PIO program to do that, when DE is enabled.
+		 *
+		 * An alternative fixup is to synthesize CSYNC from HSYNC and modified-VSYNC.


Have you still got CSYNC support? I thought you'd said it had been dropped.

DPI VSYNC waveform is somewhat arbitrary (since it can't be correct for interlace). Here it's designed to assist CSYNC generation. But I removed the PIO code that actually does generate CSYNC (for code bloat reduction).

6by9 · 2024-12-10T14:27:07Z

+{
+	static const int wrap_target = 14;
+	static const int wrap = 26;
+	u16 instructions[] = {  /* This is mutable */


Using statics means we can't instantiate the driver twice. Unlikely ever to happen, but I'd be tempted to put it in struct rpi1_dpi instead.

These are just named constants. The PIO compiler spits out lower-case #defines but I avoided those because they seemed unsightly, and there were formerly 3 different functions that defined similar-named constants differently.

I'd misread instructions as being static.

I assume the program gets copied into RP1 / PIO in some shape or form, so this doesn't need to remain in context after the pio_add_program call.

Ah, it's the other way around: instructions[] is mutable because it gets subjected to some runtime fixups (rather than have 4 or 8 different versions for different sync polarities). But I can change the style if it's unclear.

I assumed (perhaps wrongly) that using a run-time initializer is no less efficient than explicitly making a copy. And yes, it's not needed once it's been loaded into PIO instruction memory.

6by9 · 2024-12-10T14:31:05Z

BTW I've just noticed that the BITS macro is a roll-it-yourself shift.
There are macros FIELD_FIT FIELD_PREP, and FIELD_SET in include/linux/bitfield.h which also do size checking to ensure you don't end up writing too many bits into a field. See vc4_regs.h for example usage.

njhollinghurst · 2024-12-10T14:41:12Z

There are macros FIELD_FIT FIELD_PREP, and FIELD_SET in include/linux/bitfield.h which also do size checking to ensure you don't end up writing too many bits into a field. See vc4_regs.h for example usage.

I'll see if I can use those... ~~I'd like to keep the field definitions unmodified though, as they're taken straight from VRBuild output.~~ Oh, I see that I've already edited them since the VRB.

njhollinghurst · 2024-12-11T10:02:49Z

OK I just need to check it still works with the external/example program, and may adjust the offset of the "VSync helper" signal (which should make no difference to the built-in H/VSync case, that doesn't use it).

Would be nice to keep the door open for equalizing pulses, but as the generally-accepted "PAL" modeline has 576 complete lines, and I don't want to make a special case for it, it seems slightly doomed: obviously we can't put an equalizing pulse in the middle of an active scanline.

When initialized from panel_bridge_attach(), connector should allow interlaced modes rather than invariably rejecting them, so that other components can validate them. Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>

Almost no DPI hardware supports it, but it's useful for RP1 video out. Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>

Implement interlaced modes by wobbling the base pointer and VFP width for every field. This results in correct pixels but incorrect VSYNC. Now use PIO to generate a fixed-up VSYNC by sampling DE and HSYNC. This requires DPI's DE output to be mapped to GPIO1, which we check. When DE is not exposed, the internal fixup is disabled. VSYNC/GPIO2 becomes a modified signal, designed to help an external device or PIO program synthesize CSYNC or VSYNC. Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>

igorpecovnik · 2024-12-12T09:43:25Z

This PR breaks our auto-compilation:
https://github.com/armbian/os/actions/runs/12292566594/job/34305984530
https://paste.armbian.de/qiqusamowu

   ERROR: modpost: "pio_close" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   ERROR: modpost: "pio_set_error" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   ERROR: modpost: "rp1_pio_sm_unclaim" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   ERROR: modpost: "pio_open" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   ERROR: modpost: "rp1_pio_sm_claim" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   ERROR: modpost: "rp1_pio_sm_init" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   ERROR: modpost: "rp1_pio_sm_set_enabled" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   ERROR: modpost: "rp1_pio_add_program" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   ERROR: modpost: "rp1_pio_sm_exec" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   ERROR: modpost: "rp1_pio_gpio_set_function" [drivers/gpu/drm/rp1/rp1-dpi/drm-rp1-dpi.ko] undefined!
   WARNING: modpost: suppressed 3 unresolved symbol warnings because there were too many)
   make[3]: *** [scripts/Makefile.modpost:145: Module.symvers] Error 1
   make[2]: *** [Makefile:1873: modpost] Error 2
   make[1]: *** [/armbian/cache/sources/linux-kernel-worktree/6.6__bcm2711__arm64/Makefile:359: __build_one_by_one] Error 2
   make: *** [Makefile:234: __sub-make] Error 2

6by9 · 2024-12-12T09:55:41Z

drivers/gpu/drm/rp1/rp1-dpi/Kconfig needs a depends on RP1_PIO or select RP1_PIO.

CONFIG_RP1_PIO is in our standard bcm2712_defconfig, so does that mean you're using your own?

pelwell · 2024-12-12T10:01:50Z

Odd - our autobuilds are happy, as are my own builds.

Are you using a different defconfig? I notice that CONFIG_DRM_RP1_DPI is lacking a dependency on CONFIG_RP1_PIO, which could explain the build failure. However, CONFIG_PWM_PIO_RP1 is also lacking that dependency, and your build of pwm-pio-rp1 isn't failing.

njhollinghurst · 2024-12-12T10:05:41Z

I'll fix it. I'll make it an ifdef rather than a dependency as DPI should still be usable without it (apart from the interlaced sync fixup).

pelwell · 2024-12-12T10:07:39Z

I would add a select RP1_PIO in the Kconfig.

6by9 · 2024-12-12T10:10:22Z

I would add a select RP1_PIO in the Kconfig.

I'd agree.
It's possible to make things work with ifdef's, but it's non-trivial to ensure that all combinations of modules and built-ins are valid.

igorpecovnik · 2024-12-12T10:14:28Z

Are you using a different defconfig?

Yes. Still this should not break this way. I will update our configs. Thanks for the tip.

pelwell · 2024-12-12T10:16:05Z

Be careful not to introduce a recursive dependency, as I seem to have just done.

igorpecovnik · 2024-12-12T10:25:52Z

Be careful not to introduce a recursive dependency,

Yeah, I just did ;)

[🔨]   drivers/misc/Kconfig:20:error: recursive dependency detected!
[🔨]   drivers/misc/Kconfig:20:	symbol RP1_PIO is selected by PWM_PIO_RP1
[🔨]   drivers/pwm/Kconfig:468:	symbol PWM_PIO_RP1 depends on FIRMWARE_RP1
[🔨]   drivers/firmware/Kconfig:156:	symbol FIRMWARE_RP1 is selected by RP1_PIO

pelwell · 2024-12-12T10:27:06Z

That's a harsh test - it's objecting to "A selects B, B selects C, and A depends on C".

pelwell · 2024-12-12T10:30:16Z

I've unbroken the builds while we come up with a better scheme.

kernel: RP1 DPI: Add interlaced modes See: raspberrypi/linux#6529 kernel: ASoC: allo-piano-dac-plus: Fix volume limit locking See: raspberrypi/linux#6532 kernel: drm: vc4: txp: Do not allow 24bpp formats when transposing See: raspberrypi/linux#6533 kernel: ASoC: allo-piano-dac-plus: Suppress -517 errors See: raspberrypi/linux#6534 kernel: drm: rp1: rp1-dpi: Fix optional dependency on RP1_PIO See: raspberrypi/linux#6535 kernel: serial: sc16is7xx: announce support for SER_RS485_RTS_ON_SEND See: raspberrypi/linux#6540

njhollinghurst requested review from 6by9 and pelwell December 10, 2024 13:54

njhollinghurst force-pushed the dpi-interlace2 branch from 923a3f6 to 5b1b66a Compare December 10, 2024 14:13

6by9 reviewed Dec 10, 2024

View reviewed changes

njhollinghurst force-pushed the dpi-interlace2 branch from 5b1b66a to 003bfbd Compare December 10, 2024 16:36

pelwell approved these changes Dec 10, 2024

View reviewed changes

njhollinghurst force-pushed the dpi-interlace2 branch from 003bfbd to af0679f Compare December 11, 2024 15:56

njhollinghurst added 3 commits December 11, 2024 16:09

dts: overlays: vc4-kms-dpi-generic-overlay: Add "interlaced" property

2b0acbe

Almost no DPI hardware supports it, but it's useful for RP1 video out. Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>

njhollinghurst force-pushed the dpi-interlace2 branch from af0679f to 7735dd0 Compare December 11, 2024 16:09

njhollinghurst marked this pull request as ready for review December 11, 2024 16:10

pelwell merged commit 0da5bec into raspberrypi:rpi-6.6.y Dec 11, 2024

njhollinghurst deleted the dpi-interlace2 branch December 11, 2024 17:59

njhollinghurst mentioned this pull request Dec 11, 2024

piolib: examples: Overhaul dpi_interlace, now named dpi_csync raspberrypi/utils#105

Merged

njhollinghurst mentioned this pull request Dec 12, 2024

drm: rp1: rp1-dpi: Fix optional dependency on RP1_PIO #6535

Merged

Conversation

njhollinghurst commented Dec 10, 2024

Uh oh!

Uh oh!

Uh oh!

6by9 Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

6by9 Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

njhollinghurst Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

6by9 Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

njhollinghurst Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

6by9 Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

njhollinghurst Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

6by9 commented Dec 10, 2024

Uh oh!

njhollinghurst commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

njhollinghurst commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorpecovnik commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

6by9 commented Dec 12, 2024

Uh oh!

pelwell commented Dec 12, 2024

Uh oh!

njhollinghurst commented Dec 12, 2024

Uh oh!

pelwell commented Dec 12, 2024

Uh oh!

6by9 commented Dec 12, 2024

Uh oh!

igorpecovnik commented Dec 12, 2024

Uh oh!

pelwell commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorpecovnik commented Dec 12, 2024

Uh oh!

pelwell commented Dec 12, 2024

Uh oh!

pelwell commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

njhollinghurst Dec 10, 2024 •

edited

Loading

njhollinghurst Dec 10, 2024 •

edited

Loading

njhollinghurst commented Dec 10, 2024 •

edited

Loading

njhollinghurst commented Dec 11, 2024 •

edited

Loading

igorpecovnik commented Dec 12, 2024 •

edited

Loading

pelwell commented Dec 12, 2024 •

edited

Loading

pelwell commented Dec 12, 2024 •

edited

Loading