[Intel-gfx] [PATCH igt] lib/kms: Force a full reprobe if we find a bad link

[Martin Peres]

On 31/05/17 17:45, Martin Peres wrote:
> On 31/05/17 16:55, Chris Wilson wrote:
>> On Wed, May 31, 2017 at 04:44:41PM +0300, Martin Peres wrote:
>>> On 31/05/17 15:42, Chris Wilson wrote:
>>>> On Wed, May 31, 2017 at 01:40:00PM +0300, Martin Peres wrote:
>>>>> On 26/05/17 14:48, Chris Wilson wrote:
>>>>>> If we do a shallow probe of the connector and it reports the link 
>>>>>> failed
>>>>>> previous (link-status != GOOD), force a full probe of the 
>>>>>> connector to
>>>>>> give the kernel a chance to validate the mode list.
>>>>>
>>>>> Sounds good, but will this make the tests SKIP if no modes are 
>>>>> available?
>>>>
>>>> I'm actually not sure what will happen if the mode is removed. I think
>>>> the tests are just using the first mode in the list? At the moment I
>>>> hope just to stop turning a single failure into many, it is still a bug
>>>> that the link training failed and was not recovered. Alternatively, we
>>>> can ask why isn't the kernel taking the corrective action when 
>>>> presented
>>>> with a new setcrtc?
>>>
>>> No, this is not a kernel bug, it is a failure that the userspace has
>>> to handle because the kernel can't do shit about this.
>>
>> Have you demonstrated that the kernel is beyond reproach when it failed
>> the link training? Nothing changed in the connection and it works most
>> of the time, so why did the kernel accept the failure. Even if we
>> temporarily force a change of modes that is poor UX that I see no reason
>> why it should not have been prevented in the first place.
> 
> Sorry, this is not what I meant. What I meant is that the kernel is 
> allowed to have this behaviour.
> 
> I agree though that in the case of the skl bug, it is quite likely that 
> the kernel is doing something dodgy, but this is another bug. IGT should 
> learn to cope with modes disappearing.
> 
>>
>>>> I'm not sure what the correct approach here should be, just what is the
>>>> contract the kernel is expecting of userspace? Should that contract
>>>> apply to new clients unaware of the earlier error?
>>>
>>> Right, IGT assumes that if a mode is already set, it can be set
>>> again. However, this assumption has been broken when the link-status
>>> patches landed.
>>>
>>> On a hotplug event, IGT should do a full reprobe, select one mode
>>> from the list and use it. If no modes can be set and the test is
>>> trying to set one, then the test should just SKIP.
>>
>> There is no hotplug event when a new client starts so how is igt meant
>> to even know that it was supposed to pick up the pieces for the kernel.
> 
> Yes.

How about this: When the modeset call fails, check if the link-status is 
BAD. If not, return a FAIL. If so, force a full re-probe, pick the 
highest available mode and try again. Do this until a mode applies. If 
no modes are left, just SKIP the test altogether.

Does this sound reasonable?

Martin