tests/template: add IGT_TEST_DESCRIPTION
PATCH 1/4 tests/template: add IGT_TEST_DESCRIPTION
From: Paulo Zanoni <[email protected]>
So people that write tests based on the template don't forget to use the macro.
--- tests/template.c | 2 ++ 1 file changed, 2 insertions(+)
tests/template.c +2 -0
@@ -26,6 +26,8 @@#include "drmtest.h" +IGT_TEST_DESCRIPTION ("Template test." ); +/* * Note that test function (and code called by them) should generally not return * a variable indicating success/failure. Instead use the igt_require/igt_assert
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 2/4 lib/igt_aux: add functions to manipulate i915.ko parameters
From: Paulo Zanoni <[email protected]>
Some i915.ko features have very nice IGT tests, which are never executed because the features are disabled by default. This leads to unnoticed regressions both in the Kernel and in the IGT tests. We have seen this multiple times, for example, on FBC and PSR.
We want to be able to run IGT and actually test these disabled-by-default features in order to make sure we at least don't break them even more. Sometimes they may be disabled for some specific reason, and we don't want to increase the set of reasons without noticing.
To help solving this problem, this commit adds some helper functions that should make it easier to change certain i915.ko parameters and then restore their original values at the end of the test. With this, I'm hoping QA will be able to detect any regressions and automatically bisect them - or, with PRTS, reject the patches before they are even merged.
--- lib/igt_aux.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/igt_aux.h | 4 ++ 2 files changed, 138 insertions(+)
lib/igt_aux.c +134 -0
@@ -735,3 +735,137 @@ void igt_unlock_mem(void)free (locked_mem); locked_mem =NULL ; } + + +#define MODULE_PARAM_DIR "/sys/module/i915/parameters/" +#define PARAM_NAME_MAX_SZ 32 +#define PARAM_VALUE_MAX_SZ 16 +#define PARAM_FILE_PATH_MAX_SZ (strlen(MODULE_PARAM_DIR) + PARAM_NAME_MAX_SZ) + +struct module_param_data { +char name[PARAM_NAME_MAX_SZ ]; +char original_value[PARAM_VALUE_MAX_SZ ]; + +struct module_param_data *next; +}; +struct module_param_data *module_params =NULL ; + +static void igt_module_param_exit_handler (int sig) +{ +const size_t dir_len =strlen (MODULE_PARAM_DIR ); +char file_path[PARAM_FILE_PATH_MAX_SZ ]; +struct module_param_data *data; +int fd; + +/* We don't need to assert string sizes on this function since they were +* already checked before being stored on the lists. Besides, +* igt_assert() is not AS-Safe. */ +strcpy (file_path,MODULE_PARAM_DIR ); + +for (data = module_params; data !=NULL ; data = data->next) { +strcpy (file_path + dir_len, data->name); + + fd =open (file_path,O_RDWR ); +if (fd >=0 ) { +write (fd, data->original_value, +strlen (data->original_value)); +close (fd); + } + } +/* free() is not AS-Safe, so we can't call it here. */ +} + +/** +* igt_save_module_param: +* @name: name of the i915.ko module parameter +* @file_path: full sysfs file path for the parameter +* +* Reads the current value of an i915.ko module parameter, saves it on an array, +* then installs an exit handler to restore it when the program exits. +* +* It is safe to call this function multiple times for the same parameter. +* +* Notice that this function is called by igt_set_module_param(), so that one - +* or one of its wrappers - is the only function the test programs need to call. +*/ +static void igt_save_module_param (const char *name,const char *file_path) +{ +struct module_param_data *data; +size_t n; +int fd; + +/* Check if this parameter is already saved. */ +for (data = module_params; data !=NULL ; data = data->next) +if (strncmp (data->name, name,PARAM_NAME_MAX_SZ ) ==0 ) +return ; + +if (!module_params) +igt_install_exit_handler (igt_module_param_exit_handler); + + data =calloc (1 ,sizeof (*data)); +igt_assert (data); + +strncpy (data->name, name,PARAM_NAME_MAX_SZ ); + + fd =open (file_path,O_RDONLY ); +igt_assert (fd >=0 ); + + n =read (fd, data->original_value,PARAM_VALUE_MAX_SZ ); +igt_assert_f (n >0 && n <PARAM_VALUE_MAX_SZ , +"Need to increase PARAM_VALUE_MAX_SZ\n" ); + +igt_assert (close (fd) ==0 ); + + data->next = module_params; + module_params = data; +} + +/** +* igt_set_module_param: +* @name: i915.ko parameter name +* @val: i915.ko parameter value +* +* This function sets the desired value for the given i915.ko parameter. It also +* takes care of saving and restoring the values that were already set before +* the test was run by calling igt_save_module_param(). +* +* Please consider using igt_set_module_param_int() for the integer and bool +* parameters. +*/ +void igt_set_module_param (const char *name,const char *val) +{ +char file_path[PARAM_FILE_PATH_MAX_SZ ]; +size_t len =strlen (val); +int fd; + +igt_assert_f (strlen (name) <PARAM_NAME_MAX_SZ , +"Need to increase PARAM_NAME_MAX_SZ\n" ); +strcpy (file_path,MODULE_PARAM_DIR ); +strcpy (file_path +strlen (MODULE_PARAM_DIR ), name); + +igt_save_module_param (name, file_path); + + fd =open (file_path,O_RDWR ); +igt_assert (write (fd, val, len) == len); +igt_assert (close (fd) ==0 ); +} + +/** +* igt_set_module_param_int: +* @name: i915.ko parameter name +* @val: i915.ko parameter value +* +* This is a wrapper for igt_set_module_param() that takes an integer instead of +* a string. Please see igt_set_module_param(). +*/ +void igt_set_module_param_int (const char *name,int val) +{ +char str[PARAM_VALUE_MAX_SZ ]; +int n; + + n =snprintf (str,PARAM_VALUE_MAX_SZ ,"%d\n" , val); +igt_assert_f (n <PARAM_VALUE_MAX_SZ , +"Need to increase PARAM_VALUE_MAX_SZ\n" ); + +igt_set_module_param (name, str); +}
lib/igt_aux.h +4 -0
@@ -145,4 +145,8 @@ void igt_unlock_mem(void); ret_; \ }) + +void igt_set_module_param (const char *name,const char *val); +void igt_set_module_param_int (const char *name,int val); +#endif /* IGT_AUX_H */
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 3/4 tests/kms_fbc_crc: run even if FBC is disabled by default
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
We may not be perfect, but if we don't even test, we will probably only get worse over time.
The function called makes sure we restore whatever was the original FBC parameter when we exit the test, so this should not affect the other tests.
--- tests/kms_fbc_crc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
tests/kms_fbc_crc.c +4 -4
@@ -559,10 +559,10 @@ igt_mainigt_assert_lt (0 ,fread (buf,1 ,sizeof (buf), status));fclose (status); buf[sizeof (buf) -1 ] ='\0' ; -igt_require_f (!strstr (buf,"unsupported on this chipset" ) && - !strstr (buf,"disabled per module param" ) && - !strstr (buf,"disabled per chip default" ), -"FBC not supported/enabled\n" ); +igt_require_f (!strstr (buf,"unsupported on this chipset" ), +"FBC not supported\n" ); + +igt_set_module_param_int ("enable_fbc" ,1 ); data.bufmgr =drm_intel_bufmgr_gem_init (data.drm_fd,4096 );igt_assert (data.bufmgr);
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH igt 3/4] tests/kms_fbc_crc: run even if FBC is disabled by default
Quoted 20 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
We may not be perfect, but if we don't even test, we will probably only get worse over time.
The function called makes sure we restore whatever was the original FBC parameter when we exit the test, so this should not affect the other tests.
Signed-off-by Paulo Zanoni <[email protected]>--- tests/kms_fbc_crc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)tests/kms_fbc_crc.c +0
-0@@ -559,10 +559,10 @@ igt_mainigt_assert_lt (0 ,fread (buf,1 ,sizeof (buf), status));
tests/kms_fbc_crc.c igt_main · line 565
Quoted 10 lines
tests/kms_fbc_crc.c +4
-4fclose (status); buf[sizeof (buf) -1 ] ='\0' ; -igt_require_f (!strstr (buf,"unsupported on this chipset" ) && - !strstr (buf,"disabled per module param" ) && - !strstr (buf,"disabled per chip default" ), -"FBC not supported/enabled\n" ); +igt_require_f (!strstr (buf,"unsupported on this chipset" ), +"FBC not supported\n" ); + +igt_set_module_param_int ("enable_fbc" ,1 );
This is risky since on older chips fbc is known to pretty much insta-kill the box. Imo we should have a gen6+ here at least (that's roughly the point where the hw workarounds start to sound less scary).
Trying to enable this on older platforms just isn't worth it imo. -Daniel
Quoted 10 lines
tests/kms_fbc_crc.c +0
-0data.bufmgr =drm_intel_bufmgr_gem_init (data.drm_fd,4096 );igt_assert (data.bufmgr);Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH igt 3/4] tests/kms_fbc_crc: run even if FBC is disabled by default
2015-06-15 9:11 GMT-03:00 Daniel Vetter <[email protected]>:
Quoted 27 lines · Paulo Zanoni
Quoted 26 lines
From: Paulo Zanoni <[email protected]>
We may not be perfect, but if we don't even test, we will probably only get worse over time.
The function called makes sure we restore whatever was the original FBC parameter when we exit the test, so this should not affect the other tests.
Signed-off-by Paulo Zanoni <[email protected]>--- tests/kms_fbc_crc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)tests/kms_fbc_crc.c +0
-4@@ -559,10 +559,10 @@ igt_mainigt_assert_lt (0 ,fread (buf,1 ,sizeof (buf), status));fclose (status); buf[sizeof (buf) -1 ] ='\0' ; -igt_require_f (!strstr (buf,"unsupported on this chipset" ) && - !strstr (buf,"disabled per module param" ) && - !strstr (buf,"disabled per chip default" ), -"FBC not supported/enabled\n" );
Quoted 8 lines
tests/kms_fbc_crc.c igt_main · line 565
Quoted 4 lines
tests/kms_fbc_crc.c +4
-0+igt_require_f (!strstr (buf,"unsupported on this chipset" ), +"FBC not supported\n" ); + +igt_set_module_param_int ("enable_fbc" ,1 );This is risky since on older chips fbc is known to pretty much insta-kill the box. Imo we should have a gen6+ here at least (that's roughly the point where the hw workarounds start to sound less scary).
Well, we can try and then keep an eye on bugzilla, watching for the possible insta-kill bug reports, can't we? At least to me, this was not known as instal-kill since it's not documented anywhere and we don't have bug reports. Also, the move to the frontbuffer tracking infrastructure could maybe have solved the insta-kill problem.
Quoted 2 lines
Trying to enable this on older platforms just isn't worth it imo.
If we're not even going to try this, shouldn't we remove all the Kernel code support for FBC on these platforms? This is something I wanted to discuss at some point, but now you gave us a nice excuse :). Maybe it's better to just not have the code at all vs have a code that (maybe) instakills the machine and we're probably not going to fix anytime soon. I don't know, just an idea.
Btw, those patches are already on IGT. Thomas seemed to be OK with the ideas, and after one week on the mailing list I pushed them.
Quoted 18 lines
-Daniel
Quoted 10 lines
tests/kms_fbc_crc.c +0
-1data.bufmgr =drm_intel_bufmgr_gem_init (data.drm_fd,4096 );igt_assert (data.bufmgr); --2.1.4
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH igt 3/4] tests/kms_fbc_crc: run even if FBC is disabled by default
Quoted 37 lines · Paulo Zanoni
2015-06-15 9:11 GMT-03:00 Daniel Vetter <[email protected]>:
Quoted 27 lines · Paulo Zanoni
Quoted 26 lines
From: Paulo Zanoni <[email protected]>
We may not be perfect, but if we don't even test, we will probably only get worse over time.
The function called makes sure we restore whatever was the original FBC parameter when we exit the test, so this should not affect the other tests.
Signed-off-by Paulo Zanoni <[email protected]>--- tests/kms_fbc_crc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)tests/kms_fbc_crc.c +0
-4@@ -559,10 +559,10 @@ igt_mainigt_assert_lt (0 ,fread (buf,1 ,sizeof (buf), status));fclose (status); buf[sizeof (buf) -1 ] ='\0' ; -igt_require_f (!strstr (buf,"unsupported on this chipset" ) && - !strstr (buf,"disabled per module param" ) && - !strstr (buf,"disabled per chip default" ), -"FBC not supported/enabled\n" );Quoted 8 lines
tests/kms_fbc_crc.c igt_main · line 565
Quoted 4 lines
tests/kms_fbc_crc.c +4
-0+igt_require_f (!strstr (buf,"unsupported on this chipset" ), +"FBC not supported\n" ); + +igt_set_module_param_int ("enable_fbc" ,1 );This is risky since on older chips fbc is known to pretty much insta-kill the box. Imo we should have a gen6+ here at least (that's roughly the point where the hw workarounds start to sound less scary).
Quoted 5 lines
Well, we can try and then keep an eye on bugzilla, watching for the possible insta-kill bug reports, can't we? At least to me, this was not known as instal-kill since it's not documented anywhere and we don't have bug reports. Also, the move to the frontbuffer tracking infrastructure could maybe have solved the insta-kill problem.
Afaik fbc kills the chip on gen2 reliable when enabling, and iirc the same on gen3. Not sure about gen4, but on ilk the w/a list in bspec is scary enough to make grown-ups scream. Art recommended to just not bother, since some of the required w/a (which we don't implement) force the chip into an ill-defined state to provoke some kind of soft-reset. It's really all fairly horrible.
Quoted 8 lines
Quoted 1 line
Trying to enable this on older platforms just isn't worth it imo.
If we're not even going to try this, shouldn't we remove all the Kernel code support for FBC on these platforms? This is something I wanted to discuss at some point, but now you gave us a nice excuse :). Maybe it's better to just not have the code at all vs have a code that (maybe) instakills the machine and we're probably not going to fix anytime soon. I don't know, just an idea.
Yeah it might be time to nuke the old fbc code entirely.
Quoted 2 lines
Btw, those patches are already on IGT. Thomas seemed to be OK with the ideas, and after one week on the mailing list I pushed them.
Yeah didn't notice that yet. I'll follow up with a quick patch just for paranoia.
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 4/4 tests/kms_psr_sink_crc: test even if PSR is disabled by default
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Use the igt_set_module_param_int() call to enable it, then restore the previous value after we are done testing.
With this, we can change the psr_enabled() function to psr_possible():
the only requirement should be that we have a PSR capable sink. The test should now be able to make "Source_OK" and "Enabled" become true whenever it wants.
--- tests/kms_psr_sink_crc.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
tests/kms_psr_sink_crc.c +4 -10
@@ -203,7 +203,7 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color)gem_bo_busy (data->drm_fd, handle); } -static bool psr_enabled (data_t *data) +static bool psr_possible (data_t *data) {FILE *file;char buf[4096 ];
@@ -229,14 +229,6 @@ static bool psr_enabled(data_t *data)igt_require_f (ret ==1 &&strcmp (buf,"yes" ) ==0 ,"Sink_Support: %s\n" , buf); - ret =fscanf (file,"Source_OK: %s\n" , buf); -igt_require_f (ret ==1 &&strcmp (buf,"yes" ) ==0 , -"Source_OK: %s\n" , buf); - - ret =fscanf (file,"Enabled: %s\n" , buf); -igt_require_f (ret ==1 &&strcmp (buf,"yes" ) ==0 , -"Enabled: %s\n" , buf); -fclose (file);return true; }
@@ -570,7 +562,9 @@ int main(int argc, char *argv[])kmstest_set_vt_graphics_mode (); data.devid =intel_get_drm_devid (data.drm_fd); -igt_skip_on (!psr_enabled (&data)); +igt_set_module_param_int ("enable_psr" ,1 ); + +igt_skip_on (!psr_possible (&data)); data.bufmgr =drm_intel_bufmgr_gem_init (data.drm_fd,4096 );igt_assert (data.bufmgr);
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH igt 4/4] tests/kms_psr_sink_crc: test even if PSR is disabled by default
Quoted 62 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Use the
igt_set_module_param_int()call to enable it, then restore the previous value after we are done testing.With this, we can change the
psr_enabled()function topsr_possible():
the only requirement should be that we have a PSR capable sink. The test should now be able to make "Source_OK" and "Enabled" become true whenever it wants.Cc Rodrigo Vivi <[email protected]>Signed-off-by Paulo Zanoni <[email protected]>--- tests/kms_psr_sink_crc.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)tests/kms_psr_sink_crc.c +4
-11@@ -203,7 +203,7 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color)gem_bo_busy (data->drm_fd, handle); } -static bool psr_enabled (data_t *data) +static bool psr_possible (data_t *data) {FILE *file;char buf[4096 ];@@ -229,14 +229,6 @@ static bool psr_enabled(data_t *data)igt_require_f (ret ==1 &&strcmp (buf,"yes" ) ==0 ,"Sink_Support: %s\n" , buf); - ret =fscanf (file,"Source_OK: %s\n" , buf); -igt_require_f (ret ==1 &&strcmp (buf,"yes" ) ==0 , -"Source_OK: %s\n" , buf); - - ret =fscanf (file,"Enabled: %s\n" , buf); -igt_require_f (ret ==1 &&strcmp (buf,"yes" ) ==0 , -"Enabled: %s\n" , buf); -fclose (file);return true; }@@ -570,7 +562,9 @@ int main(int argc, char *argv[])kmstest_set_vt_graphics_mode (); data.devid =intel_get_drm_devid (data.drm_fd); -igt_skip_on (!psr_enabled (&data)); +igt_set_module_param_int ("enable_psr" ,1 ); + +igt_skip_on (!psr_possible (&data)); data.bufmgr =drm_intel_bufmgr_gem_init (data.drm_fd,4096 );igt_assert (data.bufmgr); --2.1.4
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 6 lines
Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 6 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
So people that write tests based on the template don't forget to use the macro.
Signed-off-by Paulo Zanoni <[email protected]>
With the nit to not try to enable fbc on pre-gen6 address the series is
Quoted 24 lines
--- tests/template.c | 2 ++ 1 file changed, 2 insertions(+)tests/template.c +2
-0@@ -26,6 +26,8 @@#include "drmtest.h" +IGT_TEST_DESCRIPTION ("Template test." ); +/* * Note that test function (and code called by them) should generally not return * a variable indicating success/failure. Instead use the igt_require/igt_assert Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx