[PATCH 0/2] Fix if multiple SDVO outputs are flagged

[PATCH 0/2] Fix if multiple SDVO outputs are flagged

[Egbert Eich]

Currently the i915 driver can only handle well if only a single
SDVO output is flagged (ie output_flags has only one bit set).
If multiple outputs are flagged the side effects are only cosmetic
in most cases (ie. the encoder may have the wrong type set),
but there are situations (namely when intel_connector_break_all_links()
is called) where this may lead to an inconsistent driver state.

The following two patches fix both situations.

Egbert Eich (2):
  drm/i915: Only break encoder linked when linked to connector
  drm/i915: Set up SDVO encoder type only at detect

 drivers/gpu/drm/i915/intel_display.c |  2 ++
 drivers/gpu/drm/i915/intel_sdvo.c    | 23 ++++++++++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

-- 
1.8.4.5

[PATCH 1/2] drm/i915: Only break encoder linked when linked to connector

[Egbert Eich]

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;
 	connector->base.encoder = NULL;
 	connector->encoder->connectors_active = false;
 	connector->encoder->base.crtc = NULL;
-- 
1.8.4.5

[PATCH 2/2] drm/i915: Set up SDVO encoder type only at detect

[Egbert Eich]

Depending on the SDVO output_flags SDVO may have multiple connectors.
These are all initialized in intel_sdvo_output_setup(). The connector
that is initialized later will override the encoder_type that has been
set up by an earlier connector type initialization. Eventually the
one that comes last wins.
Eventually when intel_sdvo_detect() is called the active connector is
determined.
Delay encoder type initialization until the active connector is known
and set it to the type that corresponds to this connector.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index d27155a..5043f16 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -154,6 +154,9 @@ struct intel_sdvo_connector {
 	/* Mark the type of connector */
 	uint16_t output_flag;
 
+	/* store encoder type for convenience */
+	int encoder_type;
+
 	enum hdmi_force_audio force_audio;
 
 	/* This contains all current supported TV format */
@@ -1746,6 +1749,7 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
 	if (response == 0)
 		return connector_status_disconnected;
 
+	intel_sdvo->base.base.encoder_type = intel_sdvo_connector->encoder_type;
 	intel_sdvo->attached_output = response;
 
 	intel_sdvo->has_hdmi_monitor = false;
@@ -2489,7 +2493,9 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 	} else {
 		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 	}
-	encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
+	/* delay encoder_type setting until detection */
+	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TMDS;
+	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
 
 	if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
@@ -2524,7 +2530,9 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 
 	intel_connector = &intel_sdvo_connector->base;
 	connector = &intel_connector->base;
-	encoder->encoder_type = DRM_MODE_ENCODER_TVDAC;
+	/* delay encoder_type setting until detection */
+	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TVDAC;
+	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 	connector->connector_type = DRM_MODE_CONNECTOR_SVIDEO;
 
 	intel_sdvo->controlled_output |= type;
@@ -2568,7 +2576,9 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
 	intel_connector = &intel_sdvo_connector->base;
 	connector = &intel_connector->base;
 	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
