From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCH] drm/tilcdc: Recover from sync lost error flood by resetting the LCDC Date: Thu, 7 Apr 2016 10:36:22 +0300 Message-ID: <57060DF6.7040706@ti.com> References: <1459969466-26489-1-git-send-email-jsarha@ti.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1231896078==" Return-path: Received: from comal.ext.ti.com (comal.ext.ti.com [198.47.26.152]) by gabe.freedesktop.org (Postfix) with ESMTPS id A23EA6E00C for ; Thu, 7 Apr 2016 07:36:30 +0000 (UTC) In-Reply-To: <1459969466-26489-1-git-send-email-jsarha@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jyri Sarha Cc: dri-devel@lists.freedesktop.org, laurent.pinchart@ideasonboard.com List-Id: dri-devel@lists.freedesktop.org --===============1231896078== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="h6t7nKcbBMfOqPnUFmoPQUNv6RCoJeXiK" --h6t7nKcbBMfOqPnUFmoPQUNv6RCoJeXiK Content-Type: multipart/mixed; boundary="Ax92Pp4mxmlDSOp8SmNCwXmlaKSfQ4Lo6" From: Tomi Valkeinen To: Jyri Sarha Cc: dri-devel@lists.freedesktop.org, airlied@linux.ie, daniel@ffwll.ch, laurent.pinchart@ideasonboard.com, robdclark@gmail.com Message-ID: <57060DF6.7040706@ti.com> Subject: Re: [PATCH] drm/tilcdc: Recover from sync lost error flood by resetting the LCDC References: <1459969466-26489-1-git-send-email-jsarha@ti.com> In-Reply-To: <1459969466-26489-1-git-send-email-jsarha@ti.com> --Ax92Pp4mxmlDSOp8SmNCwXmlaKSfQ4Lo6 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi, On 06/04/16 22:04, Jyri Sarha wrote: > Recover from sync lost error flood by resetting the LCDC in stead of "instead" > turning off the SYNC_LOST error IRQ. When LCDC starves on limited > memory bandwidth it may sometimes result an error situation when the > picture may have shifted couple of pixels to right and SYNC_LOST > interrupt is generated on every frame. LCDC main reset recovers from > this situation and causes a brief blanking on the screen. >=20 > Signed-off-by: Jyri Sarha > --- > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 34 ++++++++++++++++++++++++++++= +++--- > 1 file changed, 31 insertions(+), 3 deletions(-) >=20 > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/til= cdc/tilcdc_crtc.c > index 051e5e1..5aa86e9 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -46,6 +46,8 @@ struct tilcdc_crtc { > =20 > int sync_lost_count; > bool frame_intact; > + struct workqueue_struct *wq; > + struct work_struct recover_work; I think the system workqueue should work fine, shouldn't it? > }; > #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base) > =20 > @@ -60,6 +62,25 @@ static void unref_worker(struct drm_flip_work *work,= void *val) > mutex_unlock(&dev->mode_config.mutex); > } > =20 > +static void stop(struct drm_crtc *crtc); > +static void start(struct drm_crtc *crtc); > +static void tilcdc_crtc_recover_work(struct work_struct *work) > +{ > + struct tilcdc_crtc *tilcdc_crtc =3D > + container_of(work, struct tilcdc_crtc, recover_work); > + struct drm_crtc *crtc =3D &tilcdc_crtc->base; > + > + dev_info(crtc->dev->dev, "%s: Reset CRTC", __func__); > + > + if (tilcdc_crtc->dpms =3D=3D DRM_MODE_DPMS_OFF) > + return; The 'if' above should probably be inside the lock too. Otherwise dpms might indicate ON, but is turned OFF when inside the lock. Did you try with a big msleep here? The irqs are still enabled, so they keep getting triggered. I wonder if something bad might happen because of that. For example, set_scanout might be called from the irq handler while the code below is being ran. > + > + drm_modeset_lock_crtc(crtc, NULL); > + stop(crtc); > + start(crtc); > + drm_modeset_unlock_crtc(crtc); > +} I wonder if this works right. stop() just clears the enable bit, without waiting anything, and start() resets the LCDC. dpms() uses stop(), but after the call dpms() waits until framedone, i.e. the HW doesn't stop at stop(), but continues until the end of the frame. Now, start() does reset, so perhaps it's not a problem. But then, my experience with OMAP DSS tells me that not waiting for framedone is usually a bad thing. In fact, the TRM says "In Raster Modes, the module must be disabled before applying a software reset. When cfg_lcden is set to =E2=80=980=E2=80= =99 to disable the module, the output continues to the end of the current frame. The Done interrupt will trigger once the frame is complete. The software reset can then be applied to the module". See 13.4.6 Disable and Software Reset Sequence. > + > static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer = *fb) > { > struct tilcdc_crtc *tilcdc_crtc =3D to_tilcdc_crtc(crtc); > @@ -125,6 +146,7 @@ static void tilcdc_crtc_destroy(struct drm_crtc *cr= tc) > tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF); > =20 > of_node_put(crtc->port); > + destroy_workqueue(tilcdc_crtc->wq); Does this ensure that all the queued work has been finished? > drm_crtc_cleanup(crtc); > drm_flip_work_cleanup(&tilcdc_crtc->unref_work); > } > @@ -732,10 +754,10 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc= ) > tilcdc_crtc->frame_intact =3D false; > if (tilcdc_crtc->sync_lost_count++ > SYNC_LOST_COUNT_LIMIT) { > dev_err(dev->dev, > - "%s(0x%08x): Sync lost flood detected, disabling the interrupt", > + "%s(0x%08x): Sync lost flood detected, recovering", > __func__, stat); > - tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG, > - LCDC_SYNC_LOST); > + queue_work(tilcdc_crtc->wq, &tilcdc_crtc->recover_work); > + tilcdc_crtc->sync_lost_count =3D 0; > } > } > =20 > @@ -768,6 +790,12 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_dev= ice *dev) > "unref", unref_worker); > =20 > spin_lock_init(&tilcdc_crtc->irq_lock); > + tilcdc_crtc->wq =3D create_singlethread_workqueue("tilcdc/crtc"); > + if (!tilcdc_crtc->wq) { > + kfree(tilcdc_crtc); > + return NULL; > + } > + INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work); > =20 > ret =3D drm_crtc_init(dev, crtc, &tilcdc_crtc_funcs); > if (ret < 0) >=20 --Ax92Pp4mxmlDSOp8SmNCwXmlaKSfQ4Lo6-- --h6t7nKcbBMfOqPnUFmoPQUNv6RCoJeXiK Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXBg33AAoJEPo9qoy8lh7156UP/06heDK/EWLH556gPNI7OHQB 5sUmzn6wSSOoWl4T7uku/wzYxdJep8EiDHHv0kfjXXLGjLvgB4HXfCcNbmYLOW6Z zuS5wOgWx/NR+36TsD1/UJ4QxaomZ3xo9ukByVKCXSBa8VGSW26iOPF0lG3oT/e6 Cmb+b1R1snNSLNqQZeIaIWpXuHHxCOpndCsfuueQA911KPSt9DTy+qH+U5b/whL7 Zi1tr8CR2aW2Ggp02q65tEuKAQGyqsFSRGZM5ZYO163soVjx15mJO6zLja7EN021 +RpzxV5/3KKxAj2dvx7KXMz0CVKdhF0TuKlVn2ZpW0M6MyBUOu40QsrO7U5sWXfa iWy3EtZwzuDJdJyu6mH/+WMEeuQo8SIIGeLu8+ojRhGO6mmK+aLUQTuKCQ4ny3FQ v4qal7PfaMkoXXGpiThUq54PFmf/ofGsh0ilwWO+jE6nC5FYIoR7/iCuTG9Kifvf 9qC8j+0n0PhRtNPqX0SZbRu4EumT3hwKsTmC8P8p+yFgyNyiy+8+wf2sMQrN7bLH qgTgDfDLqFbslWYGdtmuq9PO3hQJ3hpBYhIk+45xjc9yMNOKDAjE6JeNI+nC+Fc7 wtu9z8XElSx3oCcDSRGUnogde2FRgRu1DPG2O7ruVF3zUqB5CcxcRlFWiUpL7K4G pWAG1g9O1Bd7MsLv7o4q =o2Xk -----END PGP SIGNATURE----- --h6t7nKcbBMfOqPnUFmoPQUNv6RCoJeXiK-- --===============1231896078== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1231896078==--