diff mbox

[09/11] drm/i915: check connector->encoder before using it.

Message ID 1400640904-16847-10-git-send-email-airlied@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dave Airlie May 21, 2014, 2:55 a.m. UTC 
From: Dave Airlie <airlied@redhat.com>

DP MST will need connectors that aren't connected to specific
encoders, add some checks in advance to avoid oopses.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 16 +++++++++-------
 drivers/gpu/drm/i915/i915_irq.c      |  4 ++++
 drivers/gpu/drm/i915/intel_display.c | 25 ++++++++++++++-----------
 3 files changed, 27 insertions(+), 18 deletions(-)

Comments

Todd Previte June 17, 2014, 3:23 p.m. UTC | #1 
Looks good.

Reviewed-by: Todd Previte <tprevite@gmail.com>

> Dave Airlie <mailto:airlied@gmail.com>
> Tuesday, May 20, 2014 7:55 PM
> From: Dave Airlie <airlied@redhat.com>
>
> DP MST will need connectors that aren't connected to specific
> encoders, add some checks in advance to avoid oopses.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 16 +++++++++-------
> drivers/gpu/drm/i915/i915_irq.c | 4 ++++
> drivers/gpu/drm/i915/intel_display.c | 25 ++++++++++++++-----------
> 3 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1e83ae4..88e944f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2279,13 +2279,15 @@ static void intel_connector_info(struct 
> seq_file *m,
> seq_printf(m, "\tCEA rev: %d\n",
> connector->display_info.cea_rev);
> }
> - if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> - intel_encoder->type == INTEL_OUTPUT_EDP)
> - intel_dp_info(m, intel_connector);
> - else if (intel_encoder->type == INTEL_OUTPUT_HDMI)
> - intel_hdmi_info(m, intel_connector);
> - else if (intel_encoder->type == INTEL_OUTPUT_LVDS)
> - intel_lvds_info(m, intel_connector);
> + if (intel_encoder) {
> + if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> + intel_encoder->type == INTEL_OUTPUT_EDP)
> + intel_dp_info(m, intel_connector);
> + else if (intel_encoder->type == INTEL_OUTPUT_HDMI)
> + intel_hdmi_info(m, intel_connector);
> + else if (intel_encoder->type == INTEL_OUTPUT_LVDS)
> + intel_lvds_info(m, intel_connector);
> + }
>
> seq_printf(m, "\tmodes:\n");
> list_for_each_entry(mode, &connector->modes, head)
> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> b/drivers/gpu/drm/i915/i915_irq.c
> index afa5519..5852dee 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1016,6 +1016,8 @@ static void i915_hotplug_work_func(struct 
> work_struct *work)
> dev_priv->hpd_event_bits = 0;
> list_for_each_entry(connector, &mode_config->connector_list, head) {
> intel_connector = to_intel_connector(connector);
> + if (!intel_connector->encoder)
> + continue;
> intel_encoder = intel_connector->encoder;
> if (intel_encoder->hpd_pin > HPD_NONE &&
> dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == 
> HPD_MARK_DISABLED &&
> @@ -1046,6 +1048,8 @@ static void i915_hotplug_work_func(struct 
> work_struct *work)
>
> list_for_each_entry(connector, &mode_config->connector_list, head) {
> intel_connector = to_intel_connector(connector);
> + if (!intel_connector->encoder)
> + continue;
> intel_encoder = intel_connector->encoder;
> if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> if (intel_encoder->hot_plug)
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b39d036..75b2aaf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4600,20 +4600,23 @@ static void intel_connector_check_state(struct 
> intel_connector *connector)
> "wrong connector dpms state\n");
> WARN(connector->base.encoder != &encoder->base,
> "active connector not linked to encoder\n");
> - WARN(!encoder->connectors_active,
> - "encoder->connectors_active not set\n");
>
> - encoder_enabled = encoder->get_hw_state(encoder, &pipe);
> - WARN(!encoder_enabled, "encoder not enabled\n");
> - if (WARN_ON(!encoder->base.crtc))
> - return;
> + if (encoder) {
> + WARN(!encoder->connectors_active,
> + "encoder->connectors_active not set\n");
> +
> + encoder_enabled = encoder->get_hw_state(encoder, &pipe);
> + WARN(!encoder_enabled, "encoder not enabled\n");
> + if (WARN_ON(!encoder->base.crtc))
> + return;
>
> - crtc = encoder->base.crtc;
> + crtc = encoder->base.crtc;
>
> - WARN(!crtc->enabled, "crtc not enabled\n");
> - WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
> - WARN(pipe != to_intel_crtc(crtc)->pipe,
> - "encoder active on the wrong pipe\n");
> + WARN(!crtc->enabled, "crtc not enabled\n");
> + WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
> + WARN(pipe != to_intel_crtc(crtc)->pipe,
> + "encoder active on the wrong pipe\n");
> + }
> }
> }
>
> Dave Airlie <mailto:airlied@gmail.com>
> Tuesday, May 20, 2014 7:54 PM
> Hey,
>
> So this set is pretty close to what I think we should be merging 
> initially,
>
> Since the last set, it makes fbcon and suspend/resume work a lot better,
>
> I've also fixed a couple of bugs in -intel that make things work a lot
> better.
>
> I've bashed on this a bit using kms-flip from intel-gpu-tools, hacked
> to add 3 monitor support.
>
> It still generates a fair few i915 state checker backtraces, and some
> of them are fairly hard to work out, it might be we should just tone
> down the state checker for encoders/connectors with no actual hw backing
> them.
>
> Dave.
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1e83ae4..88e944f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2279,13 +2279,15 @@  static void intel_connector_info(struct seq_file *m,
 		seq_printf(m, "\tCEA rev: %d\n",
 			   connector->display_info.cea_rev);
 	}
