Re: [PATCH v2 2/3] vkms: Add support for multiple pipes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Maíra,

Thanks a lot for taking a look!

On Thu, Apr 20, 2023 at 01:47:59PM -0300, Maíra Canal wrote:
> Hi Marius,
> 
> Thanks for the changing the commit message! Just a few nits:
> 
> On 4/20/23 05:41, Marius Vlad wrote:
> > This adds support for creating multiple virtual pipes, in case one would
> > need to display multiple independent things on different outputs.
> > 
> > We use a module parameter to specify how many to create,  defaulting to
> > just one with the possibility of specifying a maximum of 4 possible pipes.
> > 
> > This is of particular importance when testing out the DRM back-end in
> > compositors, but also to be able to independently set different DPMS states.
> 
> When I run the VKMS CI, I'm getting the following warning in most tests
> with max_pipes=3:
> 
> (kms_atomic:2085) igt_kms-WARNING: Output Virtual-2 could not be assigned to a pipe
> (kms_atomic:2085) igt_kms-WARNING: Output Virtual-3 could not be assigned to a pipe
Hmm, first time I'm seeing these. By VKMS CI you mean piglit with some a
list of igt tests? Noticed some warnings about the Writeback connector but
those warnings are with or without these patches.
> 
> > 
> > Signed-off-by: Marius Vlad <marius.vlad@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/vkms/vkms_crtc.c      |  3 +--
> >   drivers/gpu/drm/vkms/vkms_drv.c       | 27 ++++++++++++++++++++++-----
> >   drivers/gpu/drm/vkms/vkms_drv.h       | 11 ++++++++---
> >   drivers/gpu/drm/vkms/vkms_output.c    |  5 ++---
> >   drivers/gpu/drm/vkms/vkms_writeback.c | 24 ++++++++++++------------
> >   5 files changed, 45 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 57bbd32e9beb..0b6c40ac80b6 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -89,8 +89,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> >   {
> >   	struct drm_device *dev = crtc->dev;
> >   	unsigned int pipe = crtc->index;
> > -	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> > -	struct vkms_output *output = &vkmsdev->output;
> > +	struct vkms_output *output = drm_crtc_to_vkms_output(crtc);
> >   	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> >   	if (!READ_ONCE(vblank->enabled)) {
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 6d3a2d57d992..63091ce6735a 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -51,14 +51,19 @@ static bool enable_overlay;
> >   module_param_named(enable_overlay, enable_overlay, bool, 0444);
> >   MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
> > +static int max_pipes = 1;
> 
> I believe this should be unsigned int, otherwise if you configure
> max_pipes=-3, you will get an error like:
Yeah, probably need to add a also a minimum lower range range. Even if with an
uint8_t, it would wrap to unnecessary large values. I'd just ignore 
everything under 1.
> 
> [   31.015005] ------------[ cut here ]------------
> [   31.015011] WARNING: CPU: 0 PID: 1062 at mm/page_alloc.c:5568 __alloc_pages+0x65/0x300
> [   31.015021] Modules linked in: vkms(+) [last unloaded: vkms]
> [   31.015030] CPU: 0 PID: 1062 Comm: modprobe Not tainted 6.3.0-rc4-01502-g91c249b2b9f6-dirty #92
> [   31.015035] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014
> [   31.015038] RIP: 0010:__alloc_pages+0x65/0x300
> [   31.015043] Code: 00 f7 c7 00 20 00 00 75 0e 83 fe 0b 72 09 f6 05 16 02 c3 01 01 74 0c 83 fe 0a 76 15 31 ed e9 51 01 00 00 c6 05 01 02 c3 01 01 <0f> 0b 83 fe 0a 77 eb 23 1d f6 05 c5 01 65 48 8b 3d 26 77 fd 71 8b
> [   31.015047] RSP: 0018:ffffa95442247880 EFLAGS: 00010246
> [   31.015052] RAX: 0bb151d28a828b00 RBX: 0000000000040dc0 RCX: 0000000000000000
> [   31.015055] RDX: 0000000000000000 RSI: 000000000000001d RDI: 0000000000040dc0
> [   31.015059] RBP: 0000000000000000 R08: ffffffff8f6cbeac R09: 0000000000000010
> [   31.015061] R10: ffff9cb282c6a500 R11: 0000000400000001 R12: 0000000000000dc0
> [   31.015065] R13: ffffffffc01e3258 R14: 0000000000000000 R15: 000000000000001d
> [   31.015067] FS:  00007f95f41b6740(0000) GS:ffff9cb2bbc00000(0000) knlGS:0000000000000000
> [   31.015071] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   31.015075] CR2: 00007ffc3933cec8 CR3: 000000011c110000 CR4: 00000000000006f0
> [   31.015082] Call Trace:
> [   31.015102]  <TASK>
> [   31.015106]  __kmalloc_large_node+0x82/0x120
> [   31.015113]  ? drm_vblank_init+0x30/0x100
> [   31.015120]  __kmalloc_node_track_caller+0xc4/0x180
> [   31.015125]  ? __kmalloc_node_track_caller+0x8f/0x180
> [   31.015128]  ? preempt_count_add+0x5e/0xa0
> [   31.015134]  drmm_kmalloc+0x31/0x110
> [   31.015140]  drm_vblank_init+0x30/0x100
> [   31.015145]  init_module+0x179/0xff0 [vkms]
> [   31.015160]  ? __pfx_init_module+0x10/0x10 [vkms]
> [   31.015171]  do_one_initcall+0x10c/0x2d0
> [   31.015180]  ? do_init_module+0x1d/0x1c0
> [   31.015187]  do_init_module+0x45/0x1c0
> [   31.015192]  load_module+0x16eb/0x1970
> [   31.015198]  ? kernel_read_file_from_fd+0x5a/0x90
> [   31.015205]  __se_sys_finit_module+0xb3/0xf0
> [   31.015212]  do_syscall_64+0x43/0x90
> [   31.015218]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [   31.015224] RIP: 0033:0x7f95f42bf6bd
> [   31.015228] Code: 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 43 d7 0c 00 f7 d8 64 89 01 48
> [   31.015232] RSP: 002b:00007ffc3933ef08 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [   31.015236] RAX: ffffffffffffffda RBX: 000055ec9b730a10 RCX: 00007f95f42bf6bd
> [   31.015239] RDX: 0000000000000000 RSI: 000055ec9b730470 RDI: 0000000000000003
> [   31.015241] RBP: 000055ec9b730470 R08: 0000000000000000 R09: 00007ffc3933f030
> [   31.015243] R10: 0000000000000003 R11: 0000000000000246 R12: 0000000000040000
> [   31.015245] R13: 000055ec9b730ab0 R14: 0000000000000000 R15: 000055ec9b730c70
> [   31.015248]  </TASK>
> [   31.015250] ---[ end trace 0000000000000000 ]---
> [   31.015253] [drm:init_module [vkms]] *ERROR* Failed to vblank
> 
> Also, when I setted max_pipes=4, I'm getting the following error:
I'll try to replicate these with igt tests, as those seems to stress
things a bit. Seems a bit odd to happen only at four.
> 
> [   74.804396] ------------[ cut here ]------------
> [   74.804402] WARNING: CPU: 2 PID: 1587 at drivers/gpu/drm/drm_plane.c:252 __drm_universal_plane_init+0x4fe/0x550
> [   74.804415] Modules linked in: vkms(+)
> [   74.804421] CPU: 2 PID: 1587 Comm: modprobe Tainted: G        W          6.3.0-rc4-01502-g91c249b2b9f6-dirty #92
> [   74.804426] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014
> [   74.804429] RIP: 0010:__drm_universal_plane_init+0x4fe/0x550
> [   74.804434] Code: c4 20 5b 41 5c 41 5d 41 5e 41 5f 5d c3 cc cc cc cc 49 8b b6 90 05 00 00 48 8b 44 24 08 8b 10 48 8b 3c 24 e8 84 b8 ff ff eb d1 <0f> 0b 41 bc ea ff ff ff eb c7 0f 0b 41 bc ea ff ff ff eb bd 49 8b
> [   74.804438] RSP: 0018:ffffb2cc8217f858 EFLAGS: 00010246
> [   74.804453] RAX: 0000000000000000 RBX: ffffffffc0223318 RCX: ffffffffc0223318
> [   74.804456] RDX: 0000000000000008 RSI: ffffa24080913028 RDI: ffffa24098d20000
> [   74.804458] RBP: ffffb2cc8217f970 R08: ffffffffc0223300 R09: 0000000000000005
> [   74.804461] R10: ffffb2cc8217f910 R11: 0000000000000000 R12: 0000000000000008
> [   74.804463] R13: ffffa24080913028 R14: ffffa24080913028 R15: ffffa24098d20000
> [   74.804465] FS:  00007fbd9fc9b740(0000) GS:ffffa240bbd00000(0000) knlGS:0000000000000000
> [   74.804469] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   74.804471] CR2: 00007f74a59d1f30 CR3: 0000000103304000 CR4: 00000000000006e0
> [   74.804479] Call Trace:
> [   74.804483]  <TASK>
> [   74.804485]  ? preempt_count_add+0x5e/0xa0
> [   74.804492]  __drmm_universal_plane_alloc+0xce/0x140
> [   74.804497]  ? __drm_connector_init+0x3d6/0x4c0
> [   74.804502]  vkms_plane_init+0x4d/0x70 [vkms]
> [   74.804516]  vkms_output_init+0xa7/0x380 [vkms]
> [   74.804527]  init_module+0x25d/0xff0 [vkms]
> [   74.804538]  ? __pfx_init_module+0x10/0x10 [vkms]
> [   74.804548]  do_one_initcall+0x10c/0x2d0
> [   74.804557]  ? do_init_module+0x1d/0x1c0
> [   74.804564]  do_init_module+0x45/0x1c0
> [   74.804569]  load_module+0x16eb/0x1970
> [   74.804575]  ? kernel_read_file_from_fd+0x5a/0x90
> [   74.804615]  __se_sys_finit_module+0xb3/0xf0
> [   74.804624]  do_syscall_64+0x43/0x90
> [   74.804632]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [   74.804640] RIP: 0033:0x7fbd9fda46bd
> [   74.804645] Code: 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 43 d7 0c 00 f7 d8 64 89 01 48
> [   74.804649] RSP: 002b:00007ffd8cdc6d58 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [   74.804655] RAX: ffffffffffffffda RBX: 00005614c1f3ea10 RCX: 00007fbd9fda46bd
> [   74.804659] RDX: 0000000000000000 RSI: 00005614c1f3e470 RDI: 0000000000000003
> [   74.804662] RBP: 00005614c1f3e470 R08: 0000000000000000 R09: 00007ffd8cdc6e80
> [   74.804665] R10: 0000000000000003 R11: 0000000000000246 R12: 0000000000040000
> [   74.804668] R13: 00005614c1f3eab0 R14: 0000000000000000 R15: 00005614c1f3ec70
> [   74.804673]  </TASK>
> [   74.804675] ---[ end trace 0000000000000000 ]---
> [   74.804736] BUG: kernel NULL pointer dereference, address: 00000000000000a8
> [   74.806126] #PF: supervisor read access in kernel mode
> [   74.806590] #PF: error_code(0x0000) - not-present page
> [   74.807160] PGD 0 P4D 0
> [   74.807463] Oops: 0000 [#1] PREEMPT SMP PTI
> [   74.807874] CPU: 2 PID: 1587 Comm: modprobe Tainted: G        W          6.3.0-rc4-01502-g91c249b2b9f6-dirty #92
> [   74.808597] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014
> [   74.809097] RIP: 0010:destroy_workqueue+0xf/0x2f0
> [   74.809397] Code: 41 5e 41 5f 5d e9 b1 24 ff 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 55 41 57 41 56 41 54 53 48 83 ec 10 49 89 ff <48> 8b bf a8 00 00 00 48 85 ff 74 14 49 c7 87 a8 00 00 00 00 00 00
> [   74.810308] RSP: 0018:ffffb2cc8217f928 EFLAGS: 00010286
> [   74.810637] RAX: 0000000000000004 RBX: 0000000000000003 RCX: 0000000000000001
> [   74.811041] RDX: 0000000000000002 RSI: 0000000000000082 RDI: 0000000000000000
> [   74.811436] RBP: ffffa23f9df001c0 R08: 0000000000b90826 R09: 000003fffffffc00
> [   74.811857] R10: 000000000000015d R11: 0000000000000000 R12: 0000000000000282
> [   74.812256] R13: 0000000000000001 R14: ffffa24098d20000 R15: 0000000000000000
> [   74.812673] FS:  00007fbd9fc9b740(0000) GS:ffffa240bbd00000(0000) knlGS:0000000000000000
> [   74.813294] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   74.814296] CR2: 00000000000000a8 CR3: 0000000103304000 CR4: 00000000000006e0
> [   74.814824] Call Trace:
> [   74.815114]  <TASK>
> [   74.815388]  ? _raw_spin_unlock_irq+0xe/0x30
> [   74.815806]  vkms_release+0x26/0x50 [vkms]
> [   74.816187]  devm_drm_dev_init_release+0x2d/0x60
> [   74.816590]  devres_release_group+0xeb/0x140
> [   74.817005]  init_module+0x196/0xff0 [vkms]
> [   74.817394]  ? __pfx_init_module+0x10/0x10 [vkms]
> [   74.817833]  do_one_initcall+0x10c/0x2d0
> [   74.818208]  ? do_init_module+0x1d/0x1c0
> [   74.818587]  do_init_module+0x45/0x1c0
> [   74.818869]  load_module+0x16eb/0x1970
> [   74.819147]  ? kernel_read_file_from_fd+0x5a/0x90
> [   74.819458]  __se_sys_finit_module+0xb3/0xf0
> [   74.819786]  do_syscall_64+0x43/0x90
> [   74.820041]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [   74.821852] RIP: 0033:0x7fbd9fda46bd
> [   74.822118] Code: 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 43 d7 0c 00 f7 d8 64 89 01 48
> [   74.822902] RSP: 002b:00007ffd8cdc6d58 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [   74.823311] RAX: ffffffffffffffda RBX: 00005614c1f3ea10 RCX: 00007fbd9fda46bd
> [   74.823648] RDX: 0000000000000000 RSI: 00005614c1f3e470 RDI: 0000000000000003
> [   74.823977] RBP: 00005614c1f3e470 R08: 0000000000000000 R09: 00007ffd8cdc6e80
> [   74.824297] R10: 0000000000000003 R11: 0000000000000246 R12: 0000000000040000
> [   74.824612] R13: 00005614c1f3eab0 R14: 0000000000000000 R15: 00005614c1f3ec70
> [   74.824931]  </TASK>
> [   74.825080] Modules linked in: vkms(+)
> [   74.825269] CR2: 00000000000000a8
> [   74.825505] ---[ end trace 0000000000000000 ]---
> [   74.825735] RIP: 0010:destroy_workqueue+0xf/0x2f0
> [   74.825948] Code: 41 5e 41 5f 5d e9 b1 24 ff 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 55 41 57 41 56 41 54 53 48 83 ec 10 49 89 ff <48> 8b bf a8 00 00 00 48 85 ff 74 14 49 c7 87 a8 00 00 00 00 00 00
> [   74.826609] RSP: 0018:ffffb2cc8217f928 EFLAGS: 00010286
> [   74.826843] RAX: 0000000000000004 RBX: 0000000000000003 RCX: 0000000000000001
> [   74.827114] RDX: 0000000000000002 RSI: 0000000000000082 RDI: 0000000000000000
> [   74.827382] RBP: ffffa23f9df001c0 R08: 0000000000b90826 R09: 000003fffffffc00
> [   74.827622] R10: 000000000000015d R11: 0000000000000000 R12: 0000000000000282
> [   74.827818] R13: 0000000000000001 R14: ffffa24098d20000 R15: 0000000000000000
> [   74.828008] FS:  00007fbd9fc9b740(0000) GS:ffffa240bbd00000(0000) knlGS:0000000000000000
> [   74.828245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   74.828415] CR2: 00000000000000a8 CR3: 0000000103304000 CR4: 00000000000006e0
> 
> > +module_param_named(max_pipes, max_pipes, int, 0444);
> > +MODULE_PARM_DESC(max_pipes, "Specify how many pipes to create");
> > +
> >   DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
> >   static void vkms_release(struct drm_device *dev)
> >   {
> > -	struct vkms_device *vkms = drm_device_to_vkms_device(dev);
> > +	int i;
> > +	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> > -	if (vkms->output.composer_workq)
> > -		destroy_workqueue(vkms->output.composer_workq);
> > +	for (i = 0; i < vkmsdev->config->max_pipes; i++)
> > +		destroy_workqueue(vkmsdev->output[i].composer_workq);
> >   }
> >   static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> > @@ -99,6 +104,7 @@ static int vkms_config_show(struct seq_file *m, void *data)
> >   	seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
> >   	seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
> >   	seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
> > +	seq_printf(m, "pipes=%d\n", vkmsdev->config->max_pipes);
> >   	return 0;
> >   }
> > @@ -133,6 +139,7 @@ static const struct drm_mode_config_helper_funcs vkms_mode_config_helpers = {
> >   static int vkms_modeset_init(struct vkms_device *vkmsdev)
> >   {
> >   	struct drm_device *dev = &vkmsdev->drm;
> > +	int i, ret = 0;
> >   	drm_mode_config_init(dev);
> 
> I commited a patch a while ago that changed this bit. Therefore, your patch
> isn't applicable in the drm-misc-next. It would be nice if you could rebase
> the patches on top of the drm-misc-next.
Okay, will do. 
> 
> Best Regards,
> - Maíra Canal

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux