[PATCH v3 0/3] drm/i915: Fix power sequencer use before init

[PATCH v3 0/3] drm/i915: Fix power sequencer use before init

[Imre Deak]

This is v3 of [1] addressing Ville's comments. Chris also noted that
this patchset leaves open the possibility for eDP not being detected due
to us detecting a ghost LVDS. I think that's already broken since then
we would leave both LVDS and eDP connectors registered which would
contend for the shared PPS and since there wasn't this particular
problem reported yet, I left implementing a workaround for that later,
when such a bug report arises.

[1]
https://lists.freedesktop.org/archives/intel-gfx/2016-June/098748.html

CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
CC: Chris Wilson <chris@chris-wilson.co.uk>

Imre Deak (3):
  drm/i915/ibx,cpt: Don't attempt to register eDP if LVDS was detected
  drm/i915: Initialize the PPS HW before its first use
  drm/i915: Group all the PPS init steps to one place

 drivers/gpu/drm/i915/intel_display.c |  5 +++
 drivers/gpu/drm/i915/intel_dp.c      | 67 ++++++++++++++++++++++--------------
 2 files changed, 46 insertions(+), 26 deletions(-)

-- 
2.5.0

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

[PATCH v3 1/3] drm/i915/ibx, cpt: Don't attempt to register eDP if LVDS was detected

[Imre Deak]

Atm on IBX/CPT 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 DPCD, 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.
v3:
- Use IBX/CPT reference instead of the incorrect ILK, add a WARN about
  this. (Ville)

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> (v2)
---
 drivers/gpu/drm/i915/intel_display.c |  5 +++++
 drivers/gpu/drm/i915/intel_dp.c      | 17 +++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0b2cd66..1141b86 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14710,6 +14710,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 ffa43ec..9a1cef4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5303,6 +5303,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	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 intel_encoder *tmp_encoder;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *fixed_mode = NULL;
 	struct drm_display_mode *downclock_mode = NULL;
@@ -5314,6 +5315,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (!is_edp(intel_dp))
 		return true;
 
+	/*
+	 * On IBX/CPT 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, tmp_encoder) {
+		if (tmp_encoder->type != INTEL_OUTPUT_LVDS)
+			continue;
+
+		WARN_ON(!(HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv)));
+		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 v3 2/3] drm/i915: Initialize the PPS HW before its first use

[Imre Deak]

The initial DPCD read for eDP detection involves using the PPS, but so
far we only initialized the PPS registers after the DPCD 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 DPCD 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 9a1cef4..277b74a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5333,6 +5333,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. */
@@ -5349,11 +5350,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

[PATCH v3 3/3] drm/i915: Group all the PPS init steps to one place

[Imre Deak]

Move the early PPS initialization calls next to the rest of PPS
initialization steps. This allows us to forgo a duplicated call to
intel_dp_init_panel_power_sequencer_registers() on VLV/CHV.

This will swap the order of DP AUX registration wrt. PPS initialization.
There is an existing race here in case of a user space access via the
DPAUX device node after DP AUX registration and before calling
intel_dp_init_panel_power_sequencer_registers(), but this change won't
make this worse. The fix for this is to separate DP AUX initialization
and registration, that's a separate work already underway.

The order of MST wrt. PPS init as well as the order of
intel_dp_init_panel_power_sequencer_registers() wrt.
intel_edp_panel_vdd_sanitize() also swap, which is ok, there are no
dependencies between these steps.

Suggested by Ville.

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 | 46 +++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 277b74a..d15604d2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5332,8 +5332,18 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	}
 
 	pps_lock(intel_dp);
