[igt-dev] [PATCH] lib/igt_pm: dump runtime pm status on timeout

[Arkadiusz Hiler]

On Tue, Oct 08, 2019 at 09:07:50AM -0700, don.hiatt at intel.com wrote:
> From: Don Hiatt <don.hiatt at intel.com>
> 
> Display the runtime pm status if we timeout waiting for status.
> 
> Signed-off-by: Don Hiatt <don.hiatt at intel.com>
> Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda at intel.com>
> ---
>  lib/igt_pm.c | 16 ++++++++++++----
>  lib/igt_pm.h |  2 +-
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 64ce240e093f..0d3561fdf821 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -658,16 +658,17 @@ void igt_disable_runtime_pm(void)
>  
>  /**
>   * igt_get_runtime_pm_status:
> + * @buf: buffer to place pm status string in
> + * @len: length of buf
>   *
>   * Returns: The current runtime PM status.
>   */
> -enum igt_runtime_pm_status igt_get_runtime_pm_status(void)
> +enum igt_runtime_pm_status igt_get_runtime_pm_status(char *buf, int len)
>  {
>  	ssize_t n_read;
> -	char buf[32];
>  
>  	lseek(pm_status_fd, 0, SEEK_SET);
> -	n_read = read(pm_status_fd, buf, ARRAY_SIZE(buf) - 1);
> +	n_read = read(pm_status_fd, buf, len - 1);
>  	igt_assert(n_read >= 0);
>  	buf[n_read] = '\0';

Hey,

Adding extra logging here is a good idea, but I think we should improve
on implementation.

Now we return pm status in two ways - as a string and as a enum value.

My suggestion is to add a helper that would map enum back to a string
(some examples are in igt_kms.c, like mode_stereo_name()):

ret = igt_wait((status = igt_get_runtime_pm_status(buf, ARRAY_SIZE(buf))) == expected, 10000, 100);
if (!ret)
	igt_warn("timeout: pm_status expected: %s, got: %s\n",
		 _pm_status_name(expected),
		 _pm_status_name(status));

This also improves the logging, as we know the expected state
immediately.

You can try to tackle it in a different way, e.g.: by returning the
status only as "char buf[static 32]" and doing the str->enum mapping in
another helper, but that would be IMO too complicated...

> @@ -697,7 +698,14 @@ enum igt_runtime_pm_status igt_get_runtime_pm_status(void)
>   */
>  bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
>  {
> -	return igt_wait(igt_get_runtime_pm_status() == status, 10000, 100);
> +	bool ret;
> +	char buf[32];
> +
> +	ret = igt_wait(igt_get_runtime_pm_status(buf, ARRAY_SIZE(buf)) == status, 10000, 100);
> +	if (!ret)
> +		igt_warn("timeout: pm_status=%s", buf);

missing \n

--
Cheers,
Arek