Re: [PATCH v2 4/4] tests/amdgpu/amd_psr: Add support for `power saving policy` property

[Mario Limonciello <mario.limonciello@xxxxxxx>]

On 6/18/2024 15:20, Leo Li wrote:
> On 2024-05-22 18:08, Mario Limonciello wrote:
> > Verify that the property has disabled PSR
> > ---
> >   tests/amdgpu/amd_psr.c | 74 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 74 insertions(+)
> >
> > diff --git a/tests/amdgpu/amd_psr.c b/tests/amdgpu/amd_psr.c
> > index 9da161a09..a9f4a6aa5 100644
> > --- a/tests/amdgpu/amd_psr.c
> > +++ b/tests/amdgpu/amd_psr.c
> > @@ -338,6 +338,78 @@ static void run_check_psr(data_t *data, bool 
> > test_null_crtc) {
> >       test_fini(data);
> >   }
> > +static void psr_forbidden(data_t *data)
> > +{
> > +    int edp_idx, ret, i, psr_state;
> > +    igt_fb_t ref_fb, ref_fb2;
> > +    igt_fb_t *flip_fb;
> > +    igt_output_t *output;
> > +
> > +    test_init(data);
> > +
> > +    edp_idx = check_conn_type(data, DRM_MODE_CONNECTOR_eDP);
> > +    igt_skip_on_f(edp_idx == -1, "no eDP connector found\n");
> > +
> > +    /* check if eDP support PSR1, PSR-SU.
> > +     */
> > +    igt_skip_on(!psr_mode_supported(data, PSR_MODE_1) && 
> > !psr_mode_supported(data, PSR_MODE_2));
> > +    for_each_connected_output(&data->display, output) {
> > +
> > +        if (output->config.connector->connector_type != 
> > DRM_MODE_CONNECTOR_eDP)
> > +            continue;
> > +        ret = clear_power_saving_policy(data->fd, output);
> > +        if (ret == -ENODEV) {
> > +            igt_skip("No power saving policy prop\n");
> > +            return;
> > +        }
> > +        igt_require(ret == 0);
> > +
> > +        /* try to engage PSR */
> > +        ret = set_panel_power_saving_policy(data->fd, output, 
> > DRM_MODE_REQUIRE_LOW_LATENCY);
> > +        igt_assert_eq(ret, 0);
> > +
> > +        igt_create_color_fb(data->fd, data->mode->hdisplay,
> > +                    data->mode->vdisplay, DRM_FORMAT_XRGB8888, 0, 1.0,
> > +                    0.0, 0.0, &ref_fb);
> > +        igt_create_color_fb(data->fd, data->mode->hdisplay,
> > +                    data->mode->vdisplay, DRM_FORMAT_XRGB8888, 0, 0.0,
> > +                    1.0, 0.0, &ref_fb2);
> > +
> > +        igt_plane_set_fb(data->primary, &ref_fb);
> > +
> > +        igt_display_commit_atomic(&data->display, 
> > DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> > +
> > +        for (i = 0; i < N_FLIPS; i++) {
> > +            if (i % 2 == 0)
> > +                flip_fb = &ref_fb2;
> > +            else
> > +                flip_fb = &ref_fb;
> > +
> > +            ret = drmModePageFlip(data->fd, 
> > output->config.crtc->crtc_id,
> > +                          flip_fb->fb_id, DRM_MODE_PAGE_FLIP_EVENT, 
> > NULL);
> > +            igt_require(ret == 0);
> > +            kmstest_wait_for_pageflip(data->fd);
> > +        }
> > +
> > +        /* PSR state takes some time to settle its value on static 
> > screen */
> > +        sleep(PSR_SETTLE_DELAY);
> > +
> > +        psr_state =  igt_amd_read_psr_state(data->fd, output->name);
> > +        igt_require(psr_state == PSR_STATE3);
>
> igt_fail_on* or igt_assert* should be used here, igt_require simply 
> 'skips' the
> test if the condition evaluates to false.

Got it; thanks.

> Should we be instead asserting psr_state == PSR_STATE0 here for 
> disabled, since
> we've set REQUIRE_LOW_LATENCY?

Yeah I think you're right.

> I think as part of this test, we can also check that PSR re-enables after
> clearing the power saving policy. Something like
>
> ret = clear_power_saving_policy(data->fd, output);
> ... do some flipping ...
> sleep(PSR_SETTLE_DELAY);
> psr_state = igt_amd_read_psr_state(data->fd, output->name);
> igt_assert_f(psr_state == PSR_STATE3,
>               "Panel not in PSR after clearing power saving policy\n");

Agree, thanks.

> Thanks,
> Leo
>
> > +
> > +        igt_remove_fb(data->fd, &ref_fb);
> > +        igt_remove_fb(data->fd, &ref_fb2);
> > +
> > +        ret = clear_power_saving_policy(data->fd, output);
> > +        if (ret == -ENODEV) {
> > +            igt_skip("No power saving policy prop\n");
> > +            return;
> > +        }
> > +        igt_require(ret == 0);
> > +
> > +    }
> > +}
> > +
> >   static void run_check_psr_su_mpo(data_t *data, bool scaling, float 
> > scaling_ratio)
> >   {
> >       int edp_idx = check_conn_type(data, DRM_MODE_CONNECTOR_eDP);
> > @@ -756,6 +828,8 @@ igt_main_args("", long_options, help_str, 
> > opt_handler, NULL)
> >       igt_describe("Test to validate PSR SU enablement with Visual 
> > Confirm "
> >                "and to validate PSR SU disable/re-enable w/ primary 
> > scaling ratio 0.75");
> >       igt_subtest("psr_su_mpo_scaling_0_75") 
> > run_check_psr_su_mpo(&data, true, .75);
> > +    igt_describe("Test whether PSR can be forbidden");
> > +    igt_subtest("psr_forbidden") psr_forbidden(&data);
> >       igt_fixture
> >       {