[Intel-gfx] [PATCH resend 2/2] drm/i915/display: Make vlv_find_free_pps() skip pipes which are in use for non DP purposes

[Ville Syrjälä]

On Wed, Mar 24, 2021 at 03:10:59PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/24/21 3:02 PM, Ville Syrjälä wrote:
> > On Tue, Mar 23, 2021 at 11:39:09AM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 3/2/21 3:51 PM, Ville Syrjälä wrote:
> >>> On Tue, Mar 02, 2021 at 01:00:40PM +0100, Hans de Goede wrote:
> >>>> As explained by a long comment block, on VLV intel_setup_outputs()
> >>>> sometimes thinks there might be an eDP panel connected while there is none.
> >>>> In this case intel_setup_outputs() will call intel_dp_init() to check.
> >>>>
> >>>> In this scenario vlv_find_free_pps() ends up selecting pipe A for the pps,
> >>>> even though this might be in use for non DP purposes. When this is the case
> >>>> then the assert_pipe() in vlv_force_pll_on() will fail when called from
> >>>> vlv_power_sequencer_kick().
> >>>
> >>> The idea is that you *can* select a PPS from a pipe used for a non-DP
> >>> port since those don't care about the PPS stuff. So this doesn't seem
> >>> correct.
> >>
> >> They may not care about the PPS stuff, but as the WARN / backtrace
> >> shows if the DPLL_VCO_ENABLE bit is not already set for the pipe, while
> >> the pipe is "otherwise" in use then vlv_force_pll_on() becomes unhappy
> >> triggering the WARN.DPLL_VCO_ENABLE bit is not
> >>
> >>> a) I would like to see the VBT for this machine
> >>
> >> https://fedorapeople.org/~jwrdegoede/voyo-winpad-a15-vbt
> >>
> >>> b) I wonder if the DSI PLL is sufficient for getting the PPS going?
> >>
> >> I have no idea, I just noticed the WARN / backtrace and this seemed
> >> like a reasonably way to deal with it. With that said I'm fine with fixing
> >> this a different way.
> >>
> >>> c) If we do need the normal DPLL is there any harm to DSI in enabling it?
> >>
> >> I would assume this increases power-consumption and DSI panels are almost
> >> always used in battery powered devices.
> > 
> > This is just used while probing the panel, so power consumption is
> > not a concern.
> 
> Sorry I misinterpreted what you wrote, I interpreted it as have the DSI
> code enable it to avoid this problem. I see now that that is now what
> you meant.
> 
> >> Also this would impact all BYT/CHT devices, possible triggering unwanted
> >> side-effects. Where as the proposed fix below is much more narrowly targeted
> >> at the problem. It might not be the most pretty fix but AFAICT it has a low
> >> risk of causing regressions.
> > 
> > It rather significantly changes the logic of the workaround, potentially
> > causing us to not find a free PPS at all. Eg. if you were to boot with
> > a VLV with pipe A -> eDP B + eDP C inactive + pipe B -> VGA then your
> > change would cause us to not find the free pipe B PPS for probing eDP C,
> > and in the end we'd get a WARN and fall back to pipe A PPS which would
> > clobber the actually in use pipe A PPS.
> 
> I would welcome, and will happily test, another fix for this. ATM we
> have a WARN triggering on actual hardware (and not just in a hypothetical
> example) and I would like to see that WARN fixed. If you can come up with
> a better fix I would be happy to test.

Well, I think there are a couple things we want to experiment wiht:

a) Just skip the asserts and see if enabling the DPLL/poking the PPS
   perturbs the DSI output in any way.

--- a/drivers/gpu/drm/i915/display/intel_dpll.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll.c
@@ -1467,7 +1467,7 @@ void vlv_enable_pll(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 
-	assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
+	//assert_pipe_disabled(dev_priv, pipe_config->cpu_transcoder);
 
 	/* PLL is protected by panel, make sure we can write it */
 	assert_panel_unlocked(dev_priv, pipe);
@@ -1800,7 +1800,7 @@ void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 	u32 val;
 
 	/* Make sure the pipe isn't still relying on us */
-	assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
+	//assert_pipe_disabled(dev_priv, (enum transcoder)pipe);
 
 	val = DPLL_INTEGRATED_REF_CLK_VLV |
 		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -110,6 +110,8 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	intel_de_write(dev_priv, intel_dp->output_reg, DP & ~DP_PORT_EN);
 	intel_de_posting_read(dev_priv, intel_dp->output_reg);
 
+	msleep(1000); // just to make sure we keep angering DSI for a bit longer
+
 	if (!pll_enabled) {
 		vlv_force_pll_off(dev_priv, pipe);
 

b) Don't enable the DPLL at all and see if the DSI PLL is capable of
   clocking the PPS. My gut feeling says this will not work and we
   should see the PPS state machine not make progress, but not sure.

--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -77,7 +77,7 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
 	else
 		DP |= DP_PIPE_SEL(pipe);
 
-	pll_enabled = intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
+	pll_enabled = true;//intel_de_read(dev_priv, DPLL(pipe)) & DPLL_VCO_ENABLE;
 
 	/*
 	 * The DPLL for the pipe must be enabled for this to work.

I do have DSI VLV machine at the office, so I can also try to poke it a
bit next time I'm at the office.

-- 
Ville Syrjälä
Intel