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

["C, Ramalingam" <ramalingam.c@intel.com>]

On 3/26/2019 6:29 PM, 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.
>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   tests/i915/i915_pm_rpm.c | 77 +++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 69 insertions(+), 8 deletions(-)
>
> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> index be296f5..66d2a4e 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 */
If PC8 is for all Gen9+, then these macro also for all Gen9+ along with 
HSW I hope.
Why we have only few platforms mentioned here?
>   #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, connected_screens;
we can do with out this flag too, as we can rely on validness of 
screens_mode_params[i]

>   struct mode_set_data ms_data;
>   
>   /* Stuff used when creating FBs and mode setting. */
> @@ -98,8 +98,8 @@ struct mode_set_data {
>   	drmModeResPtr res;
>   	drmModeConnectorPtr connectors[MAX_CONNECTORS];
>   	drmModePropertyBlobPtr edids[MAX_CONNECTORS];
> -
unwanted change.
>   	uint32_t devid;
> +	bool pc8_needs_screen_off;
If you see the other members of mode_set_data they are retrieved from 
the kernel.
And all required flag like has_runtime_pm and has_pc8 are defined as global.
So even pc8_needs_screen_off can be defined outside to mode_set_data!?
>   };
>   
>   /* Stuff we query at different times so we can compare. */
> @@ -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,40 @@ static bool init_modeset_params_for_type(struct mode_set_data *data,
>   	return true;
>   }
>   
> +static bool init_modeset_params_for_all_screen(struct mode_set_data *data)
> +{
> +	drmModeConnectorPtr connector = NULL;
> +	drmModeModeInfoPtr mode = NULL;
> +	int screen = 0;
> +
> +	if (!data->res)
> +		return false;
> +
> +	for (int i = 0; i < data->res->count_connectors; i++) {
> +		drmModeConnectorPtr c = data->connectors[i];
Could we reuse the connector itself?
> +
> +		if (c->connection == DRM_MODE_CONNECTED && c->count_modes) {
> +			screens_mode_params[screen] =
> +				malloc(sizeof(struct modeset_params));
> +			connector = c;
> +			mode = &c->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);
Align wrapped parameters with open parenthesis.
> +			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++;
> +		}
> +	}
> +
> +	if (!connector)
> +		return false;
> +
> +	return true;
> +}
> +
>   static void init_modeset_cached_params(struct mode_set_data *data)
>   {
>   	bool lpsp, non_lpsp;
> @@ -305,6 +340,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);
> +	connected_screens = init_modeset_params_for_all_screen(data);
>   
>   	if (lpsp)
>   		default_mode_params = &lpsp_mode_params;
> @@ -353,6 +389,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(connected_screens);
> +
> +	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 +737,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))
> +		ms_data.pc8_needs_screen_off = true;
> +	else if (AT_LEAST_GEN(ms_data.devid, 9))
> +		ms_data.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. */
> @@ -778,7 +834,6 @@ static void teardown_environment(void)
>   
>   	igt_pm_restore_sata_link_power_management(pm_data);
>   	free(pm_data);
> -
unwanted diff?
>   	fini_mode_set_data(&ms_data);
>   
>   	close(debugfs);
> @@ -808,9 +863,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),
> +	if (ms_data.pc8_needs_screen_off) {
> +		enable_one_screen(&ms_data);
> +		igt_assert_f(!pc8_plus_residency_changed(10),
>   		     "PC8+ residency didn't stop with screen enabled.\n");
Could we align this with open parenthesis?
> +	} else {
> +		enable_all_screens(&ms_data);
> +		igt_assert_f(pc8_plus_residency_changed(10),
> +		     "Machine is not reaching PC8+ states with all screen enabled.\n");
Could we align this with open parenthesis?

-Ram
> +	}
>   }
>   
>   static void modeset_subtest(enum screen_type type, int rounds, int wait_flags)

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