DRM_CONNECTOR_POLL_CONNECT only enables polling for connections, not disconnections. Because of this, we end up losing hotplug polling for analog connectors once they get connected.
Easy way to reproduce: - Grab a machine with a radeon GPU and a VGA port - Plug a monitor into the VGA port, wait for it to update the connector from disconnected to connected - Disconnect the monitor on VGA, a hotplug event is never sent for the removal of the connector.
Originally, only using DRM_CONNECTOR_POLL_CONNECT might have been a good idea since doing VGA polling can sometimes result in having to mess with the DAC voltages to figure out whether or not there's actually something there since VGA doesn't have HPD. Doing this would have the potential of showing visible artifacts on the screen every time we ran a poll while a VGA display was connected. Luckily, radeon_vga_detect() only resorts to this sort of polling if the poll is forced, and DRM's polling helper doesn't force it's polls.
Additionally, this removes some assignments to connector->polled that weren't actually doing anything.
Cc: stable@vger.kernel.org Signed-off-by: Lyude cpaul@redhat.com --- drivers/gpu/drm/radeon/radeon_connectors.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index 81a63d7..b79f3b0 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -2064,7 +2064,6 @@ radeon_add_atom_connector(struct drm_device *dev, RADEON_OUTPUT_CSC_BYPASS); /* no HPD on analog connectors */ radeon_connector->hpd.hpd = RADEON_HPD_NONE; - connector->polled = DRM_CONNECTOR_POLL_CONNECT; connector->interlace_allowed = true; connector->doublescan_allowed = true; break; @@ -2314,8 +2313,10 @@ radeon_add_atom_connector(struct drm_device *dev, }
if (radeon_connector->hpd.hpd == RADEON_HPD_NONE) { - if (i2c_bus->valid) - connector->polled = DRM_CONNECTOR_POLL_CONNECT; + if (i2c_bus->valid) { + connector->polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_DISCONNECT; + } } else connector->polled = DRM_CONNECTOR_POLL_HPD;
@@ -2391,7 +2392,6 @@ radeon_add_legacy_connector(struct drm_device *dev, 1); /* no HPD on analog connectors */ radeon_connector->hpd.hpd = RADEON_HPD_NONE; - connector->polled = DRM_CONNECTOR_POLL_CONNECT; connector->interlace_allowed = true; connector->doublescan_allowed = true; break; @@ -2476,10 +2476,13 @@ radeon_add_legacy_connector(struct drm_device *dev, }
if (radeon_connector->hpd.hpd == RADEON_HPD_NONE) { - if (i2c_bus->valid) - connector->polled = DRM_CONNECTOR_POLL_CONNECT; + if (i2c_bus->valid) { + connector->polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_DISCONNECT; + } } else connector->polled = DRM_CONNECTOR_POLL_HPD; + connector->display_info.subpixel_order = subpixel_order; drm_connector_register(connector); }
DRM_CONNECTOR_POLL_CONNECT only enables polling for connections, not disconnections. Because of this, we end up losing hotplug polling for analog connectors once they get connected.
Easy way to reproduce: - Grab a machine with an AMD GPU and a VGA port - Plug a monitor into the VGA port, wait for it to update the connector from disconnected to connected - Disconnect the monitor on VGA, a hotplug event is never sent for the removal of the connector.
Originally, only using DRM_CONNECTOR_POLL_CONNECT might have been a good idea since doing VGA polling can sometimes result in having to mess with the DAC voltages to figure out whether or not there's actually something there since VGA doesn't have HPD. Doing this would have the potential of showing visible artifacts on the screen every time we ran a poll while a VGA display was connected. Luckily, amdgpu_vga_detect() only resorts to this sort of polling if the poll is forced, and DRM's polling helper doesn't force it's polls.
Additionally, this removes some assignments to connector->polled that weren't actually doing anything.
Cc: stable@vger.kernel.org Signed-off-by: Lyude cpaul@redhat.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index cb07da4..ff0b55a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -1690,7 +1690,6 @@ amdgpu_connector_add(struct amdgpu_device *adev, DRM_MODE_SCALE_NONE); /* no HPD on analog connectors */ amdgpu_connector->hpd.hpd = AMDGPU_HPD_NONE; - connector->polled = DRM_CONNECTOR_POLL_CONNECT; connector->interlace_allowed = true; connector->doublescan_allowed = true; break; @@ -1893,8 +1892,10 @@ amdgpu_connector_add(struct amdgpu_device *adev, }
if (amdgpu_connector->hpd.hpd == AMDGPU_HPD_NONE) { - if (i2c_bus->valid) - connector->polled = DRM_CONNECTOR_POLL_CONNECT; + if (i2c_bus->valid) { + connector->polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_DISCONNECT; + } } else connector->polled = DRM_CONNECTOR_POLL_HPD;
Latest version of:
https://lists.freedesktop.org/archives/intel-gfx/2016-June/098787.html
The only patch that's changed here is 4/4, the rest are just being sent so that they can be in one thread to make things easier for reviewers
Lyude (4): drm/i915/vlv: Make intel_crt_reset() per-encoder drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init() drm/i915/vlv: Disable HPD in valleyview_crt_detect_hotplug() drm/i915: Enable polling when we don't have hpd
drivers/gpu/drm/i915/i915_drv.c | 3 + drivers/gpu/drm/i915/i915_drv.h | 5 ++ drivers/gpu/drm/i915/intel_crt.c | 28 ++++++-- drivers/gpu/drm/i915/intel_drv.h | 4 +- drivers/gpu/drm/i915/intel_hotplug.c | 117 ++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++ 6 files changed, 148 insertions(+), 18 deletions(-)
This lets call intel_crt_reset() in contexts where IRQs are disabled and as such, can't hold the locks required to work with the connectors.
Cc: stable@vger.kernel.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude cpaul@redhat.com --- drivers/gpu/drm/i915/intel_crt.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index e115bcc..dfcd718 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -713,11 +713,11 @@ static int intel_crt_set_property(struct drm_connector *connector, return 0; }
-static void intel_crt_reset(struct drm_connector *connector) +static void intel_crt_reset(struct drm_encoder *encoder) { - struct drm_device *dev = connector->dev; + struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crt *crt = intel_attached_crt(connector); + struct intel_crt *crt = intel_encoder_to_crt(to_intel_encoder(encoder));
if (INTEL_INFO(dev)->gen >= 5) { u32 adpa; @@ -739,7 +739,6 @@ static void intel_crt_reset(struct drm_connector *connector) */
static const struct drm_connector_funcs intel_crt_connector_funcs = { - .reset = intel_crt_reset, .dpms = drm_atomic_helper_connector_dpms, .detect = intel_crt_detect, .fill_modes = drm_helper_probe_single_connector_modes, @@ -757,6 +756,7 @@ static const struct drm_connector_helper_funcs intel_crt_connector_helper_funcs };
static const struct drm_encoder_funcs intel_crt_enc_funcs = { + .reset = intel_crt_reset, .destroy = intel_encoder_destroy, };
@@ -901,5 +901,5 @@ void intel_crt_init(struct drm_device *dev) dev_priv->fdi_rx_config = I915_READ(FDI_RX_CTL(PIPE_A)) & fdi_config; }
- intel_crt_reset(connector); + intel_crt_reset(&crt->base.base); }
While VGA hotplugging worked(ish) before, it looks like that was mainly because we'd unintentionally enable it in valleyview_crt_detect_hotplug() when we did a force trigger. This doesn't work reliably enough because whenever the display powerwell on vlv gets disabled, the values set in VLV_ADPA get cleared and consequently VGA hotplugging gets disabled. This causes bugs such as one we found on an Intel NUC, where doing the following sequence of hotplugs:
- Disconnect all monitors - Connect VGA - Disconnect VGA - Connect HDMI
Would result in VGA hotplugging becoming disabled, due to the powerwells getting toggled in the process of connecting HDMI.
Changes since v3: - Expose intel_crt_reset() through intel_drv.h and call that in vlv_display_power_well_init() instead of encoder->base.funcs->reset(&encoder->base);
Changes since v2: - Use intel_encoder structs instead of drm_encoder structs
Changes since v1: - Instead of handling the register writes ourself, we just reuse intel_crt_detect() - Instead of resetting the ADPA during display IRQ installation, we now reset them in vlv_display_power_well_init()
Cc: stable@vger.kernel.org Acked-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Lyude cpaul@redhat.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/intel_crt.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 7 +++++++ 3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index dfcd718..220be7a 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -713,7 +713,7 @@ static int intel_crt_set_property(struct drm_connector *connector, return 0; }
-static void intel_crt_reset(struct drm_encoder *encoder) +void intel_crt_reset(struct drm_encoder *encoder) { struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 496c962..dcbfdde 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1075,7 +1075,7 @@ void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
/* intel_crt.c */ void intel_crt_init(struct drm_device *dev); - +void intel_crt_reset(struct drm_encoder *encoder);
/* intel_ddi.c */ void intel_ddi_clk_select(struct intel_encoder *encoder, diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index e856d49..f88ef76 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -1065,6 +1065,7 @@ static void vlv_init_display_clock_gating(struct drm_i915_private *dev_priv)
static void vlv_display_power_well_init(struct drm_i915_private *dev_priv) { + struct intel_encoder *encoder; enum pipe pipe;
/* @@ -1100,6 +1101,12 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)
intel_hpd_init(dev_priv);
+ /* Re-enable the ADPA, if we have one */ + for_each_intel_encoder(dev_priv->dev, encoder) { + if (encoder->type == INTEL_OUTPUT_ANALOG) + intel_crt_reset(&encoder->base); + } + i915_redisable_vga_power_on(dev_priv->dev); }
Whoops, very sorry about this! I ran git send-email, and it looks like I had forgotten to remove some other patches in my patches/ folder. Going to resend this to avoid confusing anyone trying to review this.
On Fri, 2016-06-24 at 17:45 -0400, Lyude wrote:
DRM_CONNECTOR_POLL_CONNECT only enables polling for connections, not disconnections. Because of this, we end up losing hotplug polling for analog connectors once they get connected.
Easy way to reproduce: - Grab a machine with a radeon GPU and a VGA port - Plug a monitor into the VGA port, wait for it to update the connector from disconnected to connected - Disconnect the monitor on VGA, a hotplug event is never sent for the removal of the connector.
Originally, only using DRM_CONNECTOR_POLL_CONNECT might have been a good idea since doing VGA polling can sometimes result in having to mess with the DAC voltages to figure out whether or not there's actually something there since VGA doesn't have HPD. Doing this would have the potential of showing visible artifacts on the screen every time we ran a poll while a VGA display was connected. Luckily, radeon_vga_detect() only resorts to this sort of polling if the poll is forced, and DRM's polling helper doesn't force it's polls.
Additionally, this removes some assignments to connector->polled that weren't actually doing anything.
Cc: stable@vger.kernel.org Signed-off-by: Lyude cpaul@redhat.com
drivers/gpu/drm/radeon/radeon_connectors.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index 81a63d7..b79f3b0 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -2064,7 +2064,6 @@ radeon_add_atom_connector(struct drm_device *dev, RADEON_OU TPUT_CSC_BYPASS); /* no HPD on analog connectors */ radeon_connector->hpd.hpd = RADEON_HPD_NONE;
connector->polled =DRM_CONNECTOR_POLL_CONNECT; connector->interlace_allowed = true; connector->doublescan_allowed = true; break; @@ -2314,8 +2313,10 @@ radeon_add_atom_connector(struct drm_device *dev, } if (radeon_connector->hpd.hpd == RADEON_HPD_NONE) {
if (i2c_bus->valid)connector->polled =DRM_CONNECTOR_POLL_CONNECT;
if (i2c_bus->valid) {connector->polled =DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT;
}} else connector->polled = DRM_CONNECTOR_POLL_HPD; @@ -2391,7 +2392,6 @@ radeon_add_legacy_connector(struct drm_device *dev, 1); /* no HPD on analog connectors */ radeon_connector->hpd.hpd = RADEON_HPD_NONE;
connector->polled = DRM_CONNECTOR_POLL_CONNECT;connector->interlace_allowed = true; connector->doublescan_allowed = true; break; @@ -2476,10 +2476,13 @@ radeon_add_legacy_connector(struct drm_device *dev, } if (radeon_connector->hpd.hpd == RADEON_HPD_NONE) {
if (i2c_bus->valid)connector->polled =DRM_CONNECTOR_POLL_CONNECT;
if (i2c_bus->valid) {connector->polled =DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT;
}} else connector->polled = DRM_CONNECTOR_POLL_HPD;
connector->display_info.subpixel_order = subpixel_order; drm_connector_register(connector); }
dri-devel@lists.freedesktop.org
-
Lyude