Re: [igt-dev] [PATCH i-g-t 2/2] tests/kms_psr_sink_crc: Test PSR source HW status.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jul 03, 2018 at 10:44:47AM +0200, Daniel Vetter wrote:
> On Tue, Jul 03, 2018 at 10:43:55AM +0200, Daniel Vetter wrote:
> > On Mon, Jul 02, 2018 at 04:35:59PM -0700, Dhinakaran Pandiyan wrote:
> > > We make use of the status MMIO to tell whether the HW entered and
> > > exited.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 
> > The trouble with this is that this isn't independent verification. We
> > essentially have to believe the hw folks that their hw works, and the
> > Bspec folks that they documented stuff correctly.
> > 
> > Which is very little validation :-/
> > 
> > The entire point of the sink crc stuff was that we'd independently check
> > that the panel is actually showing the right pixels. Is there no way to
> > salvage that, using some hacks to make sure the dp aux stuff doesn't wake
> > up the panel or accidentally cause a psr exit? We'd need to make sure that
> > the hw isn't using the aux channel while we poke it ofc ...
> > 
> > Before we toss in the towel here I think would be good to check once more
> > with display hw architect whether our tests can't be salvaged. Since
> > without sink crc there's not much point in having them :-(
> 
> E.g. if the entire problem is the vblank wait in the debugfs interface
> then it's easy to convert that into a polling wait.

I wish that it was the only issue. :(

sink_crc has been a black whole for us in question of time, effort and hope.

First of the problems is that HW statement is clear: "Do not attempt to use
aux communication with PSR enabled".

For a while we had hope on the aux-mutex, but that is not reliable, not tested
and we shouldn't use according to hw engineers.

DK talked a lot to many HW and SW engineers. So I trust his judgement here.

Nor source, nor sink designed and implemented the sink_crc to be used like
we are trying to use here.

Yeap, the sink side of things is also apparently not prepared for this
case. Each panel that we try here seems to have a different behavior with same
code and same source.

So, for all the time we lost on trying to ducktape all these different issues
I believe it is now time to move to a more reliable validation. Might not be
the perfect one but at least it will be reliable.

Not that this is not just a fake validation of setting a bit and checking if
the bit was set. It is actually doing an operation and reading the status
bit to see if transaction is happening.

We might loose the final peace that is the final image, but our worst PSR
cases are when HW tracking is not trying to attempt any operation at all,
so these status bits should cover that.

Thanks,
Rodrigo.

> -Daniel
> 
> > 
> > Cheers, Daniel
> > 
> > > ---
> > >  tests/kms_psr_sink_crc.c | 25 +++++++++++++++++--------
> > >  1 file changed, 17 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> > > index d36be7dd..08d0ce9a 100644
> > > --- a/tests/kms_psr_sink_crc.c
> > > +++ b/tests/kms_psr_sink_crc.c
> > > @@ -195,7 +195,7 @@ static bool sink_support(data_t *data)
> > >  		strstr(buf, "Sink_Support: yes\n");
> > >  }
> > >  
> > > -static bool psr_enabled(data_t *data)
> > > +static bool psr_hw_enabled(data_t *data)
> > >  {
> > >  	char buf[512];
> > >  
> > > @@ -205,15 +205,23 @@ static bool psr_enabled(data_t *data)
> > >  		strstr(buf, "HW Enabled & Active bit: yes\n");
> > >  }
> > >  
> > > +static bool psr_hw_status(data_t *data, bool active)
> > > +{
> > > +	char buf[512];
> > > +
> > > +	igt_debugfs_read(data->drm_fd, "i915_edp_psr_status", buf);
> > > +
> > > +	/* TODO: Update the checks for PSR2 */
> > > +	return strstr(buf, "Source PSR status:") &&
> > > +	       (active ? !!strstr(buf, "SRDENT") : !strstr(buf, "SRDENT"));
> > > +}
> > > +
> > >  static bool wait_psr_entry(data_t *data)
> > >  {
> > > -	int timeout = 5;
> > > -	while (timeout--) {
> > > -		if (psr_enabled(data))
> > > -			return true;
> > > -		sleep(1);
> > > -	}
> > > -	return false;
> > > +	if (!psr_hw_enabled(data))
> > > +		return false;
> > > +
> > > +	return igt_wait(psr_hw_status(data, true), 500, 1);
> > >  }
> > >  
> > >  static inline void manual(const char *expected)
> > > @@ -303,6 +311,7 @@ static void run_test(data_t *data)
> > >  		expected = "screen GREEN";
> > >  		break;
> > >  	}
> > > +	igt_assert(psr_hw_status(data, false));
> > >  	manual(expected);
> > >  }
> > >  
> > > -- 
> > > 2.14.1
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev