[igt-dev] [PATCH i-g-t 1/2] tests/kms_psr_sink_crc: Do not test sink crc

[igt-dev] [PATCH i-g-t 1/2] tests/kms_psr_sink_crc: Do not test sink crc

[Dhinakaran Pandiyan]

eDP sink crc reads use vblank interrupts that cause PSR exit and
therefore makes them unsuitable for PSR testing.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 tests/kms_psr_sink_crc.c | 56 ++++++++----------------------------------------
 1 file changed, 9 insertions(+), 47 deletions(-)

diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
index 28818e25..d36be7dd 100644
--- a/tests/kms_psr_sink_crc.c
+++ b/tests/kms_psr_sink_crc.c
@@ -28,12 +28,8 @@
 #include <stdbool.h>
 #include <stdio.h>
 #include <string.h>
-
 #include "intel_bufmgr.h"
 
-#define CRC_BLACK "000000000000"
-#define CRC_LEN 12
-
 enum operations {
 	PAGE_FLIP,
 	MMAP_GTT,
@@ -220,36 +216,9 @@ static bool wait_psr_entry(data_t *data)
 	return false;
 }
 
-static void get_sink_crc(data_t *data, char *crc)
-{
-	if (igt_interactive_debug)
-		return;
-
-	igt_require_f(igt_sysfs_read(data->debugfs_fd, "i915_sink_crc_eDP1",
-				     crc, CRC_LEN) == CRC_LEN,
-		      "Sink CRC is unreliable on this machine. Try manual debug with --interactive-debug=no-crc\n");
-	igt_debug("sink CRC: %.*s\n", CRC_LEN, crc);
-
-	/* Black screen is always invalid */
-	igt_assert(strncmp(crc, CRC_BLACK, CRC_LEN));
-}
-
-static bool is_green(char *crc)
-{
-	const char *mask = "0000FFFF0000";
-	uint32_t *p = (uint32_t *)crc, *mask_p = (uint32_t *)mask;
-	if (igt_interactive_debug)
-		return false;
-
-	/* Check R and B components are 0 and G is non-zero */
-	return *p == *mask_p && *(p + 2) == *(mask_p + 2) &&
-	       (*(p + 1) & *(mask_p + 1)) != 0;
-}
-
-static void assert_or_manual(bool condition, const char *expected)
+static inline void manual(const char *expected)
 {
-	igt_debug_manual_check("no-crc", expected);
-	igt_assert(igt_interactive_debug || condition);
+	igt_debug_manual_check("all", expected);
 }
 
 static bool drrs_disabled(data_t *data)
@@ -266,18 +235,14 @@ static void run_test(data_t *data)
 	uint32_t handle = data->fb_white.gem_handle;
 	igt_plane_t *test_plane = data->test_plane;
 	void *ptr;
-	char ref_crc[CRC_LEN];
-	char crc[CRC_LEN];
 	const char *expected = "";
 
 	/* Confirm that screen became Green */
-	get_sink_crc(data, ref_crc);
-	assert_or_manual(is_green(ref_crc), "screen GREEN");
+	manual("screen GREEN");
 
 	/* Confirm screen stays Green after PSR got active */
 	igt_assert(wait_psr_entry(data));
-	get_sink_crc(data, ref_crc);
-	assert_or_manual(is_green(ref_crc), "screen GREEN");
+	manual("screen GREEN");
 
 	/* Setting a secondary fb/plane */
 	igt_plane_set_fb(test_plane, &data->fb_white);
@@ -285,19 +250,17 @@ static void run_test(data_t *data)
 
 	/* Confirm it is not Green anymore */
 	igt_assert(wait_psr_entry(data));
-	get_sink_crc(data, ref_crc);
 	if (test_plane->type == DRM_PLANE_TYPE_PRIMARY)
-		assert_or_manual(!is_green(ref_crc), "screen WHITE");
+		manual("screen WHITE");
 	else
