Re: [PATCH i-g-t v2] tests/intel/kms_pm_dc: Add skip logic and logging for DC5 psr test

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Mohammed Thasleem (2025-03-09 07:06:49-03:00)
>Low power Pipe A is required to run the DC5 transaction for Gen12.
>For Gen12+ to Xe3, either Pipe A or Pipe B is needed for the DC5
>transaction using a low power pipe. Added skip logic and logging
>to enforce these requirements.
>
>v2: Updated subject title and discription.
>    Add Gen checks for low power pipe selection. (Santosh)
>
>Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com>
>---
> tests/intel/kms_pm_dc.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
>index bbb29d7d9..c7fde2af7 100644
>--- a/tests/intel/kms_pm_dc.c
>+++ b/tests/intel/kms_pm_dc.c
>@@ -156,10 +156,15 @@ static void setup_output(data_t *data)
>         igt_display_t *display = &data->display;
>         igt_output_t *output;
>         enum pipe pipe;
>+        int disp_ver = intel_display_ver(data->devid);
> 
>         for_each_pipe_with_valid_output(display, pipe, output) {
>                 drmModeConnectorPtr c = output->config.connector;
> 
>+                igt_skip_on_f((disp_ver <= 12 && pipe != PIPE_A) ||

The commit says "for Gen12+ to Xe3", implying that version 12 would also
allow pipe B, but here we are only allowing pipe A for version 12.

Looking at the specs, it seems we started to have pipe B as low power
pipe starting with display version 13, so it seems the code above is
right and the commit message needs an update.

There is a catch though: discrete devices like DG2 and BMG have
only pipe A as low power pipe.

>+                              (disp_ver <= 30 && !(pipe == PIPE_B || pipe == PIPE_A)),

Why are we limiting this for display version <= 30? This would cause any
new display release (i.e. a new display release with version > 30) to be
ignored.

I think it would be best to use ">=" checks as we usually do for
if-ladders in kernel code.

As such, my suggestion is as below:

  if (IS_BATTLEMAGE(data->devid) || IS_DG2(data->devid))
    low_power_pipe_selected = pipe == PIPE_A;
  else if (display_ver >= 13)
    low_power_pipe_selected = pipe == PIPE_A || pipe == PIPE_B;
  else
    low_power_pipe_selected = pipe == PIPE_A;

  igt_skip_on_f(!low_power_pipe_selected, ...);

By a quick look, it seems we do not have support full display version
(major and rel) in IGT yet, so we need IS_BATTLEMAGE() and IS_DG2()
here. In the future, we should replace those with full version checks.

--
Gustavo Sousa

>+                               "Low power pipe was not selected for the DC5 transaction.\n");
>+
>                 if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
>                         continue;
> 
>-- 
>2.25.1
>