[igt-dev] [PATCH][V2] tests/kms_plane_multiple: turn off pipe if all planes are off

[Alex Hung]

On 2022-09-28 08:17, Mark Yacoub wrote:
> is the second fix not necessary anymore?
> 

The second fix is still needed but it does not need to be guarded by 
is_amdgpu_device().

 From IGT's CI test this seems to work fine.

> On Tue, Sep 27, 2022 at 3:36 PM Alex Hung <alex.hung at amd.com> wrote:
>>
>> amdgpu rejects when crtc is on + all planes are off, and it
>> is necessary to turn off crtc when all planes are off.
>>
>> This is revised from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F80904%2F&data=05%7C01%7Calex.hung%40amd.com%7C9618c44c52534176fa7608daa15c3b0e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637999714757526450%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=F%2FJ0xvZA1mpVrmuEimtMYUIXolalP9GG9NUGjfRepR0%3D&reserved=0
>>
>> Signed-off-by: Alex Hung <alex.hung at amd.com>
>> ---
>>   tests/kms_plane_multiple.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
>> index cbe8c189..e900dc7f 100644
>> --- a/tests/kms_plane_multiple.c
>> +++ b/tests/kms_plane_multiple.c
>> @@ -311,6 +311,8 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
>>                  for_each_plane_on_pipe(&data->display, pipe, plane)
>>                          igt_plane_set_fb(plane, NULL);
>>
>> +               if (is_amdgpu_device(data->drm_fd))
>> +                       igt_output_set_pipe(output, PIPE_NONE);
> I'm still confused why do you need this just for AMD. If AMD has some
> requirements, I think it's gonna work for everyone else. If it breaks
> in the CI (testing on intel devices), then I think something is off
> here, and maybe it's a kernel thing.
> If it's the crtc/plan workaround that's in amdgpu, an IGT fix has
> always worked for everyone and didn't need an (is amd only) check.

It may not be for AMD only and can also work for other GPUs. See 
discussion 
https://lore.kernel.org/all/604a1d7e-1cd9-ad27-6d37-2e8535ce253b@mailbox.org/T/#m03b3196cbebcd85013b04f8e9bca3b130e4557d1

Without is_amdgpu_device() it breaks at least one i915 device. In theory 
"is_amdgpu_device()" can be replaced by "!is_i915_device()" but this 
patch was not tested on other GPUs so I did not assume that it works on 
all other GPUs.

>>                  igt_display_commit2(&data->display, COMMIT_ATOMIC);
>>
>>                  for (int x = 0; x < c; x++)
>> @@ -344,6 +346,7 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
>>                  for_each_plane_on_pipe(&data->display, pipe, plane)
>>                          igt_plane_set_fb(plane, NULL);
>>
>> +               igt_output_set_pipe(output, PIPE_NONE);
>>                  igt_display_commit2(&data->display, COMMIT_ATOMIC);
>>
>>                  for (int x = 0; x < c; x++)
>> --
>> 2.34.1
>>