[Intel-gfx] [PATCH IGT v2 2/6] lib: Add PSR utility functions to igt library.
Jim Bride
jim.bride at linux.intel.com
Fri Jul 7 13:53:57 UTC 2017
On Fri, Jun 30, 2017 at 05:54:32PM -0300, Paulo Zanoni wrote:
> Em Sex, 2017-06-30 às 12:12 -0700, Jim Bride escreveu:
> > Factor out some code that was replicated in three test utilities into
> > some new IGT library functions so that we are checking PSR status in
> > a consistent fashion across all of our PSR tests.
> >
> > v2: * Add sink CRC helper function
> > * Misc. bug fixes
> > * Rebase
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Signed-off-by: Jim Bride <jim.bride at linux.intel.com>
> > ---
> > lib/Makefile.sources | 2 +
> > lib/igt.h | 1 +
> > lib/igt_psr.c | 235
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > lib/igt_psr.h | 43 ++++++++++
> > 4 files changed, 281 insertions(+)
> > create mode 100644 lib/igt_psr.c
> > create mode 100644 lib/igt_psr.h
> >
> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> > index 53fdb54..6a73c8c 100644
> > --- a/lib/Makefile.sources
> > +++ b/lib/Makefile.sources
> > @@ -83,6 +83,8 @@ lib_source_list = \
> > uwildmat/uwildmat.c \
> > igt_kmod.c \
> > igt_kmod.h \
> > + igt_psr.c \
> > + igt_psr.h \
> > $(NULL)
> >
> > .PHONY: version.h.tmp
> > diff --git a/lib/igt.h b/lib/igt.h
> > index a069deb..0b8e3a8 100644
> > --- a/lib/igt.h
> > +++ b/lib/igt.h
> > @@ -37,6 +37,7 @@
> > #include "igt_gt.h"
> > #include "igt_kms.h"
> > #include "igt_pm.h"
> > +#include "igt_psr.h"
> > #include "igt_stats.h"
> > #ifdef HAVE_CHAMELIUM
> > #include "igt_chamelium.h"
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > new file mode 100644
> > index 0000000..a2823bf
> > --- /dev/null
> > +++ b/lib/igt_psr.c
> > @@ -0,0 +1,235 @@
> > +/*
> > + * Copyright © 2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice (including
> > the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> > OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include "igt.h"
> > +#include <stdio.h>
> > +#include <stdbool.h>
> > +#include <sys/types.h>
> > +#include <string.h>
> > +#include <fcntl.h>
> > +
> > +/**
> > + * SECTION:igt_psr
> > + * @short_description: Panel Self Refresh helpers
> > + * @title: Panel Self Refresh
> > + * @include: igt.h
> > + *
> > + * This library provides various helpers to enable Panel Self
> > Refresh,
> > + * as well as to check the state of PSR on the system (enabled vs.
> > + * disabled, active vs. inactive) or to wait for PSR to be active
> > + * or inactive.
> > + */
> > +
> > +/**
> > + * igt_psr_source_support:
> > + *
> > + * Returns true if the source supports PSR.
> > + */
> > +bool igt_psr_source_support(int fd)
> > +{
> > + char buf[512];
> > +
> > + igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +
> > + return strstr(buf, "Source_OK: yes\n");
> > +}
> > +
> > +
> > +/**
> > + * igt_psr_sink_support:
> > + *
> > + * Returns true if the current eDP panel supports PSR.
> > + */
> > +bool igt_psr_sink_support(int fd)
> > +{
> > + char buf[256];
>
> Why are some buffers 512 while the others are 256? They're both for the
> same file.
It's arbitrary. I'll make things consistent when I revisit this patch.
> > +
> > + igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > + return strstr(buf, "Sink_Support: yes\n");
> > +}
> > +
> > +/**
> > + * igt_psr_possible:
> > + *
> > + * Returns true if both the source and sink support PSR.
> > + */
> > +bool igt_psr_possible(int fd)
> > +{
> > + char buf[512];
> > +
> > + igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +
> > + return igt_psr_source_support(fd) &&
> > igt_psr_sink_support(fd);
> > +}
> > +
> > +/**
> > + * igt_psr_active:
> > + *
> > + * Returns true if PSR is active on the panel.
> > + */
> > +bool igt_psr_active(int fd)
> > +{
> > + char buf[512];
> > + bool actret = false;
> > + bool hwactret = false;
> > +
> > + igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > + hwactret = (strstr(buf, "HW Enabled & Active bit: yes\n") !=
> > NULL);
> > + actret = (strstr(buf, "Active: yes\n") != NULL);
> > + igt_debug("hwactret: %s actret: %s\n", hwactret ? "true" :
> > "false",
> > + actret ? "true" : "false");
> > + return hwactret && actret;
> > +}
> > +
> > +/**
> > + * igt_psr_await_status:
> > + * @active: A boolean that causes the function to wait for PSR to
> > activate
> > + * if set to true, or to wait for PSR to deactivate if
> > false.
> > + *
> > + * Returns true if the requested condition is met.
> > + */
> > +bool igt_psr_await_status(int fd, bool active)
> > +{
> > + const int timeout = 10;
> > + int count = 0;
> > + while (count < timeout) {
> > + if (igt_psr_active(fd) == active) {
> > + igt_debug("PSR %s after %d seconds.\n",
> > + active ? "Active" : "Inactive",
> > count);
> > + return true;
> > + }
> > + count++;
> > + sleep(1);
> > + }
> > + return false;
>
> Since this is in a lib now, can you please make use of igt_wait or just
> make it work as expected when the signal helper is active?
Sure.
>
> > +}
> > +
> > +/**
> > + * igt_psr_enabled:
> > + *
> > + * Returns true if the PSR feature is enabled.
> > + */
> > +bool igt_psr_enabled(int fd)
> > +{
> > + char buf[256];
> > +
> > + igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > + return strstr(buf, "Enabled: yes\n");
> > +}
> > +
> > +/**
> > + * igt_psr_print_status:
> > + *
> > + * Dumps the contents of i915_edp_psr_status from debugfs.
> > + */
> > +void igt_psr_print_status(int fd)
> > +{
> > + char buf[256];
> > +
> > + igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > + igt_info("PSR status:\n%s\n", buf);
> > +}
> > +
> > +/**
> > + * igt_psr_valid_connector:
> > + * @connector: a drmModeConnector pointer to check
> > + *
> > + * Returns true if connector is an eDP connector.
> > + */
> > +bool igt_psr_valid_connector(drmModeConnectorPtr connector)
> > +{
> > + return (connector->connector_type ==
> > DRM_MODE_CONNECTOR_eDP);
> > +}
> > +
> > +/**
> > + * igt_psr_find_good_mode
> > + * @connector: a drmModeConnector pointer to find the mode on
> > + * @mode: a drmModeModePtr pointer that is set to the matching mode
> > + *
> > + * Returns true (and populates *mode with the match) if a valid
> > + * PSR mdoe is found, and false otherwise.
> > + */
> > +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> > + drmModeModeInfoPtr *mode)
> > +{
> > + int i;
> > +
> > + if (!connector->count_modes) {
> > + igt_warn("no modes for connector %d.\n",
> > + connector->connector_id);
> > + return false;
> > + } else {
> > + igt_debug("Connector has %d modes.\n", connector-
> > >count_modes);
> > + }
> > +
> > + for (i = 0; i < connector->count_modes; i++) {
> > + if ((connector->modes[i].vtotal -
> > + connector->modes[i].vdisplay) >= 36) {
> > + igt_debug("Mode %d good for PSR.\n", i);
> > + *mode = &(connector->modes[i]);
> > + return true;
>
> Since the callers may be trying to find modes that fit other
> restrictions (e.g., kms_frontbuffer_tracking) I'd suggest you to
> extract igt_psr_mode_is_compatible(mode) with the vtotal/vdisplay
> check.
Can you explain what you mean by extract here? Do you mean have it live
somewhere other than with the other PSR functions? If so, what would
you suggest?
>
> > + } else {
> > + igt_debug("Throwing out mode %d\n", i);
> > + }
> > + }
> > + return false;
> > +}
> > +
> > +int igt_psr_open_sink_crc(int drm_fd)
> > +{
> > + int crc_fd;
> > +
> > + crc_fd = openat(drm_fd, "i915_sink_crc_eDP1", O_RDONLY);
> > + igt_assert_lte(0, crc_fd);
> > + return crc_fd;
> > +}
> > +
> > +
>
> Please add documentation for this one, especially around the
> expectation for "data".
Good call.
> > +int igt_psr_get_sink_crc(int fd, char *data)
> > +{
> > + int rc, errno_;
> > + char buf[13];
> > +
> > + memset(buf, 0, 13);
> > + lseek(fd, 0, SEEK_SET);
> > +
> > + rc = read(fd, buf, 12);
> > + errno_ = errno;
> > +
> > + if (rc == -1) {
> > + if (errno_ == ENOTTY)
> > + igt_info("Sink CRC not supported: panel
> > doesn't support it\n");
> > + else if (errno_ != ETIMEDOUT)
> > + igt_assert_f(rc != -1, "Unexpected error:
> > %d\n",
> > + errno_);
> > + return errno_;
> > + } else {
> > + int i;
> > + unsigned long long val = strtoull(buf, NULL, 16);
> > +
> > + for (i = 5; i >= 0; i--, val >>= 8) {
> > + data[i] = (unsigned char) (val & 0xff);
> > + }
>
> This is a very non-trivial piece of code, a comment here would be good.
> Why are we doing this? I'm not sure what's the value in converting CRC
> data formats when the only thing we do is check for equality.
We were actually dealing with the CRC values differently across tests. In
some cases the values were kept as strings and compared with string funcs,
and in others they were converted to binary values and then compared with
memcmp. I chose to have the binary values be what is used. I'll add a
comment.
> > + return 0;
> > + }
> > +}
> > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > new file mode 100644
> > index 0000000..d94cdfa
> > --- /dev/null
> > +++ b/lib/igt_psr.h
> > @@ -0,0 +1,43 @@
> > +/*
> > + * Copyright © 2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice (including
> > the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> > OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef IGT_PSR_H
> > +#define IGT_PSR_H
> > +
> > +#define igt_psr_enable() igt_set_module_param_int("enable_psr", 1)
> > +#define igt_psr_disable() igt_set_module_param_int("enable_psr", 0)
> > +
> > +bool igt_psr_sink_support(int fd);
> > +bool igt_psr_source_support(int fd);
> > +bool igt_psr_possible(int fd);
> > +bool igt_psr_active(int fd);
> > +bool igt_psr_await_status(int fd, bool active);
> > +bool igt_psr_enabled(int fd);
> > +void igt_psr_print_status(int fd);
> > +bool igt_psr_valid_connector(drmModeConnectorPtr connector);
> > +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> > + drmModeModeInfoPtr *mode);
> > +int igt_psr_open_sink_crc(int drm_fd);
> > +int igt_psr_get_sink_crc(int fd, char *data);
> > +
> > +#endif /* IGT_PSR_H */
More information about the Intel-gfx
mailing list