Re: [PATCH 0/4] add test to validate uhbr/non-uhbr over sst/mst

["Joshi, Kunal1" <kunal1.joshi@intel.com>]

Hello Jani,

On 28-01-2025 20:34, Jani Nikula wrote:
> On Tue, 28 Jan 2025, "Joshi, Kunal1" <kunal1.joshi@intel.com> wrote:
>> Hello Jani,
>>
>> Thanks for taking a look,
>>
>> On 27-01-2025 18:01, Jani Nikula wrote:
>>> On Wed, 15 Jan 2025, Kunal Joshi<kunal1.joshi@intel.com> wrote:
>>>> Add new test validating UHBR and non-UHBR link rates for
>>>> both SST and MST configurations. The series comprises three patches:
>>>>
>>>> 1. tests/intel/kms_joiner_helper:
>>>>      - Introduces kms_joiner_helper.c/h for pipe assignments taking care of joiners
>>>>      - Moves common joiner-related logic out of kms_joiner.c.
>>>>
>>>> 2. tests/intel/kms_mst_helper:
>>>>      - Adds kms_mst_helper.c/h for MST-specific operations.
>>>>      - Centralizes code for identifying and assigning MST outputs.
>>>>
>>>> 3. tests/kms_feature_discovery:
>>>>      - Implements tests verifying UHBR and non-UHBR link rates with SST and MST.
>>>>      - Uses the new helpers for pipe assignments and topology discovery.
>>> AFAICT this adapts to what the driver reports. If the driver never
>>> enables DP SST UHBR, it'll happily pass, and just skip the tests. Am I
>>> right? Is that the kind of testing you want?
>>>
>>>
>>> BR,
>>> Jani.
>> The purpose of the |kms_feature_discovery| test is to verify that all
>> necessary configurations are in place. If a DP 2.1 SST monitor is not
>> connected in the CI environment, the test will be skipped, indicating
>> that SST UHBR coverage is missing. Since DP 2.1 SST support is already
>> provided by existing tests, this test simply confirms whether the
>> appropriate monitor is available to enable that coverage.
> I haven't been closely following IGT testing strategy lately, but you
> asked me to have a look at this regardless, so I did. I'm not going to
> comment on the details in this series or review it. The only question
> is, if the driver decides to incorrectly use non-UHBR SST on a UHBR SST
> capable sink, who is going to notice? Or if it works now, and breaks
> later, is some test going to fail instead of skip?
>
>
> BR,
> Jani.

Your concern is valid Jani, as of now we rely on driver to see what 
link_rate/lane_count
we are currently operating at, Will explore better ways to not rely on 
driver for this information.

For now I have added a fail criteria and link_status check.
Please have a look at the new revision.

Thanks and Regards
Kunal Joshi

>> Thanks and Regards Kunal Joshi
>>>
>>>> Kunal Joshi (4):
>>>>     tests/intel/kms_joiner_helper: helper for joiner-related functions
>>>>     tests/intel/kms_mst_helper: Add helper for MST-related functions
>>>>     tests/kms_feature_discovery: Add tests for UHBR/non-UHBR over SST/MST
>>>>     HAX: DO NOT MERGE
>>>>
>>>>    tests/intel-ci/fast-feedback.testlist    |  14 ++
>>>>    tests/intel-ci/xe-fast-feedback.testlist |  14 ++
>>>>    tests/intel/kms_dp_linktrain_fallback.c  |  28 +---
>>>>    tests/intel/kms_joiner.c                 |  15 +-
>>>>    tests/intel/kms_joiner_helper.c          | 179 +++++++++++++++++++++++
>>>>    tests/intel/kms_joiner_helper.h          |  15 ++
>>>>    tests/intel/kms_mst_helper.c             |  48 ++++++
>>>>    tests/intel/kms_mst_helper.h             |  10 ++
>>>>    tests/kms_feature_discovery.c            | 177 ++++++++++++++++++++++
>>>>    tests/meson.build                        |   6 +
>>>>    10 files changed, 469 insertions(+), 37 deletions(-)
>>>>    create mode 100644 tests/intel/kms_joiner_helper.c
>>>>    create mode 100644 tests/intel/kms_joiner_helper.h
>>>>    create mode 100644 tests/intel/kms_mst_helper.c
>>>>    create mode 100644 tests/intel/kms_mst_helper.h