[PATCH v2 3/4] tests/amdgpu/amd_abm: Add support for panel_power_saving property

[Mario Limonciello]

On 6/18/2024 15:20, Leo Li wrote:
> 
> Thanks for the tests! FYI IGT patches should also cc 
> igt-dev at lists.freedesktop.org
> 
> Some comments inline:
> 
> On 2024-05-22 18:08, Mario Limonciello wrote:
>> From: Mario Limonciello <superm1 at ubuntu.com>
>>
>> When the "panel power saving" property is set to forbidden the
>> compositor has indicated that userspace prefers to have color
>> accuracy and fidelity instead of power saving.
>>
>> Verify that the sysfs file behaves as expected in this situation.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>> ---
>>   tests/amdgpu/amd_abm.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/tests/amdgpu/amd_abm.c b/tests/amdgpu/amd_abm.c
>> index f74c3012c..3fa1366fa 100644
>> --- a/tests/amdgpu/amd_abm.c
>> +++ b/tests/amdgpu/amd_abm.c
>> @@ -365,6 +365,43 @@ static void abm_gradual(data_t *data)
>>       }
>>   }
>> +
>> +static void abm_forbidden(data_t *data)
>> +{
>> +    igt_output_t *output;
>> +    enum pipe pipe;
>> +    int target, r;
>> +
>> +    for_each_pipe_with_valid_output(&data->display, pipe, output) {
>> +        if (output->config.connector->connector_type != 
>> DRM_MODE_CONNECTOR_eDP)
>> +            continue;
>> +
>> +        r = clear_power_saving_policy(data->drm_fd, output);
>> +        if (r == -ENODEV) {
>> +            igt_skip("No power saving policy prop\n");
>> +            return;
>> +        }
>> +        igt_assert_eq(r, 0);
>> +
>> +        target = 3;
>> +        r = set_abm_level(data, output, target);
>> +        igt_assert_eq(r, 0);
>> +
>> +        r = set_panel_power_saving_policy(data->drm_fd, output, 
>> DRM_MODE_REQUIRE_COLOR_ACCURACY);
>> +        igt_assert_eq(r, 0);
>> +
>> +        target = 0;
> 
> Is there a purpose of setting target abm to 0 (disabled) here?
> 
> I suppose it should fail given that we've set REQUIRE_COLOR_ACCURACY. 
> Though I'm
> not sure why we can't keep target = 3.

Yes I think this would work as well to prove a failure.  I'll change it.

> 
> Thanks,
> Leo
> 
>> +        r = set_abm_level(data, output, target);
>> +        igt_assert_eq(r, -1);
>> +
>> +        r = clear_power_saving_policy(data->drm_fd, output);
>> +        igt_assert_eq(r, 0);
>> +
>> +        r = set_abm_level(data, output, target);
>> +        igt_assert_eq(r, 0);
>> +    }
>> +}
>> +
>>   igt_main
>>   {
>>       data_t data = {};
>> @@ -393,6 +430,8 @@ igt_main
>>           abm_enabled(&data);
>>       igt_subtest("abm_gradual")
>>           abm_gradual(&data);
>> +    igt_subtest("abm_forbidden")
>> +        abm_forbidden(&data);
>>       igt_fixture {
>>           igt_display_fini(&data.display);