Re: [PATCH i-g-t v8 6/6] tests/kms_flip: test suspend at most twice on SNB && BMG

["Thasleem, Mohammed" <mohammed.thasleem@intel.com>]

On 26-03-2026 02:58 pm, Michał Grzelak wrote:
> On Wed, 25 Mar 2026, Thasleem, Mohammed wrote:
>> On 09-03-2026 02:55 pm, Michał Grzelak wrote:
>>
>> gettime_us() on SNB and BMG does not return proper time after starting
>> suspend tests. This results in too many suspends issued before we hit
>> the duration_ms. Break the loop on second execution.
>>
>> Signed-off-by: Michał Grzelak <michal.grzelak@intel.com>
>> ---
>>  tests/kms_flip.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/tests/kms_flip.c b/tests/kms_flip.c
>> index f17d027cc..3c5428ff0 100755
>> --- a/tests/kms_flip.c
>> +++ b/tests/kms_flip.c
>> @@ -1322,6 +1322,12 @@ static bool event_loop(struct test_output *o, 
>> unsigned duration_ms,
>>  {
>>      unsigned long start, end;
>>      int count = 0;
>> +    int devid;
-->Use "uint32_t" instead "int" as" intel_get_drm_devid" returns 
"uint32_t".
>> +    bool wa;
>> +
>> +    devid = intel_get_drm_devid(drm_fd);
>> +
>> +    wa = IS_SANDYBRIDGE(devid) || IS_BATTLEMAGE(devid);
>>
>> Why only two specific platforms?
>
> That I would also like to know. :)
> As it is described in the first sentence of the commit message, these
> two platforms have been reported by CI to encounter such issue. If it is
> not clear, I can update the commit message.
>
>> I think, platform specific wa not needed here, instead we can have 
>> generic execution for all,
>> which can work on all platforms. plz check.
>
> Since we are observing the issue only on those two platforms, I don't
> see how to work around it without specifying those two platforms,
> without changing the already present functionality.

I mean to say, use the below suggested code belwo instead using wa.

>
>>
>>
>>      start = gettime_us();
>>
>> @@ -1342,6 +1348,9 @@ static bool event_loop(struct test_output *o, 
>> unsigned duration_ms,
>>          if (count && (gettime_us() - start) / 1000 >= duration_ms)
>>              break;
>>
>> +        if (count && wa && o->flags & TEST_SUSPEND)
>>
>> if (count && wa && o->flags & TEST_SUSPEND) --> if (count && o->flags 
>> & TEST_SUSPEND)
>>
>> +            break;
>> +
>
> I am unsure of this approach. This will break the loop on second run on
> every suspend test, on every platform. While commit c75dcbdac51d
> ("tests/kms_flip: various improvements") says explicitly to do at least
> two rounds of tests, so it would change functionality on every platform.
> I can refactor the patch, but I would retain the WA. Can you provide
> some rationale for changing the functionality on non-affected platforms?

Thanks for exploring this. My suggestion was based on fact that 
clock_monotonic freezes during suspend,
which casue the duration check to never fire correctly  after 
suspend/resume cycle.
Instead adding pltform-specific WA for each new effected platform, I 
suggested a generic solution.

However, as per Daniel  vetter's commit c75dcbdac51d points intent is 
"at lease 2 rounds
always complite, as suspend tests were bailing after just 1 round and 
testing nothing".

I am okey with current WA approach and provided minor comment,
plz check  befrore next version.

>
> BR,
> Michał
>
>>          count++;
>>      }
>>
>>
>>
>>