Message-ID: <20151112161233.GA3913@ulmo>
Date:	Thu, 12 Nov 2015 17:12:33 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Liviu Dudau <Liviu.Dudau@....com>
Cc:	Mark yao <mark.yao@...k-chips.com>,
	DRI devel <dri-devel@...ts.freedesktop.org>,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: drm: Bogus WARN() in
 drm_atomic_helper_update_legacy_modeset_state() ?

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年11月12日 18:36, Liviu Dudau wrote:
> > > >On Thu, Nov 12, 2015 at 04:32:33PM +0800, Mark yao wrote:
> > > >>    On 2015年11月10日 23:01, Liviu Dudau wrote:
> > > >>
> > > >>  Hello,
> > > >>
> > > >>  When booting my Juno board with the HDLCD driver that I have converted 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-20151109+ #5
> > > >>  Hardware name: ARM Juno development board (r0) (DT)
> > > >>  task: ffffffc974888b00 ti: ffffffc9755dc000 task.ti: ffffffc9755dc000
> > > >>  PC is at drm_atomic_helper_update_legacy_modeset_state+0x204/0x20c
> > > >>  LR is at drm_atomic_helper_commit_modeset_disables+0x1c0/0x394
> > > >>  pc : [<ffffffc0003e6d18>] lr : [<ffffffc0003e8214>] pstate: 20000145
> > > >>  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:
> > > >>  [<ffffffc0003e6d18>] drm_atomic_helper_update_legacy_modeset_state+0x204/0x20c
> > > >>  [<ffffffc0003e8214>] drm_atomic_helper_commit_modeset_disables+0x1c0/0x394
> > > >>  [<ffffffc0003e84c8>] drm_atomic_helper_commit+0xe0/0x150
> > > >>  [<ffffffc00040bcc0>] drm_atomic_commit+0x40/0x6c
> > > >>  [<ffffffc0003e92c0>] restore_fbdev_mode+0x294/0x2d4
> > > >>  [<ffffffc0003eb6ec>] drm_fb_helper_restore_fbdev_mode_unlocked+0x34/0x8c
> > > >>  [<ffffffc0003eb770>] drm_fb_helper_set_par+0x2c/0x58
> > > >>  [<ffffffc000378048>] fbcon_init+0x4d4/0x534
> > > >>  [<ffffffc0003b44f4>] visual_init+0xac/0x104
> > > >>  [<ffffffc0003b63f8>] do_bind_con_driver+0x16c/0x398
> > > >>  [<ffffffc0003b6968>] do_take_over_console+0xd8/0x1f4
> > > >>  [<ffffffc00037811c>] do_fbcon_takeover+0x74/0xf8
> > > >>  [<ffffffc00037c5cc>] fbcon_event_notify+0x8a4/0x8f8
> > > >>  [<ffffffc0000d19f0>] notifier_call_chain+0x4c/0x88
> > > >>  [<ffffffc0000d1d78>] __blocking_notifier_call_chain+0x44/0x74
> > > >>  [<ffffffc0000d1dbc>] blocking_notifier_call_chain+0x14/0x1c
> > > >>  [<ffffffc00037d614>] fb_notifier_call_chain+0x1c/0x24
> > > >>  [<ffffffc00037f8a8>] register_framebuffer+0x1c0/0x2ac
> > > >>  [<ffffffc0003eb9f8>] drm_fb_helper_initial_config+0x25c/0x3ec
> > > >>  [<ffffffc0003ec28c>] drm_fbdev_cma_init+0x98/0x134
> > > >>  [<ffffffbffc006780>] hdlcd_drm_bind+0x180/0x498 [hdlcd]
> > > >>  [<ffffffc0004158e0>] try_to_bring_up_master.part.5+0xd4/0x118
> > > >>  [<ffffffc0004159e8>] component_master_add_with_match+0xc4/0x148
> > > >>  [<ffffffc000415a7c>] component_master_add+0x10/0x18
> > > >>  [<ffffffbffc0065ec>] hdlcd_probe+0x14/0x28 [hdlcd]
> > > >>  [<ffffffc00041c504>] platform_drv_probe+0x54/0xc0
> > > >>  [<ffffffc00041a9b0>] driver_probe_device+0x1ec/0x2e8
> > > >>  [<ffffffc00041ab48>] __driver_attach+0x9c/0xa0
> > > >>  [<ffffffc000418bcc>] bus_for_each_dev+0x58/0x98
> > > >>  [<ffffffc00041a41c>] driver_attach+0x20/0x28
> > > >>  [<ffffffc00041a058>] bus_add_driver+0x1c8/0x22c
> > > >>  [<ffffffc00041b3d0>] driver_register+0x68/0x108
> > > >>  [<ffffffc00041c408>] __platform_driver_register+0x4c/0x54
> > > >>  [<ffffffbffc00b018>] hdlcd_init+0x18/0x30 [hdlcd]
> > > >>  [<ffffffc00008293c>] do_one_initcall+0x90/0x1a8
> > > >>  [<ffffffc0001424dc>] do_init_module+0x60/0x1c8
> > > >>  [<ffffffc00011d364>] load_module+0x1554/0x1c98
> > > >>  [<ffffffc00011dc64>] SyS_finit_module+0x7c/0x88
> > > >>  [<ffffffc000085cb0>] 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 = connector->state->crtc;
> > > >>   - in drm_crtc_helper_set_config(drm_mode_set *set):
> > > >>                  encoder->crtc = new_crtc;
> > > >>
> > > >>  Nothing in the call path from drm_atomic_commit() calls crtc_funcs->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 = encoder;
> > > >>             +//     hdmi->connector.encoder = encoder;
> > > >>
> > > >>                       drm_mode_connector_attach_encoder(&hdmi->connector, encoder);
> > > >>
> > > >>         I found some other drivers set connector.encoder before drm_mode_connector_attach_encoder, some are not.
> > > >>
> > > >>         drm_mode_connector_attach_encoder already describe the link of connector and encoder,
> > > >>    so I think "connector.encoder = encoder" is not needed, is that right?
> > > >I'll have a look, thanks for pointing this. However, my setup uses the tda998x driver for encoder, so I will
> > > >have to go look there rather than in my driver.
> > > >
> > > >Best regards,
> > > >Liviu
> > > >
> > > >>    Thanks.
> > > >>
> > > >>  --
> > > >>  Ｍark Yao
> > > Hi Liviu
> > >      drivers/gpu/drm/i2c/tda998x_drv.c do the same things:
> > >             priv->connector.encoder = &priv->encoder;
> > > drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
> > > 
> > >      I believe must be same problem.
> > 
> > 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.
> > 
> > Thierry
> > 
> > --- >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;
> >  
> > +	/*
> > +	 * 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 = encoder. This connection
> > +	 * 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 = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> >  		if (connector->encoder_ids[i] == 0) {
> >  			connector->encoder_ids[i] = encoder->base.id;
> 
> Hi Thierry,
> 
> 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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists