[PATCH] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[PATCH] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Imre Deak]

Atm on ILK we attempt to detect if eDP is present even if LVDS was
already detected and an encoder for it was registered. This involves
trying to read out the eDP EDID, which in turn needs the same power
sequencer that LVDS uses. Poking at the VDD line at an unexpected time
may or may not interfere with the LVDS panel, but it's probably safer to
prevent this.  Registering both an LVDS and an eDP connector would also
present a similar problem accessing the shared PPS at any point later in
an unexpected way.

This was caught by CI with the PPS sanity checks in place and the
initial eDP EDID readout waiting for the panel power cycle timeout
without the PPS registers being initialized. To solve this latter
problem move PPS register init earlier before the first use of the
PPS.

CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index be08351..fe543d7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5316,8 +5316,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
+	/*
+	 * On ILK we may get here with LVDS already registered. Since the
+	 * driver uses the only internal power sequencer available for both
+	 * eDP and LVDS bail out early in this case to prevent interfering
+	 * with an already powered-on LVDS power sequencer.
+	 */
+	for_each_intel_encoder(dev, intel_encoder) {
+		if (intel_encoder->type == INTEL_OUTPUT_LVDS) {
+			DRM_INFO("LVDS was detected, not registering eDP\n");
+			return false;
+		}
+	}
+
 	pps_lock(intel_dp);
 	intel_edp_panel_vdd_sanitize(intel_dp);
+	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 	pps_unlock(intel_dp);
 
 	/* Cache DPCD and EDID for edp. */
@@ -5334,11 +5348,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		return false;
 	}
 
-	/* We now know it's not a ghost, init power sequence regs. */
-	pps_lock(intel_dp);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
-	pps_unlock(intel_dp);
-
 	mutex_lock(&dev->mode_config.mutex);
 	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
 	if (edid) {
-- 
2.5.0

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

✓ Ro.CI.BAT: success for drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Patchwork]

== Series Details ==

Series: drm/i915/ilk: Don't attempt to register eDP if LVDS was detected
URL   : https://patchwork.freedesktop.org/series/8846/
State : success

== Summary ==

Series 8846v1 drm/i915/ilk: Don't attempt to register eDP if LVDS was detected
http://patchwork.freedesktop.org/api/1.0/series/8846/revisions/1/mbox


ro-hsw-i7-4770r  total:213  pass:189  dwarn:1   dfail:0   fail:0   skip:23 
ro-bdw-i5-5250u failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot
ro-bdw-i7-5600u failed to connect after reboot
ro-bsw-n3050 failed to connect after reboot
ro-byt-n2820 failed to connect after reboot
ro-hsw-i3-4010u failed to connect after reboot
ro-ilk1-i5-650 failed to connect after reboot
ro-ilk-i7-620lm failed to connect after reboot
ro-ivb2-i7-3770 failed to connect after reboot
ro-ivb-i7-3770 failed to connect after reboot
ro-skl3-i5-6260u failed to connect after reboot
ro-snb-i7-2620M failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1217/

2d92dd8 drm-intel-nightly: 2016y-06m-17d-14h-42m-29s UTC integration manifest
05e124d drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

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

Re: [PATCH] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Chris Wilson]

On Fri, Jun 17, 2016 at 06:48:45PM +0300, Imre Deak wrote:
> Atm on ILK we attempt to detect if eDP is present even if LVDS was
> already detected and an encoder for it was registered. This involves
> trying to read out the eDP EDID, which in turn needs the same power
> sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> may or may not interfere with the LVDS panel, but it's probably safer to
> prevent this.  Registering both an LVDS and an eDP connector would also
> present a similar problem accessing the shared PPS at any point later in
> an unexpected way.
> 
> This was caught by CI with the PPS sanity checks in place and the
> initial eDP EDID readout waiting for the panel power cycle timeout
> without the PPS registers being initialized. To solve this latter
> problem move PPS register init earlier before the first use of the
> PPS.

This looks like two patches. To the best of my knowledge a system cannot
have both LVDS and eDP, so that part looks sane - the only danger being
ghost LVDS. And checking for LVDS in eDP init seems like the easiest way
to pull it all together, but it does depend upon LVDS being completed
first. That's an issue in a parallel future, the dependency will have to
be tracked.

The second issue of using pps too early looks trivial... That part I can
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/2] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Imre Deak]

Atm on ILK we attempt to detect if eDP is present even if LVDS was
already detected and an encoder for it was registered. This involves
trying to read out the eDP EDID, which in turn needs the same power
sequencer that LVDS uses. Poking at the VDD line at an unexpected time
may or may not interfere with the LVDS panel, but it's probably safer to
prevent this. Registering both an LVDS and an eDP connector would also
present a similar problem accessing the shared PPS at any point later in
an unexpected way.

We also need this to be able fix PPS initialization before its first use
in the next patch. For that we want to be sure that PPS is not in use
by LVDS.

v2:
- Split out the PPS init fix to a separate patch. (Chris)
- Add comment about eDP init depending on LVDS init. (Chris)
- Make the use of the intel_encoder ptr less error prone.

CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  5 +++++
 drivers/gpu/drm/i915/intel_dp.c      | 17 +++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8c6f4e2..b6bb438 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14708,6 +14708,11 @@ static void intel_setup_outputs(struct drm_device *dev)
 	struct intel_encoder *encoder;
 	bool dpd_is_edp = false;
 
+	/*
+	 * intel_edp_init_connector() depends on this completing first, to
+	 * prevent the registeration of both eDP and LVDS and the incorrect
+	 * sharing of the PPS.
+	 */
 	intel_lvds_init(dev);
 
 	if (intel_crt_present(dev))
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index be08351..5f7d731 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5303,9 +5303,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 {
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct drm_device *dev = intel_encoder->base.dev;
+	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *intel_encoder;
 	struct drm_display_mode *fixed_mode = NULL;
 	struct drm_display_mode *downclock_mode = NULL;
 	bool has_dpcd;
@@ -5316,6 +5316,19 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
+	/*
+	 * On ILK we may get here with LVDS already registered. Since the
+	 * driver uses the only internal power sequencer available for both
+	 * eDP and LVDS bail out early in this case to prevent interfering
+	 * with an already powered-on LVDS power sequencer.
+	 */
+	for_each_intel_encoder(dev, intel_encoder) {
+		if (intel_encoder->type == INTEL_OUTPUT_LVDS) {
+			DRM_INFO("LVDS was detected, not registering eDP\n");
+			return false;
+		}
+	}
+
 	pps_lock(intel_dp);
 	intel_edp_panel_vdd_sanitize(intel_dp);
 	pps_unlock(intel_dp);
-- 
2.5.0

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

[PATCH v2 2/2] drm/i915: Initialize the PPS HW before its first use

[Imre Deak]

The initial EDID read for eDP detection involves using the PPS, but so
far we only initialized the PPS registers after the EDID read. The
reason this was done so far is to preserve a possible LVDS PPS HW setup
if LVDS is detected but eDP is not. This is not an issue any more after
the previous patch, so we can move the init earlier now.

This was caught by CI with the PPS sanity checks in place and the
initial eDP EDID readout waiting for the panel power cycle timeout
without the PPS registers being initialized.

CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5f7d731..6fdc6d1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5331,6 +5331,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 
 	pps_lock(intel_dp);
 	intel_edp_panel_vdd_sanitize(intel_dp);
+	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 	pps_unlock(intel_dp);
 
 	/* Cache DPCD and EDID for edp. */
@@ -5347,11 +5348,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		return false;
 	}
 
-	/* We now know it's not a ghost, init power sequence regs. */
-	pps_lock(intel_dp);
-	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
-	pps_unlock(intel_dp);
-
 	mutex_lock(&dev->mode_config.mutex);
 	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
 	if (edid) {
-- 
2.5.0

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

Re: [PATCH v2 1/2] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Chris Wilson]

On Sat, Jun 18, 2016 at 12:29:24AM +0300, Imre Deak wrote:
> Atm on ILK we attempt to detect if eDP is present even if LVDS was
> already detected and an encoder for it was registered. This involves
> trying to read out the eDP EDID, which in turn needs the same power
> sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> may or may not interfere with the LVDS panel, but it's probably safer to
> prevent this. Registering both an LVDS and an eDP connector would also
> present a similar problem accessing the shared PPS at any point later in
> an unexpected way.
> 
> We also need this to be able fix PPS initialization before its first use
> in the next patch. For that we want to be sure that PPS is not in use
> by LVDS.
> 
> v2:
> - Split out the PPS init fix to a separate patch. (Chris)
> - Add comment about eDP init depending on LVDS init. (Chris)
> - Make the use of the intel_encoder ptr less error prone.
> 
> CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Main worry here is what if the LVDS detection was false? (I believe that
LVDS/eDP doesn't coexist...)

I'm just wondering if calling lvds_encoder->post_disable() to force the
LVDS off in this circumstance is viable. Worst case (false eDP, real
LVDS), we lose the output on the console until a mode is restored by fbdev.
Best case (false LVDS, real eDP) we don't regress detection of eDP.

(Or knowing the internals, we could just do a save restore of LVDS
PP_CONTROL around the eDP detection.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Imre Deak]

On Fri, 2016-06-17 at 23:35 +0100, Chris Wilson wrote:
> On Sat, Jun 18, 2016 at 12:29:24AM +0300, Imre Deak wrote:
> > Atm on ILK we attempt to detect if eDP is present even if LVDS was
> > already detected and an encoder for it was registered. This involves
> > trying to read out the eDP EDID, which in turn needs the same power
> > sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> > may or may not interfere with the LVDS panel, but it's probably safer to
> > prevent this. Registering both an LVDS and an eDP connector would also
> > present a similar problem accessing the shared PPS at any point later in
> > an unexpected way.
> > 
> > We also need this to be able fix PPS initialization before its first use
> > in the next patch. For that we want to be sure that PPS is not in use
> > by LVDS.
> > 
> > v2:
> > - Split out the PPS init fix to a separate patch. (Chris)
> > - Add comment about eDP init depending on LVDS init. (Chris)
> > - Make the use of the intel_encoder ptr less error prone.
> > 
> > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Main worry here is what if the LVDS detection was false?

That wouldn't really work anyway atm. We'd end up with both LVDS and
eDP registered and the subsequent LVDS modeset toggling the eDP panel
power out of order with respect to eDP's own pipe, PLL, port enabling
sequence. In the worst case we'd violate panel specs, for instance with
an LVDS panel off->eDP forced VDD sequence.

> (I believe that LVDS/eDP doesn't coexist...)

Right, they both use the single PPS we have which can't be shared.

> I'm just wondering if calling lvds_encoder->post_disable() to force the
> LVDS off in this circumstance is viable. Worst case (false eDP, real
> LVDS), we lose the output on the console until a mode is restored by fbdev.
> Best case (false LVDS, real eDP) we don't regress detection of eDP.
> 
> (Or knowing the internals, we could just do a save restore of LVDS
> PP_CONTROL around the eDP detection.)

The proper way to implement that kind of workaround would be to
unregister (or permanently disable) the LVDS encoder/connector if eDP
detection succeeds. We would also have to disable LVDS unconditionally
on ILK before eDP detect, even on a correctly detected LVDS output,
since we run the eDP detection also unconditionally. I think we should
only add support for this if we know that such broken systems exist.

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

Re: [PATCH v2 1/2] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Chris Wilson]

On Sun, Jun 19, 2016 at 01:18:32PM +0300, Imre Deak wrote:
> On Fri, 2016-06-17 at 23:35 +0100, Chris Wilson wrote:
> > On Sat, Jun 18, 2016 at 12:29:24AM +0300, Imre Deak wrote:
> > > Atm on ILK we attempt to detect if eDP is present even if LVDS was
> > > already detected and an encoder for it was registered. This involves
> > > trying to read out the eDP EDID, which in turn needs the same power
> > > sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> > > may or may not interfere with the LVDS panel, but it's probably safer to
> > > prevent this. Registering both an LVDS and an eDP connector would also
> > > present a similar problem accessing the shared PPS at any point later in
> > > an unexpected way.
> > > 
> > > We also need this to be able fix PPS initialization before its first use
> > > in the next patch. For that we want to be sure that PPS is not in use
> > > by LVDS.
> > > 
> > > v2:
> > > - Split out the PPS init fix to a separate patch. (Chris)
> > > - Add comment about eDP init depending on LVDS init. (Chris)
> > > - Make the use of the intel_encoder ptr less error prone.
> > > 
> > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > Main worry here is what if the LVDS detection was false?
> 
> That wouldn't really work anyway atm. We'd end up with both LVDS and
> eDP registered and the subsequent LVDS modeset toggling the eDP panel
> power out of order with respect to eDP's own pipe, PLL, port enabling
> sequence. In the worst case we'd violate panel specs, for instance with
> an LVDS panel off->eDP forced VDD sequence.
> 
> > (I believe that LVDS/eDP doesn't coexist...)
> 
> Right, they both use the single PPS we have which can't be shared.
> 
> > I'm just wondering if calling lvds_encoder->post_disable() to force the
> > LVDS off in this circumstance is viable. Worst case (false eDP, real
> > LVDS), we lose the output on the console until a mode is restored by fbdev.
> > Best case (false LVDS, real eDP) we don't regress detection of eDP.
> > 
> > (Or knowing the internals, we could just do a save restore of LVDS
> > PP_CONTROL around the eDP detection.)
> 
> The proper way to implement that kind of workaround would be to
> unregister (or permanently disable) the LVDS encoder/connector if eDP
> detection succeeds. We would also have to disable LVDS unconditionally
> on ILK before eDP detect, even on a correctly detected LVDS output,
> since we run the eDP detection also unconditionally. I think we should
> only add support for this if we know that such broken systems exist.

Ah, but we don't do the eDP detection unconditionally. We only try and
register the eDP ports if the hardware tells us it is present...

has_edp_a() (DP on port D invokes trust in the VBT)

So we are in a situation where the hw claims there is both a LVDS and
eDP connection. You have already demonstrated that such broken HW
exists, have you not?  Either that or we have missed a fuse to override
the DP detected bit.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Chris Wilson]

On Sat, Jun 18, 2016 at 12:29:24AM +0300, Imre Deak wrote:
> Atm on ILK we attempt to detect if eDP is present even if LVDS was
> already detected and an encoder for it was registered. This involves
> trying to read out the eDP EDID, which in turn needs the same power
> sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> may or may not interfere with the LVDS panel, but it's probably safer to
> prevent this. Registering both an LVDS and an eDP connector would also
> present a similar problem accessing the shared PPS at any point later in
> an unexpected way.
> 
> We also need this to be able fix PPS initialization before its first use
> in the next patch. For that we want to be sure that PPS is not in use
> by LVDS.
> 
> v2:
> - Split out the PPS init fix to a separate patch. (Chris)
> - Add comment about eDP init depending on LVDS init. (Chris)
> - Make the use of the intel_encoder ptr less error prone.
> 
> CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Regardless of the discussion we are having about whether there may be
more complications, or if we have missed something, this patch seems a
sensible guard.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Ville Syrjälä]

On Fri, Jun 17, 2016 at 06:48:45PM +0300, Imre Deak wrote:
> Atm on ILK we attempt to detect if eDP is present even if LVDS was
> already detected and an encoder for it was registered. This involves
> trying to read out the eDP EDID, which in turn needs the same power
> sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> may or may not interfere with the LVDS panel, but it's probably safer to
> prevent this.  Registering both an LVDS and an eDP connector would also
> present a similar problem accessing the shared PPS at any point later in
> an unexpected way.
> 
> This was caught by CI with the PPS sanity checks in place and the
> initial eDP EDID readout waiting for the panel power cycle timeout
> without the PPS registers being initialized. To solve this latter
> problem move PPS register init earlier before the first use of the
> PPS.
> 
> CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index be08351..fe543d7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5316,8 +5316,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	if (!is_edp(intel_dp))
>  		return true;
>  
> +	/*
> +	 * On ILK we may get here with LVDS already registered. Since the

ILK,SNB,IVB, if you want to accurately list the platforms.

> +	 * driver uses the only internal power sequencer available for both
> +	 * eDP and LVDS bail out early in this case to prevent interfering
> +	 * with an already powered-on LVDS power sequencer.
> +	 */
> +	for_each_intel_encoder(dev, intel_encoder) {
> +		if (intel_encoder->type == INTEL_OUTPUT_LVDS) {
> +			DRM_INFO("LVDS was detected, not registering eDP\n");
> +			return false;
> +		}
> +	}

We could abort much earlier, in the encoder init.

> +
>  	pps_lock(intel_dp);
>  	intel_edp_panel_vdd_sanitize(intel_dp);
> +	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
>  	pps_unlock(intel_dp);

For VLV/CHV we do this stuff in the encoder init already. Maybe we
should move all that stuff here as well?

>  
>  	/* Cache DPCD and EDID for edp. */
> @@ -5334,11 +5348,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  		return false;
>  	}
>  
> -	/* We now know it's not a ghost, init power sequence regs. */
> -	pps_lock(intel_dp);
> -	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
> -	pps_unlock(intel_dp);
> -
>  	mutex_lock(&dev->mode_config.mutex);
>  	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
>  	if (edid) {
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Imre Deak]

On ma, 2016-06-20 at 08:54 +0100, Chris Wilson wrote:
> On Sun, Jun 19, 2016 at 01:18:32PM +0300, Imre Deak wrote:
> > On Fri, 2016-06-17 at 23:35 +0100, Chris Wilson wrote:
> > > On Sat, Jun 18, 2016 at 12:29:24AM +0300, Imre Deak wrote:
> > > > Atm on ILK we attempt to detect if eDP is present even if LVDS was
> > > > already detected and an encoder for it was registered. This involves
> > > > trying to read out the eDP EDID, which in turn needs the same power
> > > > sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> > > > may or may not interfere with the LVDS panel, but it's probably safer to
> > > > prevent this. Registering both an LVDS and an eDP connector would also
> > > > present a similar problem accessing the shared PPS at any point later in
> > > > an unexpected way.
> > > > 
> > > > We also need this to be able fix PPS initialization before its first use
> > > > in the next patch. For that we want to be sure that PPS is not in use
> > > > by LVDS.
> > > > 
> > > > v2:
> > > > - Split out the PPS init fix to a separate patch. (Chris)
> > > > - Add comment about eDP init depending on LVDS init. (Chris)
> > > > - Make the use of the intel_encoder ptr less error prone.
> > > > 
> > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > Main worry here is what if the LVDS detection was false?
> > 
> > That wouldn't really work anyway atm. We'd end up with both LVDS and
> > eDP registered and the subsequent LVDS modeset toggling the eDP panel
> > power out of order with respect to eDP's own pipe, PLL, port enabling
> > sequence. In the worst case we'd violate panel specs, for instance with
> > an LVDS panel off->eDP forced VDD sequence.
> > 
> > > (I believe that LVDS/eDP doesn't coexist...)
> > 
> > Right, they both use the single PPS we have which can't be shared.
> > 
> > > I'm just wondering if calling lvds_encoder->post_disable() to force the
> > > LVDS off in this circumstance is viable. Worst case (false eDP, real
> > > LVDS), we lose the output on the console until a mode is restored by fbdev.
> > > Best case (false LVDS, real eDP) we don't regress detection of eDP.
> > > 
> > > (Or knowing the internals, we could just do a save restore of LVDS
> > > PP_CONTROL around the eDP detection.)
> > 
> > The proper way to implement that kind of workaround would be to
> > unregister (or permanently disable) the LVDS encoder/connector if eDP
> > detection succeeds. We would also have to disable LVDS unconditionally
> > on ILK before eDP detect, even on a correctly detected LVDS output,
> > since we run the eDP detection also unconditionally. I think we should
> > only add support for this if we know that such broken systems exist.
> 
> Ah, but we don't do the eDP detection unconditionally. We only try and
> register the eDP ports if the hardware tells us it is present...
> 
> has_edp_a() (DP on port D invokes trust in the VBT)

Ah, yes, missed that check.

> So we are in a situation where the hw claims there is both a LVDS and
> eDP connection. You have already demonstrated that such broken HW
> exists, have you not?

I didn't, the issue this and the next patch fixes was on SKL. But yes

commit f30d26e468322b50d5e376bec40658683aff8ece
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Wed Jan 16 10:53:40 2013 +0200

    drm/i915/eDP: do not write power sequence registers for ghost eDP

show that there is such systems out there, although in the above case
the LVDS output wasn't a ghost. In any case I agree now that we should
consider this case and add proper support for this. However I think if
BIOS has enabled the LVDS output that should already give enough
confidence that the LVDS is real. In that way we could avoid disabling
a properly functioning LVDS output (leading to flicker even with
fastboot). I've put something together for this (top three commits):
https://github.com/ideak/linux/commits/lvds_edp_init

This still misses disabling the LVDS connector/encoder, I could still
add that on top.

> Either that or we have missed a fuse to override the DP detected bit.

We do check the strap bit on ILK, but we have the same issue on SNB/IVB
as pointed out by Ville, where we don't. The machine in the above
commit was an IVB/port A, so it's possible we could've avoided the
issue by checking a strap bit if it exists. I will check the docs.

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

Re: [PATCH] drm/i915/ilk: Don't attempt to register eDP if LVDS was detected

[Imre Deak]

On ma, 2016-06-20 at 13:39 +0300, Ville Syrjälä wrote:
> On Fri, Jun 17, 2016 at 06:48:45PM +0300, Imre Deak wrote:
> > Atm on ILK we attempt to detect if eDP is present even if LVDS was
> > already detected and an encoder for it was registered. This involves
> > trying to read out the eDP EDID, which in turn needs the same power
> > sequencer that LVDS uses. Poking at the VDD line at an unexpected time
> > may or may not interfere with the LVDS panel, but it's probably safer to
> > prevent this.  Registering both an LVDS and an eDP connector would also
> > present a similar problem accessing the shared PPS at any point later in
> > an unexpected way.
> > 
> > This was caught by CI with the PPS sanity checks in place and the
> > initial eDP EDID readout waiting for the panel power cycle timeout
> > without the PPS registers being initialized. To solve this latter
> > problem move PPS register init earlier before the first use of the
> > PPS.
> > 
> > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index be08351..fe543d7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5316,8 +5316,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  	if (!is_edp(intel_dp))
> >  		return true;
> >  
> > +	/*
> > +	 * On ILK we may get here with LVDS already registered. Since the
> 
> ILK,SNB,IVB, if you want to accurately list the platforms.

Oops. I made the wrong assumption that IBX/CPT=ILK. Will fix this.

> 
> > +	 * driver uses the only internal power sequencer available for both
> > +	 * eDP and LVDS bail out early in this case to prevent interfering
> > +	 * with an already powered-on LVDS power sequencer.
> > +	 */
> > +	for_each_intel_encoder(dev, intel_encoder) {
> > +		if (intel_encoder->type == INTEL_OUTPUT_LVDS) {
> > +			DRM_INFO("LVDS was detected, not registering eDP\n");
> > +			return false;
> > +		}
> > +	}
> 
> We could abort much earlier, in the encoder init.

Yes, with an is_edp() check. Although preserving eDP detect for a non-
active LVDS output as discussed with Chris, intel_edp_init_connector()
would have to know about such an LVDS output to save/restore the PPS
state. Do you still want me then to move this earlier passing a flag to
intel_edp_init_connector()?

> > +
> >  	pps_lock(intel_dp);
> >  	intel_edp_panel_vdd_sanitize(intel_dp);
> > +	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
> >  	pps_unlock(intel_dp);
> 
> For VLV/CHV we do this stuff in the encoder init already. Maybe we
> should move all that stuff here as well?

Ok, I did this:
https://github.com/ideak/linux/commit/72a224980515a87d6102c0f5c63306d35b337efe

AUX should be also inited after PPS setup, so I also moved that after PPS setup.
Ideally we had separate AUX init/register phases, then we could init AUX upfront
for both DP and eDP and register it after eDP init. B/c of the AUX/I2C
register/init is also conflated atm, this needs more work, I think Chris has
already something to separate those.

> > 
> >  	/* Cache DPCD and EDID for edp. */
> > @@ -5334,11 +5348,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > -	/* We now know it's not a ghost, init power sequence regs. */
> > -	pps_lock(intel_dp);
> > -	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
> > -	pps_unlock(intel_dp);
> > -
> >  	mutex_lock(&dev->mode_config.mutex);
> >  	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
> >  	if (edid) {
> > -- 
> > 2.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx