| Message ID | 1397639805-12229-1-git-send-email-eich@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 16, 2014 at 11:16:45AM +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 break 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 if this happens to come later in the list 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 remove intel_connector_break_all_links() and move its > code to its two calling functions: intel_sanitize_crtc() and > intel_sanitize_encoder(). > This allows to implement the link breaking more flexibly matching > the surrounding code: ie. in intel_sanitize_crtc() we can break the > crtc link separatly after the links to the encoders have been > broken which avoids above problem. > > Signed-off-by: Egbert Eich <eich@suse.de> > > v2: This patch takes care of the concernes voiced by Chris Wilson > and Daniel Vetter that only breaking links if the drm_connector > is linked to an encoder may miss some links. > --- > drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1390ab5..c276733 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11386,15 +11386,6 @@ void intel_modeset_init(struct drm_device *dev) > } > } > > -static void > -intel_connector_break_all_links(struct intel_connector *connector) > -{ > - connector->base.dpms = DRM_MODE_DPMS_OFF; > - connector->base.encoder = NULL; > - connector->encoder->connectors_active = false; > - connector->encoder->base.crtc = NULL; > -} > - > static void intel_enable_pipe_a(struct drm_device *dev) > { > struct intel_connector *connector; > @@ -11476,8 +11467,16 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > if (connector->encoder->base.crtc != &crtc->base) > continue; > > - intel_connector_break_all_links(connector); > + connector->base.dpms = DRM_MODE_DPMS_OFF; > + connector->encoder->connectors_active = false; I'd move the encoder->connectors_active = false down into the encoder loop, but doesn't really matter. Jani can frob this when applying for 3.15-fixes. This regression has been introduced in commit 24929352481f085c5f85d4d4cbc919ddf106d381 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Mon Jul 2 20:28:59 2012 +0200 drm/i915: read out the modeset hw state at load and resume time so goes back to the very beginning of the modeset rework. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: stable@vger.kernel.org Thanks, Daniel > + connector->base.encoder = NULL; > } > + /* multiple connectors may have the same encoder: > + * break crtc link separately */ > + list_for_each_entry(connector, &dev->mode_config.connector_list, > + base.head) > + if (connector->encoder->base.crtc == &crtc->base) > + connector->encoder->base.crtc = NULL; > > WARN_ON(crtc->active); > crtc->base.enabled = false; > @@ -11559,6 +11558,8 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) > drm_get_encoder_name(&encoder->base)); > encoder->disable(encoder); > } > + encoder->base.crtc = NULL; > + encoder->connectors_active = false; > > /* Inconsistent output/port/pipe state happens presumably due to > * a bug in one of the get_hw_state functions. Or someplace else > @@ -11569,8 +11570,8 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) > base.head) { > if (connector->encoder != encoder) > continue; > - > - intel_connector_break_all_links(connector); > + connector->base.dpms = DRM_MODE_DPMS_OFF; > + connector->base.encoder = NULL; > } > } > /* Enabled encoders without active connectors will be fixed in > -- > 1.8.4.5 >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1390ab5..c276733 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11386,15 +11386,6 @@ void intel_modeset_init(struct drm_device *dev) } } -static void -intel_connector_break_all_links(struct intel_connector *connector) -{ - connector->base.dpms = DRM_MODE_DPMS_OFF; - connector->base.encoder = NULL; - connector->encoder->connectors_active = false; - connector->encoder->base.crtc = NULL; -} - static void intel_enable_pipe_a(struct drm_device *dev) { struct intel_connector *connector; @@ -11476,8 +11467,16 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) if (connector->encoder->base.crtc != &crtc->base) continue; - intel_connector_break_all_links(connector); + connector->base.dpms = DRM_MODE_DPMS_OFF; + connector->encoder->connectors_active = false; + connector->base.encoder = NULL; } + /* multiple connectors may have the same encoder: + * break crtc link separately */ + list_for_each_entry(connector, &dev->mode_config.connector_list, + base.head) + if (connector->encoder->base.crtc == &crtc->base) + connector->encoder->base.crtc = NULL; WARN_ON(crtc->active); crtc->base.enabled = false; @@ -11559,6 +11558,8 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) drm_get_encoder_name(&encoder->base)); encoder->disable(encoder); } + encoder->base.crtc = NULL; + encoder->connectors_active = false; /* Inconsistent output/port/pipe state happens presumably due to * a bug in one of the get_hw_state functions. Or someplace else @@ -11569,8 +11570,8 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) base.head) { if (connector->encoder != encoder) continue; - - intel_connector_break_all_links(connector); + connector->base.dpms = DRM_MODE_DPMS_OFF; + connector->base.encoder = NULL; } } /* Enabled encoders without active connectors will be fixed in
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 break 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 if this happens to come later in the list 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 remove intel_connector_break_all_links() and move its code to its two calling functions: intel_sanitize_crtc() and intel_sanitize_encoder(). This allows to implement the link breaking more flexibly matching the surrounding code: ie. in intel_sanitize_crtc() we can break the crtc link separatly after the links to the encoders have been broken which avoids above problem. Signed-off-by: Egbert Eich <eich@suse.de> v2: This patch takes care of the concernes voiced by Chris Wilson and Daniel Vetter that only breaking links if the drm_connector is linked to an encoder may miss some links. --- drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)