-		assert_or_manual(!is_green(ref_crc), "GREEN background with WHITE box");
+		manual("GREEN background with WHITE box");
 
 	switch (data->op) {
 	case PAGE_FLIP:
 		/* Only in use when testing primary plane */
 		igt_assert(drmModePageFlip(data->drm_fd, data->crtc_id,
 					   data->fb_green.fb_id, 0, NULL) == 0);
-		get_sink_crc(data, crc);
-		assert_or_manual(is_green(crc), "screen GREEN");
+		manual("screen GREEN");
 		expected = "still GREEN";
 		break;
 	case MMAP_GTT:
@@ -340,8 +303,7 @@ static void run_test(data_t *data)
 		expected = "screen GREEN";
 		break;
 	}
-	get_sink_crc(data, crc);
-	assert_or_manual(strncmp(ref_crc, crc, CRC_LEN) != 0, expected);
+	manual(expected);
 }
 
 static void test_cleanup(data_t *data) {
@@ -442,7 +404,7 @@ static int opt_handler(int opt, int opt_index, void *_data)
 int main(int argc, char *argv[])
 {
 	const char *help_str =
-	       "  --no-psr\tRun test without PSR to check the CRC test logic.";
+	       "  --no-psr\tRun test without PSR.";
 	static struct option long_options[] = {
 		{"no-psr", 0, 0, 'n'},
 		{ 0, 0, 0, 0 }
-- 
2.14.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Dhinakaran Pandiyan]

We make use of the status MMIO to tell whether the HW entered and
exited.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Rodrigo Vivi]

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@intel.com>
> ---
>  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);

this read will trigger aux read and consequently psr exit right?

drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == 1)
                        seq_printf(m, "Sink PSR status: 0x%x [%s]\n", val,

I believe we need to split the debugfs interfaces to handle the mmio bits
without touching the aux ones what by itself triggers the psr exit


> +
> +	/* TODO: Update the checks for PSR2 */

hm... probably better to find a psr2 bit already here....

> +	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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Dhinakaran Pandiyan]

On Mon, 2018-07-02 at 17:17 -0700, Rodrigo Vivi 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@intel.com>
> > ---
> >  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);
> this read will trigger aux read and consequently psr exit right?
> 
> drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) == 1)
>                         seq_printf(m, "Sink PSR status: 0x%x [%s]\n",
> val,
> 
> I believe we need to split the debugfs interfaces to handle the mmio
> bits
> without touching the aux ones what by itself triggers the psr exit
> 
Yeah, I was going to make that change later but this patch exposes the
problem.

> 
> > 
> > +
> > +	/* TODO: Update the checks for PSR2 */
> hm... probably better to find a psr2 bit already here....
> 
Couldn't definitively gather the corresponding PSR2 state from the
spec. I'll have to test with PSR2 hardware and find out.

> > 
> > +	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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

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@intel.com>

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. Since
without sink crc there's not much point in having them :-(

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

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@intel.com>
> 
> 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. 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.
-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@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
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Rodrigo Vivi]

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@intel.com>
> > 
> > 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. 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.

> -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@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
> http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

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

[Daniel Vetter]

On Tue, Jul 3, 2018 at 8:43 PM, Dhinakaran Pandiyan
<dhinakaran.pandiyan@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@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@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

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@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@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@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Pandiyan, Dhinakaran]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, July 4, 2018 7:19 AM
> To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; IGT development <igt-
> dev@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@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@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@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

On Thu, Jul 05, 2018 at 08:31:10AM +0000, Pandiyan, Dhinakaran wrote:
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Wednesday, July 4, 2018 7:19 AM
> > To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
> > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; IGT development <igt-
> > dev@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@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@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@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/2] tests/kms_psr_sink_crc: Do not test sink crc

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/2] tests/kms_psr_sink_crc: Do not test sink crc
URL   : https://patchwork.freedesktop.org/series/45796/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4404 -> IGTPW_1523 =

== Summary - FAILURE ==

  Serious unknown changes coming with IGTPW_1523 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1523, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45796/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1523:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_sink_crc_basic:
      fi-whl-u:           PASS -> FAIL

    
    ==== Warnings ====

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      {fi-cfl-8109u}:     PASS -> SKIP +36

    
== Known issues ==

  Here are the changes found in IGTPW_1523 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_psr_sink_crc@basic:
      fi-cfl-s3:          PASS -> DMESG-FAIL (fdo#106678)
      fi-whl-u:           PASS -> DMESG-FAIL (fdo#106678)
      fi-kbl-7560u:       PASS -> DMESG-FAIL (fdo#106678)
      fi-skl-6700hq:      PASS -> DMESG-FAIL (fdo#105537)
      fi-skl-6600u:       PASS -> DMESG-FAIL (fdo#105537)
      fi-kbl-r:           PASS -> DMESG-FAIL (fdo#106678)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#105537 https://bugs.freedesktop.org/show_bug.cgi?id=105537
  fdo#106678 https://bugs.freedesktop.org/show_bug.cgi?id=106678


== Participating hosts (45 -> 40) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * IGT: IGT_4531 -> IGTPW_1523

  CI_DRM_4404: ceaab659002c938f1788b7458d5081fadc3c1ddc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1523: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1523/
  IGT_4531: a14bc8b4d69eaca189665de505e6b10cbfbb7730 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1523/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/2] tests/kms_psr_sink_crc: Do not test sink crc

[Rodrigo Vivi]

On Mon, Jul 02, 2018 at 04:35:58PM -0700, Dhinakaran Pandiyan wrote:
> eDP sink crc reads use vblank interrupts that cause PSR exit and
> therefore makes them unsuitable for PSR testing.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  tests/kms_psr_sink_crc.c | 56 ++++++++----------------------------------------
>  1 file changed, 9 insertions(+), 47 deletions(-)
> 
> diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> index 28818e25..d36be7dd 100644
> --- a/tests/kms_psr_sink_crc.c
> +++ b/tests/kms_psr_*sink_crc*.c

^^ ;)

rename the file now?
start a kms_psr.c from scratch?

> @@ -28,12 +28,8 @@
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <string.h>
> -
>  #include "intel_bufmgr.h"
>  
> -#define CRC_BLACK "000000000000"
> -#define CRC_LEN 12
> -
>  enum operations {
>  	PAGE_FLIP,
>  	MMAP_GTT,
> @@ -220,36 +216,9 @@ static bool wait_psr_entry(data_t *data)
>  	return false;
>  }
>  
> -static void get_sink_crc(data_t *data, char *crc)
> -{
> -	if (igt_interactive_debug)
> -		return;
> -
> -	igt_require_f(igt_sysfs_read(data->debugfs_fd, "i915_sink_crc_eDP1",
> -				     crc, CRC_LEN) == CRC_LEN,
> -		      "Sink CRC is unreliable on this machine. Try manual debug with --interactive-debug=no-crc\n");
> -	igt_debug("sink CRC: %.*s\n", CRC_LEN, crc);
> -
> -	/* Black screen is always invalid */
> -	igt_assert(strncmp(crc, CRC_BLACK, CRC_LEN));
> -}
> -
> -static bool is_green(char *crc)
> -{
> -	const char *mask = "0000FFFF0000";
> -	uint32_t *p = (uint32_t *)crc, *mask_p = (uint32_t *)mask;
> -	if (igt_interactive_debug)
> -		return false;
> -
> -	/* Check R and B components are 0 and G is non-zero */
> -	return *p == *mask_p && *(p + 2) == *(mask_p + 2) &&
> -	       (*(p + 1) & *(mask_p + 1)) != 0;
> -}
> -
> -static void assert_or_manual(bool condition, const char *expected)
> +static inline void manual(const char *expected)
>  {
> -	igt_debug_manual_check("no-crc", expected);
> -	igt_assert(igt_interactive_debug || condition);
> +	igt_debug_manual_check("all", expected);
>  }
>  
>  static bool drrs_disabled(data_t *data)
> @@ -266,18 +235,14 @@ static void run_test(data_t *data)
>  	uint32_t handle = data->fb_white.gem_handle;
>  	igt_plane_t *test_plane = data->test_plane;
>  	void *ptr;
> -	char ref_crc[CRC_LEN];
> -	char crc[CRC_LEN];
>  	const char *expected = "";
>  
>  	/* Confirm that screen became Green */
> -	get_sink_crc(data, ref_crc);
> -	assert_or_manual(is_green(ref_crc), "screen GREEN");
> +	manual("screen GREEN");
>  
>  	/* Confirm screen stays Green after PSR got active */
>  	igt_assert(wait_psr_entry(data));
> -	get_sink_crc(data, ref_crc);
> -	assert_or_manual(is_green(ref_crc), "screen GREEN");
> +	manual("screen GREEN");
>  
>  	/* Setting a secondary fb/plane */
>  	igt_plane_set_fb(test_plane, &data->fb_white);
> @@ -285,19 +250,17 @@ static void run_test(data_t *data)
>  
>  	/* Confirm it is not Green anymore */
>  	igt_assert(wait_psr_entry(data));
> -	get_sink_crc(data, ref_crc);
>  	if (test_plane->type == DRM_PLANE_TYPE_PRIMARY)
> -		assert_or_manual(!is_green(ref_crc), "screen WHITE");
> +		manual("screen WHITE");
>  	else
> -		assert_or_manual(!is_green(ref_crc), "GREEN background with WHITE box");
> +		manual("GREEN background with WHITE box");
>  
>  	switch (data->op) {
>  	case PAGE_FLIP:
>  		/* Only in use when testing primary plane */
>  		igt_assert(drmModePageFlip(data->drm_fd, data->crtc_id,
>  					   data->fb_green.fb_id, 0, NULL) == 0);
> -		get_sink_crc(data, crc);
> -		assert_or_manual(is_green(crc), "screen GREEN");
> +		manual("screen GREEN");
>  		expected = "still GREEN";
>  		break;
>  	case MMAP_GTT:
> @@ -340,8 +303,7 @@ static void run_test(data_t *data)
>  		expected = "screen GREEN";
>  		break;
>  	}
> -	get_sink_crc(data, crc);
> -	assert_or_manual(strncmp(ref_crc, crc, CRC_LEN) != 0, expected);
> +	manual(expected);
>  }
>  
>  static void test_cleanup(data_t *data) {
> @@ -442,7 +404,7 @@ static int opt_handler(int opt, int opt_index, void *_data)
>  int main(int argc, char *argv[])
>  {
>  	const char *help_str =
> -	       "  --no-psr\tRun test without PSR to check the CRC test logic.";
> +	       "  --no-psr\tRun test without PSR.";
>  	static struct option long_options[] = {
>  		{"no-psr", 0, 0, 'n'},
>  		{ 0, 0, 0, 0 }
> -- 
> 2.14.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/2] tests/kms_psr_sink_crc: Do not test sink crc

[Dhinakaran Pandiyan]

On Mon, 2018-07-02 at 17:13 -0700, Rodrigo Vivi wrote:
> On Mon, Jul 02, 2018 at 04:35:58PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > eDP sink crc reads use vblank interrupts that cause PSR exit and
> > therefore makes them unsuitable for PSR testing.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  tests/kms_psr_sink_crc.c | 56 ++++++++--------------------------
> > --------------
> >  1 file changed, 9 insertions(+), 47 deletions(-)
> > 
> > diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> > index 28818e25..d36be7dd 100644
> > --- a/tests/kms_psr_sink_crc.c
> > +++ b/tests/kms_psr_*sink_crc*.c
> ^^ ;)
> 
> rename the file now?
Will do :)

> start a kms_psr.c from scratch?
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev