From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] Revert "drm: Stop resetting connector state to unknown" Date: Tue, 15 Dec 2015 20:33:39 +0200 Message-ID: <20151215183339.GO4437@intel.com> References: <1450202871-15062-1-git-send-email-cpaul@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Rob Clark Cc: Lyude , kuddel.mail@gmx.de, Daniel Vetter , open list , "open list:DRM DRIVERS" , Julien Wajsberg , Benjamin Tissoires , Lennart Poettering List-Id: dri-devel@lists.freedesktop.org On Tue, Dec 15, 2015 at 01:23:06PM -0500, Rob Clark wrote: > On Tue, Dec 15, 2015 at 1:07 PM, Lyude wrote: > > This reverts commit 5677d67ae394 ("drm: Stop resetting connector st= ate to > > unknown") > > > > Unfortunately, not resetting the connector status to unknown actual= ly > > breaks reprobing on suspend/resume in i915, which is important to h= ave > > working since it means a user docking their laptop in suspend won't= have > > their monitors work after resume. This commit was originally pushed= to fix > > a bug with systemd[1], however said bug has already been fixed in > > userspace. > > > > Since "unknown" is technically a valid option to return to userspac= e for a > > connector's status, I would think that this sort of behavior should > > probably be expected from userspace. Some good examples of this are= the > > radeon driver reporting "unknown" for connectors that have done som= ething > > wonky during a hotplug event (e.g. part of the initialization of th= e > > connector failed), and the omapdrm driver returns "unknown" for cer= tain > > connector types by default. > > > > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=3D100641 > > > > Signed-off-by: Lyude > > --- > > drivers/gpu/drm/drm_crtc.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.= c > > index 24c5434..474e636 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -5312,11 +5312,12 @@ void drm_mode_config_reset(struct drm_devic= e *dev) > > if (encoder->funcs->reset) > > encoder->funcs->reset(encoder); > > > > - mutex_lock(&dev->mode_config.mutex); > > - drm_for_each_connector(connector, dev) > > + list_for_each_entry(connector, &dev->mode_config.connector_= list, head) { > > + connector->status =3D connector_status_unknown; > > + > > if (connector->funcs->reset) > > connector->funcs->reset(connector); > > - mutex_unlock(&dev->mode_config.mutex); > > + } > > } >=20 > looks like git-revert might have been a bit over-ambitious and > clobbered a couple subsequent changes.. but that is easy enough to fi= x > once we figure out what the right thing to do is. >=20 > Beyond that.. I'm not really sure how to apply the "do not break > userspace" rule here.. since prior to > c484f02d0f02fbbfc6decc945a69aae011041a27 userspace could see "unknown= " > for certain hardware. But after that commit it could start seeing > "unknown" for drivers/connectors that never would have returned > "unknown" before. If userspace had a problem with "unknown", it > sounds like a userspace bug that was just unnoticed because no one > tested on the right hardware. >=20 > But anyways, one idea to revert things to original behavior prior to > c484f02d0f02fbbfc6decc945a69aae011041a27 (so at least userspace > doesn't see 'unknown' for drivers/connectors that never used to repor= t > 'unknown') would be to do something roughly like this in > status_show(): >=20 > if (status =3D=3D unknown) > status =3D connector->funcs->detect(connector) >=20 > So I could go with either just reverting this commit, or reverting > commit plus above change. My $0.02 anyways.. Hmm. Or maybe leave the states alone, and just fire off the uevent unconditionally and let userspace initiate the probe. That way we could skip all ->detect() calls during resume for a bit of extra speed. Not 100% if that would be safe. Maybe something internal depends on the ->detect() having been called? DP stuff perhaps? Which makes me wonder how i915 copes with a DP monitor geting unplugged/change while suspended. I should probably try that. --=20 Ville Syrj=E4l=E4 Intel OTC