Skip to content
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.

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

VGA666 DPI adapter #36

Open
sigmaris opened this issue Jun 5, 2016 · 9 comments
Open

VGA666 DPI adapter #36

sigmaris opened this issue Jun 5, 2016 · 9 comments

Comments

@sigmaris
Copy link

sigmaris commented Jun 5, 2016

Thanks to your recent work on support for the DPI interface, I've been able to use the VGA666 adapter with the open-source vc4 driver. It's a very simplified D/A converter which turns the DPI signal into VGA-like analog video. I'm using it with an arcade CRT monitor that's rather different from the normal VGA monitor - it only supports video modes with 15 or 25kHz horizontal scan rate, i.e. 240p/384p/480i/768i. I had to make what feels like a bit of a hack to get this working and I'm really looking for pointers on how to do it "properly".

There is already a device tree overlay for the VGA666 adapter but I found it needed some additions to enable the vc4 driver to work with it: raspberrypi/linux@rpi-4.4.y...sigmaris:vga666-dpi
I also added a panel_desc into the kernel code with some supported video modes for my particular monitor. Now I can switch between custom modes on the monitor without a reboot, which I couldn't do with the closed-source VC4 firmware.

But having to add custom modes to the kernel source and recompile seems awkward, since all kinds of monitors could be connected to the VGA666 adapter but my changes are very specific to the non-standard arcade monitor I'm using. Is there a better way to tell the driver what modes are supported without recompiling (device tree parameters?) or a way to set up custom modes after boot (i.e. set the pixel clock, h-sync and v-sync timings, etc)?

@anholt
Copy link
Owner

anholt commented Jun 6, 2016

I'm glad the DPI stuff is working for you!

If you're using X, you can add modes at runtime with xrandr --newmode, xrandr --addmode, xrandr --addmode, xrandr --mode (yes, this is an overly complicated workflow, but custom modelines are a very unusual use case these days so nobody's made a nicer tool afaik), and that sits on top of standard DRM KMS functionality that any other compositor could use as well.

If you had a standard monitor, we could upstream that, but it doesn't seem like your monitor is normal enough for that to make sense?

@sigmaris
Copy link
Author

Thanks for the tip about xrandr, that's working for me to add custom modes in X.

I also tested a different patch with a more "standard" VGA monitor that supports up to 1280x1024: sigmaris@0fdbb5c. If I define a panel_desc in panel-simple.c with no modes, then drm_add_modes_noedid() will populate some standard VESA modes from 640x480 to 1024x768. This seems like a good approach to ensure that the VGA666 adapter works on the Pi at a basic level if just dtoverlay=vga666 is added to config.txt. Beyond that, I can select 1280x1024 using a cmdline argument video=DPI-1:1280x1024M-32@60. I expect it should be possible to customise this further by using drm_kms_helper.edid_firmware= to force an EDID. The VGA666 adapter doesn't have any connection for the DDC pins so I don't think there's any way to read the monitor EDID.

I did try omitting the modifications to panel-simple.c completely, and just relying on the Device Tree overlay with compatible = "simple-panel", but that didn't seem to work, and there was this error in the kernel log:

[drm:vc4_dpi_encoder_enable [vc4]] *ERROR* Panel failed to prepare

so I guess the panel_desc is needed. If this is something that'd be suitable / worthwhile to upstream, let me know and I'll prepare a patch.

@anholt
Copy link
Owner

anholt commented Jul 5, 2016

Yeah, just "simple-panel" won't do anything, as nothing matches that. I think you need panel-simple.c changes, because even with specifying a mode on the command line, you still need the .bus_format, .bpc, and .size fields of panel_desc.

@anholt
Copy link
Owner

anholt commented Oct 6, 2016

http://www.spinics.net/lists/devicetree/msg145160.html may be an interesting route to go for supporting this.

@sigmaris
Copy link
Author

I wanted to try the approach in your last comment, basing it on your drm-panel-bridge branch since I think that branch allows the DPI to drive bridges as well as panels? tbh I don't know a lot about kernel development or DT so I'm probably doing various things wrong.

I've added a bridge node and VGA connector here - it's only added to bcm283x.dtsi as a hack, eventually this would be in the vga666 DT Overlay. So now I am wondering where is the right place to attach the bus format information, in DT or code? vc4_dpi_encoder_enable() currently tries to look at dpi->connector->display_info but dpi->connector is NULL, so I guess I need to change this (hardcoded it to DPI_FORMAT_18BIT_666_RGB_1 for now)

