From mboxrd@z Thu Jan 1 00:00:00 1970 From: Egbert Eich Subject: Re: [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector Date: Tue, 15 Apr 2014 11:14:56 +0200 Message-ID: <21324.63632.960293.629651@linux-qknr.fritz.box> References: <1397496369-2746-1-git-send-email-eich@suse.de> <1397496369-2746-2-git-send-email-eich@suse.de> <20140415074607.GH4415@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTP id 5CC5C6E89D for ; Tue, 15 Apr 2014 02:17:40 -0700 (PDT) In-Reply-To: chris@chris-wilson.co.uk wrote on Tuesday, 15 April 2014 at 08:46:07 +0100 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: Egbert Eich , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org Chris Wilson writes: > 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 > > This just leaves me with a nagging doubt that not all links will then be > broken. Do we have sufficient sanity checks to detect the obverse? Would > it be worth adding a second pass over the connector_list to assert that > all links are then broken? Possibly. Maybe we should rework intel_sanitize_encoder() a bit: loop over all drm_connectors, see if a drm_connector->encoder points to the encoder in question and make sure that the intel_encoder state (ie connectors_active and base.crtc) is in sync with the results. I'll think about it some more ... Cheers, Egbert.