From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [RFC PATCH xserver] modesetting: re-set the crtc's mode when link-status goes BAD Date: Thu, 02 Feb 2017 09:57:20 -0800 Message-ID: <87bmukxypr.fsf@eliezer.anholt.net> References: <20170126123728.5680-1-martin.peres@linux.intel.com> <87tw8fxa22.fsf@eliezer.anholt.net> <87poj21b42.fsf@intel.com> <87efzhr8dj.fsf@eliezer.anholt.net> <20170202090146.t6fuijsolgfpgq4o@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0159659236==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 65E706E087 for ; Thu, 2 Feb 2017 17:57:23 +0000 (UTC) In-Reply-To: <20170202090146.t6fuijsolgfpgq4o@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Manasi Navare , xorg-devel@lists.x.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0159659236== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Daniel Vetter writes: > On Wed, Feb 01, 2017 at 11:58:16AM -0800, Eric Anholt wrote: >> Jani Nikula writes: >>=20 >> > On Tue, 31 Jan 2017, Eric Anholt wrote: >> >> Martin Peres writes: >> >> >> >>> Despite all the careful planing of the kernel, a link may become >> >>> insufficient to handle the currently-set mode. At this point, the >> >>> kernel should mark this particular configuration as being broken >> >>> and potentially prune the mode before setting the offending connecto= r's >> >>> link-status to BAD and send the userspace a hotplug event. This may >> >>> happen right after a modeset or later on. >> >>> >> >>> When available, we should use the link-status information to reset >> >>> the wanted mode. >> >>> >> >>> Signed-off-by: Martin Peres >> >> >> >> If I understand this right, there are two failure modes being handled: >> >> >> >> 1) A mode that won't actually work because the link isn't good enough. >> >> >> >> 2) A mode that should work, but link parameters were too optimistic a= nd >> >> if we just ask the kernel to set the mode again it'll use more >> >> conservative parameters that work. >> >> >> >> This patch seems good for 2). For 1), the drmmode_set_mode_major is >> >> going to set our old mode back. Won't the modeset then fail to link >> >> train again, and bring us back into this loop? The only escape that I >> >> see would be some other userspace responding to the advertised mode l= ist >> >> changing, and then asking X to modeset to something new. >> >> >> >> To avoid that failure busy loop, should we re-fetching modes at this >> >> point, and only re-setting if our mode still exists? >> > >> > Disclaimer: I don't know anything about the internals of the modesetti= ng >> > driver. >> > >> > Perhaps we can identify the two cases now, but I'd put this more >> > generally: if the link status has gone bad, it's an indicator to >> > userspace that the circumstances may have changed, and userspace should >> > query the kernel for the list of available modes again. It should no >> > longer trust information obtained prior to getting the bad link status, >> > including the current mode. >> > >> > But specifically, I think you're right, and AFAICT asking for the list >> > of modes again is the only way for the userspace to distinguish between >> > the two cases. I don't think there's a shortcut for deciding the curre= nt >> > mode is still valid. >>=20 >> To avoid the busy-loop problem, I think I'd like this patch to re-query >> the kernel to ask about the current mode list, and only try to re-set >> the mode if our mode is still there. > > Yeah, I guess that sounds like a reasonable heuristics. More integrated > compositors (aka the wayland ones) might be able to make more useful > decisions, but for -modesetting that's probably the best we can do. >=20=20 >> If the mode isn't there, then it's up to the DE to take action in >> response to the notification of new modes. If you don't have a DE to >> take appropriate action, you're kind of out of luck. >>=20 >> As far as the ABI goes, this seems fine to me. The only concern I had >> about ABI was having to walk all the connectors on every uevent to see >> if any had gone bad -- couldn't we have a flag of some sort about what >> the uevent indicates? But uevents should be super rare, so I'd say the >> kernel could go ahead with the current plan. > > We've discussed finer-grained uevent singalling a few times, and I'd like > that to be an orthogonal abi extension. Right now we just don't have the > required tracking even within the kernel to know which connector changed > (and the tracking we do have missed a few things, too). My idea is to push > the tracking into the leaves of the callchains with a function that > increases some per-connector epoch counter. Then we'd just have to expose > that somehow cheaply to userspace (could probably just send it along in > the uevent). Plus expose it as a read-only property so that userspace can > re-synchronize. > > But again, that should be an orthogonal thing imo. Yeah, that was roughly my thought process, too. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAliTcwAACgkQtdYpNtH8 nuhgaA//VGR76iGPfi2jd3ihzrfa4mnpDnm2fgjW8nkWPEKfjrYNLkEDfBoDhIH1 DiKvi6OrPnTYS1WOuH1/bRURshMF0p2SiRsZvsadT2FET6BSnBNXGa53DeBhAPKM QV3Z9kl3UQiRJ0mrUvk/BIaav1hMET4yES6p1r6RQAaiOWjbmbYjL5sB+xGG0ev/ 97huv+tvGWbxpkWqUwooVxYfU/gY0rcjX+5ghlz4xtMs/QUQ9/6FeQes4rG3kjz9 fu+5I3qQOeX608Ak2368fiH+QsWLlZTAJr+oYvZ68LiRaPnEv1XzQt8gvz8IEL0+ PB29hGLlCB5IHtq5k/pG1e4j0Zl5b/2q6J+OykCQvFhw8YzR0I2GK4JB54anAaFs Bv0oig0eyHmam2tkE2QsgCzHEJHVgrLdND6lE8SbmRU/WXNCz/Vy5VSf2m2TGWFr QWMlGfi/ZjvZ3iVtNV5AsZJQhfRA10fpVUEZF/3n7N+ZqlrcwqMrD3G5uat5qmIy KVd4/Kk+GNpfQd1H3trjC2CkR9OqfjNev6/fe+rDP2n8/6qY7AMNjpUHeD1ptITl OlILbAzH6uI6RPJWYeW/S8k71mxCXVlba5ty/mCGYJV5v0C/yt7sHH05Q0qHasG1 iK0IxvoIhq7KezRI1BQtLbszqYjcI9HXm29lxnVimeC2Xb/pZC0= =GCcq -----END PGP SIGNATURE----- --=-=-=-- --===============0159659236== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0159659236==--