[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 04:37:14PM +0200, Ville Syrjälä wrote:
> 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.

Finally managed to frob the VLV DSI a bit.

I pushed a couple of ideas here:
git://github.com/vsyrjala/linux.git vlv_pps_vs_dsi

The first patch just skips the DPLL forcing if the pipe
is already on. At least the power sequencer didn't get totally
upset about it, but it can cause some visual issues for the DSI
output; the screen just goes all gray while the DP port is 
temporarily enabled, though I guess that might also happen
with HDMI/CRT outputs, haven't actually double checked that
so not sure.

The other more complicated patch tries to avoid touching
the PPS on any pipe currently driving DSI. Not sure if it's
worth the extra complexity for this niche use case if the
simple approach of just not forcing the DPLL works well
enough.

The third approach I didn't even impelement would be just declare
DSI and eDP mutually exclusive. But in theory the display engine
at least supports such mixed configurations, not so sure about
the pin muxing on SoC though.

-- 
Ville Syrjälä
Intel