RE: [PATCH i-g-t v1 1/2] lib/igt_psr: Skip the test when PSR is disabled

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 02 Jun 2025, "Grzelak, Michal" <michal.grzelak@intel.com> wrote:
> Hi Jeevan,
>
>> -----Original Message-----
>> From: B, Jeevan <jeevan.b@intel.com> 
>> Sent: Monday, May 26, 2025 7:38 AM
>> To: Grzelak, Michal <michal.grzelak@intel.com>; igt-dev@lists.freedesktop.org
>> Cc: Grzelak, Michal <michal.grzelak@intel.com>
>> Subject: RE: [PATCH i-g-t v1 1/2] lib/igt_psr: Skip the test when PSR is disabled
>
>>> -----Original Message-----
>>> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of 
>>> Michal Grzelak
>>> Sent: Tuesday, May 13, 2025 5:49 PM
>>> To: igt-dev@lists.freedesktop.org
>>> Cc: Grzelak, Michal <michal.grzelak@intel.com>
>>> Subject: [PATCH i-g-t v1 1/2] lib/igt_psr: Skip the test when PSR is 
>>> disabled
>>> 
>>> If PSR setup timing is not met, then PSR will stay disabled and test 
>>> will fail anyway. Skip the test upon finding that setup timing was not met.
>>> 
>>> Signed-off-by: Michał Grzelak <michal.grzelak@intel.com>
>>> ---
>>>  lib/igt_psr.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/lib/igt_psr.c b/lib/igt_psr.c index 3d8f1949b..f35051ebc 
>>> 100644
>>> --- a/lib/igt_psr.c
>>> +++ b/lib/igt_psr.c
>>> @@ -38,7 +38,7 @@ bool psr_disabled_check(int debugfs_fd)
>>>  	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
>>>  				sizeof(buf));
>>> 
>>> -	return strstr(buf, "PSR mode: disabled\n");
>>Lets not change the format here. We can still keep it as PSR mode : disabled/enabled
>>> +	return strstr(buf, "PSR disabled:");
>>>  }
>>> 
>>>  bool selective_fetch_check(int debugfs_fd, igt_output_t *output) @@ 
>>> -122,6
>>> +122,8 @@ static bool psr_active_check(int debugfs_fd, enum psr_mode 
>>> +mode,
>>> igt_output_t *o
>>> 
>>>  	igt_skip_on(strstr(buf, "PSR sink not reliable: yes"));
>>> 
>>> +	igt_skip_on(strstr(buf, "PSR setup timing not met"));
>>> +
>> I think instead of printing like this. It can be:  PSR sink not reliable: yes (setup timing not met)
>
> My current understanding is that kernel on JSL reports that sink is reliable. But it disables PSR basing on the setup timing. Thus, questioned lines in i915_edp_psr_status with this change look like this:
>
> PSR mode: disabled
> PSR sink not reliable: no (PSR setup timing not met)
>
> I thought it looks a bit misleading, because it seems like we are giving the reason why the sink is reliable, and not why PSR is disabled. That's why I also changed the format, to include this case. Also, because of that, in the IGT test we would still need two different checks for skip, since "sink not reliable:" is independent of "PSR setup timing not met".
>
> What are your thoughts on this? Should we keep the reason after "sink not reliable:", change the format, or something else?

I think overall the PSR should migrate towards what we have for FBC. If
PSR is disabled, we want to know the reason.

"PSR sink not reliable: yes (setup timing not met)" is not
interesting. Moreover, "setup timing not met" is not the reason sink is
not reliable. Both "setup timing not met" *and* "sink is not reliable"
are orthogonal reasons for not enabling PSR.

I think we should go towards something like "PSR disabled: <reason>".

BR,
Jani.

-- 
Jani Nikula, Intel