Re: [igt-dev] [PATCH i-g-t v3 1/2] tests/i915/i915_pm_rpm: Enable PC8+ residency test for all Gen9+

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

On 3/27/2019 5:58 PM, Ramalingam C wrote:
> On 2019-03-27 at 15:25:22 +0530, Anshuman Gupta wrote:
>> Enabled has_pc8 global for ICL and Gen9+.
>> Modified PC8+ residency sub-test with all screen enabled.
>>
>> v2:Fixed the issue of skipped test on HSW.
>>     Improved the code comment for MSR_PKG_CST_CONFIG_CONTROL mask and PC8
>>     bits, it holds good for SKL/ICL and Goldmont microarchitecture.
>>     Code readabilty improvement.
>> v3:Removed the connected_screens global. [Ram]
>>     Removed pc8_needs_screen_off from mode_set_data structure,
>>     made it global, aligning to has_pc8 and has_runtime_pm globals. [Ram]
>>     Reuse connector lcoal variable in init_modeset_params_for_all_screen(). [Ram]
>>     Addressed Coding guide lines comments. [Ram]
>>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>>   tests/i915/i915_pm_rpm.c | 73 +++++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 66 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
>> index be296f5..7e0c370 100644
>> --- a/tests/i915/i915_pm_rpm.c
>> +++ b/tests/i915/i915_pm_rpm.c
>> @@ -52,7 +52,7 @@
>>   #include "igt_device.h"
>>   
>>   #define MSR_PKG_CST_CONFIG_CONTROL	0xE2
>> -/* HSW/BDW: */
>> +/* HSW/BDW SKL/ICL and Goldmont */
> This is not HSW or BDW. I guess you want to list the platforms on which
> these bit mask will work. Please update the comment accordingly.
>>   #define  PKG_CST_LIMIT_MASK		0xF
>>   #define  PKG_CST_LIMIT_C8		0x6
>>   
>> @@ -90,7 +90,7 @@ enum plane_type {
>>   
>>   int drm_fd, msr_fd, pc8_status_fd;
>>   int debugfs;
>> -bool has_runtime_pm, has_pc8;
>> +bool has_runtime_pm, has_pc8, pc8_needs_screen_off;
>>   struct mode_set_data ms_data;
>>   
>>   /* Stuff used when creating FBs and mode setting. */
>> @@ -121,6 +121,7 @@ struct modeset_params {
>>   struct modeset_params lpsp_mode_params;
>>   struct modeset_params non_lpsp_mode_params;
>>   struct modeset_params *default_mode_params;
>> +struct modeset_params *screens_mode_params[MAX_CONNECTORS];
>>   
>>   static int8_t *pm_data = NULL;
>>   
>> @@ -297,6 +298,37 @@ static bool init_modeset_params_for_type(struct mode_set_data *data,
>>   	return true;
>>   }
>>   
>> +static void init_modeset_params_for_all_screen(struct mode_set_data *data)
> BTW why do we need to pass the ms_data as a parameter when it is
> declared in global space? Am I missing something here?
I am not sure about it, exisitng code uses this global at so many places.
>> +{
>> +	drmModeConnectorPtr connector = NULL;
>> +	drmModeModeInfoPtr mode = NULL;
>> +	int screen = 0;
>> +
>> +	if (!data->res)
>> +		return false;
>> +
>> +	for (int i = 0; i < data->res->count_connectors; i++) {
>> +		connector = data->connectors[i];
>> +
>> +		if (connector->connection == DRM_MODE_CONNECTED
>> +		    && connector->count_modes) {
>> +			screens_mode_params[screen] =
>> +				malloc(sizeof(struct modeset_params));
>> +			mode = &connector->modes[0];
>> +			igt_create_pattern_fb(drm_fd, mode->hdisplay, mode->vdisplay,
>> +					      DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
>> +					      &screens_mode_params[screen]->fb);
> Line length exceeds 80chars. Please wrap it. Applicable for elsewhere
> too.
here it will not be possible, it will not be readable, need to create a 
separate function and move all these chunk of big lines, hope that will 
be ok.
>> +			screens_mode_params[screen]->crtc_id =
>> +				kmstest_find_crtc_for_connector(drm_fd, data->res, connector, 0);
>> +			screens_mode_params[screen]->connector_id = connector->connector_id;
>> +			screens_mode_params[screen]->mode = mode;
>> +			screen++;
>> +		}
>> +	}
>> +
>> +	return;
>> +}
>> +
>>   static void init_modeset_cached_params(struct mode_set_data *data)
>>   {
>>   	bool lpsp, non_lpsp;
>> @@ -305,6 +337,7 @@ static void init_modeset_cached_params(struct mode_set_data *data)
>>   					    SCREEN_TYPE_LPSP);
>>   	non_lpsp = init_modeset_params_for_type(data, &non_lpsp_mode_params,
>>   						SCREEN_TYPE_NON_LPSP);
>> +	init_modeset_params_for_all_screen(data);
>>   
>>   	if (lpsp)
>>   		default_mode_params = &lpsp_mode_params;
>> @@ -353,6 +386,22 @@ static bool enable_one_screen_with_type(struct mode_set_data *data,
>>   	return set_mode_for_params(params);
>>   }
>>   
>> +static void enable_all_screens(struct mode_set_data *data)
>> +{
>> +	struct modeset_params *params = NULL;
>> +
>> +	/* SKIP if there are no connected screens. */
>> +	igt_require(screens_mode_params[0]);
>> +
>> +	for (int i = 0; i < MAX_CONNECTORS ; i++) {
>> +		params = screens_mode_params[i];
>> +		if (params)
>> +			set_mode_for_params(params);
>> +		else
>> +			break;
>> +	}
>> +}
>> +
>>   static void enable_one_screen(struct mode_set_data *data)
>>   {
>>   	/* SKIP if there are no connected screens. */
>> @@ -685,8 +734,12 @@ static void setup_pc8(void)
>>   {
>>   	has_pc8 = false;
>>   
>> -	/* Only Haswell supports the PC8 feature. */
>> -	if (!IS_HASWELL(ms_data.devid) && !IS_BROADWELL(ms_data.devid))
>> +	if (IS_HASWELL(ms_data.devid) || IS_BROADWELL(ms_data.devid))
>> +		pc8_needs_screen_off = true;
>> +	else if (AT_LEAST_GEN(ms_data.devid, 9))
>> +		pc8_needs_screen_off = false;
>> +	/* Only Haswell supports the PC8 feature on lesser than GEN9. */
>> +	else
>>   		return;
>>   
>>   	/* Make sure our Kernel supports MSR and the module is loaded. */
>> @@ -808,9 +861,15 @@ static void pc8_residency_subtest(void)
>>   		     "configuration.\n");
>>   
>>   	/* Make sure PC8+ residencies stop! */
>> -	enable_one_screen(&ms_data);
>> -	igt_assert_f(!pc8_plus_residency_changed(10),
>> -		     "PC8+ residency didn't stop with screen enabled.\n");
>> +	if (pc8_needs_screen_off) {
>> +		enable_one_screen(&ms_data);
>> +		igt_assert_f(!pc8_plus_residency_changed(10),
> Could we use a macro for this timeout "10" ?
yes we can but there are other existing instances too are using 
different timeut values, so there will be pleanty of timeout
macros.
>> +			     "PC8+ residency didn't stop with screen enabled.\n");
>> +	} else {
>> +		enable_all_screens(&ms_data);
>> +		igt_assert_f(pc8_plus_residency_changed(10),
> macro should be good here.
>> +			     "Machine is not reaching PC8+ states with all screen enabled.\n");
> Please consider to make the log crisp to meet the 80 char limit.
> 
> -Ram
>> +	}
>>   }
>>   
>>   static void modeset_subtest(enum screen_type type, int rounds, int wait_flags)
>> -- 
>> 2.7.4
>>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev