[igt-dev] [PATCH i-g-t 2/2] tests/kms_psr_sink_crc: Test PSR source HW status.
Daniel Vetter
daniel at ffwll.ch
Wed Jul 4 14:19:25 UTC 2018
On Tue, Jul 03, 2018 at 11:35:46PM +0200, Daniel Vetter wrote:
> On Tue, Jul 3, 2018 at 8:43 PM, Dhinakaran Pandiyan
> <dhinakaran.pandiyan at intel.com> wrote:
> > 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.
>
> Hm ok. Might be good to summarize this more in the commit message.
>
> Also this means we can't validate PSR as-is. Have we adequately
> communicated this to hw engineers, and have they come up with a solid
> plan for at least future generations? I'd trust the status stuff much
> more if we'd have a 100% software-driven PSR implementation, but with
> the magic black-box I really don't like this kind of "validation".
> That's a huge gap, we need to somehow close it.
>
> Probably best if we start an internal thread, would be good to pull
> Kalyan into this too.
One idea we could try to explore is using one of the port CRCs, but making
sure we don't grab vblanks or interrupts or anything else that might
disturb the magic hw.
We ofc still need to get the guarantee from hw folks that they're blessing
that approach and guarantee that it'll keep working in the future. Same
way we've gotten their buy-in for using the pipe crc stuff.
-Daniel
>
> Cheers, Daniel
>
>
> > -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
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the igt-dev
mailing list