* [PATCH 2/2] drm/amdgpu: Poll for both connect/disconnect on analog connectors
2016-06-24 21:45 [PATCH 1/2] drm/radeon: Poll for both connect/disconnect on analog connectors Lyude
@ 2016-06-24 21:45 ` Lyude
2016-06-24 21:45 ` [PATCH v6 1/4] drm/i915/vlv: Make intel_crt_reset() per-encoder Lyude
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Lyude @ 2016-06-24 21:45 UTC (permalink / raw)
To: xorg-driver-ati, amd-gfx
Cc: Lyude, stable, Alex Deucher, Christian König, David Airlie,
Mario Kleiner, Arindam Nath, Nils Wallménius,
open list:RADEON and AMDGPU DRM DRIVERS, open list
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;
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v6 1/4] drm/i915/vlv: Make intel_crt_reset() per-encoder
2016-06-24 21:45 [PATCH 1/2] drm/radeon: Poll for both connect/disconnect on analog connectors Lyude
2016-06-24 21:45 ` [PATCH 2/2] drm/amdgpu: " Lyude
@ 2016-06-24 21:45 ` Lyude
2016-06-24 21:46 ` [PATCH v6 2/4] drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init() Lyude
2016-06-24 21:52 ` [PATCH 1/2] drm/radeon: Poll for both connect/disconnect on analog connectors Lyude
3 siblings, 0 replies; 5+ messages in thread
From: Lyude @ 2016-06-24 21:45 UTC (permalink / raw)
To: xorg-driver-ati, amd-gfx
Cc: Lyude, stable, Ville Syrjälä, Daniel Vetter,
Jani Nikula, David Airlie,
open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), dri-devel@lists.freedesktop.org (open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list))
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);
}
--
2.5.5
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v6 2/4] drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()
2016-06-24 21:45 [PATCH 1/2] drm/radeon: Poll for both connect/disconnect on analog connectors Lyude
2016-06-24 21:45 ` [PATCH 2/2] drm/amdgpu: " Lyude
2016-06-24 21:45 ` [PATCH v6 1/4] drm/i915/vlv: Make intel_crt_reset() per-encoder Lyude
@ 2016-06-24 21:46 ` Lyude
2016-06-24 21:52 ` [PATCH 1/2] drm/radeon: Poll for both connect/disconnect on analog connectors Lyude
3 siblings, 0 replies; 5+ messages in thread
From: Lyude @ 2016-06-24 21:46 UTC (permalink / raw)
To: xorg-driver-ati, amd-gfx
Cc: Lyude, stable, Daniel Vetter, Jani Nikula, David Airlie,
open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), dri-devel@lists.freedesktop.org (open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list))
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);
}
--
2.5.5
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] drm/radeon: Poll for both connect/disconnect on analog connectors
2016-06-24 21:45 [PATCH 1/2] drm/radeon: Poll for both connect/disconnect on analog connectors Lyude
` (2 preceding siblings ...)
2016-06-24 21:46 ` [PATCH v6 2/4] drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init() Lyude
@ 2016-06-24 21:52 ` Lyude
3 siblings, 0 replies; 5+ messages in thread
From: Lyude @ 2016-06-24 21:52 UTC (permalink / raw)
To: xorg-driver-ati, amd-gfx
Cc: stable, Alex Deucher, Christian König, David Airlie,
open list:RADEON and AMDGPU DRM DRIVERS, open list
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_DISCO
> NNECT;
> + }
> } 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_DISCO
> NNECT;
> + }
> } else
> connector->polled = DRM_CONNECTOR_POLL_HPD;
> +
> connector->display_info.subpixel_order = subpixel_order;
> drm_connector_register(connector);
> }
--
Cheers,
Lyude
^ permalink raw reply [flat|nested] 5+ messages in thread