[igt-dev] [PATCH i-g-t 2/2] tests/kms_psr_sink_crc: Test PSR source HW status.
Pandiyan, Dhinakaran
dhinakaran.pandiyan at intel.com
Thu Jul 5 08:31:10 UTC 2018
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, July 4, 2018 7:19 AM
> To: Pandiyan, Dhinakaran <dhinakaran.pandiyan at intel.com>
> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>; IGT development <igt-
> dev at lists.freedesktop.org>
> Subject: Re: [igt-dev] [PATCH i-g-t 2/2] tests/kms_psr_sink_crc: Test PSR source
> HW status.
>
> 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.
>
There is PSR CRC as well - CRC of the last frame that was sent before the
hardware entered PSR. I have plans to add that as a CRC source, but haven't
checked how well it works.
This test here is to verify that the driver responded to a front buffer modification
and hardware tracking responded to a flip/cursor move, a reasonable replacement
for sink CRC to get us going.
> 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