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

Arkadiusz Hiler
2019-10-10T11:01:45Z

On Tue, Oct 08, 2019 at 09:07:50AM -0700, don.hiatt@intel.com wrote:
> From: Don Hiatt <don.hiatt@intel.com>
>
> Display the runtime pm status if we timeout waiting for status.
>
> Signed-off-by: Don Hiatt <don.hiatt@intel.com>
> Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@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