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

Thu Apr 6 11:29:24 UTC 2023

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.

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.

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

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230406/9ee5fb08/attachment-0001.sig>