Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965299AbbLOSdq (ORCPT ); Tue, 15 Dec 2015 13:33:46 -0500 Received: from mga03.intel.com ([134.134.136.65]:42766 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964973AbbLOSdp (ORCPT ); Tue, 15 Dec 2015 13:33:45 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,433,1444719600"; d="scan'208";a="861347026" Date: Tue, 15 Dec 2015 20:33:39 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Rob Clark Cc: Lyude , kuddel.mail@gmx.de, Daniel Vetter , open list , "open list:DRM DRIVERS" , Julien Wajsberg , Benjamin Tissoires , Lennart Poettering Subject: Re: [PATCH] Revert "drm: Stop resetting connector state to unknown" 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-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4032 Lines: 88 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 state to > > unknown") > > > > Unfortunately, not resetting the connector status to unknown actually > > breaks reprobing on suspend/resume in i915, which is important to have > > 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 userspace 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 something > > wonky during a hotplug event (e.g. part of the initialization of the > > connector failed), and the omapdrm driver returns "unknown" for certain > > connector types by default. > > > > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=100641 > > > > 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_device *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 = connector_status_unknown; > > + > > if (connector->funcs->reset) > > connector->funcs->reset(connector); > > - mutex_unlock(&dev->mode_config.mutex); > > + } > > } > > looks like git-revert might have been a bit over-ambitious and > clobbered a couple subsequent changes.. but that is easy enough to fix > once we figure out what the right thing to do is. > > 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. > > 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 report > 'unknown') would be to do something roughly like this in > status_show(): > > if (status == unknown) > status = connector->funcs->detect(connector) > > 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. -- Ville Syrj?l? Intel OTC -- 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/