+
+	intel_dp_init_panel_power_timestamps(intel_dp);
+
+	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
+		vlv_initial_power_sequencer_setup(intel_dp);
+	} else {
+		intel_dp_init_panel_power_sequencer(dev, intel_dp);
+		intel_dp_init_panel_power_sequencer_registers(dev, 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,7 +5357,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	} else {
 		/* if this fails, presume the device is a ghost */
 		DRM_INFO("failed to retrieve link info, disabling eDP\n");
-		return false;
+		goto out_vdd_off;
 	}
 
 	mutex_lock(&dev->mode_config.mutex);
@@ -5417,6 +5427,18 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	intel_panel_setup_backlight(connector, pipe);
 
 	return true;
+
+out_vdd_off:
+	cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+	/*
+	 * vdd might still be enabled do to the delayed vdd off.
+	 * Make sure vdd is actually turned off here.
+	 */
+	pps_lock(intel_dp);
+	edp_panel_vdd_off_sync(intel_dp);
+	pps_unlock(intel_dp);
+
+	return false;
 }
 
 bool
@@ -5522,16 +5544,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		BUG();
 	}
 
-	if (is_edp(intel_dp)) {
-		pps_lock(intel_dp);
-		intel_dp_init_panel_power_timestamps(intel_dp);
-		if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
-			vlv_initial_power_sequencer_setup(intel_dp);
-		else
-			intel_dp_init_panel_power_sequencer(dev, intel_dp);
-		pps_unlock(intel_dp);
-	}
-
 	ret = intel_dp_aux_init(intel_dp, intel_connector);
 	if (ret)
 		goto fail;
@@ -5564,16 +5576,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	return true;
 
 fail:
-	if (is_edp(intel_dp)) {
-		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-		/*
-		 * vdd might still be enabled do to the delayed vdd off.
-		 * Make sure vdd is actually turned off here.
-		 */
-		pps_lock(intel_dp);
-		edp_panel_vdd_off_sync(intel_dp);
-		pps_unlock(intel_dp);
-	}
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 
-- 
2.5.0

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

✗ Ro.CI.BAT: warning for drm/i915: Fix power sequencer use before init

[Patchwork]

== Series Details ==

Series: drm/i915: Fix power sequencer use before init
URL   : https://patchwork.freedesktop.org/series/8967/
State : warning

== Summary ==

Series 8967v1 drm/i915: Fix power sequencer use before init
http://patchwork.freedesktop.org/api/1.0/series/8967/revisions/1/mbox

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-snb-i7-2620M)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-b:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)

fi-skl-i5-6260u  total:225  pass:200  dwarn:0   dfail:0   fail:2   skip:23 
fi-skl-i7-6700k  total:225  pass:186  dwarn:0   dfail:0   fail:2   skip:37 
fi-snb-i7-2600   total:225  pass:172  dwarn:0   dfail:0   fail:2   skip:51 
ro-bdw-i5-5250u  total:225  pass:197  dwarn:4   dfail:0   fail:0   skip:24 
ro-bdw-i7-5600u  total:225  pass:185  dwarn:0   dfail:0   fail:0   skip:40 
ro-byt-n2820     total:225  pass:173  dwarn:0   dfail:0   fail:3   skip:49 
ro-hsw-i3-4010u  total:225  pass:190  dwarn:0   dfail:0   fail:0   skip:35 
ro-hsw-i7-4770r  total:225  pass:190  dwarn:0   dfail:0   fail:0   skip:35 
ro-ilk-i7-620lm  total:225  pass:150  dwarn:0   dfail:0   fail:1   skip:74 
ro-ilk1-i5-650   total:220  pass:150  dwarn:0   dfail:0   fail:1   skip:69 
ro-ivb-i7-3770   total:225  pass:181  dwarn:0   dfail:0   fail:0   skip:44 
ro-ivb2-i7-3770  total:225  pass:185  dwarn:0   dfail:0   fail:0   skip:40 
ro-skl3-i5-6260u total:225  pass:201  dwarn:1   dfail:0   fail:0   skip:23 
ro-snb-i7-2620M  total:225  pass:174  dwarn:0   dfail:0   fail:1   skip:50 
fi-hsw-i7-4770k failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1249/

79c69c3 drm-intel-nightly: 2016y-06m-21d-08h-53m-58s UTC integration manifest
8464015 drm/i915: Group all the PPS init steps to one place
d07dffd drm/i915: Initialize the PPS HW before its first use
1ce6454 drm/i915/ibx, cpt: 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: ✗ Ro.CI.BAT: warning for drm/i915: Fix power sequencer use before init

[Imre Deak]

On ti, 2016-06-21 at 09:45 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Fix power sequencer use before init
> URL   : https://patchwork.freedesktop.org/series/8967/
> State : warning
> 
> == Summary ==
> 
> Series 8967v1 drm/i915: Fix power sequencer use before init
> http://patchwork.freedesktop.org/api/1.0/series/8967/revisions/1/mbox
> 
> Test kms_flip:
>         Subgroup basic-flip-vs-wf_vblank:
>                 fail       -> PASS       (ro-snb-i7-2620M)
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-a:
>                 skip       -> DMESG-WARN (ro-bdw-i5-5250u)
>         Subgroup suspend-read-crc-pipe-b:
>                 skip       -> DMESG-WARN (ro-bdw-i5-5250u)
>         Subgroup suspend-read-crc-pipe-c:
>                 skip       -> DMESG-WARN (ro-bdw-i5-5250u)

All the above are pre-existing issues and not related to these patches
since the machine above doesn't have an eDP connection. I opened a bug:
https://bugs.freedesktop.org/show_bug.cgi?id=96614

After suspend-to-ram and resume on an external DP output:

[  291.867864] [drm:intel_enable_shared_dpll] enable LCPLL 1350 (active 1, on? 0) for crtc 26
[  291.867865] [drm:intel_enable_shared_dpll] enabling LCPLL 1350
[  291.870603] [drm:intel_dp_aux_ch] dp_aux_ch timeout status 0x7145000c
...
[  291.950407] [drm:drm_dp_dpcd_access] too many retries, giving up
...
[  292.119813] [drm:intel_dp_sink_dpms] failed to enable sink power state
...
[  292.373085] [drm:intel_dp_link_training_clock_recovery [i915]] *ERROR* failed to enable link training
...
[  292.458254] [drm:intel_dp_start_link_train [i915]] *ERROR* failed to start channel equalization

> 
> fi-skl-i5-
> 6260u  total:225  pass:200  dwarn:0   dfail:0   fail:2   skip:23 
> fi-skl-i7-
> 6700k  total:225  pass:186  dwarn:0   dfail:0   fail:2   skip:37 
> fi-snb-i7-
> 2600   total:225  pass:172  dwarn:0   dfail:0   fail:2   skip:51 
> ro-bdw-i5-
> 5250u  total:225  pass:197  dwarn:4   dfail:0   fail:0   skip:24 
> ro-bdw-i7-
> 5600u  total:225  pass:185  dwarn:0   dfail:0   fail:0   skip:40 
> ro-byt-
> n2820     total:225  pass:173  dwarn:0   dfail:0   fail:3   skip:49 
> ro-hsw-i3-
> 4010u  total:225  pass:190  dwarn:0   dfail:0   fail:0   skip:35 
> ro-hsw-i7-
> 4770r  total:225  pass:190  dwarn:0   dfail:0   fail:0   skip:35 
> ro-ilk-i7-
> 620lm  total:225  pass:150  dwarn:0   dfail:0   fail:1   skip:74 
> ro-ilk1-i5-
> 650   total:220  pass:150  dwarn:0   dfail:0   fail:1   skip:69 
> ro-ivb-i7-
> 3770   total:225  pass:181  dwarn:0   dfail:0   fail:0   skip:44 
> ro-ivb2-i7-
> 3770  total:225  pass:185  dwarn:0   dfail:0   fail:0   skip:40 
> ro-skl3-i5-6260u
> total:225  pass:201  dwarn:1   dfail:0   fail:0   skip:23 
> ro-snb-i7-
> 2620M  total:225  pass:174  dwarn:0   dfail:0   fail:1   skip:50 
> fi-hsw-i7-4770k failed to connect after reboot
> ro-bdw-i7-5557U failed to connect after reboot
> 
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1249/
> 
> 79c69c3 drm-intel-nightly: 2016y-06m-21d-08h-53m-58s UTC integration
> manifest
> 8464015 drm/i915: Group all the PPS init steps to one place
> d07dffd drm/i915: Initialize the PPS HW before its first use
> 1ce6454 drm/i915/ibx, cpt: 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 v3 1/3] drm/i915/ibx, cpt: Don't attempt to register eDP if LVDS was detected

[Ville Syrjälä]

On Tue, Jun 21, 2016 at 11:51:47AM +0300, Imre Deak wrote:
> Atm on IBX/CPT 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 DPCD, 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.
> v3:
> - Use IBX/CPT reference instead of the incorrect ILK, add a WARN about
>   this. (Ville)
> 
> 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> (v2)
> ---
>  drivers/gpu/drm/i915/intel_display.c |  5 +++++
>  drivers/gpu/drm/i915/intel_dp.c      | 17 +++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0b2cd66..1141b86 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14710,6 +14710,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 ffa43ec..9a1cef4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5303,6 +5303,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	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 intel_encoder *tmp_encoder;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_display_mode *fixed_mode = NULL;
>  	struct drm_display_mode *downclock_mode = NULL;
> @@ -5314,6 +5315,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	if (!is_edp(intel_dp))
>  		return true;
>  
> +	/*
> +	 * On IBX/CPT 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, tmp_encoder) {
> +		if (tmp_encoder->type != INTEL_OUTPUT_LVDS)
> +			continue;
> +
> +		WARN_ON(!(HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv)));

I think I usually prefer '!A && !B' for whatever reason. Either works.

> +		DRM_INFO("LVDS was detected, not registering eDP\n");
> +
> +		return false;
> +	}

I would have probably stuffed the loop into a small helper
(eg. has_lvds_encoder() or something along those lines). But
this works too, so

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>  	pps_lock(intel_dp);
>  	intel_edp_panel_vdd_sanitize(intel_dp);
>  	pps_unlock(intel_dp);
> -- 
> 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 v3 2/3] drm/i915: Initialize the PPS HW before its first use

[Ville Syrjälä]

On Tue, Jun 21, 2016 at 11:51:48AM +0300, Imre Deak wrote:
> The initial DPCD read for eDP detection involves using the PPS, but so
> far we only initialized the PPS registers after the DPCD 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 DPCD 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>

Seems fine.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  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 9a1cef4..277b74a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5333,6 +5333,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. */
> @@ -5349,11 +5350,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 v3 3/3] drm/i915: Group all the PPS init steps to one place

[Ville Syrjälä]

On Tue, Jun 21, 2016 at 11:51:49AM +0300, Imre Deak wrote:
> Move the early PPS initialization calls next to the rest of PPS
> initialization steps. This allows us to forgo a duplicated call to
> intel_dp_init_panel_power_sequencer_registers() on VLV/CHV.
> 
> This will swap the order of DP AUX registration wrt. PPS initialization.
> There is an existing race here in case of a user space access via the
> DPAUX device node after DP AUX registration and before calling
> intel_dp_init_panel_power_sequencer_registers(), but this change won't
> make this worse. The fix for this is to separate DP AUX initialization
> and registration, that's a separate work already underway.
> 
> The order of MST wrt. PPS init as well as the order of
> intel_dp_init_panel_power_sequencer_registers() wrt.
> intel_edp_panel_vdd_sanitize() also swap, which is ok, there are no
> dependencies between these steps.
> 
> Suggested by Ville.
> 
> CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

