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

[Todd Previte <tprevite@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5118 bytes --]


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

-- 
Sent using Postbox:
http://www.getpostbox.com

[-- Attachment #1.2.1: Type: text/html, Size: 8394 bytes --]

[-- Attachment #1.2.2: postbox-contact.jpg --]
[-- Type: image/jpeg, Size: 1291 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx