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

[Hans de Goede]

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.

Regards,

Hans