From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH] drm/fb-helper: Fix hpd vs. initial config races Date: Tue, 22 Apr 2014 10:56:35 +0200 Message-ID: <20140422085634.GG17275@ulmo> References: <1397659521-32027-1-git-send-email-daniel.vetter@ffwll.ch> <20140417083814.GA23723@ulmo> <20140422082637.GK10722@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0124845534==" Return-path: In-Reply-To: <20140422082637.GK10722@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Daniel Vetter , Intel Graphics Development , Thierry Reding , DRI Development List-Id: intel-gfx@lists.freedesktop.org --===============0124845534== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="RwGu8mu1E+uYXPWP" Content-Disposition: inline --RwGu8mu1E+uYXPWP Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Apr 22, 2014 at 10:26:37AM +0200, Daniel Vetter wrote: > On Thu, Apr 17, 2014 at 10:38:17AM +0200, Thierry Reding wrote: > > On Wed, Apr 16, 2014 at 04:45:21PM +0200, Daniel Vetter wrote: > > > Some drivers need to be able to have a perfect race-free fbcon setup. > > > Current drivers only enable hotplug processing after the call to > > > drm_fb_helper_initial_config which leaves a tiny but important race. > > >=20 > > > This race is especially noticable on embedded platforms where the > > > driver itself enables the voltage for the hdmi output, since only then > > > will monitors (after a bit of delay, as usual) respond by asserting > > > the hpd pin. > > >=20 > > > Most of the infrastructure is already there with the split-out > > > drm_fb_helper_init. And drm_fb_helper_initial_config already has all > > > the required locking to handle concurrent hpd events since > > >=20 > > > commit 53f1904bced78d7c00f5d874c662ec3ac85d0f9f > > > Author: Daniel Vetter > > > Date: Thu Mar 20 14:26:35 2014 +0100 > > >=20 > > > drm/fb-helper: improve drm_fb_helper_initial_config locking > > >=20 > > > The only missing bit is making drm_fb_helper_hotplug_event save > > > against concurrent calls of drm_fb_helper_initial_config. The only > > > unprotected bit is the check for fb_helper->fb. > > >=20 > > > With that drivers can first initialize the fb helper, then enabel > > > hotplug processing and then set up the initial config all in a > > > completely race-free manner. Update kerneldoc and convert i915 as a > > > proof of concept. > > >=20 > > > Feature requested by Thierry since his tegra driver atm reliably boots > > > slowly enough to misses the hotplug event for an external hdmi screen, > > > but also reliably boots to quickly for the hpd pin to be asserted when > > > the fb helper calls into the hdmi ->detect function. > > >=20 > > > Cc: Thierry Reding > > > Signed-off-by: Daniel Vetter > > > --- > > > drivers/gpu/drm/drm_fb_helper.c | 11 +++++------ > > > drivers/gpu/drm/i915/i915_dma.c | 3 --- > > > drivers/gpu/drm/i915/i915_drv.c | 2 -- > > > drivers/gpu/drm/i915/i915_drv.h | 1 - > > > drivers/gpu/drm/i915/i915_irq.c | 4 ---- > > > 5 files changed, 5 insertions(+), 16 deletions(-) > >=20 > > The FB helper parts: > >=20 > > Tested-by: Thierry Reding > >=20 > > But I have one comment below... > >=20 > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb= _helper.c > > [...] > > > - * Note that the driver must ensure that this is only called _after_= the fb has > > > - * been fully set up, i.e. after the call to drm_fb_helper_initial_c= onfig. > > > + * Note that drivers may call this even before calling > > > + * drm_fb_helper_initial_config but only aftert drm_fb_helper_init. = This allows > >=20 > > I don't think the requirement is that strict. To elaborate: on Tegra we > > cannot call drm_fb_helper_init() because the number of CRTCs and > > connectors isn't known this early. We determine that dynamically after > > all sub-devices have been initialized. So instead of calling > > drm_fb_helper_init() before drm_kms_helper_poll_init(), I did something > > more minimal (see attached patch). It's kind of difficult to tell > > because of the context, but tegra_drm_fb_prepare() sets up the mode > > config and functions and allocate memory for the FB helper and sets the > > FB helper functions. > >=20 > > This may not be enough for all drivers, but on Tegra the implementation > > of .output_poll_changed() simply calls drm_fb_helper_hotplug_event(), > > which will work fine with just that rudimentary initialization. >=20 > Hm yeah I think this should be sufficient, too. It would be good to > extract this minimal initialization into a new drm_fb_helper_prepare > function and update the kerneldoc a bit more. Maybe as a patch on top of > mine? >=20 > Then we could merge this all as an early tegra-next pull to Dave. Sounds like a good idea. I'll go prepare a patch. Thierry --RwGu8mu1E+uYXPWP Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTVi7CAAoJEN0jrNd/PrOhSdoP/iY11qISf2idADD0+uQpoFUJ H8R7PfUM3g2Xpp0XUqFdgBXiHNm/79yL2j2kjcEqaHW8PyRbyxuBUiI1v+hddw3E wZHOT5/OC/QM9/PzFvpTva8VtCo9FJXpjnrzOySFPzmB6mXYZvq9vpWFml/1BJLU TD8pPRWrtLiBlm9wQpVkLzWSAxnA35B+04kj5HcKMLldAd2opg1cpQ3Z3cT27709 jBWribZmYq7TDLdaZaXfztkGCiHbzdvP/VvIzckj80NKeMABZ9JLhjbzj3rl8TDY XfEDJi0Z4iOTx9sohLfZSrMCnNoSEvJCbwtXB/OrAEzA45anW/Xh+IHPua4Ye3iL E5Fh8kciiWBMFJqL4McaOYyuXEGpibYrY/tGGM4PE1mXdVPyNqJN1+DOE4VykXfc 4dSnvCPHbVupF84Sh212M97riEVN94LeFv8xIZR/ajRRlHQ5sCkWSkQoqYBrioXT n49xGV3cuwjkz6BbdcRXUoSCnzWOA7EAoJ70ZkWnjzJ8W5VAcAgcn+R5wrTJHv9P MfVgUp7/vJRbhN+iX9nawUjyvHKh0VjbnbUbs5O4LmRc+0HhLz166homFdFcEo8k a53IerZrcx3jdhz34hzih3B0WKh16B/XKR70dRxnV/EIh5tlJzuhk70XlzZE7v5K HkwsIBU06//7rJ5OSEbT =yqce -----END PGP SIGNATURE----- --RwGu8mu1E+uYXPWP-- --===============0124845534== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0124845534==--