@anholt
Copy link
Owner

anholt commented Jun 12, 2017

The dumb-vga-dac assumes that you have DDC for the modes, which vga666 won't get you. DRM also doesn't currently have a way to tell you the bus_format of the next link in the output chain, just the last, so you'll have issues getting it with a bridge setup until we fix that.

I still think you'd be best off with panel-simple changes.

anholt pushed a commit that referenced this issue Jul 19, 2017
Correct these checkpatch.pl warnings:

|WARNING: Block comments use * on subsequent lines
|#34: FILE: include/linux/bug.h:34:
|+/* Force a compilation error if condition is true, but also produce a
|+   result (of value 0 and type size_t), so the expression can be used

|WARNING: Block comments use a trailing */ on a separate line
|#36: FILE: include/linux/bug.h:36:
|+   aren't permitted). */

Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ian Abbott <[email protected]>
Acked-by: Michal Nazarewicz <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
@grigorig
Copy link

grigorig commented Feb 3, 2018

Is there any progress with this? What's needed with a current upstream kernel to make VGA666 work, ideally without changes to the kernel? Note that I'm actually using my own "VGA555" adapter which uses less bits to free up some I/O.

@sigmaris
Copy link
Author

I had another look at porting my changes to newer kernels but still run into a NULL pointer dereference here:

