[igt-dev] [PATCH i-g-t v6 1/4] tests/i915/i915_pm_rpm: Enable PC8+ residency test for all Gen9+
Ramalingam C
ramalingam.c at intel.com
Fri May 3 10:46:53 UTC 2019
On 2019-04-26 at 23:15:23 +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]
>
> v4:Improved the code comment for MSR_PKG_CST_CONFIG_CONTROL mask and PC8 bits. [Ram]
> Introduced set_screens_mode_params() function to fix warning of line exceeding
> 80 char, fixed this warning at other places too. [Ram]
> Using ms_data modset_data global structure. [Ram]
> Introduced macros for timeout values, given to PC8+ residency check function. [Ram]
>
> Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> ---
> tests/i915/i915_pm_rpm.c | 91 +++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 83 insertions(+), 8 deletions(-)
>
> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> index a2c9d0e..9315dbd 100644
> --- a/tests/i915/i915_pm_rpm.c
> +++ b/tests/i915/i915_pm_rpm.c
> @@ -52,7 +52,11 @@
> #include "igt_device.h"
>
> #define MSR_PKG_CST_CONFIG_CONTROL 0xE2
> -/* HSW/BDW: */
> +/*
> + * Below PKG CST limit mask and PC8 bits are meant for
> + * HSW,BDW SKL,ICL and Goldmont Microarch.
> + * Refer IA S/W developers manual vol3c part3 chapter:35
> + */
> #define PKG_CST_LIMIT_MASK 0xF
> #define PKG_CST_LIMIT_C8 0x6
>
> @@ -64,6 +68,10 @@
> #define MAX_ENCODERS 32
> #define MAX_CRTCS 16
>
> +#define TIME_OUT_SEC_5 5
> +#define TIME_OUT_SEC_10 10
> +#define TIME_OUT_SEC_30 30
Macros are supposed to represent the purpose of it. I would expect macro
itself should answer the question of "timeout for what?"
> +
> enum pc8_status {
> PC8_ENABLED,
> PC8_DISABLED
> @@ -90,7 +98,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,9 +129,25 @@ 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;
>
> +static inline void set_screens_mode_param(drmModeConnectorPtr connector,
> + struct modeset_params *params)
> +{
> + drmModeModeInfoPtr mode = NULL;
> +
> + mode = &connector->modes[0];
> + igt_create_pattern_fb(drm_fd, mode->hdisplay, mode->vdisplay,
> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> + ¶ms->fb);
> + params->crtc_id = kmstest_find_crtc_for_connector(drm_fd, ms_data.res,
> + connector, 0);
> + params->connector_id = connector->connector_id;
> + params->mode = mode;
> +}
> +
> static int modprobe(const char *driver)
> {
> return igt_kmod_load(driver, NULL);
> @@ -297,6 +321,28 @@ static bool init_modeset_params_for_type(struct mode_set_data *data,
> return true;
> }
>
> +static void init_modeset_params_for_all_screen(void)
> +{
> + drmModeConnectorPtr connector = NULL;
> + int screen = 0;
> +
> + if (!ms_data.res)
> + return;
> +
> + for (int i = 0; i < ms_data.res->count_connectors; i++) {
> + connector = ms_data.connectors[i];
> +
> + if (connector->connection == DRM_MODE_CONNECTED
> + && connector->count_modes) {
> + screens_mode_params[screen] =
> + malloc(sizeof(struct modeset_params));
> + set_screens_mode_param(connector,
> + screens_mode_params[screen]);
> + screen++;
> + }
> + }
> +}
> +
> static void init_modeset_cached_params(struct mode_set_data *data)
> {
> bool lpsp, non_lpsp;
> @@ -305,6 +351,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();
>
> if (lpsp)
> default_mode_params = &lpsp_mode_params;
> @@ -353,6 +400,22 @@ static bool enable_one_screen_with_type(struct mode_set_data *data,
> return set_mode_for_params(params);
> }
>
> +static void enable_all_screens(void)
> +{
> + 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 +748,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. */
> @@ -803,14 +870,22 @@ static void pc8_residency_subtest(void)
>
> /* Make sure PC8+ residencies move! */
> disable_all_screens(&ms_data);
> - igt_assert_f(pc8_plus_residency_changed(30),
> + igt_assert_f(pc8_plus_residency_changed(TIME_OUT_SEC_30),
Drop this, as this is irrelevant for this patch. If this is needed do it
as separate patch.
-Ram.
> "Machine is not reaching PC8+ states, please check its "
> "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(TIME_OUT_SEC_10),
> + "PC8+ residency didn't stop with "
> + "screen enabled.\n");
> + } else {
> + enable_all_screens();
> + igt_assert_f(pc8_plus_residency_changed(TIME_OUT_SEC_10),
> + "Machine is not reaching PC8+ states "
> + "with all screen enabled.\n");
> + }
> }
>
> static void modeset_subtest(enum screen_type type, int rounds, int wait_flags)
> --
> 2.7.4
>
More information about the igt-dev
mailing list