From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector Date: Tue, 15 Apr 2014 21:08:53 +0200 Message-ID: <20140415190853.GC1023@phenom.ffwll.local> References: <1397496369-2746-1-git-send-email-eich@suse.de> <1397496369-2746-2-git-send-email-eich@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f46.google.com (mail-ee0-f46.google.com [74.125.83.46]) by gabe.freedesktop.org (Postfix) with ESMTP id 5F42F6E2B8 for ; Tue, 15 Apr 2014 12:08:59 -0700 (PDT) Received: by mail-ee0-f46.google.com with SMTP id t10so8061670eei.33 for ; Tue, 15 Apr 2014 12:08:57 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1397496369-2746-2-git-send-email-eich@suse.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Egbert Eich Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Apr 14, 2014 at 07:26:08PM +0200, Egbert Eich wrote: > Depending on the SDVO output_flags SDVO may have multiple connectors > linking to the same encoder (in intel_connector->encoder->base). > Only one of those connectors should be active (ie link to the encoder > thru drm_connector->encoder. > If intel_connector_break_all_links() is called from intel_sanitize_crtc() > we may brake the crtc connection of an encoder thru an inactive connector > in which case intel_connector_break_all_links() will not be called again > for the active connector due to > if (connector->encoder->base.crtc != &crtc->base) > continue; > in intel_sanitize_crtc(). > This will however leave the drm_connector->encoder linkage for this > active connector in place. Subsequently this will cause multiple > warnings in intel_connector_check_state() to trigger and the driver > will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL). > > To avoid this break the encoder links only if the connector is active > (ie. has drm_connector->encoder set). > > Signed-off-by: Egbert Eich > --- > drivers/gpu/drm/i915/intel_display.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1390ab5..041f847 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11390,6 +11390,8 @@ static void > intel_connector_break_all_links(struct intel_connector *connector) > { > connector->base.dpms = DRM_MODE_DPMS_OFF; > + if (!connector->base.encoder) > + return; Hm, I think to address Chris' concern we should split this into two pieces: connector_break_all links which only breaks connector->encoder stuff, used in both places as is. And a new encoder_break_all links which we'll use in sanitize_crtc in a new encoder loop. Really nice catch though! -Daniel > connector->base.encoder = NULL; > connector->encoder->connectors_active = false; > connector->encoder->base.crtc = NULL; > -- > 1.8.4.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch