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

[Animesh Manna <animesh.manna@intel.com>]

Hi,


On 5/9/2019 5:28 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.
>
> v2: Removed global no_lpsp_pw_idx. [Animesh]
>      Check only state value for power well. [Animesh]
>      Adding igt_wait of 1 second as sometimes screens_disabled
>      test fails because AUDIO power domain was having non-zero
>      power domain reference count.[Imre]
>      Dumping i915_pm_runtime_status and i915_power_domain_info
>      in case test fails.[Imre]
> v3: Having igt_wait() to check lpsp_enabled() only to the part
>      of test affected by delayed audio codec disabling.
>
> Cc: imre.deak@intel.com
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   tests/i915/i915_pm_lpsp.c | 88 +++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 70 insertions(+), 18 deletions(-)
>
> diff --git a/tests/i915/i915_pm_lpsp.c b/tests/i915/i915_pm_lpsp.c
> index b319dbe..1abe587 100644
> --- a/tests/i915/i915_pm_lpsp.c
> +++ b/tests/i915/i915_pm_lpsp.c
> @@ -30,26 +30,68 @@
>   #include <fcntl.h>
>   #include <unistd.h>
>   
> +#include "igt_sysfs.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))

HSW_PWR_WELL_CTL_REQ can be removed as not used anywhere.


> +#define   HSW_PWR_WELL_CTL_STATE(pw_idx)        (0x1 << ((pw_idx) * 2))

I can understand that here tried to follow similar driver design to 
calculate mask for power well register,
but validation point of view not necessarily need to have same design 
like driver and maybe better not to follow to catch driver issue if 
possible.

As per me, <platform>_PW2_ENABLE_MASK is sufficient to check the state 
of power well 2.

> +
> +#define DUMP_DBGFS(file1, file2, fd)			\
> +	"%s:\n%s\n%s:\n%s\n", file1,			\
> +	igt_sysfs_get(fd, file1), file2,		\
> +	igt_sysfs_get(fd, file2)			\
>   
>   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 int get_no_lpsp_pw_idx(uint32_t devid)
> +{

We can return <platform>_PW2_ENABLE_MASK from this function. Function 
name should be updated accordingly.

Regards,
Animesh

> +	int no_lpsp_pw_idx;
> +
> +	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
> +		no_lpsp_pw_idx = -1;
> +
> +	return no_lpsp_pw_idx;
>   }
>   
> -static bool lpsp_is_enabled(int drm_fd)
> +static bool lpsp_is_enabled(uint32_t devid)
>   {
> -	uint32_t val;
> +	uint32_t val, mask;
> +	int pw_idx;
> +
> +	pw_idx = get_no_lpsp_pw_idx(devid);
> +	igt_require(pw_idx > 0);
> +	mask = HSW_PWR_WELL_CTL_STATE(pw_idx);
>   
>   	val = INREG(HSW_PWR_WELL_CTL2);
> -	return !(val & HSW_PWR_WELL_STATE_ENABLED);
> +	return !(val & mask);
>   }
>   
>   /* The LPSP mode is all about an enabled pipe, but we expect to also be in the
>    * low power mode when no pipes are enabled, so do this check anyway. */
> -static void screens_disabled_subtest(int drm_fd, drmModeResPtr drm_res)
> +static void screens_disabled_subtest(int drm_fd, drmModeResPtr drm_res,
> +				     uint32_t devid, int fd)
>   {
>   	kmstest_unset_all_crtcs(drm_fd, drm_res);
> -	igt_assert(lpsp_is_enabled(drm_fd));
> +
> +	/* Some times delayed audio codec disabling causes to fail the test.
> +	 * Using igt_wait to check lpsp after drm_mode_setcrtc().
> +	 */
> +	igt_assert_f(igt_wait(lpsp_is_enabled(devid), 1000, 100),
> +		     DUMP_DBGFS("i915_runtime_pm_status",
> +				"i915_power_domain_info", fd));
>   }
>   
>   static uint32_t create_fb(int drm_fd, int width, int height)
> @@ -62,7 +104,7 @@ static uint32_t create_fb(int drm_fd, int width, int height)
>   
>   static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
>   			drmModeConnectorPtr *drm_connectors, uint32_t devid,
> -			bool use_panel_fitter)
> +			bool use_panel_fitter, int fd)
>   {
>   	int i, rc;
>   	uint32_t crtc_id = 0, buffer_id = 0;
> @@ -133,16 +175,18 @@ static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
>   
>   	if (use_panel_fitter) {
>   		if (IS_HASWELL(devid))
> -			igt_assert(!lpsp_is_enabled(drm_fd));
> +			igt_assert(!lpsp_is_enabled(devid));
>   		else
> -			igt_assert(lpsp_is_enabled(drm_fd));
> +			igt_assert(lpsp_is_enabled(devid));
>   	} else {
> -		igt_assert(lpsp_is_enabled(drm_fd));
> +		igt_assert_f(lpsp_is_enabled(devid),
> +			     DUMP_DBGFS("i915_runtime_pm_status",
> +					"i915_power_domain_info", fd));
>   	}
>   }
>   
> -static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
> -			    drmModeConnectorPtr *drm_connectors)
> +static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res, uint32_t devid,
> +			    drmModeConnectorPtr *drm_connectors, int fd)
>   {
>   	int i, rc;
>   	uint32_t crtc_id = 0, buffer_id = 0;
> @@ -177,8 +221,9 @@ static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
>   	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
>   			    &connector->connector_id, 1, mode);
>   	igt_assert_eq(rc, 0);
> -
> -	igt_assert(!lpsp_is_enabled(drm_fd));
> +	igt_assert_f(!lpsp_is_enabled(devid),
> +		     DUMP_DBGFS("i915_runtime_pm_status",
> +				"i915_power_domain_info", fd));
>   }
>   
>   #define MAX_CONNECTORS 32
> @@ -190,6 +235,8 @@ drmModeConnectorPtr drm_connectors[MAX_CONNECTORS];
>   
>   igt_main
>   {
> +	int debugfs;
> +
>   	igt_fixture {
>   		int i;
>   
> @@ -212,17 +259,22 @@ igt_main
>   
>   		intel_register_access_init(intel_get_pci_device(), 0, drm_fd);
>   
> +		igt_require(get_no_lpsp_pw_idx(devid) > 0);
>   		kmstest_set_vt_graphics_mode();
> +		debugfs = igt_debugfs_dir(drm_fd);
>   	}
>   
>   	igt_subtest("screens-disabled")
> -		screens_disabled_subtest(drm_fd, drm_res);
> +		screens_disabled_subtest(drm_fd, drm_res, devid, debugfs);
>   	igt_subtest("edp-native")
> -		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
> +		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false,
> +			    debugfs);
>   	igt_subtest("edp-panel-fitter")
> -		edp_subtest(drm_fd, drm_res, drm_connectors, devid, true);
> +		edp_subtest(drm_fd, drm_res, drm_connectors, devid, true,
> +			    debugfs);
>   	igt_subtest("non-edp")
> -		non_edp_subtest(drm_fd, drm_res, drm_connectors);
> +		non_edp_subtest(drm_fd, drm_res, devid, drm_connectors,
> +				debugfs);
>   
>   	igt_fixture {
>   		int i;

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