Re: [Intel-gfx] [PATCH] Fixed the review issues for pm_rc6_residency IGT case
On Tue, Jun 17, 2014 at 12:03 AM, Jesse Barnes jbar...@virtuousgeek.org wrote: So, what is revert-worthy in i-g-t? Open review items? Requests for change? False test failures? False test passes? Crashing tests? I'd vote for the latter 3 myself; did this fall into any of those categories? Imo this isn't about when to revert, but about the responsibilities of the committer. Especially for new contributors that imo includes full review and making sure the test has a bit polish applied and uses all the latest igt infrastructure. My impression was that Ben didn't do that so I asked him whether he'll follow up. From irc I've gotten the impression that he won't do that work (maybe that was a misunderstanding) and that I should go ahead. That I reverted the patch is just personal choice - for polishing patches I much prefer one clean patch that gets revised than committing early and then applying a few fixup patches. It's simply the process I'm used to from the kernel side. I don't mind at all if other people do this differently, as long as it gets done. Which in this case here didn't look like. And if you've read my first mail in this thread there's still work left to do here. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Fixed the review issues for pm_rc6_residency IGT case
Hi Wendy. Daniel has reverted your original commit here: commit 35554a1bcaaea55c1cfa88c0176c58d2fb3b8013 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Tue Jun 10 11:05:16 2014 +0200 Revert Add rc6_residency_counter subtest Note that I absolutely do not agree with the decision to revert your patch as was stated in the commit message. I am not sure how Daniel got the impression that I thought this was in order. Can you please resubmit the patch based on the latest intel-gpu-tools? -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Fixed the review issues for pm_rc6_residency IGT case
On Mon, 16 Jun 2014 11:43:30 -0700 Ben Widawsky benjamin.widaw...@intel.com wrote: Hi Wendy. Daniel has reverted your original commit here: commit 35554a1bcaaea55c1cfa88c0176c58d2fb3b8013 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Tue Jun 10 11:05:16 2014 +0200 Revert Add rc6_residency_counter subtest Note that I absolutely do not agree with the decision to revert your patch as was stated in the commit message. I am not sure how Daniel got the impression that I thought this was in order. Can you please resubmit the patch based on the latest intel-gpu-tools? I also made that clear when Daniel and I discussed it. I simply don't understand why a revert was necessary, especially given that we had an incremental patch to address many of the comments. Was the test breaking i-g-t runs (i.e. preventing tests from running)? Was it somehow crashing and causing false reports? -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Fixed the review issues for pm_rc6_residency IGT case
On Mon, Jun 16, 2014 at 10:38 PM, Jesse Barnes jbar...@virtuousgeek.org wrote: On Mon, 16 Jun 2014 11:43:30 -0700 Ben Widawsky benjamin.widaw...@intel.com wrote: Hi Wendy. Daniel has reverted your original commit here: commit 35554a1bcaaea55c1cfa88c0176c58d2fb3b8013 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Tue Jun 10 11:05:16 2014 +0200 Revert Add rc6_residency_counter subtest Note that I absolutely do not agree with the decision to revert your patch as was stated in the commit message. I am not sure how Daniel got the impression that I thought this was in order. Can you please resubmit the patch based on the latest intel-gpu-tools? I also made that clear when Daniel and I discussed it. I simply don't understand why a revert was necessary, especially given that we had an incremental patch to address many of the comments. Was the test breaking i-g-t runs (i.e. preventing tests from running)? Was it somehow crashing and causing false reports? Ok, I've reverted the revert since people are too unhappy with it. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Fixed the review issues for pm_rc6_residency IGT case
On Mon, Jun 09, 2014 at 04:36:47PM +0800, Wendy Wang wrote:
Why need add rc6_residency_counter subtest case:
RC6 feature support residency counter,from power consumption aspect,
the counter closer to 1,the better.If the counter is 0.9, the residency
is not good and will impact power consumption value, if the counter is 1,
sysfs file is inaccurate.
Attach the test result message:
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency
IGT-Version: 1.6-g9a70e29 (x86_64) (Linux:
3.15.0-rc7_drm-intel-nightly_0a37b5_20140604+ x86_64)
Subtest rc6-residency-check: SUCCESS
This machine doesn't support rc6pp
This machine doesn't support rc6p
The residency counter : 0.987000
This machine entry rc6 state.
Subtest rc6-residency-counter: SUCCESS
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency --run-subtest rc6-residency-counter
IGT-Version: 1.6-g9a70e29 (x86_64) (Linux:
3.15.0-rc7_drm-intel-nightly_0a37b5_20140604+ x86_64)
This machine doesn't support rc6pp
This machine doesn't support rc6p
The residency counter : 0.987000
This machine entry rc6 state.
Subtest rc6-residency-counter: SUCCESS
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency --run-subtest rc6-residency-check
IGT-Version: 1.6-g9a70e29 (x86_64) (Linux:
3.15.0-rc7_drm-intel-nightly_0a37b5_20140604+ x86_64)
Subtest rc6-residency-check: SUCCESS
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency --list
rc6-residency-check
rc6-residency-counter
Run as non-root
[haha@x-pk home]$ ./pm_rc6_residency
IGT-Version: 1.6-g18d2130 (x86_64) (Linux:
3.13.0-rc3_drm-intel-nightly_639e4d_20131210+ x86_64)
No intel gpu found
Subtest rc6-residency-check: SKIP
Subtest rc6-residency-counter: SKIP
Run on non-intel platform
[root@x-pk5 home]# ./pm_rc6_residency
IGT-Version: 1.6-g18d2130 (x86_64) (Linux:
3.13.0-rc3_drm-intel-nightly_639e4d_20131210+ x86_64)
Test requirement not met in function read_rc6_residency, file
pm_rc6_residency.c:77:
Last errno: 2, No such file or directory
Test requirement: (!(file))
Subtest rc6-residency-check: SKIP
Subtest rc6-residency-counter: SKIP
Signed-off-by: Wendy Wang wendy.w...@intel.com
Patch applied, thanks.
-Daniel
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index e4c23b3..214c055 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -72,6 +72,7 @@ TESTS_progs_M = \
pm_lpsp \
pm_rpm \
pm_rps \
+ pm_rc6_residency \
prime_self_import \
template \
$(NULL)
@@ -138,7 +139,6 @@ TESTS_progs = \
kms_sink_crc_basic \
kms_fence_pin_leak \
pm_psr \
- pm_rc6_residency \
prime_udl \
$(NULL)
diff --git a/tests/pm_rc6_residency.c b/tests/pm_rc6_residency.c
index 550e3ad..d364369 100644
--- a/tests/pm_rc6_residency.c
+++ b/tests/pm_rc6_residency.c
@@ -38,7 +38,6 @@
#define SLEEP_DURATION 3000 // in milliseconds
#define RC6_FUDGE 900 // in milliseconds
-
static unsigned int readit(const char *path)
{
unsigned int ret;
@@ -60,6 +59,7 @@ static unsigned int readit(const char *path)
static void read_rc6_residency( int value[], const char
*name_of_rc6_residency[])
{
+ unsigned int i;
const int device = drm_get_card();
char *path ;
int ret;
@@ -72,32 +72,33 @@ static void read_rc6_residency( int value[], const char
*name_of_rc6_residency[]
ret = asprintf(path, /sys/class/drm/card%d/power/rc6_enable, device);
igt_assert(ret != -1);
- file = fopen(path, r);//open
+ file = fopen(path, r);
igt_require(file);
/* claim success if no rc6 enabled. */
if (readit(path) == 0)
igt_success();
- for(unsigned int i = 0; i 6; i++)
+ for(i = 0; i 6; i++)
{
if(i == 3)
sleep(SLEEP_DURATION / 1000);
- ret = asprintf(path,
/sys/class/drm/card%d/power/%s_residency_ms,device,name_of_rc6_residency[i
% 3] );
+ ret = asprintf(path,
/sys/class/drm/card%d/power/%s_residency_ms,device,name_of_rc6_residency[i
% 3]);
igt_assert(ret != -1);
value[i] = readit(path);
}
free(path);
}
-static void rc6_residency_counter(int value[],const char *
name_of_rc6_residency[])
+static void rc6_residency_counter(int value[],const char
*name_of_rc6_residency[])
{
+ int flag;
unsigned int flag_counter,flag_support;
- double counter_result = 0;
+ double counter_result = 0;
flag_counter = 0;
flag_support = 0;
- for(int flag = NUMBER_OF_RC6_RESIDENCY-1; flag = 0 ; flag --)
+ for(flag = NUMBER_OF_RC6_RESIDENCY-1; flag = 0; flag --)
{
unsigned int tmp_counter, tmp_support;
double counter;
@@ -124,11 +125,10 @@ static void
Re: [Intel-gfx] [PATCH] Fixed the review issues for pm_rc6_residency IGT case
On Mon, 16 Jun 2014 23:55:24 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Mon, Jun 16, 2014 at 10:38 PM, Jesse Barnes jbar...@virtuousgeek.org wrote: On Mon, 16 Jun 2014 11:43:30 -0700 Ben Widawsky benjamin.widaw...@intel.com wrote: Hi Wendy. Daniel has reverted your original commit here: commit 35554a1bcaaea55c1cfa88c0176c58d2fb3b8013 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Tue Jun 10 11:05:16 2014 +0200 Revert Add rc6_residency_counter subtest Note that I absolutely do not agree with the decision to revert your patch as was stated in the commit message. I am not sure how Daniel got the impression that I thought this was in order. Can you please resubmit the patch based on the latest intel-gpu-tools? I also made that clear when Daniel and I discussed it. I simply don't understand why a revert was necessary, especially given that we had an incremental patch to address many of the comments. Was the test breaking i-g-t runs (i.e. preventing tests from running)? Was it somehow crashing and causing false reports? Ok, I've reverted the revert since people are too unhappy with it. Fine, but that doesn't address why we needed the revert in the first place. Until we have some clear explanation of that and some criteria, this will just happen all over again down the road. So, what is revert-worthy in i-g-t? Open review items? Requests for change? False test failures? False test passes? Crashing tests? I'd vote for the latter 3 myself; did this fall into any of those categories? Thanks, -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Fixed the review issues for pm_rc6_residency IGT case
Hi Wendy,
I think it's better to squash this into the original patch so I've
reverted your patch from i-g-t for now. Also chatted with Ben on irc and
he's ok with that.
More comments after I've read your test more carefully:
- Please don't use abort() or exit() anywhere in your test. Use the igt
macros and functions instead so that the test result handling is done
properly. We have rather good igt documentation now, I suggest
http://people.freedesktop.org/~danvet/igt/intel-gpu-tools-i-g-t-core.html
as a starting point.
- The new subtest doesn't really test anything new, but only adds more
informational output afaics. I think it's better to just merge that into
the existing basic subtest for rc6 residency.
You could also use igt_debug() to print optional diagnostical
information. See the above documentation for how to use it.
- When we convert an existing simple testcase into a test with subtests we
call the basic subtest basic. This way it's easier for QA to track
regressions and test status across such reorganizations. Also for
subtests there's no reason to repeat the overall testname so here you
can drop the rc6_residency part since that's already included in the
pm_rc6_residency name.
On a quick glance your fixup here looks good. Please cc me on the next
revision so that I can work together with you to polish this patch.
Thanks, Daniel
On Mon, Jun 09, 2014 at 04:36:47PM +0800, Wendy Wang wrote:
Why need add rc6_residency_counter subtest case:
RC6 feature support residency counter,from power consumption aspect,
the counter closer to 1,the better.If the counter is 0.9, the residency
is not good and will impact power consumption value, if the counter is 1,
sysfs file is inaccurate.
Attach the test result message:
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency
IGT-Version: 1.6-g9a70e29 (x86_64) (Linux:
3.15.0-rc7_drm-intel-nightly_0a37b5_20140604+ x86_64)
Subtest rc6-residency-check: SUCCESS
This machine doesn't support rc6pp
This machine doesn't support rc6p
The residency counter : 0.987000
This machine entry rc6 state.
Subtest rc6-residency-counter: SUCCESS
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency --run-subtest rc6-residency-counter
IGT-Version: 1.6-g9a70e29 (x86_64) (Linux:
3.15.0-rc7_drm-intel-nightly_0a37b5_20140604+ x86_64)
This machine doesn't support rc6pp
This machine doesn't support rc6p
The residency counter : 0.987000
This machine entry rc6 state.
Subtest rc6-residency-counter: SUCCESS
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency --run-subtest rc6-residency-check
IGT-Version: 1.6-g9a70e29 (x86_64) (Linux:
3.15.0-rc7_drm-intel-nightly_0a37b5_20140604+ x86_64)
Subtest rc6-residency-check: SUCCESS
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency --list
rc6-residency-check
rc6-residency-counter
Run as non-root
[haha@x-pk home]$ ./pm_rc6_residency
IGT-Version: 1.6-g18d2130 (x86_64) (Linux:
3.13.0-rc3_drm-intel-nightly_639e4d_20131210+ x86_64)
No intel gpu found
Subtest rc6-residency-check: SKIP
Subtest rc6-residency-counter: SKIP
Run on non-intel platform
[root@x-pk5 home]# ./pm_rc6_residency
IGT-Version: 1.6-g18d2130 (x86_64) (Linux:
3.13.0-rc3_drm-intel-nightly_639e4d_20131210+ x86_64)
Test requirement not met in function read_rc6_residency, file
pm_rc6_residency.c:77:
Last errno: 2, No such file or directory
Test requirement: (!(file))
Subtest rc6-residency-check: SKIP
Subtest rc6-residency-counter: SKIP
Signed-off-by: Wendy Wang wendy.w...@intel.com
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index e4c23b3..214c055 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -72,6 +72,7 @@ TESTS_progs_M = \
pm_lpsp \
pm_rpm \
pm_rps \
+ pm_rc6_residency \
prime_self_import \
template \
$(NULL)
@@ -138,7 +139,6 @@ TESTS_progs = \
kms_sink_crc_basic \
kms_fence_pin_leak \
pm_psr \
- pm_rc6_residency \
prime_udl \
$(NULL)
diff --git a/tests/pm_rc6_residency.c b/tests/pm_rc6_residency.c
index 550e3ad..d364369 100644
--- a/tests/pm_rc6_residency.c
+++ b/tests/pm_rc6_residency.c
@@ -38,7 +38,6 @@
#define SLEEP_DURATION 3000 // in milliseconds
#define RC6_FUDGE 900 // in milliseconds
-
static unsigned int readit(const char *path)
{
unsigned int ret;
@@ -60,6 +59,7 @@ static unsigned int readit(const char *path)
static void read_rc6_residency( int value[], const char
*name_of_rc6_residency[])
{
+ unsigned int i;
const int device = drm_get_card();
char *path ;
int ret;
@@ -72,32 +72,33 @@ static void read_rc6_residency( int value[], const char
*name_of_rc6_residency[]
ret = asprintf(path, /sys/class/drm/card%d/power/rc6_enable, device);
igt_assert(ret !=
[Intel-gfx] [PATCH] Fixed the review issues for pm_rc6_residency IGT case
Why need add rc6_residency_counter subtest case:
RC6 feature support residency counter,from power consumption aspect,
the counter closer to 1,the better.If the counter is 0.9, the residency
is not good and will impact power consumption value, if the counter is 1,
sysfs file is inaccurate.
Attach the test result message:
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests# ./pm_rc6_residency
IGT-Version: 1.6-g9a70e29 (x86_64) (Linux:
3.15.0-rc7_drm-intel-nightly_0a37b5_20140604+ x86_64)
Subtest rc6-residency-check: SUCCESS
This machine doesn't support rc6pp
This machine doesn't support rc6p
The residency counter : 0.987000
This machine entry rc6 state.
Subtest rc6-residency-counter: SUCCESS
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency --run-subtest rc6-residency-counter
IGT-Version: 1.6-g9a70e29 (x86_64) (Linux:
3.15.0-rc7_drm-intel-nightly_0a37b5_20140604+ x86_64)
This machine doesn't support rc6pp
This machine doesn't support rc6p
The residency counter : 0.987000
This machine entry rc6 state.
Subtest rc6-residency-counter: SUCCESS
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency --run-subtest rc6-residency-check
IGT-Version: 1.6-g9a70e29 (x86_64) (Linux:
3.15.0-rc7_drm-intel-nightly_0a37b5_20140604+ x86_64)
Subtest rc6-residency-check: SUCCESS
root@x-bdw05:/GFX/Test/Intel_gpu_tools/intel-gpu-tools/tests#
./pm_rc6_residency --list
rc6-residency-check
rc6-residency-counter
Run as non-root
[haha@x-pk home]$ ./pm_rc6_residency
IGT-Version: 1.6-g18d2130 (x86_64) (Linux:
3.13.0-rc3_drm-intel-nightly_639e4d_20131210+ x86_64)
No intel gpu found
Subtest rc6-residency-check: SKIP
Subtest rc6-residency-counter: SKIP
Run on non-intel platform
[root@x-pk5 home]# ./pm_rc6_residency
IGT-Version: 1.6-g18d2130 (x86_64) (Linux:
3.13.0-rc3_drm-intel-nightly_639e4d_20131210+ x86_64)
Test requirement not met in function read_rc6_residency, file
pm_rc6_residency.c:77:
Last errno: 2, No such file or directory
Test requirement: (!(file))
Subtest rc6-residency-check: SKIP
Subtest rc6-residency-counter: SKIP
Signed-off-by: Wendy Wang wendy.w...@intel.com
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index e4c23b3..214c055 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -72,6 +72,7 @@ TESTS_progs_M = \
pm_lpsp \
pm_rpm \
pm_rps \
+ pm_rc6_residency \
prime_self_import \
template \
$(NULL)
@@ -138,7 +139,6 @@ TESTS_progs = \
kms_sink_crc_basic \
kms_fence_pin_leak \
pm_psr \
- pm_rc6_residency \
prime_udl \
$(NULL)
diff --git a/tests/pm_rc6_residency.c b/tests/pm_rc6_residency.c
index 550e3ad..d364369 100644
--- a/tests/pm_rc6_residency.c
+++ b/tests/pm_rc6_residency.c
@@ -38,7 +38,6 @@
#define SLEEP_DURATION 3000 // in milliseconds
#define RC6_FUDGE 900 // in milliseconds
-
static unsigned int readit(const char *path)
{
unsigned int ret;
@@ -60,6 +59,7 @@ static unsigned int readit(const char *path)
static void read_rc6_residency( int value[], const char
*name_of_rc6_residency[])
{
+ unsigned int i;
const int device = drm_get_card();
char *path ;
int ret;
@@ -72,32 +72,33 @@ static void read_rc6_residency( int value[], const char
*name_of_rc6_residency[]
ret = asprintf(path, /sys/class/drm/card%d/power/rc6_enable, device);
igt_assert(ret != -1);
- file = fopen(path, r);//open
+ file = fopen(path, r);
igt_require(file);
/* claim success if no rc6 enabled. */
if (readit(path) == 0)
igt_success();
- for(unsigned int i = 0; i 6; i++)
+ for(i = 0; i 6; i++)
{
if(i == 3)
sleep(SLEEP_DURATION / 1000);
- ret = asprintf(path,
/sys/class/drm/card%d/power/%s_residency_ms,device,name_of_rc6_residency[i %
3] );
+ ret = asprintf(path,
/sys/class/drm/card%d/power/%s_residency_ms,device,name_of_rc6_residency[i %
3]);
igt_assert(ret != -1);
value[i] = readit(path);
}
free(path);
}
-static void rc6_residency_counter(int value[],const char *
name_of_rc6_residency[])
+static void rc6_residency_counter(int value[],const char
*name_of_rc6_residency[])
{
+ int flag;
unsigned int flag_counter,flag_support;
- double counter_result = 0;
+ double counter_result = 0;
flag_counter = 0;
flag_support = 0;
- for(int flag = NUMBER_OF_RC6_RESIDENCY-1; flag = 0 ; flag --)
+ for(flag = NUMBER_OF_RC6_RESIDENCY-1; flag = 0; flag --)
{
unsigned int tmp_counter, tmp_support;
double counter;
@@ -124,11 +125,10 @@ static void rc6_residency_counter(int value[],const char
* name_of_rc6_residency
flag_support = flag_support 1;
}
- printf(The