if (dpi->connector->display_info.num_bus_formats) {

because dpi->connector is NULL. In fact I can't see where it is initialised anywhere; looks like the code to initialise it was removed in 7b1298e, so I think this is a bug and no DPI displays (panel or VGA DAC) will work until it is fixed.

anholt pushed a commit that referenced this issue Mar 1, 2018
commit a6da002 upstream.

We need to ensure that tracepoints are registered and unregistered
with the users of them. The existing atomic count isn't enough for
that. Add a lock around the tracepoints, so we serialize access
to them.

This fixes cases where we have multiple users setting up and
tearing down tracepoints, like this:

CPU: 0 PID: 2995 Comm: syzkaller857118 Not tainted
4.14.0-rc5-next-20171018+ #36
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:16 [inline]
  dump_stack+0x194/0x257 lib/dump_stack.c:52
  panic+0x1e4/0x41c kernel/panic.c:183
  __warn+0x1c4/0x1e0 kernel/panic.c:546
  report_bug+0x211/0x2d0 lib/bug.c:183
  fixup_bug+0x40/0x90 arch/x86/kernel/traps.c:177
  do_trap_no_signal arch/x86/kernel/traps.c:211 [inline]
  do_trap+0x260/0x390 arch/x86/kernel/traps.c:260
  do_error_trap+0x120/0x390 arch/x86/kernel/traps.c:297
  do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:310
  invalid_op+0x18/0x20 arch/x86/entry/entry_64.S:905
RIP: 0010:tracepoint_add_func kernel/tracepoint.c:210 [inline]
RIP: 0010:tracepoint_probe_register_prio+0x397/0x9a0 kernel/tracepoint.c:283
RSP: 0018:ffff8801d1d1f6c0 EFLAGS: 00010293
RAX: ffff8801d22e8540 RBX: 00000000ffffffef RCX: ffffffff81710f07
RDX: 0000000000000000 RSI: ffffffff85b679c0 RDI: ffff8801d5f19818
RBP: ffff8801d1d1f7c8 R08: ffffffff81710c10 R09: 0000000000000004
R10: ffff8801d1d1f6b0 R11: 0000000000000003 R12: ffffffff817597f0
R13: 0000000000000000 R14: 00000000ffffffff R15: ffff8801d1d1f7a0
  tracepoint_probe_register+0x2a/0x40 kernel/tracepoint.c:304
  register_trace_block_rq_insert include/trace/events/block.h:191 [inline]
  blk_register_tracepoints+0x1e/0x2f0 kernel/trace/blktrace.c:1043
  do_blk_trace_setup+0xa10/0xcf0 kernel/trace/blktrace.c:542
  blk_trace_setup+0xbd/0x180 kernel/trace/blktrace.c:564
  sg_ioctl+0xc71/0x2d90 drivers/scsi/sg.c:1089
  vfs_ioctl fs/ioctl.c:45 [inline]
  do_vfs_ioctl+0x1b1/0x1520 fs/ioctl.c:685
  SYSC_ioctl fs/ioctl.c:700 [inline]
  SyS_ioctl+0x8f/0xc0 fs/ioctl.c:691
  entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x444339
RSP: 002b:00007ffe05bb5b18 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00000000006d66c0 RCX: 0000000000444339
RDX: 000000002084cf90 RSI: 00000000c0481273 RDI: 0000000000000009
RBP: 0000000000000082 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000206 R12: ffffffffffffffff
R13: 00000000c0481273 R14: 0000000000000000 R15: 0000000000000000

since we can now run these in parallel. Ensure that the exported helpers
for doing this are grabbing the queue trace mutex.

Reported-by: Steven Rostedt <[email protected]>
Tested-by: Dmitry Vyukov <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
@anholt
Copy link
Owner

anholt commented Mar 10, 2018

stschake pushed a commit to stschake/linux that referenced this issue Mar 16, 2018
l2tp_tunnel_get walks the tunnel list to find a matching tunnel
instance and if a match is found, its refcount is increased before
returning the tunnel pointer. But when tunnel objects are destroyed,
they are on the tunnel list after their refcount hits zero. Fix this
by moving the code that removes the tunnel from the tunnel list from
the tunnel socket destructor into in the l2tp_tunnel_delete path,
before the tunnel refcount is decremented.

refcount_t: increment on 0; use-after-free.
WARNING: CPU: 3 PID: 13507 at lib/refcount.c:153 refcount_inc+0x47/0x50
Modules linked in:
CPU: 3 PID: 13507 Comm: syzbot_6e6a5ec8 Not tainted 4.16.0-rc2+ anholt#36
Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
RIP: 0010:refcount_inc+0x47/0x50
RSP: 0018:ffff8800136ffb20 EFLAGS: 00010286
RAX: dffffc0000000008 RBX: ffff880017068e68 RCX: ffffffff814d3333
RDX: 0000000000000000 RSI: ffff88001a59f6d8 RDI: ffff88001a59f6d8
RBP: ffff8800136ffb28 R08: 0000000000000000 R09: 0000000000000000
R10: ffff8800136ffab0 R11: 0000000000000000 R12: ffff880017068e50
R13: 0000000000000000 R14: ffff8800174da800 R15: 0000000000000004
FS:  00007f403ab1e700(0000) GS:ffff88001a580000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000205fafd2 CR3: 0000000016770000 CR4: 00000000000006e0
Call Trace:
 l2tp_tunnel_get+0x2dd/0x4e0
 pppol2tp_connect+0x428/0x13c0
 ? pppol2tp_session_create+0x170/0x170
 ? __might_fault+0x115/0x1d0
 ? lock_downgrade+0x860/0x860
 ? __might_fault+0xe5/0x1d0
 ? security_socket_connect+0x8e/0xc0
 SYSC_connect+0x1b6/0x310
 ? SYSC_bind+0x280/0x280
 ? __do_page_fault+0x5d1/0xca0
 ? up_read+0x1f/0x40
 ? __do_page_fault+0x3c8/0xca0
 SyS_connect+0x29/0x30
 ? SyS_accept+0x40/0x40
 do_syscall_64+0x1e0/0x730
 ? trace_hardirqs_off_thunk+0x1a/0x1c
 entry_SYSCALL_64_after_hwframe+0x42/0xb7
RIP: 0033:0x7f403a42f259
RSP: 002b:00007f403ab1dee8 EFLAGS: 00000296 ORIG_RAX: 000000000000002a
RAX: ffffffffffffffda RBX: 00000000205fafe4 RCX: 00007f403a42f259
RDX: 000000000000002e RSI: 00000000205fafd2 RDI: 0000000000000004
RBP: 00007f403ab1df20 R08: 00007f403ab1e700 R09: 0000000000000000
R10: 00007f403ab1e700 R11: 0000000000000296 R12: 0000000000000000
R13: 00007ffc81906cbf R14: 0000000000000000 R15: 00007f403ab2b040
Code: 3b ff 5b 5d c3 e8 ca 5f 3b ff 80 3d 49 8e 66 04 00 75 ea e8 bc 5f 3b ff 48 c7 c7 60 69 64 85 c6 05 34 8e 66 04 01 e8 59 49 15 ff <0f> 0b eb ce 0f 1f 44 00 00 55 48 89 e5 41 56 41 55 41 54 53 49

Fixes: f8ccac0 ("l2tp: put tunnel socket release on a workqueue")
Reported-and-tested-by: [email protected]
Reported-and-tested-by: [email protected]
Reported-and-tested-by: [email protected]
Reported-and-tested-by: [email protected]
Signed-off-by: James Chapman <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants