| Message ID | 1397496369-2746-2-git-send-email-eich@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <eich@suse.de> 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? -Chris
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 <eich@suse.de> > > 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.
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 <eich@suse.de> > --- > 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 writes: > > 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. I've got a different solution, although it requires a bit more code: Instead of having a single break_all_links() function I move the link breaking code into the two functions where it is used (ie sanitize_encoder() and sanitize_crtc()). This gives one a bit more flexibility in implementing what is needed and makes the code a bit clearer. I will send later - once I had the opportunity to test. Cheers, Egbert.
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; connector->base.encoder = NULL; connector->encoder->connectors_active = false; connector->encoder->base.crtc = NULL;
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 <eich@suse.de> --- drivers/gpu/drm/i915/intel_display.c | 2 ++ 1 file changed, 2 insertions(+)