* [PATCH igt] lib/debugfs: wait_for_keypress("crc") when collecting CRC
@ 2015-05-07 18:23 Paulo Zanoni
2015-05-08 7:29 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Paulo Zanoni @ 2015-05-07 18:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Let's just steal the "crc" namespace and add this by default to
igt_pipe_crc_collect_crc() instead of adding more calls to other
tests. If tests want special waits on just some of their collect_crc()
calls, they can use another name instead of "crc".
This is very useful when developing, especially when the CRC we get is
wrong: we want to look at the screen to see what's going on before we
can think about how to fix the problem. So let's add this to the lib
instead of adding this to every single test I need to debug.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
lib/igt_debugfs.c | 3 +++
tests/kms_draw_crc.c | 3 ---
tests/kms_plane.c | 2 --
3 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index e7b69de..d9761d8 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -35,6 +35,7 @@
#include <i915_drm.h>
#include "drmtest.h"
+#include "igt_aux.h"
#include "igt_kms.h"
#include "igt_debugfs.h"
@@ -512,6 +513,8 @@ static void crc_sanity_checks(igt_crc_t *crc)
*/
void igt_pipe_crc_collect_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t *out_crc)
{
+ igt_debug_wait_for_keypress("crc");
+
igt_pipe_crc_start(pipe_crc);
read_one_crc(pipe_crc, out_crc);
igt_pipe_crc_stop(pipe_crc);
diff --git a/tests/kms_draw_crc.c b/tests/kms_draw_crc.c
index 1630cc2..9fcf997 100644
--- a/tests/kms_draw_crc.c
+++ b/tests/kms_draw_crc.c
@@ -102,7 +102,6 @@ static void get_method_crc(enum igt_draw_method method, uint64_t tiling,
&ms.connector_id, 1, ms.mode);
igt_assert(rc == 0);
- igt_debug_wait_for_keypress("crc");
igt_pipe_crc_collect_crc(pipe_crc, crc);
kmstest_unset_all_crtcs(drm_fd, drm_res);
@@ -144,7 +143,6 @@ static void get_fill_crc(uint64_t tiling, igt_crc_t *crc)
&ms.connector_id, 1, ms.mode);
igt_assert(rc == 0);
- igt_debug_wait_for_keypress("crc");
igt_pipe_crc_collect_crc(pipe_crc, crc);
kmstest_unset_all_crtcs(drm_fd, drm_res);
@@ -171,7 +169,6 @@ static void fill_fb_subtest(void)
&ms.connector_id, 1, ms.mode);
igt_assert(rc == 0);
- igt_debug_wait_for_keypress("crc");
igt_pipe_crc_collect_crc(pipe_crc, &base_crc);
get_fill_crc(LOCAL_DRM_FORMAT_MOD_NONE, &crc);
diff --git a/tests/kms_plane.c b/tests/kms_plane.c
index dc83a89..741d0b4 100644
--- a/tests/kms_plane.c
+++ b/tests/kms_plane.c
@@ -330,8 +330,6 @@ test_plane_panning_with_output(data_t *data,
igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
- igt_debug_wait_for_keypress("crc");
-
if (flags & TEST_PANNING_TOP_LEFT)
igt_assert_crc_equal(&test.red_crc, &crc);
else
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH igt] lib/debugfs: wait_for_keypress("crc") when collecting CRC
2015-05-07 18:23 [PATCH igt] lib/debugfs: wait_for_keypress("crc") when collecting CRC Paulo Zanoni
@ 2015-05-08 7:29 ` Daniel Vetter
2015-05-08 14:07 ` Paulo Zanoni
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2015-05-08 7:29 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Thu, May 07, 2015 at 03:23:17PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Let's just steal the "crc" namespace and add this by default to
> igt_pipe_crc_collect_crc() instead of adding more calls to other
> tests. If tests want special waits on just some of their collect_crc()
> calls, they can use another name instead of "crc".
>
> This is very useful when developing, especially when the CRC we get is
> wrong: we want to look at the screen to see what's going on before we
> can think about how to fix the problem. So let's add this to the lib
> instead of adding this to every single test I need to debug.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
My idea was that we'd only capture crc for test pictures and not reference
pictures. But references pictures can equally go wrong, so this makes a
lot of sense. A-b: me. One request though: Can you please update the docs
to link to the interactive debug section and say that "crc" is the magic
keyword?
-Daniel
> ---
> lib/igt_debugfs.c | 3 +++
> tests/kms_draw_crc.c | 3 ---
> tests/kms_plane.c | 2 --
> 3 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index e7b69de..d9761d8 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -35,6 +35,7 @@
> #include <i915_drm.h>
>
> #include "drmtest.h"
> +#include "igt_aux.h"
> #include "igt_kms.h"
> #include "igt_debugfs.h"
>
> @@ -512,6 +513,8 @@ static void crc_sanity_checks(igt_crc_t *crc)
> */
> void igt_pipe_crc_collect_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t *out_crc)
> {
> + igt_debug_wait_for_keypress("crc");
> +
> igt_pipe_crc_start(pipe_crc);
> read_one_crc(pipe_crc, out_crc);
> igt_pipe_crc_stop(pipe_crc);
> diff --git a/tests/kms_draw_crc.c b/tests/kms_draw_crc.c
> index 1630cc2..9fcf997 100644
> --- a/tests/kms_draw_crc.c
> +++ b/tests/kms_draw_crc.c
> @@ -102,7 +102,6 @@ static void get_method_crc(enum igt_draw_method method, uint64_t tiling,
> &ms.connector_id, 1, ms.mode);
> igt_assert(rc == 0);
>
> - igt_debug_wait_for_keypress("crc");
> igt_pipe_crc_collect_crc(pipe_crc, crc);
>
> kmstest_unset_all_crtcs(drm_fd, drm_res);
> @@ -144,7 +143,6 @@ static void get_fill_crc(uint64_t tiling, igt_crc_t *crc)
> &ms.connector_id, 1, ms.mode);
> igt_assert(rc == 0);
>
> - igt_debug_wait_for_keypress("crc");
> igt_pipe_crc_collect_crc(pipe_crc, crc);
>
> kmstest_unset_all_crtcs(drm_fd, drm_res);
> @@ -171,7 +169,6 @@ static void fill_fb_subtest(void)
> &ms.connector_id, 1, ms.mode);
> igt_assert(rc == 0);
>
> - igt_debug_wait_for_keypress("crc");
> igt_pipe_crc_collect_crc(pipe_crc, &base_crc);
>
> get_fill_crc(LOCAL_DRM_FORMAT_MOD_NONE, &crc);
> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> index dc83a89..741d0b4 100644
> --- a/tests/kms_plane.c
> +++ b/tests/kms_plane.c
> @@ -330,8 +330,6 @@ test_plane_panning_with_output(data_t *data,
>
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
>
> - igt_debug_wait_for_keypress("crc");
> -
> if (flags & TEST_PANNING_TOP_LEFT)
> igt_assert_crc_equal(&test.red_crc, &crc);
> else
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH igt] lib/debugfs: wait_for_keypress("crc") when collecting CRC
2015-05-08 7:29 ` Daniel Vetter
@ 2015-05-08 14:07 ` Paulo Zanoni
2015-05-08 14:27 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Paulo Zanoni @ 2015-05-08 14:07 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Paulo Zanoni
2015-05-08 4:29 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, May 07, 2015 at 03:23:17PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Let's just steal the "crc" namespace and add this by default to
>> igt_pipe_crc_collect_crc() instead of adding more calls to other
>> tests. If tests want special waits on just some of their collect_crc()
>> calls, they can use another name instead of "crc".
>>
>> This is very useful when developing, especially when the CRC we get is
>> wrong: we want to look at the screen to see what's going on before we
>> can think about how to fix the problem. So let's add this to the lib
>> instead of adding this to every single test I need to debug.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> My idea was that we'd only capture crc for test pictures and not reference
> pictures.
And how do we differentiate this inside the lib? This patch was
suggested by you while reviewing one of my previous patches. You
explicitly said we should have the wait_for_keypress("crc") call
inside igt_pipe_crc_collect_crc().
Since the library can't guess what's what, if someone wants to get
just the test pictures, they should add their own
wait_for_keypress("somethingelse") calls.
> But references pictures can equally go wrong, so this makes a
> lot of sense. A-b: me. One request though: Can you please update the docs
> to link to the interactive debug section and say that "crc" is the magic
> keyword?
Good idea. I'll do this.
> -Daniel
>
>> ---
>> lib/igt_debugfs.c | 3 +++
>> tests/kms_draw_crc.c | 3 ---
>> tests/kms_plane.c | 2 --
>> 3 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
>> index e7b69de..d9761d8 100644
>> --- a/lib/igt_debugfs.c
>> +++ b/lib/igt_debugfs.c
>> @@ -35,6 +35,7 @@
>> #include <i915_drm.h>
>>
>> #include "drmtest.h"
>> +#include "igt_aux.h"
>> #include "igt_kms.h"
>> #include "igt_debugfs.h"
>>
>> @@ -512,6 +513,8 @@ static void crc_sanity_checks(igt_crc_t *crc)
>> */
>> void igt_pipe_crc_collect_crc(igt_pipe_crc_t *pipe_crc, igt_crc_t *out_crc)
>> {
>> + igt_debug_wait_for_keypress("crc");
>> +
>> igt_pipe_crc_start(pipe_crc);
>> read_one_crc(pipe_crc, out_crc);
>> igt_pipe_crc_stop(pipe_crc);
>> diff --git a/tests/kms_draw_crc.c b/tests/kms_draw_crc.c
>> index 1630cc2..9fcf997 100644
>> --- a/tests/kms_draw_crc.c
>> +++ b/tests/kms_draw_crc.c
>> @@ -102,7 +102,6 @@ static void get_method_crc(enum igt_draw_method method, uint64_t tiling,
>> &ms.connector_id, 1, ms.mode);
>> igt_assert(rc == 0);
>>
>> - igt_debug_wait_for_keypress("crc");
>> igt_pipe_crc_collect_crc(pipe_crc, crc);
>>
>> kmstest_unset_all_crtcs(drm_fd, drm_res);
>> @@ -144,7 +143,6 @@ static void get_fill_crc(uint64_t tiling, igt_crc_t *crc)
>> &ms.connector_id, 1, ms.mode);
>> igt_assert(rc == 0);
>>
>> - igt_debug_wait_for_keypress("crc");
>> igt_pipe_crc_collect_crc(pipe_crc, crc);
>>
>> kmstest_unset_all_crtcs(drm_fd, drm_res);
>> @@ -171,7 +169,6 @@ static void fill_fb_subtest(void)
>> &ms.connector_id, 1, ms.mode);
>> igt_assert(rc == 0);
>>
>> - igt_debug_wait_for_keypress("crc");
>> igt_pipe_crc_collect_crc(pipe_crc, &base_crc);
>>
>> get_fill_crc(LOCAL_DRM_FORMAT_MOD_NONE, &crc);
>> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
>> index dc83a89..741d0b4 100644
>> --- a/tests/kms_plane.c
>> +++ b/tests/kms_plane.c
>> @@ -330,8 +330,6 @@ test_plane_panning_with_output(data_t *data,
>>
>> igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
>>
>> - igt_debug_wait_for_keypress("crc");
>> -
>> if (flags & TEST_PANNING_TOP_LEFT)
>> igt_assert_crc_equal(&test.red_crc, &crc);
>> else
>> --
>> 2.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH igt] lib/debugfs: wait_for_keypress("crc") when collecting CRC
2015-05-08 14:07 ` Paulo Zanoni
@ 2015-05-08 14:27 ` Daniel Vetter
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2015-05-08 14:27 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni
On Fri, May 08, 2015 at 11:07:30AM -0300, Paulo Zanoni wrote:
> 2015-05-08 4:29 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, May 07, 2015 at 03:23:17PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Let's just steal the "crc" namespace and add this by default to
> >> igt_pipe_crc_collect_crc() instead of adding more calls to other
> >> tests. If tests want special waits on just some of their collect_crc()
> >> calls, they can use another name instead of "crc".
> >>
> >> This is very useful when developing, especially when the CRC we get is
> >> wrong: we want to look at the screen to see what's going on before we
> >> can think about how to fix the problem. So let's add this to the lib
> >> instead of adding this to every single test I need to debug.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > My idea was that we'd only capture crc for test pictures and not reference
> > pictures.
>
> And how do we differentiate this inside the lib? This patch was
> suggested by you while reviewing one of my previous patches. You
> explicitly said we should have the wait_for_keypress("crc") call
> inside igt_pipe_crc_collect_crc().
>
> Since the library can't guess what's what, if someone wants to get
> just the test pictures, they should add their own
> wait_for_keypress("somethingelse") calls.
Fully agreed, my comment wasn't meant as critique, just some information
why I didn't do this right away.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-05-08 14:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-07 18:23 [PATCH igt] lib/debugfs: wait_for_keypress("crc") when collecting CRC Paulo Zanoni
2015-05-08 7:29 ` Daniel Vetter
2015-05-08 14:07 ` Paulo Zanoni
2015-05-08 14:27 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox