[igt-dev] [PATCH i-g-t v3 1/2] tests/i915/i915_pm_rpm: Enable PC8+ residency test for all Gen9+
Ramalingam C
ramalingam.c at intel.com
Wed Mar 27 15:36:36 UTC 2019
On 2019-03-27 at 19:15:01 +0530, Gupta, Anshuman wrote:
>
>
> 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 at 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.
We need to worry about what we add here. If you are thinking existing
code can be improvised, feel free to do that in separate patch.
-Ram
> > > + "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
> > >
More information about the igt-dev
mailing list