Re: [PATCH v2] drm/amd/amdgpu: Add 5K Support for iMac20,1 and iMac20,2

[Ed Schofield]

Hi Atharva,

Thanks for posting this. I've built v2 unchanged and tested it on my
iMac20,1 over the weekend (on 7.3 plus the t2linux patches).

It works well in normal use. I ran automated tests over 32 boots, 55
display off/on cycles and about 40 suspend/resume cycles, and both
halves of the panel came back at 2560x2880 and 10 bpc every time,
including from scaled modes.

However, your v2 hangs the GPU when the two halves are set to different
modes. This happened 3 times out of 3, just as with my very similar "r2"
variant that I described in [1].

To reproduce this, leave the left half (eDP) at 2560x2880, set the right
half (DP-1) to 2048x2304, which its EDID advertises, then start a
second program that uses the GPU. Each time the log showed:

  amdgpu: SMU: No response msg_reg: 2a resp_reg: 0
  amdgpu: Failed to disable gfxoff!
  (repeated four times, about 4 s apart)
  amdgpu: ring sdma1 timeout, signaled seq=17, emitted seq=19
  amdgpu: Ring sdma1 reset failed
  amdgpu: GPU reset begin!

The reset never finishes and the machine has to be rebooted. Since the
halves are separate connectors, anything that sets modes per connector
can trigger this.

I think the cause is the early "return true" in
resource_are_streams_timing_synchronizable(), which tells DC the two
streams are synchronized even when their timings differ. Taking the
shortcut only when the timings really match avoids the hang:

        if (stream1->ctx->dc->config.imac5K_support &&
            stream1->timing.h_total == stream2->timing.h_total &&
            stream1->timing.v_total == stream2->timing.v_total &&
            stream1->timing.pix_clk_100hz == stream2->timing.pix_clk_100hz &&
            ((stream1->signal == SIGNAL_TYPE_EDP && ...

With that check, the same test passed 9 times out of 9 on a kernel
("r3" [2]) that is otherwise very close to your v2, and the normal 5K
case still takes the shortcut. Without the check it failed 7 times out
of 7 before v2, so the result is consistent. Dropping the shortcut
altogether also passed (8 of 8, "r1" [3]).

Could you add that check in v3? I'm happy to retest quickly.

One note: my Tested-by and Signed-off-by tags were added before
I had seen or tested this version. I'm glad to give both for a version
with the check above. Please use ed@xxxxxxxxxxxxxxxxxxxx.

Thanks again for all your work on this!

Ed

[1] https://github.com/t2linux/wiki/issues/743#issuecomment-6588960822
[2] https://github.com/edschofield/linux-t2-patches/blob/7273823e425200e9f371e9272e02ebf9abb6f1c2/7003-amdgpu-add-support-for-5K.patch
[3] https://github.com/edschofield/linux-t2-patches/blob/5d3775923bf481c487ff96efc8fb3e24ecf8c9cd/7003-amdgpu-add-support-for-5K.patch