From mboxrd@z Thu Jan 1 00:00:00 1970 From: Todd Previte Subject: Re: [PATCH 09/11] drm/i915: check connector->encoder before using it. Date: Tue, 17 Jun 2014 08:23:31 -0700 Message-ID: <53A05D73.2000509@gmail.com> References: <1400640904-16847-1-git-send-email-airlied@gmail.com> <1400640904-16847-10-git-send-email-airlied@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0371115426==" Return-path: In-Reply-To: <1400640904-16847-10-git-send-email-airlied@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Dave Airlie Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org This is a multi-part message in MIME format. --===============0371115426== Content-Type: multipart/alternative; boundary="------------070507040306050004020105" This is a multi-part message in MIME format. --------------070507040306050004020105 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Looks good. Reviewed-by: Todd Previte > Dave Airlie > Tuesday, May 20, 2014 7:55 PM > From: Dave Airlie > > 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 > --- > 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 > 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 -- Sent using Postbox: http://www.getpostbox.com --------------070507040306050004020105 Content-Type: multipart/related; boundary="------------060604000302030901090504" --------------060604000302030901090504 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit
Looks good.

Reviewed-by: Todd Previte <tprevite@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");
+ }
}
}

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

--
Sent using Postbox:
http://www.getpostbox.com
--------------060604000302030901090504 Content-Type: image/jpeg; x-apple-mail-type=stationery; name="postbox-contact.jpg" Content-Transfer-Encoding: base64 Content-ID: Content-Disposition: inline; filename="postbox-contact.jpg" /9j/4AAQSkZJRgABAQAAAQABAAD/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgK CgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/2wBDAQMDAwQDBAgEBAgQCwkL EBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBD/wAAR CAAZABkDAREAAhEBAxEB/8QAHwAAAQUBAQEBAQEAAAAAAAAAAAECAwQFBgcICQoL/8QAtRAA AgEDAwIEAwUFBAQAAAF9AQIDAAQRBRIhMUEGE1FhByJxFDKBkaEII0KxwRVS0fAkM2JyggkK FhcYGRolJicoKSo0NTY3ODk6Q0RFRkdISUpTVFVWV1hZWmNkZWZnaGlqc3R1dnd4eXqDhIWG h4iJipKTlJWWl5iZmqKjpKWmp6ipqrKztLW2t7i5usLDxMXGx8jJytLT1NXW19jZ2uHi4+Tl 5ufo6erx8vP09fb3+Pn6/8QAHwEAAwEBAQEBAQEBAQAAAAAAAAECAwQFBgcICQoL/8QAtREA AgECBAQDBAcFBAQAAQJ3AAECAxEEBSExBhJBUQdhcRMiMoEIFEKRobHBCSMzUvAVYnLRChYk NOEl8RcYGRomJygpKjU2Nzg5OkNERUZHSElKU1RVVldYWVpjZGVmZ2hpanN0dXZ3eHl6goOE hYaHiImKkpOUlZaXmJmaoqOkpaanqKmqsrO0tba3uLm6wsPExcbHyMnK0tPU1dbX2Nna4uPk 5ebn6Onq8vP09fb3+Pn6/9oADAMBAAIRAxEAPwD4ikvNV8RaufG/jlrTXL/Wre3vtt7YwSvc O8KGSaeVkMhUybwsasvC8FFVA3DjcfDBru30PquGOFcRxHVfK+WnHeX6JdWWHv8AdxHofhaJ O0aeF9NYAem54Gf8SxPvXiPOq97pI/UYeGGVKnyynNvvdflYpx2Nxda7p11o2n6TYXUF1FMU s9Ngi8yNGDOYnCeZDKqguCr87SV2MoDetgcxji/datI/O+KuDK/DyVenLnpPS/VPsyH/AIbY /ao/6LNrX/fuD/43XpnxB9O+BP2LLr4meGzdW/xi0Lw7ruk2WnwT6ZqNqwhkhXT7bZLHOG5B O5SCg5U9eteHicNHF1FPuff5FxbVyHDfUuXRO911v3v9xoXH/BN/4mwafNqn/C6vh+LW2nSC eeS9mWGJ3UMoaQRlRnK85xyMkc4j+yPQ9r/iIf8Ai/AW1/YpuPA0Fh4v1T46+FdaubPVtPEW naJC92JSbyJG3TZVUAViehzx65ohgvqs/a9jz8043qZrhpYLlup6a2089OqPm3/hlW1/6HEf +AP/ANsr0PrnkfB/U/M9R+DHxf8Agf8AFbxKfB37TVveeDdcsrO00vT9WgnNtAJICyyw3QkR xCzFj8zKVUpg4zWiouh8GqMalb2rXMj7Ptf2ZP2VLTwLdaFZ3iXcepXNvdR6nNK0978gbaIr yEi1AIk442c5bdhSuftal7k2Vj5F/aM+KH7PPwCWPwn+z1OPE/jRr+Ce61y7uor+DRreJsm3 iliVYpZpGxv2gqqjbktyNVCVZWqbAp+zfNHc+e/+Ek/al/6FbxR/4Tj/APxqr+r0+w/bz7jv 22f+TqPiJ/2E0/8AREdbmJ5En/ILb/eNSwNn4Tf8lU8G/wDYwad/6Ux1QH77UAf/2Q== --------------060604000302030901090504-- --------------070507040306050004020105-- --===============0371115426== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0371115426==--