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

From: Animesh Manna <animesh.manna@intel.com>
To: Anshuman Gupta <anshuman.gupta@intel.com>, igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t v3 1/4] igt/i915/i915_pm_lpsp enable pm_lpsp for platforms till Gen11.
Date: Wed, 17 Apr 2019 11:50:01 +0530	[thread overview]
Message-ID: <312f575a-a401-5ad0-8aec-6fd176b77770@intel.com> (raw)
In-Reply-To: <1555421738-15223-2-git-send-email-anshuman.gupta@intel.com>

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();
>   	}
>   

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