Re: [Intel-gfx] [PATCH v3] drm/i915: Break encoder-crtc link separately in intel_sanitize_crtc()
On Fri, 25 Apr 2014, Egbert Eich e...@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.vet...@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 e...@suse.de
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
Cc: sta...@vger.kernel.org
Cc: Jani Nikula jani.nik...@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
[Intel-gfx] [PATCH v3] drm/i915: Break encoder-crtc link separately in intel_sanitize_crtc()
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.vet...@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 e...@suse.de
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
Cc: sta...@vger.kernel.org
Cc: Jani Nikula jani.nik...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
