[igt-dev] [PATCH i-g-t 2/2] tests/kms_psr_sink_crc: Test PSR source HW status.
Daniel Vetter
daniel at ffwll.ch
Tue Jul 3 21:35:46 UTC 2018
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.
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
More information about the igt-dev
mailing list