| Message ID | 20180112210450.17350-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 12 Jan 2018, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Allow encoders to customize their hotplug processing by moving the > intel_hpd_irq_event() code into an encoder hotplug vfunc. Currently > only SDVO needs this to re-enable hotplug signalling in the SDVO > chip. We'll use this same hook for DP/HDMI link management later. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Nice! Reviewed-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_crt.c | 4 +++- > drivers/gpu/drm/i915/intel_ddi.c | 1 + > drivers/gpu/drm/i915/intel_dp.c | 1 + > drivers/gpu/drm/i915/intel_drv.h | 6 ++++-- > drivers/gpu/drm/i915/intel_hdmi.c | 1 + > drivers/gpu/drm/i915/intel_hotplug.c | 24 ++++++++++++------------ > drivers/gpu/drm/i915/intel_sdvo.c | 12 ++++++++++-- > 7 files changed, 32 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > index 9f31aea51dff..9bc47cff5409 100644 > --- a/drivers/gpu/drm/i915/intel_crt.c > +++ b/drivers/gpu/drm/i915/intel_crt.c > @@ -966,8 +966,10 @@ void intel_crt_init(struct drm_i915_private *dev_priv) > crt->base.power_domain = POWER_DOMAIN_PORT_CRT; > > if (I915_HAS_HOTPLUG(dev_priv) && > - !dmi_check_system(intel_spurious_crt_detect)) > + !dmi_check_system(intel_spurious_crt_detect)) { > crt->base.hpd_pin = HPD_CRT; > + crt->base.hotplug = intel_encoder_hotplug; > + } > > if (HAS_DDI(dev_priv)) { > crt->base.port = PORT_E; > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 6260a882fbe4..1aeae3e97013 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2866,6 +2866,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > drm_encoder_init(&dev_priv->drm, encoder, &intel_ddi_funcs, > DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port)); > > + intel_encoder->hotplug = intel_encoder_hotplug; > intel_encoder->compute_output_type = intel_ddi_compute_output_type; > intel_encoder->compute_config = intel_ddi_compute_config; > intel_encoder->enable = intel_enable_ddi; > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 68229f53d5b8..6bbf14410c2a 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -6400,6 +6400,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv, > "DP %c", port_name(port))) > goto err_encoder_init; > > + intel_encoder->hotplug = intel_encoder_hotplug; > intel_encoder->compute_config = intel_dp_compute_config; > intel_encoder->get_hw_state = intel_dp_get_hw_state; > intel_encoder->get_config = intel_dp_get_config; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 731dc36d7129..7537b2d542fd 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -214,7 +214,8 @@ struct intel_encoder { > enum intel_output_type type; > enum port port; > unsigned int cloneable; > - void (*hot_plug)(struct intel_encoder *); > + bool (*hotplug)(struct intel_encoder *encoder, > + struct intel_connector *connector); > enum intel_output_type (*compute_output_type)(struct intel_encoder *, > struct intel_crtc_state *, > struct drm_connector_state *); > @@ -1690,7 +1691,8 @@ int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector); > void intel_dvo_init(struct drm_i915_private *dev_priv); > /* intel_hotplug.c */ > void intel_hpd_poll_init(struct drm_i915_private *dev_priv); > - > +bool intel_encoder_hotplug(struct intel_encoder *encoder, > + struct intel_connector *connector); > > /* legacy fbdev emulation in intel_fbdev.c */ > #ifdef CONFIG_DRM_FBDEV_EMULATION > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 691f15b59124..4a93cfd7a28e 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -2348,6 +2348,7 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv, > &intel_hdmi_enc_funcs, DRM_MODE_ENCODER_TMDS, > "HDMI %c", port_name(port)); > > + intel_encoder->hotplug = intel_encoder_hotplug; > intel_encoder->compute_config = intel_hdmi_compute_config; > if (HAS_PCH_SPLIT(dev_priv)) { > intel_encoder->disable = pch_disable_hdmi; > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c > index 875d5d218d5c..0191c7831a06 100644 > --- a/drivers/gpu/drm/i915/intel_hotplug.c > +++ b/drivers/gpu/drm/i915/intel_hotplug.c > @@ -263,24 +263,25 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) > intel_runtime_pm_put(dev_priv); > } > > -static bool intel_hpd_irq_event(struct drm_device *dev, > - struct drm_connector *connector) > +bool intel_encoder_hotplug(struct intel_encoder *encoder, > + struct intel_connector *connector) > { > + struct drm_device *dev = connector->base.dev; > enum drm_connector_status old_status; > > WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); > - old_status = connector->status; > + old_status = connector->base.status; > > - connector->status = drm_helper_probe_detect(connector, NULL, false); > + connector->base.status = drm_helper_probe_detect(&connector->base, NULL, false); > > - if (old_status == connector->status) > + if (old_status == connector->base.status) > return false; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", > - connector->base.id, > - connector->name, > + connector->base.base.id, > + connector->base.name, > drm_get_connector_status_name(old_status), > - drm_get_connector_status_name(connector->status)); > + drm_get_connector_status_name(connector->base.status)); > > return true; > } > @@ -370,10 +371,9 @@ static void i915_hotplug_work_func(struct work_struct *work) > if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { > DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n", > connector->name, intel_encoder->hpd_pin); > - if (intel_encoder->hot_plug) > - intel_encoder->hot_plug(intel_encoder); > - if (intel_hpd_irq_event(dev, connector)) > - changed = true; > + > + changed |= intel_encoder->hotplug(intel_encoder, > + intel_connector); > } > } > drm_connector_list_iter_end(&conn_iter); > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index 2b8764897d68..5b1ad42ec446 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -1692,7 +1692,15 @@ static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder) > struct intel_sdvo *intel_sdvo = to_sdvo(encoder); > > intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_SET_ACTIVE_HOT_PLUG, > - &intel_sdvo->hotplug_active, 2); > + &intel_sdvo->hotplug_active, 2); > +} > + > +static bool intel_sdvo_hotplug(struct intel_encoder *encoder, > + struct intel_connector *connector) > +{ > + intel_sdvo_enable_hotplug(encoder); > + > + return intel_encoder_hotplug(encoder, connector); > } > > static bool > @@ -2496,7 +2504,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) > /* Some SDVO devices have one-shot hotplug interrupts. > * Ensure that they get re-enabled when an interrupt happens. > */ > - intel_encoder->hot_plug = intel_sdvo_enable_hotplug; > + intel_encoder->hotplug = intel_sdvo_hotplug; > intel_sdvo_enable_hotplug(intel_encoder); > } else { > intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
On Mon, Jan 15, 2018 at 11:51:19AM -0000, Patchwork wrote: > == Series Details == > > Series: series starting with [1/3] drm/i915: Convert intel_hpd_irq_event() into an encoder hotplug hook > URL : https://patchwork.freedesktop.org/series/36431/ > State : failure > > == Summary == > > Series 36431v1 series starting with [1/3] drm/i915: Convert intel_hpd_irq_event() into an encoder hotplug hook > https://patchwork.freedesktop.org/api/1.0/series/36431/revisions/1/mbox/ > > Test debugfs_test: > Subgroup read_all_entries: > incomplete -> PASS (fi-snb-2520m) fdo#103713 > pass -> DMESG-WARN (fi-bdw-gvtdvm) fdo#103938 +1 > Test kms_cursor_legacy: > Subgroup basic-busy-flip-before-cursor-atomic: > pass -> FAIL (fi-skl-6770hq) > Subgroup basic-busy-flip-before-cursor-legacy: > pass -> FAIL (fi-skl-6770hq) > Subgroup basic-flip-after-cursor-atomic: > pass -> FAIL (fi-skl-6770hq) > Subgroup basic-flip-after-cursor-legacy: > pass -> FAIL (fi-skl-6770hq) > Subgroup basic-flip-after-cursor-varying-size: > pass -> FAIL (fi-skl-6770hq) > Subgroup basic-flip-before-cursor-atomic: > pass -> FAIL (fi-skl-6770hq) > Subgroup basic-flip-before-cursor-legacy: > pass -> FAIL (fi-skl-6770hq) > Subgroup basic-flip-before-cursor-varying-size: > pass -> FAIL (fi-skl-6770hq) This is LSPCON being silly. It throws a short HPD during the enable sequence before link training. The short hpd handler then thinks the link has fallen over and schedules the hotplug work to retrain the link. The hotplug work will wait for the modeset to finish and won't actually retrain the link needlessly though. But all this is apparently sufficient amount of extra work to throw the test off. Not quite sure what to do about this. The whole point here was to make the short hpd handler not take the modeset locks, so we'd need some other way to deal with the concurrent modeset. I guess I could just add some kind of 'bool link_trained;' type of thing to intel_dp to go alongside the link params we already store there. But I've been hoping we could eliminate this extra state being tracked in intel_dp. But I can immediately think of anything better that would avoid the modeset locks in the short pulse handler. > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-a: > pass -> DMESG-WARN (fi-kbl-r) fdo#104172 > > fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713 > fdo#103938 https://bugs.freedesktop.org/show_bug.cgi?id=103938 > fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172 > > fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:419s > fi-bdw-gvtdvm total:288 pass:262 dwarn:2 dfail:0 fail:0 skip:24 time:425s > fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:372s > fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:490s > fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:279s > fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:485s > fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:470s > fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:457s > fi-cnl-y2 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:528s > fi-elk-e7500 total:224 pass:168 dwarn:10 dfail:0 fail:0 skip:45 > fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:277s > fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:512s > fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:400s > fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:462s > fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:410s > fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:469s > fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:500s > fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:456s > fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:509s > fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:576s > fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:429s > fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:516s > fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:527s > fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:491s > fi-skl-6770hq total:288 pass:260 dwarn:0 dfail:0 fail:8 skip:20 time:488s > fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:536s > fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:399s > Blacklisted hosts: > fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:574s > fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:469s > > 254125b984264731491e1eafbe58bc50e84a032d drm-tip: 2018y-01m-15d-09h-31m-31s UTC integration manifest > 40530efe1001 drm/i915: Move SST DP link retraining into the ->post_hotplug() hook > 54738b5b5101 drm/i915: Reinitialize sink scrambling/TMDS clock ratio on HPD > 523afd823a2c drm/i915: Convert intel_hpd_irq_event() into an encoder hotplug hook > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7669/issues.html
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 9f31aea51dff..9bc47cff5409 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -966,8 +966,10 @@ void intel_crt_init(struct drm_i915_private *dev_priv) crt->base.power_domain = POWER_DOMAIN_PORT_CRT; if (I915_HAS_HOTPLUG(dev_priv) && - !dmi_check_system(intel_spurious_crt_detect)) + !dmi_check_system(intel_spurious_crt_detect)) { crt->base.hpd_pin = HPD_CRT; + crt->base.hotplug = intel_encoder_hotplug; + } if (HAS_DDI(dev_priv)) { crt->base.port = PORT_E; diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 6260a882fbe4..1aeae3e97013 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2866,6 +2866,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) drm_encoder_init(&dev_priv->drm, encoder, &intel_ddi_funcs, DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port)); + intel_encoder->hotplug = intel_encoder_hotplug; intel_encoder->compute_output_type = intel_ddi_compute_output_type; intel_encoder->compute_config = intel_ddi_compute_config; intel_encoder->enable = intel_enable_ddi; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 68229f53d5b8..6bbf14410c2a 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -6400,6 +6400,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv, "DP %c", port_name(port))) goto err_encoder_init; + intel_encoder->hotplug = intel_encoder_hotplug; intel_encoder->compute_config = intel_dp_compute_config; intel_encoder->get_hw_state = intel_dp_get_hw_state; intel_encoder->get_config = intel_dp_get_config; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 731dc36d7129..7537b2d542fd 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -214,7 +214,8 @@ struct intel_encoder { enum intel_output_type type; enum port port; unsigned int cloneable; - void (*hot_plug)(struct intel_encoder *); + bool (*hotplug)(struct intel_encoder *encoder, + struct intel_connector *connector); enum intel_output_type (*compute_output_type)(struct intel_encoder *, struct intel_crtc_state *, struct drm_connector_state *); @@ -1690,7 +1691,8 @@ int intel_dsi_dcs_init_backlight_funcs(struct intel_connector *intel_connector); void intel_dvo_init(struct drm_i915_private *dev_priv); /* intel_hotplug.c */ void intel_hpd_poll_init(struct drm_i915_private *dev_priv); - +bool intel_encoder_hotplug(struct intel_encoder *encoder, + struct intel_connector *connector); /* legacy fbdev emulation in intel_fbdev.c */ #ifdef CONFIG_DRM_FBDEV_EMULATION diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 691f15b59124..4a93cfd7a28e 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -2348,6 +2348,7 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv, &intel_hdmi_enc_funcs, DRM_MODE_ENCODER_TMDS, "HDMI %c", port_name(port)); + intel_encoder->hotplug = intel_encoder_hotplug; intel_encoder->compute_config = intel_hdmi_compute_config; if (HAS_PCH_SPLIT(dev_priv)) { intel_encoder->disable = pch_disable_hdmi; diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c index 875d5d218d5c..0191c7831a06 100644 --- a/drivers/gpu/drm/i915/intel_hotplug.c +++ b/drivers/gpu/drm/i915/intel_hotplug.c @@ -263,24 +263,25 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work) intel_runtime_pm_put(dev_priv); } -static bool intel_hpd_irq_event(struct drm_device *dev, - struct drm_connector *connector) +bool intel_encoder_hotplug(struct intel_encoder *encoder, + struct intel_connector *connector) { + struct drm_device *dev = connector->base.dev; enum drm_connector_status old_status; WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); - old_status = connector->status; + old_status = connector->base.status; - connector->status = drm_helper_probe_detect(connector, NULL, false); + connector->base.status = drm_helper_probe_detect(&connector->base, NULL, false); - if (old_status == connector->status) + if (old_status == connector->base.status) return false; DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", - connector->base.id, - connector->name, + connector->base.base.id, + connector->base.name, drm_get_connector_status_name(old_status), - drm_get_connector_status_name(connector->status)); + drm_get_connector_status_name(connector->base.status)); return true; } @@ -370,10 +371,9 @@ static void i915_hotplug_work_func(struct work_struct *work) if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n", connector->name, intel_encoder->hpd_pin); - if (intel_encoder->hot_plug) - intel_encoder->hot_plug(intel_encoder); - if (intel_hpd_irq_event(dev, connector)) - changed = true; + + changed |= intel_encoder->hotplug(intel_encoder, + intel_connector); } } drm_connector_list_iter_end(&conn_iter); diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 2b8764897d68..5b1ad42ec446 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1692,7 +1692,15 @@ static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder) struct intel_sdvo *intel_sdvo = to_sdvo(encoder); intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_SET_ACTIVE_HOT_PLUG, - &intel_sdvo->hotplug_active, 2); + &intel_sdvo->hotplug_active, 2); +} + +static bool intel_sdvo_hotplug(struct intel_encoder *encoder, + struct intel_connector *connector) +{ + intel_sdvo_enable_hotplug(encoder); + + return intel_encoder_hotplug(encoder, connector); } static bool @@ -2496,7 +2504,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) /* Some SDVO devices have one-shot hotplug interrupts. * Ensure that they get re-enabled when an interrupt happens. */ - intel_encoder->hot_plug = intel_sdvo_enable_hotplug; + intel_encoder->hotplug = intel_sdvo_hotplug; intel_sdvo_enable_hotplug(intel_encoder); } else { intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;