Re: [PATCH i-g-t v3] 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-21 06:21:20-03:00)
>Prior to Gen12, Pipe A is required to run the DC5 transaction,
>ensuring low power operation. In display versions greater than 12,
>either Pipe A or Pipe B can be used for the DC5 transaction,
>providing flexibility in pipe selection
>
>However, display version 20 specifically requires Pipe A.
>Additionally, for Battlemage and DG2 devices, only Pipe A is
>required due to specific hardware constraints. To enforce these
>conditions, skip logic and logging have been added, ensuring that
>tests are only executed when the appropriate pipes are selected.
>
>v2: Updated subject title and discription.
>    Add Gen checks for low power pipe selection. (Santosh)
>v3: Add check for bmg and dg2 for Pipe A also other checks for Pipe A
>    and B. (Gustavo)
>
>Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com>
>Reviewed-by: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
>---
> tests/intel/kms_pm_dc.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
>index 50473321a..b23c13363 100644
>--- a/tests/intel/kms_pm_dc.c
>+++ b/tests/intel/kms_pm_dc.c
>@@ -148,10 +148,16 @@ 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(((IS_BATTLEMAGE(data->devid) || IS_DG2(data->devid)) && pipe != PIPE_A) ||
>+                              (disp_ver <= 12 && pipe != PIPE_A) || (disp_ver == 20 && pipe != PIPE_A) ||
>+                              (disp_ver > 12 && disp_ver != 20 && !(pipe == PIPE_A || pipe == PIPE_B)),
>+                               "Low power pipe was not selected for the DC5 transaction.\n");
>+

This seems correct, so

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

.

That said, I believe that condition is complex and not very readable. I
still think an if-ladder would make things more readable. Example:

    if (disp_ver >= 13) {
        if (disp_ver == 20 || IS_BATTLEMAGE(data->devid) || IS_DG2(data->devid))
            is_low_power = pipe == PIPE_A;
        else
            is_low_power = pipe == PIPE_A || pipe == PIPE_B;
    } else {
        is_low_power = pipe == PIPE_A;
    }

    igt_skip_on_f(!is_low_power, "Low power pipe was not selected for the DC5 transaction.\n");

--
Gustavo Sousa

>                 if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
>                         continue;
> 
>-- 
>2.43.0
>