[PATCH 2/2] Revert "drm/amd/display: disable CRTCs with NULL FB on their primary plane (V2)"

[Harry Wentland]

On 2018-04-13 03:15 AM, Michel Dänzer wrote:
> On 2018-04-13 04:55 AM, S, Shirish wrote:
>> Hi Harry, Alex,
>>
>> The solution given while reviewing my patch was that "DC should support enabling a CRTC without a framebuffer."
> 
> I think it's weird that an enabled CRTC would end up having a NULL FB
> during normal operation in the first place. Any idea how that happens?
> 

I think we've only seen that with ChromeOS and in one case on Ubuntu. If I remember right it was when running GDM with Wayland and logging into X. I imagine our DDX driver never pulls this scenario on us.

Technically this would be a reasonable case for our HW, i.e. we could keep pipe running and blanked and just not scan-out a FB.

> 
>> Since the revert is a temporary workaround to address issue at hand and considering the bigger regression it will cause on ChromeOS(explained below),
>> I would strongly recommend that the revert should not be mainlined (to linus tree), until a proper fix for both the issues i.e., flickering and BUG hit on atomic is found.
>>
>> For the sake of everyone's understanding in the list below is a brief background.
>>
>> Mainline patch from intel folks, "846c7df drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2." 
>> introduces a slight behavioral change to rmfb. Instead of disabling a crtc when the primary plane is disabled, it now preserves it.
>>
>> This change leads to BUG hit while performing atomic commit on amd driver leading to reboot/system instability on ChromeOS which has enabled
>> drm atomic way of rendering, I also remember it causing some issue on other OS as well.
> 
> Can you provide more information about "some issue on other OS"?
> Otherwise I'm afraid this is a no-brainer, since nobody's using ChromeOS
> with an AMD GPU in the wild, whereas many people have been running into
> issues with these commits in the wild, especially since they're in the
> 4.16 release.
> 

I tend to agree with Michel here, there's been quite the fallout from that patch for people's daily drivers.

Harry

>