[igt-dev] [PATCH i-g-t 2/2] tests/kms_psr_sink_crc: Test PSR source HW status.
Daniel Vetter
daniel at ffwll.ch
Thu Jul 5 09:39:22 UTC 2018
On Thu, Jul 05, 2018 at 08:31:10AM +0000, Pandiyan, Dhinakaran wrote:
> > -----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.
Ok if this is just interim then I think we're good. Note that the other
frontbuffer tracking tests don't just check whate we've entered/left the
power saving modes, but also checks the CRCs to make sure stuff looks
correct. But there we use the pipe CRC, and that doesn't work for PSR.
If PSR gives us a special PSR crc then that would be great. But we need
buy-in from hw engineers to make sure we can use it, and they promise to
us they'll keep it working.
-Daniel
>
>
> > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the igt-dev
mailing list