Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753907AbbKLQMh (ORCPT ); Thu, 12 Nov 2015 11:12:37 -0500 Received: from mail-wm0-f46.google.com ([74.125.82.46]:35112 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751035AbbKLQMf (ORCPT ); Thu, 12 Nov 2015 11:12:35 -0500 Date: Thu, 12 Nov 2015 17:12:33 +0100 From: Thierry Reding To: Liviu Dudau Cc: Mark yao , DRI devel , lkml Subject: Re: drm: Bogus WARN() in drm_atomic_helper_update_legacy_modeset_state() ? Message-ID: <20151112161233.GA3913@ulmo> References: <20151110150102.GP963@e106497-lin.cambridge.arm.com> <56444EA1.3050506@rock-chips.com> <20151112103618.GY963@e106497-lin.cambridge.arm.com> <56446EC2.6020301@rock-chips.com> <20151112133411.GA645@ulmo> <20151112140335.GC963@e106497-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="M9NhX3UHpAaciwkO" Content-Disposition: inline In-Reply-To: <20151112140335.GC963@e106497-lin.cambridge.arm.com> User-Agent: Mutt/1.5.23+102 (2ca89bed6448) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10289 Lines: 238 --M9NhX3UHpAaciwkO Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 12, 2015 at 02:03:35PM +0000, Liviu Dudau wrote: > On Thu, Nov 12, 2015 at 02:34:11PM +0100, Thierry Reding wrote: > > On Thu, Nov 12, 2015 at 06:49:38PM +0800, Mark yao wrote: > > > On 2015=E5=B9=B411=E6=9C=8812=E6=97=A5 18:36, Liviu Dudau wrote: > > > >On Thu, Nov 12, 2015 at 04:32:33PM +0800, Mark yao wrote: > > > >> On 2015=E5=B9=B411=E6=9C=8810=E6=97=A5 23:01, Liviu Dudau wrote: > > > >> > > > >> Hello, > > > >> > > > >> When booting my Juno board with the HDLCD driver that I have conv= erted to > > > >> atomic operations I'm getting the following warning: > > > >> > > > >> ------------[ cut here ]------------ > > > >> WARNING: at /work/repositories/kernel/drivers/gpu/drm/drm_atomic_= helper.c:674 > > > >> Modules linked in: hdlcd(+) clk_scpi > > > >> > > > >> CPU: 3 PID: 1375 Comm: systemd-udevd Not tainted 4.3.0-next-20151= 109+ #5 > > > >> Hardware name: ARM Juno development board (r0) (DT) > > > >> task: ffffffc974888b00 ti: ffffffc9755dc000 task.ti: ffffffc9755d= c000 > > > >> PC is at drm_atomic_helper_update_legacy_modeset_state+0x204/0x20c > > > >> LR is at drm_atomic_helper_commit_modeset_disables+0x1c0/0x394 > > > >> pc : [] lr : [] pstate: 20000= 145 > > > >> sp : ffffffc9755df430 > > > >> x29: ffffffc9755df430 x28: ffffffc975703600 > > > >> x27: 0000000000000000 x26: ffffffc976253960 > > > >> x25: ffffffc976254040 x24: ffffffc000819000 > > > >> x23: ffffffc000689ea0 x22: ffffffc976251800 > > > >> x21: ffffffc976251800 x20: 0000000000000000 > > > >> x19: ffffffc9766b1f80 x18: 00000000715fe015 > > > >> x17: 0000007fb4b855b0 x16: 0000000000000220 > > > >> x15: 0000000000000001 x14: 0ffffffffffffffe > > > >> x13: 0000000000000008 x12: 0101010101010101 > > > >> x11: ffffffc000964000 x10: ffffffc0009d2000 > > > >> x9 : 0000000000000000 x8 : ffffffc97ff5f700 > > > >> x7 : ffffffc97566cb80 x6 : ffffffc9766b1700 > > > >> x5 : ffffffc975665100 x4 : 0000000000000000 > > > >> x3 : ffffffc976253960 x2 : ffffffc97566cd00 > > > >> x1 : ffffffc976253900 x0 : 0000000000000000 > > > >> > > > >> ---[ end trace 9fe289f798e7178e ]--- > > > >> Call trace: > > > >> [] drm_atomic_helper_update_legacy_modeset_stat= e+0x204/0x20c > > > >> [] drm_atomic_helper_commit_modeset_disables+0x= 1c0/0x394 > > > >> [] drm_atomic_helper_commit+0xe0/0x150 > > > >> [] drm_atomic_commit+0x40/0x6c > > > >> [] restore_fbdev_mode+0x294/0x2d4 > > > >> [] drm_fb_helper_restore_fbdev_mode_unlocked+0x= 34/0x8c > > > >> [] drm_fb_helper_set_par+0x2c/0x58 > > > >> [] fbcon_init+0x4d4/0x534 > > > >> [] visual_init+0xac/0x104 > > > >> [] do_bind_con_driver+0x16c/0x398 > > > >> [] do_take_over_console+0xd8/0x1f4 > > > >> [] do_fbcon_takeover+0x74/0xf8 > > > >> [] fbcon_event_notify+0x8a4/0x8f8 > > > >> [] notifier_call_chain+0x4c/0x88 > > > >> [] __blocking_notifier_call_chain+0x44/0x74 > > > >> [] blocking_notifier_call_chain+0x14/0x1c > > > >> [] fb_notifier_call_chain+0x1c/0x24 > > > >> [] register_framebuffer+0x1c0/0x2ac > > > >> [] drm_fb_helper_initial_config+0x25c/0x3ec > > > >> [] drm_fbdev_cma_init+0x98/0x134 > > > >> [] hdlcd_drm_bind+0x180/0x498 [hdlcd] > > > >> [] try_to_bring_up_master.part.5+0xd4/0x118 > > > >> [] component_master_add_with_match+0xc4/0x148 > > > >> [] component_master_add+0x10/0x18 > > > >> [] hdlcd_probe+0x14/0x28 [hdlcd] > > > >> [] platform_drv_probe+0x54/0xc0 > > > >> [] driver_probe_device+0x1ec/0x2e8 > > > >> [] __driver_attach+0x9c/0xa0 > > > >> [] bus_for_each_dev+0x58/0x98 > > > >> [] driver_attach+0x20/0x28 > > > >> [] bus_add_driver+0x1c8/0x22c > > > >> [] driver_register+0x68/0x108 > > > >> [] __platform_driver_register+0x4c/0x54 > > > >> [] hdlcd_init+0x18/0x30 [hdlcd] > > > >> [] do_one_initcall+0x90/0x1a8 > > > >> [] do_init_module+0x60/0x1c8 > > > >> [] load_module+0x1554/0x1c98 > > > >> [] SyS_finit_module+0x7c/0x88 > > > >> [] el0_svc_naked+0x24/0x28 > > > >> > > > >> > > > >> The line that triggers the warning is: > > > >> > > > >> 674: WARN_ON(!connector->encoder->crtc); > > > >> > > > >> As far as I can see the encoder->crtc value is being set to a non= -NULL value only > > > >> in two places: > > > >> - in drm_atomic_helper_update_legacy_modeset_state() after WARN_= ON() > > > >> encoder->crtc =3D connector->state->crtc; > > > >> - in drm_crtc_helper_set_config(drm_mode_set *set): > > > >> encoder->crtc =3D new_crtc; > > > >> > > > >> Nothing in the call path from drm_atomic_commit() calls crtc_func= s->set_config() or > > > >> drm_crtc_helper_set_config() directly, so the question is if this= WARN() is actually > > > >> valid. > > > >> > > > >> Call path from drm_atomic_commit: > > > >> > > > >> drm_atomic_helper_commit() > > > >> - drm_atomic_helper_prepare_planes() > > > >> - drm_atomic_helper_swap_state() > > > >> - drm_atomic_helper_commit_modeset_disables() > > > >> - disable_outputs() > > > >> - drm_atomic_helper_update_legacy_modeset_state() > > > >> - WARN_ON(!connector->encoder->crtc) > > > >> > > > >> Best regards, > > > >> Liviu > > > >> > > > >> > > > >> Hi Liviu > > > >> I solved this problem by following change, you can check = if your driver do the same things: > > > >> drivers/gpu/drm/bridge/dw_hdmi.c: > > > >> - hdmi->connector.encoder =3D encoder; > > > >> +// hdmi->connector.encoder =3D encoder; > > > >> > > > >> drm_mode_connector_attach_encoder(&hdmi->con= nector, encoder); > > > >> > > > >> I found some other drivers set connector.encoder before dr= m_mode_connector_attach_encoder, some are not. > > > >> > > > >> drm_mode_connector_attach_encoder already describe the lin= k of connector and encoder, > > > >> so I think "connector.encoder =3D encoder" is not needed, is th= at right? > > > >I'll have a look, thanks for pointing this. However, my setup uses t= he tda998x driver for encoder, so I will > > > >have to go look there rather than in my driver. > > > > > > > >Best regards, > > > >Liviu > > > > > > > >> Thanks. > > > >> > > > >> -- > > > >> =EF=BC=ADark Yao > > > Hi Liviu > > > drivers/gpu/drm/i2c/tda998x_drv.c do the same things: > > > priv->connector.encoder =3D &priv->encoder; > > > drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder); > > >=20 > > > I believe must be same problem. > >=20 > > Oh, I hadn't noticed this subthread, but you came to the same conclusion > > as I did. I have the below local change, which I think might be good to > > have given that there are at least two drivers that got this wrong. > >=20 > > Thierry > >=20 > > --- >8 --- > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 24c5434abd1c..56d53106b32d 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -4983,6 +4983,20 @@ int drm_mode_connector_attach_encoder(struct drm= _connector *connector, > > { > > int i; > > =20 > > + /* > > + * In the past, drivers have attempted to model the static association > > + * of connector to encoder in simple connector/encoder devices using a > > + * direct assignment of connector->encoder =3D encoder. This connecti= on > > + * is a logical one and the responsibility of the core, so drivers are > > + * expected not to mess with this. > > + * > > + * Note that the error return should've been enough here, but a large > > + * majority of drivers ignores the return value, so add in a big WARN > > + * to get people's attention. > > + */ > > + if (WARN_ON(connector->encoder)) > > + return -EINVAL; > > + > > for (i =3D 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { > > if (connector->encoder_ids[i] =3D=3D 0) { > > connector->encoder_ids[i] =3D encoder->base.id; >=20 > Hi Thierry, >=20 > This patch together with your tda998x proposed patch should solve > nicely the problem we are detecting, as long as no one calls > drm_connector_get_encoder() before drm_crtc_helper_set_config(). The only caller of drm_connector_get_encoder() is drm_mode_getconnector() and that handles the NULL case gracefully, so I don't think there are any issues. Thierry --M9NhX3UHpAaciwkO Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJWRLpuAAoJEN0jrNd/PrOhWmYQAI5i+bjccX7Yd3OdeNAFBnq9 voh0CmVaKusFgGMug5iDQrG0Eqo1ve9ecczAHh5muebOwqxPWeYqiTQKT2TeviX7 ZFq+R2l5lpJJ4OpOEoTVwOO5VCP+s1MWd2oBor0hDSKEaYpgKNIE8s0RKDa+9UKY nZBwI7pIBLYlzJr6CUFaJbF/9Tuk0MRyIhpYOoH51+z3vfl2rxKij22QjF6/+x1j ZAX/EDl7a59XiKmyeHINtTjsLRvOb8k7ze9keqAJY1aOE9h+m67gOf/iKLol5BTi O2uUX+/hyLUP11GehXtz8C+fR+DCMKRAKHQNFTe4Fyexsiz/efsOi3rejqnyBlcH hhvdcxF+kF9mUH0MCFgym4+/a0hISXNQfbrX2/AFIGwzuoYP2q+4Uqsvt/kKPALP ChyfyosDN2lLLwuA88rRLaf3s7fYbVF4irseR2O6FH3rwz7Xni0B5jj8kWcayTKt YbtQxdbbsxyOB9AO7EDTTU0m7v4HAOIXbsK22T35Q/Bp4UNyzz7bGIUpVAikWVrN 9PqrrcGe8TT8COmjAFsEv0xQFMnsC1t8d95lQovJWkZHFDMSVAMiKOxQhx1O/Xqd DuRdiRlbtCv1Ao1xbuE01zeEIq2TYjNHaI5Vk7O7bLuUNrkb0TS963NCyXoDMU9R 3fa4wnE9fbWRKw2ALNRU =gtYT -----END PGP SIGNATURE----- --M9NhX3UHpAaciwkO-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/