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

[Dhinakaran Pandiyan]

On Tue, 2018-07-03 at 09:32 -0700, Rodrigo Vivi wrote:
> 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 at intel.c
> > > > om>
> > > 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
The recommendation was not to use the aux ch with PSR enabled.

> > > . 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.
> 
Rodrigo has covered all the points. Even if we fix issues with sink crc
reads in the driver, I doubt we can get it to work consistently well
enough for testing.

-DK


> > 
> > -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 at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch