From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH v3 1/4] drm/vc4: Wait for display list synchronization when completing commit Date: Wed, 23 Jan 2019 10:34:28 -0800 Message-ID: <87d0onussr.fsf@anholt.net> References: <20190108145056.2276-1-paul.kocialkowski@bootlin.com> <20190108145056.2276-2-paul.kocialkowski@bootlin.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0309535828==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 83E906EFFC for ; Wed, 23 Jan 2019 18:34:31 +0000 (UTC) In-Reply-To: <20190108145056.2276-2-paul.kocialkowski@bootlin.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Cc: Thomas Petazzoni , Boris Brezillon , David Airlie , Eben Upton , Paul Kocialkowski , Maxime Ripard List-Id: dri-devel@lists.freedesktop.org --===============0309535828== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Paul Kocialkowski writes: > During an atomic commit, the HVS is configured with a display list > for the channel matching the associated CRTC. The Pixel Valve (CRTC) > and encoder are also configured for the new setup at that time. > While the Pixel Valve and encoder are reconfigured synchronously, the > HVS is only reconfigured after the display list address (DISPLIST) has > been updated to the current display list address (DISPLACTX), which is > the responsibility of the hardware. > > The time frame during which the HVS is still running on its previous > configuration but the CRTC and encoder have been reconfigured already > can lead to a number of synchronization issues. They will eventually > cause errors reported on the FIFOs, such as underruns. > > With underrun detection enabled (from Boris Brezillon's series), this > leads to unreliable underrun detection with random false positives. > > To ensure a coherent state, wait for each enabled channel of the HVS > to synchronize its current display list address. This fixes the issue > of random underrun reporting on commits. > > Signed-off-by: Paul Kocialkowski > --- > drivers/gpu/drm/vc4/vc4_drv.h | 1 + > drivers/gpu/drm/vc4/vc4_hvs.c | 17 +++++++++++++++++ > drivers/gpu/drm/vc4/vc4_kms.c | 2 ++ > drivers/gpu/drm/vc4/vc4_regs.h | 2 ++ > 4 files changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index c24b078f0593..955f157f5ad0 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -772,6 +772,7 @@ void vc4_irq_reset(struct drm_device *dev); > extern struct platform_driver vc4_hvs_driver; > void vc4_hvs_dump_state(struct drm_device *dev); > int vc4_hvs_debugfs_regs(struct seq_file *m, void *unused); > +void vc4_hvs_sync_dlist(struct drm_device *dev); >=20=20 > /* vc4_kms.c */ > int vc4_kms_load(struct drm_device *dev); > diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c > index 5d8c749c9749..1ba60b8e0c2d 100644 > --- a/drivers/gpu/drm/vc4/vc4_hvs.c > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c > @@ -166,6 +166,23 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_h= vs *hvs, > return 0; > } >=20=20 > +void vc4_hvs_sync_dlist(struct drm_device *dev) > +{ > + struct vc4_dev *vc4 =3D to_vc4_dev(dev); > + unsigned int i; > + int ret; > + > + for (i =3D 0; i < SCALER_CHANNELS_COUNT; i++) { > + if (!(HVS_READ(SCALER_DISPCTRLX(i)) & SCALER_DISPCTRLX_ENABLE)) > + continue; > + > + ret =3D wait_for(HVS_READ(SCALER_DISPLACTX(i)) =3D=3D > + HVS_READ(SCALER_DISPLISTX(i)), 1000); > + WARN(ret, "Timeout waiting for channel %d display list sync\n", > + i); > + } > +} > + > static int vc4_hvs_bind(struct device *dev, struct device *master, void = *data) > { > struct platform_device *pdev =3D to_platform_device(dev); > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 0490edb192a1..2d66a2b57a91 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -155,6 +155,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *s= tate) >=20=20 > drm_atomic_helper_commit_hw_done(state); >=20=20 > + vc4_hvs_sync_dlist(dev); > + > drm_atomic_helper_wait_for_flip_done(dev, state); wait_for_flip_done should already be waiting for DISPLACTX to match our crtc's display list, though, right (see vc4_crtc_handle_page_flip())? This seems like a no-op to me. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlxIs7QACgkQtdYpNtH8 nuggiA//bWr2s7jgOPZ3iugGZA383KlyLd52nnKPkRBsRvY8w3UkDc8XAPsB22Yh uvdbSmGCOKO15y/r2MFWmjpTk66wTowJeaqnPAvU2HlmI90vOCH0kv7b8gFjzL5W l4hhj9P26HtKKGWSfzvYQhguinw3VbdFmclvfcvzg2hxAY35kz2qc5W4/edRW3KZ fts1sNipEmcUWQI+gw530sMpo6NWqr12aRIaA6pUdoDGH/+TrzGvJiZ92FFhdxk4 ROjQk4hpcPSZTCVWx/v3igra1YyyZopg3nLECMFeEBnux1FcQsBzVZokIo//WC4L MOPjIt1DEtWdONcAGtLU/Hxn62PrOSAd8ASaM9I5niJ/H4bfPGy5h2HdDKqYQTTC WX7fdINp4haTtYcfL7HlIDO9zThdSqLSSEBgUCC7V1P+lesJFC7Mg9mztiIa32bg SvYCDcgYfYLqabBN9ui/VcmfKyeFEbaaL9psdFOwnc2c+nHoLatI81uQxOJ+2PoC fdTTKg8aOOl7iGVoVSQ9Z42zA1uLyMPXWgrtvxcIu14S72oAp2BAgZOaX48elJzs 4IaefnIPlOpKtA8d8jhGiLEiYZI8BQfzpuPAC1NrXzBzjb2visUvf8c2U2WcMhVN V8ybytZt52fzxLWjEafdTqB3V7mSClIgNq/kRyyzDVaw05Tfkj8= =683S -----END PGP SIGNATURE----- --=-=-=-- --===============0309535828== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0309535828==--