-	if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
-	    intel_encoder->type == INTEL_OUTPUT_EDP)
-		intel_dp_info(m, intel_connector);
-	else if (intel_encoder->type == INTEL_OUTPUT_HDMI)
-		intel_hdmi_info(m, intel_connector);
-	else if (intel_encoder->type == INTEL_OUTPUT_LVDS)
-		intel_lvds_info(m, intel_connector);
+	if (intel_encoder) {
+		if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+		    intel_encoder->type == INTEL_OUTPUT_EDP)
+			intel_dp_info(m, intel_connector);
+		else if (intel_encoder->type == INTEL_OUTPUT_HDMI)
+			intel_hdmi_info(m, intel_connector);
+		else if (intel_encoder->type == INTEL_OUTPUT_LVDS)
+			intel_lvds_info(m, intel_connector);
+	}
 
 	seq_printf(m, "\tmodes:\n");
 	list_for_each_entry(mode, &connector->modes, head)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index afa5519..5852dee 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1016,6 +1016,8 @@  static void i915_hotplug_work_func(struct work_struct *work)
 	dev_priv->hpd_event_bits = 0;
 	list_for_each_entry(connector, &mode_config->connector_list, head) {
 		intel_connector = to_intel_connector(connector);
+		if (!intel_connector->encoder)
+			continue;
 		intel_encoder = intel_connector->encoder;
 		if (intel_encoder->hpd_pin > HPD_NONE &&
 		    dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_MARK_DISABLED &&
@@ -1046,6 +1048,8 @@  static void i915_hotplug_work_func(struct work_struct *work)
 
 	list_for_each_entry(connector, &mode_config->connector_list, head) {
 		intel_connector = to_intel_connector(connector);
+		if (!intel_connector->encoder)
+			continue;
 		intel_encoder = intel_connector->encoder;
 		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
 			if (intel_encoder->hot_plug)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b39d036..75b2aaf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4600,20 +4600,23 @@  static void intel_connector_check_state(struct intel_connector *connector)
 		     "wrong connector dpms state\n");
 		WARN(connector->base.encoder != &encoder->base,
 		     "active connector not linked to encoder\n");
-		WARN(!encoder->connectors_active,
-		     "encoder->connectors_active not set\n");
 
-		encoder_enabled = encoder->get_hw_state(encoder, &pipe);
-		WARN(!encoder_enabled, "encoder not enabled\n");
-		if (WARN_ON(!encoder->base.crtc))
-			return;
+		if (encoder) {
+			WARN(!encoder->connectors_active,
+			     "encoder->connectors_active not set\n");
+
+			encoder_enabled = encoder->get_hw_state(encoder, &pipe);
+			WARN(!encoder_enabled, "encoder not enabled\n");
+			if (WARN_ON(!encoder->base.crtc))
+				return;
 
-		crtc = encoder->base.crtc;
+			crtc = encoder->base.crtc;
 
-		WARN(!crtc->enabled, "crtc not enabled\n");
-		WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
-		WARN(pipe != to_intel_crtc(crtc)->pipe,
-		     "encoder active on the wrong pipe\n");
+			WARN(!crtc->enabled, "crtc not enabled\n");
+			WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
+			WARN(pipe != to_intel_crtc(crtc)->pipe,
+			     "encoder active on the wrong pipe\n");
+		}
 	}
 }