Re: [igt-dev] [PATCH i-g-t v3 1/4] igt/i915/i915_pm_lpsp enable pm_lpsp for platforms till Gen11.

Animesh Manna
2019-04-17T06:20:08Z

Hi,

On 4/16/2019 7:05 PM, Anshuman Gupta wrote:
> Enabling i915_pm_lpsp igt tests for all platforms till Gen11.
> Earlier these test were enabled only on haswell and broadwell
> platforms.
>
> Cc: imre.deak@intel.com
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
> tests/i915/i915_pm_lpsp.c | 30 ++++++++++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/tests/i915/i915_pm_lpsp.c b/tests/i915/i915_pm_lpsp.c
> index b319dbe..020f8b3 100644
> --- a/tests/i915/i915_pm_lpsp.c
> +++ b/tests/i915/i915_pm_lpsp.c
> @@ -30,18 +30,43 @@
> #include <fcntl.h>
> #include <unistd.h>
>
> +#define HSW_PW_CTL_IDX_GLOBAL 15
> +#define SKL_PW_CTL_IDX_PW_2 15
> +#define ICL_PW_CTL_IDX_PW_3 2
> +
> +#define HSW_PWR_WELL_CTL_REQ(pw_idx) (0x2 << ((pw_idx) * 2))
> +#define HSW_PWR_WELL_CTL_STATE(pw_idx) (0x1 << ((pw_idx) * 2))
> +
> +int no_lpsp_pw_idx;
Please add a code-comment about the purpose of using global variable if
it is really needed.
>
> static bool supports_lpsp(uint32_t devid)
> {
> - return IS_HASWELL(devid) || IS_BROADWELL(devid);
> + return IS_HASWELL(devid) || IS_BROADWELL(devid)
> + || AT_LEAST_GEN(devid, 9);
> +}
> +
> +static bool get_no_lpsp_pw_idx(uint32_t devid)

This function return type is confusing to me.

> +{
> + if (IS_HASWELL(devid) || IS_BROADWELL(devid))
> + no_lpsp_pw_idx = HSW_PW_CTL_IDX_GLOBAL;
> + else if (IS_GEN(devid, 11))
> + no_lpsp_pw_idx = ICL_PW_CTL_IDX_PW_3;
> + else if (AT_LEAST_GEN(devid, 9))
> + no_lpsp_pw_idx = SKL_PW_CTL_IDX_PW_2;
> + else
> + return false;
> +
> + return true;

This function is updating the power well index and also checking lpsp is
supported or not which is duplication of existing function supports_lpsp().
As per the above code "update" tag is more suitable than "get".

> }
>
> static bool lpsp_is_enabled(int drm_fd)
> {
> uint32_t val;
> + uint32_t mask = HSW_PWR_WELL_CTL_REQ(no_lpsp_pw_idx) |
> + HSW_PWR_WELL_CTL_STATE(no_lpsp_pw_idx);

get_no_lpsp_pw_idx() can be used here to get the power-well index and then use it to create the mask, then global variable can be avoided.

>
> val = INREG(HSW_PWR_WELL_CTL2);
> - return !(val & HSW_PWR_WELL_STATE_ENABLED);
> + return !((val & mask) == mask);

0 or non-zero should be the decision maker. Better not compare with
"mask" after masking,

> }
>
> /* The LPSP mode is all about an enabled pipe, but we expect to also be in the
> @@ -212,6 +237,7 @@ igt_main
>
> intel_register_access_init(intel_get_pci_device(), 0, drm_fd);
>
> + igt_require(get_no_lpsp_pw_idx(devid));

supports_lpsp() maybe appropriate here to check igt is required or not.

Regards,
Animesh

> kmstest_set_vt_graphics_mode();
> }
>