Re: [PATCH] drm/atomic-helper: Do not assume vblank is always present

2023-04-05 Thread Martin Krastev (VMware) 

From: Martin Krastev 


LGTM!

Reviewed-by: Martin Krastev 


Regards,

Martin


On 5.04.23 г. 7:56 ч., Zack Rusin wrote:

From: Zack Rusin 

Many drivers (in particular all of the virtualized drivers) do not
implement vblank handling. Assuming vblank is always present
leads to crashes.

Fix the crashes by making sure the device supports vblank before using
it.

Fixes crashes on boot, as in:
Oops:  [#1] PREEMPT SMP PTI
CPU: 0 PID: 377 Comm: systemd-modules Not tainted 6.3.0-rc4-vmwgfx #1
Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference 
Platform, BIOS 6.00 11/12/2020
RIP: 0010:drm_crtc_next_vblank_start+0x2c/0x80 [drm]
Code: 1e fa 0f 1f 44 00 00 8b 87 90 00 00 00 48 8b 17 55 48 8d 0c c0 48 89 e5 41 54 53 
48 8d 1c 48 48 c1 e3 04 48 03 9a 40 01 00 00 <8b> 53 74 85 d2 74 3f 8b 4>
RSP: 0018:b825004073c8 EFLAGS: 00010246
RAX:  RBX:  RCX: 
RDX: 9b18cf8d RSI: b825004073e8 RDI: 9b18d0f4
RBP: b825004073d8 R08: 9b18cf8d R09: 
R10: 9b18c57ef6e8 R11: 9b18c2d59e00 R12: 
R13: 9b18cfa99280 R14:  R15: 9b18cf8d
FS:  7f2b82538900() GS:9b19f7c0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0074 CR3: 0001060a6003 CR4: 003706f0
Call Trace:
  
  drm_atomic_helper_wait_for_fences+0x87/0x1e0 [drm_kms_helper]
  drm_atomic_helper_commit+0xa1/0x160 [drm_kms_helper]
  drm_atomic_commit+0x9a/0xd0 [drm]
  ? __pfx___drm_printfn_info+0x10/0x10 [drm]
  drm_client_modeset_commit_atomic+0x1e9/0x230 [drm]
  drm_client_modeset_commit_locked+0x5f/0x160 [drm]
  ? mutex_lock+0x17/0x50
  drm_client_modeset_commit+0x2b/0x50 [drm]
  __drm_fb_helper_restore_fbdev_mode_unlocked+0xca/0x100 [drm_kms_helper]
  drm_fb_helper_set_par+0x40/0x80 [drm_kms_helper]
  fbcon_init+0x2c5/0x690
  visual_init+0xee/0x190
  do_bind_con_driver.isra.0+0x1ce/0x4b0
  do_take_over_console+0x136/0x220
  ? vprintk_default+0x21/0x30
  do_fbcon_takeover+0x78/0x130
  do_fb_registered+0x139/0x270
  fbcon_fb_registered+0x3b/0x90
  ? fb_add_videomode+0x81/0xf0
  register_framebuffer+0x22f/0x330
  __drm_fb_helper_initial_config_and_unlock+0x349/0x520 [drm_kms_helper]
  ? kmalloc_large+0x25/0xc0
  drm_fb_helper_initial_config+0x3f/0x50 [drm_kms_helper]
  drm_fbdev_generic_client_hotplug+0x73/0xc0 [drm_kms_helper]
  drm_fbdev_generic_setup+0x99/0x170 [drm_kms_helper]

Signed-off-by: Zack Rusin 
Fixes: d39e48ca80c0 ("drm/atomic-helper: Set fence deadline for vblank")
Cc: Rob Clark 
Cc: Daniel Vetter 
---
  drivers/gpu/drm/drm_vblank.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 299fa2a19a90..6438aeb1c65f 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -997,12 +997,16 @@ int drm_crtc_next_vblank_start(struct drm_crtc *crtc, 
ktime_t *vblanktime)
  {
unsigned int pipe = drm_crtc_index(crtc);
struct drm_vblank_crtc *vblank = >dev->vblank[pipe];
-   struct drm_display_mode *mode = >hwmode;
+   struct drm_display_mode *mode;
u64 vblank_start;
  
+	if (!drm_dev_has_vblank(crtc->dev))

+   return -EINVAL;
+
if (!vblank->framedur_ns || !vblank->linedur_ns)
return -EINVAL;
  
+	mode = >hwmode;

if (!drm_crtc_get_last_vbltimestamp(crtc, vblanktime, false))
return -EINVAL;

Re: [PATCH] drm/atomic-helper: Do not assume vblank is always present

2023-04-05 Thread Daniel Vetter 
On Wed, Apr 05, 2023 at 09:35:45AM +0200, Javier Martinez Canillas wrote:
> 
> Hello Zack,
> 
> Thanks for your patch.
> 
> Zack Rusin  writes:
> 
> > From: Zack Rusin 
> >
> > Many drivers (in particular all of the virtualized drivers) do not
> > implement vblank handling. Assuming vblank is always present
> > leads to crashes.
> >
> > Fix the crashes by making sure the device supports vblank before using
> > it.
> >
> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 299fa2a19a90..6438aeb1c65f 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -997,12 +997,16 @@ int drm_crtc_next_vblank_start(struct drm_crtc *crtc, 
> > ktime_t *vblanktime)
> >  {
> > unsigned int pipe = drm_crtc_index(crtc);
> > struct drm_vblank_crtc *vblank = >dev->vblank[pipe];
> > -   struct drm_display_mode *mode = >hwmode;
> > +   struct drm_display_mode *mode;
> > u64 vblank_start;
> >  
> > +   if (!drm_dev_has_vblank(crtc->dev))
> > +   return -EINVAL;
> > +
> > if (!vblank->framedur_ns || !vblank->linedur_ns)
> > return -EINVAL;
> >  
> > +   mode = >hwmode;
> > if (!drm_crtc_get_last_vbltimestamp(crtc, vblanktime, false))
> > return -EINVAL;
> >  
> 
> Rob already posted more or less the same fix:
> 
> https://lore.kernel.org/lkml/caf6aegsdt95-uakcv2+hdmlkd2xwfpen+fmudtrmc-ps7wb...@mail.gmail.com/T/

Yeah I pushed it to drm-misc-next yesterday, please check that works. Also
note that we still (in some cases at least where) need another fix to
handle the crtc on/off state transitions (but only for drivers which do
have vblank), I'll send that out asap.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/atomic-helper: Do not assume vblank is always present

2023-04-05 Thread Javier Martinez Canillas 


Hello Zack,

Thanks for your patch.

Zack Rusin  writes:

> From: Zack Rusin 
>
> Many drivers (in particular all of the virtualized drivers) do not
> implement vblank handling. Assuming vblank is always present
> leads to crashes.
>
> Fix the crashes by making sure the device supports vblank before using
> it.
>

[...]

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 299fa2a19a90..6438aeb1c65f 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -997,12 +997,16 @@ int drm_crtc_next_vblank_start(struct drm_crtc *crtc, 
> ktime_t *vblanktime)
>  {
>   unsigned int pipe = drm_crtc_index(crtc);
>   struct drm_vblank_crtc *vblank = >dev->vblank[pipe];
> - struct drm_display_mode *mode = >hwmode;
> + struct drm_display_mode *mode;
>   u64 vblank_start;
>  
> + if (!drm_dev_has_vblank(crtc->dev))
> + return -EINVAL;
> +
>   if (!vblank->framedur_ns || !vblank->linedur_ns)
>   return -EINVAL;
>  
> + mode = >hwmode;
>   if (!drm_crtc_get_last_vbltimestamp(crtc, vblanktime, false))
>   return -EINVAL;
>  

Rob already posted more or less the same fix:

https://lore.kernel.org/lkml/caf6aegsdt95-uakcv2+hdmlkd2xwfpen+fmudtrmc-ps7wb...@mail.gmail.com/T/

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH] drm/atomic-helper: Do not assume vblank is always present

2023-04-04 Thread Zack Rusin 
From: Zack Rusin 

Many drivers (in particular all of the virtualized drivers) do not
implement vblank handling. Assuming vblank is always present
leads to crashes.

Fix the crashes by making sure the device supports vblank before using
it.

Fixes crashes on boot, as in:
Oops:  [#1] PREEMPT SMP PTI
CPU: 0 PID: 377 Comm: systemd-modules Not tainted 6.3.0-rc4-vmwgfx #1
Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference 
Platform, BIOS 6.00 11/12/2020
RIP: 0010:drm_crtc_next_vblank_start+0x2c/0x80 [drm]
Code: 1e fa 0f 1f 44 00 00 8b 87 90 00 00 00 48 8b 17 55 48 8d 0c c0 48 89 e5 
41 54 53 48 8d 1c 48 48 c1 e3 04 48 03 9a 40 01 00 00 <8b> 53 74 85 d2 74 3f 8b 
4>
RSP: 0018:b825004073c8 EFLAGS: 00010246
RAX:  RBX:  RCX: 
RDX: 9b18cf8d RSI: b825004073e8 RDI: 9b18d0f4
RBP: b825004073d8 R08: 9b18cf8d R09: 
R10: 9b18c57ef6e8 R11: 9b18c2d59e00 R12: 
R13: 9b18cfa99280 R14:  R15: 9b18cf8d
FS:  7f2b82538900() GS:9b19f7c0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0074 CR3: 0001060a6003 CR4: 003706f0
Call Trace:
 
 drm_atomic_helper_wait_for_fences+0x87/0x1e0 [drm_kms_helper]
 drm_atomic_helper_commit+0xa1/0x160 [drm_kms_helper]
 drm_atomic_commit+0x9a/0xd0 [drm]
 ? __pfx___drm_printfn_info+0x10/0x10 [drm]
 drm_client_modeset_commit_atomic+0x1e9/0x230 [drm]
 drm_client_modeset_commit_locked+0x5f/0x160 [drm]
 ? mutex_lock+0x17/0x50
 drm_client_modeset_commit+0x2b/0x50 [drm]
 __drm_fb_helper_restore_fbdev_mode_unlocked+0xca/0x100 [drm_kms_helper]
 drm_fb_helper_set_par+0x40/0x80 [drm_kms_helper]
 fbcon_init+0x2c5/0x690
 visual_init+0xee/0x190
 do_bind_con_driver.isra.0+0x1ce/0x4b0
 do_take_over_console+0x136/0x220
 ? vprintk_default+0x21/0x30
 do_fbcon_takeover+0x78/0x130
 do_fb_registered+0x139/0x270
 fbcon_fb_registered+0x3b/0x90
 ? fb_add_videomode+0x81/0xf0
 register_framebuffer+0x22f/0x330
 __drm_fb_helper_initial_config_and_unlock+0x349/0x520 [drm_kms_helper]
 ? kmalloc_large+0x25/0xc0
 drm_fb_helper_initial_config+0x3f/0x50 [drm_kms_helper]
 drm_fbdev_generic_client_hotplug+0x73/0xc0 [drm_kms_helper]
 drm_fbdev_generic_setup+0x99/0x170 [drm_kms_helper]

Signed-off-by: Zack Rusin 
Fixes: d39e48ca80c0 ("drm/atomic-helper: Set fence deadline for vblank")
Cc: Rob Clark 
Cc: Daniel Vetter 
---
 drivers/gpu/drm/drm_vblank.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 299fa2a19a90..6438aeb1c65f 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -997,12 +997,16 @@ int drm_crtc_next_vblank_start(struct drm_crtc *crtc, 
ktime_t *vblanktime)
 {
unsigned int pipe = drm_crtc_index(crtc);
struct drm_vblank_crtc *vblank = >dev->vblank[pipe];
-   struct drm_display_mode *mode = >hwmode;
+   struct drm_display_mode *mode;
u64 vblank_start;
 
+   if (!drm_dev_has_vblank(crtc->dev))
+   return -EINVAL;
+
if (!vblank->framedur_ns || !vblank->linedur_ns)
return -EINVAL;
 
+   mode = >hwmode;
if (!drm_crtc_get_last_vbltimestamp(crtc, vblanktime, false))
return -EINVAL;
 
-- 
2.39.2