lgtm

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 46 +++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 277b74a..d15604d2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5332,8 +5332,18 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	}
>  
>  	pps_lock(intel_dp);
> +
> +	intel_dp_init_panel_power_timestamps(intel_dp);
> +
> +	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> +		vlv_initial_power_sequencer_setup(intel_dp);
> +	} else {
> +		intel_dp_init_panel_power_sequencer(dev, intel_dp);
> +		intel_dp_init_panel_power_sequencer_registers(dev, 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,7 +5357,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	} else {
>  		/* if this fails, presume the device is a ghost */
>  		DRM_INFO("failed to retrieve link info, disabling eDP\n");
> -		return false;
> +		goto out_vdd_off;
>  	}
>  
>  	mutex_lock(&dev->mode_config.mutex);
> @@ -5417,6 +5427,18 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	intel_panel_setup_backlight(connector, pipe);
>  
>  	return true;
> +
> +out_vdd_off:
> +	cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> +	/*
> +	 * vdd might still be enabled do to the delayed vdd off.
> +	 * Make sure vdd is actually turned off here.
> +	 */
> +	pps_lock(intel_dp);
> +	edp_panel_vdd_off_sync(intel_dp);
> +	pps_unlock(intel_dp);
> +
> +	return false;
>  }
>  
>  bool
> @@ -5522,16 +5544,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		BUG();
>  	}
>  
> -	if (is_edp(intel_dp)) {
> -		pps_lock(intel_dp);
> -		intel_dp_init_panel_power_timestamps(intel_dp);
> -		if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> -			vlv_initial_power_sequencer_setup(intel_dp);
> -		else
> -			intel_dp_init_panel_power_sequencer(dev, intel_dp);
> -		pps_unlock(intel_dp);
> -	}
> -
>  	ret = intel_dp_aux_init(intel_dp, intel_connector);
>  	if (ret)
>  		goto fail;
> @@ -5564,16 +5576,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	return true;
>  
>  fail:
> -	if (is_edp(intel_dp)) {
> -		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> -		/*
> -		 * vdd might still be enabled do to the delayed vdd off.
> -		 * Make sure vdd is actually turned off here.
> -		 */
> -		pps_lock(intel_dp);
> -		edp_panel_vdd_off_sync(intel_dp);
> -		pps_unlock(intel_dp);
> -	}
>  	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);
>  
> -- 
> 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 v3 1/3] drm/i915/ibx, cpt: Don't attempt to register eDP if LVDS was detected

[Imre Deak]

On ke, 2016-06-22 at 15:26 +0300, Ville Syrjälä wrote:
> On Tue, Jun 21, 2016 at 11:51:47AM +0300, Imre Deak wrote:
> > Atm on IBX/CPT 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 DPCD, 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.
> > v3:
> > - Use IBX/CPT reference instead of the incorrect ILK, add a WARN about
> >   this. (Ville)
> > 
> > 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> (v2)
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  5 +++++
> >  drivers/gpu/drm/i915/intel_dp.c      | 17 +++++++++++++++++
> >  2 files changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0b2cd66..1141b86 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14710,6 +14710,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 ffa43ec..9a1cef4 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5303,6 +5303,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  	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 intel_encoder *tmp_encoder;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_display_mode *fixed_mode = NULL;
> >  	struct drm_display_mode *downclock_mode = NULL;
> > @@ -5314,6 +5315,22 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >  	if (!is_edp(intel_dp))
> >  		return true;
> >  
> > +	/*
> > +	 * On IBX/CPT 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, tmp_encoder) {
> > +		if (tmp_encoder->type != INTEL_OUTPUT_LVDS)
> > +			continue;
> > +
> > +		WARN_ON(!(HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv)));
> 
> I think I usually prefer '!A && !B' for whatever reason. Either works.

I couldn't decide either, we have it both ways in the driver.

> > +		DRM_INFO("LVDS was detected, not registering eDP\n");
> > +
> > +		return false;
> > +	}
> 
> I would have probably stuffed the loop into a small helper
> (eg. has_lvds_encoder() or something along those lines). But
> this works too, so

There was the same loop in intel_lvds.c, so I added a helper there and
changed it here while applying.

I pushed all 3 patches to -dinq, thanks for the reviews.

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +
> >  	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