Re: kern/56557: Boot fails with DRM assertion failure

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The following reply was made to PR kern/56557; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Andreas Gustafsson <gson%gson.org@localhost>
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/56557: Boot fails with DRM assertion failure
Date: Sun, 26 Dec 2021 16:08:11 +0000

 This is a multi-part message in MIME format.
 --=_17Wymd7lPaKOmlAiVMiz9N/EY5eA0fm1
 
 Can you try the attached patch?  If that doesn't work, can you try a
 DIAGNOSTIC/DEBUG/LOCKDEBUG kernel?
 
 --=_17Wymd7lPaKOmlAiVMiz9N/EY5eA0fm1
 Content-Type: text/plain; charset="ISO-8859-1"; name="vblank"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="vblank.patch"
 
 From 025745d6f43aff21c88bc9e3b393ce146fc9af04 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Sun, 26 Dec 2021 16:05:20 +0000
 Subject: [PATCH] drm: Fix locking around accurate vblank counts.
 
 - Make drm_crtc_accurate_vblank_count require the caller to hold the
   event lock, rather than take it internally.
 
 - Fix locking around drm_crtc_accurate_vblank_count and related
   operations in amdgpu and nouveau interrupt handlers.
 
 - Use drm_crtc_vblank_put_locked, not drm_crtc_vblank_put, when we
   already hold the event lock.
 ---
  .../dist/drm/amd/display/amdgpu_dm/amdgpu_dm.c    |  2 +-
  .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c     |  2 ++
  sys/external/bsd/drm2/dist/drm/drm_vblank.c       | 15 ++-------------
  .../drm/nouveau/dispnv50/nouveau_dispnv50_disp.c  |  2 +-
  4 files changed, 6 insertions(+), 15 deletions(-)
 
 diff --git a/sys/external/bsd/drm2/dist/drm/amd/display/amdgpu_dm/amdgpu_dm=
 .c b/sys/external/bsd/drm2/dist/drm/amd/display/amdgpu_dm/amdgpu_dm.c
 index c8cbf7a6f68e..d0386f514457 100644
 --- a/sys/external/bsd/drm2/dist/drm/amd/display/amdgpu_dm/amdgpu_dm.c
 +++ b/sys/external/bsd/drm2/dist/drm/amd/display/amdgpu_dm/amdgpu_dm.c
 @@ -361,7 +361,7 @@ static void dm_pflip_high_irq(void *interrupt_params)
  			drm_crtc_send_vblank_event(&amdgpu_crtc->base, e);
 =20
  			/* Event sent, so done with vblank for this flip */
 -			drm_crtc_vblank_put(&amdgpu_crtc->base);
 +			drm_crtc_vblank_put_locked(&amdgpu_crtc->base);
  		}
  	} else if (e) {
  		/* VRR active and inside front-porch: vblank count and
 diff --git a/sys/external/bsd/drm2/dist/drm/amd/display/amdgpu_dm/amdgpu_dm=
 _crc.c b/sys/external/bsd/drm2/dist/drm/amd/display/amdgpu_dm/amdgpu_dm_crc=
 .c
 index 79cb94ea67dc..5a154037c524 100644
 --- a/sys/external/bsd/drm2/dist/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
 +++ b/sys/external/bsd/drm2/dist/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
 @@ -319,7 +319,9 @@ void amdgpu_dm_crtc_handle_crc_irq(struct drm_crtc *crt=
 c)
  				       &crcs[0], &crcs[1], &crcs[2]))
  			return;
 =20
 +		spin_lock(&crtc->dev->event_lock);
  		drm_crtc_add_crc_entry(crtc, true,
  				       drm_crtc_accurate_vblank_count(crtc), crcs);
 +		spin_unlock(&crtc->dev->event_lock);
  	}
  }
 diff --git a/sys/external/bsd/drm2/dist/drm/drm_vblank.c b/sys/external/bsd=
 /drm2/dist/drm/drm_vblank.c
 index 9ced81bd7ed8..ec91bbe9391b 100644
 --- a/sys/external/bsd/drm2/dist/drm/drm_vblank.c
 +++ b/sys/external/bsd/drm2/dist/drm/drm_vblank.c
 @@ -337,7 +337,7 @@ static u64 drm_vblank_count(struct drm_device *dev, uns=
 igned int pipe)
   * This is mostly useful for hardware that can obtain the scanout position=
 , but
   * doesn't have a hardware frame counter.
   */
 -static u64 drm_crtc_accurate_vblank_count_locked(struct drm_crtc *crtc)
 +u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
  {
  	struct drm_device *dev =3D crtc->dev;
  	unsigned int pipe =3D drm_crtc_index(crtc);
 @@ -358,17 +358,6 @@ static u64 drm_crtc_accurate_vblank_count_locked(struc=
 t drm_crtc *crtc)
 =20
  	return vblank;
  }
 -
 -u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
 -{
 -	u64 vblank;
 -
 -	spin_lock(&crtc->dev->event_lock);
 -	vblank =3D drm_crtc_accurate_vblank_count_locked(crtc);
 -	spin_unlock(&crtc->dev->event_lock);
 -
 -	return vblank;
 -}
  EXPORT_SYMBOL(drm_crtc_accurate_vblank_count);
 =20
  static void __disable_vblank(struct drm_device *dev, unsigned int pipe)
 @@ -972,7 +961,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
  	assert_spin_locked(&dev->event_lock);
 =20
  	e->pipe =3D pipe;
 -	e->sequence =3D drm_crtc_accurate_vblank_count_locked(crtc) + 1;
 +	e->sequence =3D drm_crtc_accurate_vblank_count(crtc) + 1;
  	list_add_tail(&e->base.link, &dev->vblank_event_list);
  }
  EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
 diff --git a/sys/external/bsd/drm2/dist/drm/nouveau/dispnv50/nouveau_dispnv=
 50_disp.c b/sys/external/bsd/drm2/dist/drm/nouveau/dispnv50/nouveau_dispnv5=
 0_disp.c
 index b9423cdc84fb..f49bb2ddf56d 100644
 --- a/sys/external/bsd/drm2/dist/drm/nouveau/dispnv50/nouveau_dispnv50_disp=
 .c
 +++ b/sys/external/bsd/drm2/dist/drm/nouveau/dispnv50/nouveau_dispnv50_disp=
 .c
 @@ -2136,9 +2136,9 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state =
 *state)
  		if (new_crtc_state->event) {
  			unsigned long flags;
  			/* Get correct count/ts if racing with vblank irq */
 +			spin_lock_irqsave(&crtc->dev->event_lock, flags);
  			if (new_crtc_state->active)
  				drm_crtc_accurate_vblank_count(crtc);
 -			spin_lock_irqsave(&crtc->dev->event_lock, flags);
  			drm_crtc_send_vblank_event(crtc, new_crtc_state->event);
  			spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 =20
 
 --=_17Wymd7lPaKOmlAiVMiz9N/EY5eA0fm1--