Date: Thu, 12 Nov 2015 17:28:01 +0100
From: Thierry Reding <thierry.reding@gmail.com>
To: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
        Rob Clark <robdclark@gmail.com>,
        DRI devel <dri-devel@lists.freedesktop.org>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: drm: Bogus WARN() in
 drm_atomic_helper_update_legacy_modeset_state() ?
Message-ID: <20151112162801.GC3913@ulmo>
References: <20151110150102.GP963@e106497-lin.cambridge.arm.com>
 <20151110165615.GB27962@ulmo>
 <20151111160942.GS963@e106497-lin.cambridge.arm.com>
 <20151112121655.GA31019@ulmo>
 <20151112135711.GB963@e106497-lin.cambridge.arm.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="adJ1OR3c6QgCpb/j"
Content-Disposition: inline
In-Reply-To: <20151112135711.GB963@e106497-lin.cambridge.arm.com>
User-Agent: Mutt/1.5.23+102 (2ca89bed6448) (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5901
Lines: 146


--adJ1OR3c6QgCpb/j
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Nov 12, 2015 at 01:57:11PM +0000, Liviu Dudau wrote:
> On Thu, Nov 12, 2015 at 01:16:55PM +0100, Thierry Reding wrote:
> > On Wed, Nov 11, 2015 at 04:09:42PM +0000, Liviu Dudau wrote:
> > > On Tue, Nov 10, 2015 at 05:56:15PM +0100, Thierry Reding wrote:
> > > > On Tue, Nov 10, 2015 at 03:01:03PM +0000, Liviu Dudau wrote:
> > > > > Hello,
> > > > >=20
> > > > > When booting my Juno board with the HDLCD driver that I have conv=
erted to
> > > > > atomic operations I'm getting the following warning:
> > > >=20
> > > > Perhaps you can provide pointers to the source code, that might mak=
e it
> > > > easier for people to spot what's going wrong.
> > > >=20
> > > > Thierry
> > >=20
> > > Hi Thierry,
> > >=20
> > > I have just pushed to the mailing lists my patch series. [1] [2]
> > >=20
> > > If you want to checkout the code I also have a branch here:
> > >=20
> > > git://git://linux-arm.org/linux-ld testing/hdlcd
> >=20
> > Okay, so if I understand correctly you're using the tda998x as encoder/
> > connector attached to your new HDLCD driver. I think the problem isn't
> > so much that nothing has set up the CRTC for the encoder, but rather
> > that the tda998x encoder tries to statically associate the encoder with
> > the connector. While that might be correct in that it represents the
> > hardware state (i.e. there is no way to physically route it anywhere
> > else), the DRM logical state is that it's not connected until a complete
> > pipeline has been set up, in which case a CRTC would have been assigned
> > to the encoder.
> >=20
> > If your setup were working correctly you'd never reach the WARN_ON
> > because the
> >=20
> > 	if (connector->encoder) {
> >=20
> > conditional (on line 673 in drivers/gpu/drm/drm_atomic_helper.c) would
> > have evaluated to false already, since logically there'd be no encoder
> > associated with the connector yet.
>=20
> Your analysis is spot on and matches my conditions. However ...
>=20
> >=20
> > Does the patch below help? Let me know and I can produce a proper patch.
> >=20
> > Thierry
> >=20
> > --- >8 ---
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/td=
a998x_drv.c
> > index 896b6aaf8c4d..8b0a402190eb 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > @@ -1453,7 +1453,6 @@ static int tda998x_bind(struct device *dev, struc=
t device *master, void *data)
> >  	if (ret)
> >  		goto err_sysfs;
> > =20
> > -	priv->connector.encoder =3D &priv->encoder;
>=20
> while this patch helps (no WARN() printed) I'm not sure this is the right=
 fix.
> TDA998x is also used by armada DRM driver which has not been converted (y=
et) to
> atomic modesetting. And judging by the code and comment of drm_connector_=
get_encoder()
> in drm_crtc.c, having access to the encoder through the connector is the =
non-atomic
> way of doing things:
>=20
> static struct drm_encoder *drm_connector_get_encoder(struct drm_connector=
 *connector)
> {
> 	/* For atomic drivers only state objects are synchronously updated and
> 	 * protected by modeset locks, so check those first. */
> 	if (connector->state)
> 		return connector->state->best_encoder;
> 	return connector->encoder;
> }
>=20
> To me, it looks like the WARN() is bogus when the atomic modesetting is u=
sed in
> drm_atomic_helper_update_legacy_modeset_state(). Either we acknowledge th=
at the "legacy"
> code sets the connector in the encoder structure before complete pipeline=
 is setup
> and we remove the WARN(), or we find a better way of detecting that some =
sort of mixed
> support is in place (i.e. atomic and non-atomic aware code running) and w=
e clean up
> in a way that is compatible with both worlds.

I think the problem you're seeing here is specifically caused by drivers
setting up the connector->encoder explicitly. There should be no reason
to do that. The DRM core will automatically do that when setting up a
default configuration, or as a result of userspace setting up the wanted
configuration. You're also likely only seeing this the first time around
and subsequent calls will not trigger this anymore because at that point
the core will have reset connector->encoder to NULL as part of tearing
down the pipeline.

To corroborate that, the Tegra driver doesn't trigger this WARN_ON() and
also never sets up the connector->encoder manually. The same is probably
true for many other drivers that have already been converted. That said,
=66rom a brief look it seems like many more drivers get this wrong and may
run into this WARN when they get converted to atomic.

Thierry

--adJ1OR3c6QgCpb/j
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAABCAAGBQJWRL4OAAoJEN0jrNd/PrOh0pgP/At/etINAQtSa4XRGglWSyIP
W7gVWlxiESlGLA2ZhfaKvnYZm/Gd+lwJE+JpR+cXdWpbCWpduJJGSGhkmKt5e+jn
VP8IUZk6A0MO51Z9p5x3lPZokp/9vZf7KNT5ArcsCV9V9cj8p335cvtQ1zymmju9
kJMZvDdJWEY373NbCtZvU/Zd0mjFAfYogFOrZCUThdTP3KeKjFf2z0VTN3wHXaiA
df49JIip6ttvMIdJPpmJIK7Ko1gGxZRPGFl1/zOX2OrWeQDxPSHF6Mfc9gzvRBRQ
XpbRx6VZSPwXAlBvrF/Lyugcbt8BktIocz3EtrQxfDf6lfMTUZE82gJKOb2OFBK4
mnsoZAB0lBkABzv0Nw2VcNqNbchtcd/lXFOOG1ZGWcwwMooZ8vgM/WGJwKSzhxP2
eEoAxfLwe4wxyqGhWuvjkqovPtgSKWk7R8EXY9xU/hJwcAvk5MMeMK+YEMrC+/5N
n7564mSyTTmg34nsMZnvMnLLqQwSN6ek9Ye4FEeNX/Bd1MZ5Pb24PIKA+lw1yrqI
2W/Ds/3A9u5WUm/pb61hIZ34RfgwlaC+HhwEOuzsUCg+Zv24mGbqzhFhfDHTWfqU
KeuJ+8J33A9ytlKCxb5RS4EpbNap6aCji3YujsVU/RnIURfzaq3F28DSxtHzQDH5
WLF/6TAvkZjChqvUn8FQ
=cWAV
-----END PGP SIGNATURE-----

--adJ1OR3c6QgCpb/j--
--
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/