Re: [igt-dev] [PATCH i-g-t 2/2] tests/i915/i915_pm_dc: Add DC6 PSR test for mtl

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Thu, 2023-03-16 at 02:31 +0530, Mohammed Thasleem wrote:
> The flow for DC6 validation has changed for MTL.
> DC6 state achieved based on PKGC10 entry.
> This test validates PKGC10 entry and confirm DC6 achieved.

Can you please clarify in commit message why this has changed? Using
PKGC10 as indicator for DC6 entry is much more error prone. There are
much more possible blockers for PKGC* state entry compared to DC* state
entry. You should have good reasoning for your change.
 
> 
> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com>
> ---
>  tests/i915/i915_pm_dc.c | 54
> ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
> index 322a0c60..ed573ce6 100644
> --- a/tests/i915/i915_pm_dc.c
> +++ b/tests/i915/i915_pm_dc.c
> @@ -407,6 +407,38 @@ static void dpms_on(data_t *data)
>                            (IGT_RUNTIME_PM_STATUS_ACTIVE));
>  }
>  
> +static void psr_pkc_enable(data_t *data)

I think this naming doesn't make sense. Anyways, eventually this
function is doing same thing as:

kmstest_set_connector_dpms(data->drm_fd, data->display.outputs-
>config.connector, DRM_MODE_DPMS_OFF);

So maybe you could directly do that instead of splitting this into it's
own function? I.e. loop below doesn't make sense.

> +{
> +       igt_output_t *output;
> +
> +       for_each_connected_output(&data->display, output) {
> +               drmModeConnectorPtr c = output->config.connector;
> +
> +               if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
> +                       continue;
> +
> +               kmstest_set_connector_dpms(data->drm_fd,
> +                                          data->display.outputs-
> >config.connector,
> +                                          DRM_MODE_DPMS_OFF);

As mentioned above this loop doesn't make sense. Maybe you wanted :

kmstest_set_connector_dpms(data->drm_fd, c, DRM_MODE_DPMS_OFF);

?
> +       }
> +}
> +
> +static void psr_pkc_disable(data_t *data)
> +{
> +       igt_output_t *output;
> +
> +       for_each_connected_output(&data->display, output) {
> +               drmModeConnectorPtr c = output->config.connector;
> +
> +               if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
> +                       continue;
> +
> +               kmstest_set_connector_dpms(data->drm_fd,
> +                                          data->display.outputs-
> >config.connector,
> +                                          DRM_MODE_DPMS_ON);
> +       }
> +}
> +
>  static void test_dc_state_dpms(data_t *data, int dc_flag)
>  {
>         uint32_t dc_counter;
> @@ -551,6 +583,23 @@ static void test_pkc_state_dpms(data_t *data)
>         dpms_on(data);
>  }
>  
> +static void test_pkc_state_psr(data_t *data)

Would it make more sense to differentiate in test_dc_state_psr instead
of having own function?

> +{
> +       unsigned int timeout_msec = 30;
> +       unsigned int prev_value = 0, cur_value = 0;
> +
> +       prev_value = read_pkc_state_value(data->pmc_fd);
> +       setup_output(data);
> +       setup_primary(data);
> +       igt_assert(psr_wait_entry(data->debugfs_fd, data-
> >op_psr_mode));
> +       psr_pkc_enable(data);
> +       cleanup_dc_psr(data);
> +       igt_wait((cur_value = read_pkc_state_value(data->pmc_fd)),
> +                 timeout_msec * 1000, 100);

I think you want to have:

igt_wait((cur_value = read_pkc_state_value(data->pmc_fd)) >
prev_value,timeout_msec * 1000, 100);


> +       igt_assert_f(cur_value > prev_value, "PK10 is not
> achieived.\n");
> +       psr_pkc_disable(data);
> +}
> +
>  static void kms_poll_state_restore(int sig)
>  {
>         int sysfs_fd;
> @@ -609,7 +658,10 @@ igt_main
>                 psr_enable(data.drm_fd, data.debugfs_fd,
> data.op_psr_mode);
>                 igt_require_f(igt_pm_pc8_plus_residencies_enabled(dat
> a.msr_fd),
>                               "PC8+ residencies not supported\n");
> -               test_dc_state_psr(&data, CHECK_DC6);
> +               if (intel_display_ver(data.devid) == 14)
> +                       test_pkc_state_psr(&data);
> +               else
> +                       test_dc_state_psr(&data, CHECK_DC6);
>         }
>  
>         igt_describe("This test validates display engine entry to DC5
> state "

BR,

Jouni Högander