-	encoder->encoder_type = DRM_MODE_ENCODER_DAC;
+	/* delay encoder_type setting until detection */
+	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_DAC;
+	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 	connector->connector_type = DRM_MODE_CONNECTOR_VGA;
 
 	if (device == 0) {
@@ -2603,7 +2613,9 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 
 	intel_connector = &intel_sdvo_connector->base;
 	connector = &intel_connector->base;
-	encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
+	/* delay encoder_type setting until detection */
+	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_LVDS;
+	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 	connector->connector_type = DRM_MODE_CONNECTOR_LVDS;
 
 	if (device == 0) {
@@ -2984,7 +2996,8 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	/* encoder type will be decided later */
 	intel_encoder = &intel_sdvo->base;
 	intel_encoder->type = INTEL_OUTPUT_SDVO;
-	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs, 0);
+	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs,
+			 DRM_MODE_ENCODER_NONE);
 
 	/* Read the regs to test if we can talk to the device */
 	for (i = 0; i < 0x40; i++) {
-- 
1.8.4.5

Re: [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector

[Chris Wilson]

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, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector

[Egbert Eich]

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.

Re: [PATCH 2/2] drm/i915: Set up SDVO encoder type only at detect

[Chris Wilson]

On Mon, Apr 14, 2014 at 07:26:09PM +0200, Egbert Eich wrote:
> Depending on the SDVO output_flags SDVO may have multiple connectors.
> These are all initialized in intel_sdvo_output_setup(). The connector
> that is initialized later will override the encoder_type that has been
> set up by an earlier connector type initialization. Eventually the
> one that comes last wins.
> Eventually when intel_sdvo_detect() is called the active connector is
> determined.
> Delay encoder type initialization until the active connector is known
> and set it to the type that corresponds to this connector.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

Please kill the redundant encoder->encoder_type = DRM_MODE_ENCODER_NONE
as I think that will help clarify that during init we set the
intel_sdvo->type and then during the detection of the actual connected
output we assign the encoder_type.

Otherwise, lgtm.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector

[Daniel Vetter]

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
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Set up SDVO encoder type only at detect

[Daniel Vetter]

On Mon, Apr 14, 2014 at 07:26:09PM +0200, Egbert Eich wrote:
> Depending on the SDVO output_flags SDVO may have multiple connectors.
> These are all initialized in intel_sdvo_output_setup(). The connector
> that is initialized later will override the encoder_type that has been
> set up by an earlier connector type initialization. Eventually the
> one that comes last wins.
> Eventually when intel_sdvo_detect() is called the active connector is
> determined.
> Delay encoder type initialization until the active connector is known
> and set it to the type that corresponds to this connector.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

Hm, has this any effect on the code itself? I think if we want to fix this
just for optics we should add a new DRM_MODE_ENCODER_MULTI. At least I
have an sdvo here which has a dac, hdmi and tv-out ...

This also reminds that I should finally polish of the multi-sdvo fixes I
have around here - currently the last encoder detected wins on a
multi-encoder chip, which means if you have an lvds+hdmi combo and plug in
a screeen you'll never be able to use the lvds again until the hdmi is
unplugged.

Much worse if there's a tv-out detect issue ;-)

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/intel_sdvo.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index d27155a..5043f16 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -154,6 +154,9 @@ struct intel_sdvo_connector {
>  	/* Mark the type of connector */
>  	uint16_t output_flag;
>  
> +	/* store encoder type for convenience */
> +	int encoder_type;
> +
>  	enum hdmi_force_audio force_audio;
>  
>  	/* This contains all current supported TV format */
> @@ -1746,6 +1749,7 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
>  	if (response == 0)
>  		return connector_status_disconnected;
>  
> +	intel_sdvo->base.base.encoder_type = intel_sdvo_connector->encoder_type;
>  	intel_sdvo->attached_output = response;
>  
>  	intel_sdvo->has_hdmi_monitor = false;
> @@ -2489,7 +2493,9 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
>  	} else {
>  		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>  	}
> -	encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
> +	/* delay encoder_type setting until detection */
> +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TMDS;
> +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
>  	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
>  
>  	if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
> @@ -2524,7 +2530,9 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
>  
>  	intel_connector = &intel_sdvo_connector->base;
>  	connector = &intel_connector->base;
> -	encoder->encoder_type = DRM_MODE_ENCODER_TVDAC;
> +	/* delay encoder_type setting until detection */
> +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TVDAC;
> +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
>  	connector->connector_type = DRM_MODE_CONNECTOR_SVIDEO;
>  
>  	intel_sdvo->controlled_output |= type;
> @@ -2568,7 +2576,9 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
>  	intel_connector = &intel_sdvo_connector->base;
>  	connector = &intel_connector->base;
>  	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> -	encoder->encoder_type = DRM_MODE_ENCODER_DAC;
> +	/* delay encoder_type setting until detection */
> +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_DAC;
> +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
>  	connector->connector_type = DRM_MODE_CONNECTOR_VGA;
>  
>  	if (device == 0) {
> @@ -2603,7 +2613,9 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
>  
>  	intel_connector = &intel_sdvo_connector->base;
>  	connector = &intel_connector->base;
> -	encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
> +	/* delay encoder_type setting until detection */
> +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_LVDS;
> +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
>  	connector->connector_type = DRM_MODE_CONNECTOR_LVDS;
>  
>  	if (device == 0) {
> @@ -2984,7 +2996,8 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  	/* encoder type will be decided later */
>  	intel_encoder = &intel_sdvo->base;
>  	intel_encoder->type = INTEL_OUTPUT_SDVO;
> -	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs, 0);
> +	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs,
> +			 DRM_MODE_ENCODER_NONE);
>  
>  	/* Read the regs to test if we can talk to the device */
>  	for (i = 0; i < 0x40; i++) {
> -- 
> 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

[PATCH v2] drm/i915: Set up SDVO encoder type only at detect

[Egbert Eich]

Depending on the SDVO output_flags SDVO may have multiple connectors.
These are all initialized in intel_sdvo_output_setup(). The connector
that is initialized later will override the encoder_type that has been
set up by an earlier connector type initialization. Eventually the
one that comes last wins.
Eventually when intel_sdvo_detect() is called the active connector is
determined.
Delay encoder type initialization until the active connector is known
and set it to the type that corresponds to this connector.

v2: Remove explicit encoder type initialization to
    DRM_MODE_ENCODER_NONE in the SDVO connector setup functions
    as suggested by Chris Wilson <chris@chris-wilson.co.uk>.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index d27155a..f324ca1 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -154,6 +154,9 @@ struct intel_sdvo_connector {
 	/* Mark the type of connector */
 	uint16_t output_flag;
 
+	/* store encoder type for convenience */
+	int encoder_type;
+
 	enum hdmi_force_audio force_audio;
 
 	/* This contains all current supported TV format */
@@ -1746,6 +1749,7 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
 	if (response == 0)
 		return connector_status_disconnected;
 
+	intel_sdvo->base.base.encoder_type = intel_sdvo_connector->encoder_type;
 	intel_sdvo->attached_output = response;
 
 	intel_sdvo->has_hdmi_monitor = false;
@@ -2489,7 +2493,8 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 	} else {
 		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 	}
-	encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
+	/* delay encoder_type setting until detection */
+	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TMDS;
 	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
 
 	if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
@@ -2524,7 +2529,8 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 
 	intel_connector = &intel_sdvo_connector->base;
 	connector = &intel_connector->base;
-	encoder->encoder_type = DRM_MODE_ENCODER_TVDAC;
+	/* delay encoder_type setting until detection */
+	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TVDAC;
 	connector->connector_type = DRM_MODE_CONNECTOR_SVIDEO;
 
 	intel_sdvo->controlled_output |= type;
@@ -2568,7 +2574,8 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
 	intel_connector = &intel_sdvo_connector->base;
 	connector = &intel_connector->base;
 	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
-	encoder->encoder_type = DRM_MODE_ENCODER_DAC;
+	/* delay encoder_type setting until detection */
+	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_DAC;
 	connector->connector_type = DRM_MODE_CONNECTOR_VGA;
 
 	if (device == 0) {
@@ -2603,7 +2610,8 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 
 	intel_connector = &intel_sdvo_connector->base;
 	connector = &intel_connector->base;
-	encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
+	/* delay encoder_type setting until detection */
+	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_LVDS;
 	connector->connector_type = DRM_MODE_CONNECTOR_LVDS;
 
 	if (device == 0) {
@@ -2981,10 +2989,11 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	if (!intel_sdvo_init_ddc_proxy(intel_sdvo, dev))
 		goto err_i2c_bus;
 
-	/* encoder type will be decided later */
+	/* encoder type will be decided in intel_sdvo_detect() */
 	intel_encoder = &intel_sdvo->base;
 	intel_encoder->type = INTEL_OUTPUT_SDVO;
-	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs, 0);
+	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs,
+			 DRM_MODE_ENCODER_NONE);
 
 	/* Read the regs to test if we can talk to the device */
 	for (i = 0; i < 0x40; i++) {
-- 
1.8.4.5

Re: [PATCH v2] drm/i915: Set up SDVO encoder type only at detect

[Chris Wilson]

On Tue, Apr 15, 2014 at 09:14:13PM +0200, Egbert Eich wrote:
> Depending on the SDVO output_flags SDVO may have multiple connectors.
> These are all initialized in intel_sdvo_output_setup(). The connector
> that is initialized later will override the encoder_type that has been
> set up by an earlier connector type initialization. Eventually the
> one that comes last wins.
> Eventually when intel_sdvo_detect() is called the active connector is
> determined.
> Delay encoder type initialization until the active connector is known
> and set it to the type that corresponds to this connector.
> 
> v2: Remove explicit encoder type initialization to
>     DRM_MODE_ENCODER_NONE in the SDVO connector setup functions
>     as suggested by Chris Wilson <chris@chris-wilson.co.uk>.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Set up SDVO encoder type only at detect

[Egbert Eich]

Daniel Vetter writes:
 > On Mon, Apr 14, 2014 at 07:26:09PM +0200, Egbert Eich wrote:
 > > Depending on the SDVO output_flags SDVO may have multiple connectors.
 > > These are all initialized in intel_sdvo_output_setup(). The connector
 > > that is initialized later will override the encoder_type that has been
 > > set up by an earlier connector type initialization. Eventually the
 > > one that comes last wins.
 > > Eventually when intel_sdvo_detect() is called the active connector is
 > > determined.
 > > Delay encoder type initialization until the active connector is known
 > > and set it to the type that corresponds to this connector.
 > > 
 > > Signed-off-by: Egbert Eich <eich@suse.de>
 > 
 > Hm, has this any effect on the code itself? I think if we want to fix this
 > just for optics we should add a new DRM_MODE_ENCODER_MULTI. At least I
 > have an sdvo here which has a dac, hdmi and tv-out ...

With the present logic the last connector in the list matching a flag bit
will win the encoder type. The encoder type is presently just used for
(debug) messages, so it is cosmetic.

 > 
 > This also reminds that I should finally polish of the multi-sdvo fixes I
 > have around here - currently the last encoder detected wins on a
 > multi-encoder chip, which means if you have an lvds+hdmi combo and plug in
 > a screeen you'll never be able to use the lvds again until the hdmi is
 > unplugged.

You know, from looking at the code I was wondering already if this was 
possible at all. 
I stayed away tinkering with this - there doesn't seem to be documentation 
on the SDVO command set publically available. Moreover I'm moslty dealing 
with cash registers here - so I doubt they have all the SVIDEO connectors 
they advertise ;) But I can't tell as I don't even have physical access!

As I read the current code it seems like bad things could happen 
if more than one bit is set in intel_sdvo->attached_output: after all
tv-out should have quite different timing requirements than a TMDS.
 > 
 > Much worse if there's a tv-out detect issue ;-)

;p

Cheers,
	Egbert.
 > 
 > Cheers, Daniel
 > 
 > > ---
 > >  drivers/gpu/drm/i915/intel_sdvo.c | 23 ++++++++++++++++++-----
 > >  1 file changed, 18 insertions(+), 5 deletions(-)
 > > 
 > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
 > > index d27155a..5043f16 100644
 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
 > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
 > > @@ -154,6 +154,9 @@ struct intel_sdvo_connector {
 > >  	/* Mark the type of connector */
 > >  	uint16_t output_flag;
 > >  
 > > +	/* store encoder type for convenience */
 > > +	int encoder_type;
 > > +
 > >  	enum hdmi_force_audio force_audio;
 > >  
 > >  	/* This contains all current supported TV format */
 > > @@ -1746,6 +1749,7 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
 > >  	if (response == 0)
 > >  		return connector_status_disconnected;
 > >  
 > > +	intel_sdvo->base.base.encoder_type = intel_sdvo_connector->encoder_type;
 > >  	intel_sdvo->attached_output = response;
 > >  
 > >  	intel_sdvo->has_hdmi_monitor = false;
 > > @@ -2489,7 +2493,9 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 > >  	} else {
 > >  		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 > >  	}
 > > -	encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
 > > +	/* delay encoder_type setting until detection */
 > > +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TMDS;
 > > +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 > >  	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
 > >  
 > >  	if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
 > > @@ -2524,7 +2530,9 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 > >  
 > >  	intel_connector = &intel_sdvo_connector->base;
 > >  	connector = &intel_connector->base;
 > > -	encoder->encoder_type = DRM_MODE_ENCODER_TVDAC;
 > > +	/* delay encoder_type setting until detection */
 > > +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TVDAC;
 > > +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 > >  	connector->connector_type = DRM_MODE_CONNECTOR_SVIDEO;
 > >  
 > >  	intel_sdvo->controlled_output |= type;
 > > @@ -2568,7 +2576,9 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
 > >  	intel_connector = &intel_sdvo_connector->base;
 > >  	connector = &intel_connector->base;
 > >  	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 > > -	encoder->encoder_type = DRM_MODE_ENCODER_DAC;
 > > +	/* delay encoder_type setting until detection */
 > > +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_DAC;
 > > +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 > >  	connector->connector_type = DRM_MODE_CONNECTOR_VGA;
 > >  
 > >  	if (device == 0) {
 > > @@ -2603,7 +2613,9 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 > >  
 > >  	intel_connector = &intel_sdvo_connector->base;
 > >  	connector = &intel_connector->base;
 > > -	encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
 > > +	/* delay encoder_type setting until detection */
 > > +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_LVDS;
 > > +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 > >  	connector->connector_type = DRM_MODE_CONNECTOR_LVDS;
 > >  
 > >  	if (device == 0) {
 > > @@ -2984,7 +2996,8 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 > >  	/* encoder type will be decided later */
 > >  	intel_encoder = &intel_sdvo->base;
 > >  	intel_encoder->type = INTEL_OUTPUT_SDVO;
 > > -	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs, 0);
 > > +	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs,
 > > +			 DRM_MODE_ENCODER_NONE);
 > >  
 > >  	/* Read the regs to test if we can talk to the device */
 > >  	for (i = 0; i < 0x40; i++) {
 > > -- 
 > > 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

-- 
Egbert Eich (Res. & Dev.)                SUSE LINUX Products GmbH
SUSE Labs KMS / DRM / X Window System Development
Tel: +49 911-740 53 0                       http://www.suse.de 
-----------------------------------------------------------------
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 

Re: [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector

[Egbert Eich]

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.

Re: [PATCH 2/2] drm/i915: Set up SDVO encoder type only at detect

[Daniel Vetter]

On Wed, Apr 16, 2014 at 07:58:48AM +0200, Egbert Eich wrote:
> Daniel Vetter writes:
>  > On Mon, Apr 14, 2014 at 07:26:09PM +0200, Egbert Eich wrote:
>  > > Depending on the SDVO output_flags SDVO may have multiple connectors.
>  > > These are all initialized in intel_sdvo_output_setup(). The connector
>  > > that is initialized later will override the encoder_type that has been
>  > > set up by an earlier connector type initialization. Eventually the
>  > > one that comes last wins.
>  > > Eventually when intel_sdvo_detect() is called the active connector is
>  > > determined.
>  > > Delay encoder type initialization until the active connector is known
>  > > and set it to the type that corresponds to this connector.
>  > > 
>  > > Signed-off-by: Egbert Eich <eich@suse.de>
>  > 
>  > Hm, has this any effect on the code itself? I think if we want to fix this
>  > just for optics we should add a new DRM_MODE_ENCODER_MULTI. At least I
>  > have an sdvo here which has a dac, hdmi and tv-out ...
> 
> With the present logic the last connector in the list matching a flag bit
> will win the encoder type. The encoder type is presently just used for
> (debug) messages, so it is cosmetic.

I'm vary of changing the encoder type at runtime tbh. We use the same hack
in intel_ddi.c to switch between hdmi and dp mode, and the results aren't
pretty imo. For debug output imo adding a new "multi" encoder type is
better.

And if we need the type somehow to set up the right connector for sdvo
encoders with more than one detected output, then we need to track this
piece of state somewhere in our modeset state (either with the
connector->encoder links or with a bit of state in the pipe config
structure). Which also means we need state read-out and cross-check
support to make sure it really does what we want.

Storing such state in global structure tends to lead to subtile bugs and
prevent proper pre-compute/commit semantics for modeset changes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Break encoder->crtc link separately in intel_sanitize_crtc()

[Egbert Eich]

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;
+			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

Re: [PATCH] drm/i915: Break encoder->crtc link separately in intel_sanitize_crtc()

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v3] drm/i915: Break encoder->crtc link separately in intel_sanitize_crtc()

[Egbert Eich]

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.

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.

Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>

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.
v3: move all encoder handling to encoder loop as suggested by
Daniel Vetter.
---
 drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b57210c..7ba8bf5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11373,15 +11373,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;
@@ -11463,8 +11454,17 @@ 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->base.encoder = NULL;
 		}
+		/* multiple connectors may have the same encoder:
+		 *  handle them and 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;
+				connector->encoder->connectors_active = false;
+			}
 
 		WARN_ON(crtc->active);
 		crtc->base.enabled = false;
@@ -11546,6 +11546,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
@@ -11556,8 +11558,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

Re: [PATCH v3] drm/i915: Break encoder->crtc link separately in intel_sanitize_crtc()

[Jani Nikula]

On Fri, 25 Apr 2014, Egbert Eich <eich@suse.de> 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.
>
> 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.
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>

Pushed to -fixes, thanks for the patch and review.

BR,
Jani.


>
> 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.
> v3: move all encoder handling to encoder loop as suggested by
> Daniel Vetter.
> ---
>  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b57210c..7ba8bf5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11373,15 +11373,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;
> @@ -11463,8 +11454,17 @@ 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->base.encoder = NULL;
>  		}
> +		/* multiple connectors may have the same encoder:
> +		 *  handle them and 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;
> +				connector->encoder->connectors_active = false;
> +			}
>  
>  		WARN_ON(crtc->active);
>  		crtc->base.enabled = false;
> @@ -11546,6 +11546,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
> @@ -11556,8 +11558,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
>

-- 
Jani Nikula, Intel Open Source Technology Center