[PATCH 2/2] vkms: Add support for multiple connectors

Thu Apr 20 08:44:55 UTC 2023

Hi Melissa,

Added a note about that in the latest version.

Thanks!

On Mon, Apr 10, 2023 at 05:17:56PM -0100, Melissa Wen wrote:
> On 04/06, Marius Vlad wrote:
> > Hi Thomas,
> > 
> > Thanks for the clarifications! Made a couple of remarks in-line.
> > 
> > On 4/6/23 14:29, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 06.04.23 um 11:17 schrieb Marius Vlad:
> > > > Hi Maira,
> > > > 
> > > > Thanks a lot for taking a look. Yeah, indeed, this creates a new
> > > > connector, a CRTC and planes for it. Terminology wise, multiple
> > > > pipes. The idea is to have multiple outputs, each with its own
> > > > connector, as to be able to simulate (a few) more outputs. At least
> > > > that's what I'm looking for.
> > > > 
> > > > I'll adjust the commits description to clarify that.
> > > > 
> > > > Regarding the planes, it seemed a bit easier to just create a new
> > > > tuple of planes for each output, as in to reuse vkms_output_init().
> > > > So I guess that you're suggestion would be to make use the existing
> > > > planes.
> > > > 
> > > > Seems a bit more work, not that keen on changing that, but I do have
> > > > some follow-up questions for my own curiosity in case I do this:
> > > > 
> > > > -  Don't I need an entire pipe (a primary plane, crtc, encoder,
> > > > connector) to power up the end side of the sink (display)?
> > > 
> > > Yes, you need at least one full pipeline. I don't see how you'd get
> > > something displayed otherwise.
> > > 
> > > > - Can the primary one be sufficient for multiple outputs?
> > > 
> > > We have no concept of "primary pipelines." The individual components
> > > have index numbers. There's a primary plane attached to each CRTC, but
> > > even that concept comes more from HW limitations and historical designs,
> > > than fundamental requirements.
> > 
> > Right, meant the primary plane, rather than pipeline.
> > 
> > > 
> > > For multiple outputs, you can attach multiple encoders/connectors to the
> > > same CRTC. They will then all display the same screen at the same
> > > display resolution
> > Yeah, this kind of sounds like cloning to me, and would like to display
> > different things at the same time, on different outputs, to me it sounds I
> > need primary plane and a CRTC for each connector. Right?
> > 
> > > 
> > > > - Can the overlay planes be shared or I need to
> > > >    discard the ones that are already in-use by other outputs?
> > > 
> > > Even if your hardware planes support it, you cannot share planes among
> > > CRTCs with DRM. At least I'm not aware how to. Each plane struct has a
> > > designated CRTC struct. For most flexibility I'd have to match HW planes
> > > and DRM planes dynamically. For example if you have 2 CRTCs that can
> > > share 10 HW planes, you can allocate 10 DRM planes for each CRTC. The
> > > atomic_check functions have to implement the mapping from hardware to
> > > software plane and fail if no more hardware planes are available.
> > > 
> > > If you want to display the same screen on multiple CRTCs, it's possible
> > > to share the DRM framebuffers among similar the planes.
> > 
> > Aham, understood, thanks again!
> 
> Some drivers allow overlay planes to move between CRTCs. We have briefly
> discussed on IRC that it would be interesting to have this (even for all
> plane types) in vkms for testing and validation, but in a next step. As
> it's not included in this proposal here, I'd suggest you to include this
> feature/improvement in the vkms TODO in your next version, to keep it in
> our minds for future works.
> 
> BR,
> 
> Melissa
> 
> > 
> > > 
> > > Best regards
> > > Thomas
> > > 
> > > > 
> > > > I'll have a look at that writeback test as well see what's causing that
> > > > NULL pointer deref.
> > > > 
> > > > 
> > > > On 4/5/23 16:51, Maíra Canal wrote:
> > > > > Hi Marius,
> > > > > 
> > > > > > This patch adds support for creating multiple virtual connectors, in
> > > > > > case one would need it. Use module parameters to specify how many,
> > > > > > defaulting to just one, allocating from the start, a maximum of 4
> > > > > > possible outputs.
> > > > > 
> > > > > I got a bit confused by this description. The commit message
> > > > > says that you
> > > > > are creating multiple connectors, but it seems like you are
> > > > > creating multiple
> > > > > pipes. From what I could see, for each new connector created,
> > > > > you are also
> > > > > creating a new CRTC and new planes.
> > > > > 
> > > > > Moreover, if your real intention is to create multiple pipes, I
> > > > > believe that
> > > > > you don't really need to duplicate the planes as well.
> > > > > 
> > > > > I ran the VKMS CI [1] with your patches applied and, although
> > > > > most of the
> > > > > tests are now passing with multiple pipes, which is really nice,
> > > > > the test
> > > > > kms_writeback crashes with the following error:
> > > > > 
> > > > > [ 1997.244347] [IGT] kms_writeback: starting subtest
> > > > > writeback-check-output
> > > > > [ 1997.250673] BUG: kernel NULL pointer dereference, address:
> > > > > 000000000000016c
> > > > > [ 1997.250691] #PF: supervisor read access in kernel mode
> > > > > [ 1997.250693] #PF: error_code(0x0000) - not-present page
> > > > > [ 1997.250694] PGD 800000001a877067 P4D 800000001a877067 PUD
> > > > > 1a872067 PMD 0
> > > > > [ 1997.250697] Oops: 0000 [#1] PREEMPT SMP PTI
> > > > > [ 1997.250699] CPU: 2 PID: 3223 Comm: kms_writeback Not tainted
> > > > > 6.3.0-rc4-g8c2c29ba129f-dirty #257
> > > > > [ 1997.250701] Hardware name: QEMU Standard PC (i440FX + PIIX,
> > > > > 1996), BIOS 1.16.2-1.fc37 04/01/2014
> > > > > [ 1997.250703] RIP: 0010:drm_vblank_get+0xa/0xe0
> > > > > [ 1997.250710] Code: a9 66 66 66 66 66 66 2e 0f 1f 84 00 00 00
> > > > > 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 55 41 57
> > > > > 41 56 41 55 41 54 53 <8b> 87 6c 01 00 00 41 bc ea ff ff ff 85 c0
> > > > > 74 74 89 f5 48 89 fb 39
> > > > > [ 1997.250712] RSP: 0018:ffffb84d407a3b08 EFLAGS: 00010202
> > > > > [ 1997.250714] RAX: 0000000000000000 RBX: ffffa2eb02bf8b70 RCX:
> > > > > 0000000000000000
> > > > > [ 1997.250718] RDX: ffffa2eb180d2420 RSI: 0000000000000000 RDI:
> > > > > 0000000000000000
> > > > > [ 1997.250719] RBP: ffffa2eb02bf99e8 R08: 0000000000000036 R09:
> > > > > ffffa2eb01d620c0
> > > > > [ 1997.250720] R10: ffffe82b84027e40 R11: ffffffffc0042520 R12:
> > > > > ffffa2eb01c01900
> > > > > [ 1997.250721] R13: ffffa2eb02bf9b60 R14: 0000000000000001 R15:
> > > > > ffffa2ea1ecd6b80
> > > > > [ 1997.250722] FS:  00007f7d44e89a80(0000)
> > > > > GS:ffffa2eb3bd00000(0000) knlGS:0000000000000000
> > > > > [ 1997.250723] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > [ 1997.250725] CR2: 000000000000016c CR3: 000000001ec02000 CR4:
> > > > > 00000000000006e0
> > > > > [ 1997.250728] Call Trace:
> > > > > [ 1997.250735]  <TASK>
> > > > > [ 1997.250736]  vkms_set_composer+0x18/0x60 [vkms]
> > > > > [ 1997.250745]  vkms_wb_atomic_commit+0x93/0x150 [vkms]
> > > > > [ 1997.250749]  drm_atomic_helper_commit_modeset_enables+0x1d9/0x250
> > > > > [ 1997.250754]  vkms_atomic_commit_tail+0x33/0xb0 [vkms]
> > > > > [ 1997.250758]  commit_tail+0x8d/0x170
> > > > > [ 1997.250761]  drm_atomic_helper_commit+0x26b/0x280
> > > > > [ 1997.250763]  drm_atomic_commit+0x9f/0xc0
> > > > > [ 1997.250766]  ? __pfx___drm_printfn_info+0x10/0x10
> > > > > [ 1997.250769]  drm_mode_atomic_ioctl+0x34c/0x480
> > > > > [ 1997.250771]  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> > > > > [ 1997.250773]  drm_ioctl_kernel+0xd7/0x150
> > > > > [ 1997.250780]  drm_ioctl+0x31f/0x490
> > > > > [ 1997.250781]  ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
> > > > > [ 1997.250783]  __se_sys_ioctl+0x6d/0xb0
> > > > > [ 1997.250789]  do_syscall_64+0x43/0x90
> > > > > [ 1997.250795]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> > > > > 
> > > > > [1] https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/tree/master/tests/vkms_ci
> > > > > 
> > > > > Best Regards,
> > > > > - Maíra Canal
> > > > > 
> > > > > > 
> > > > > > This is of particular importance when testing out the DRM backend in
> > > > > > compositors, but also to be able to independently handle multiple
> > > > > > outputs/connectors, like setting one to off/sleep on while the others
> > > > > > are on, and combinations that arise from that.
> > > > > > 
> > > > > > Signed-off-by: Marius Vlad <marius.vlad at collabora.com>
> > > > > > ---
> > > > > >   drivers/gpu/drm/vkms/vkms_crtc.c      |  3 +--
> > > > > >   drivers/gpu/drm/vkms/vkms_drv.c       | 26 ++++++++++++++++++++++----
> > > > > >   drivers/gpu/drm/vkms/vkms_drv.h       |  8 +++++---
> > > > > >   drivers/gpu/drm/vkms/vkms_output.c    |  5 ++---
> > > > > >   drivers/gpu/drm/vkms/vkms_writeback.c | 18 ++++++++----------
> > > > > >   5 files changed, 38 insertions(+), 22 deletions(-)
> > > > > > 
> > > 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230420/1e38f67a/attachment.sig>