[igt-dev] [PATCH v2 2/2] tests/debugfs_test: Add subtest guc_log_relay

[Piorkowski, Piotr]

On Tue, 2018-04-24 at 09:34 +0100, Chris Wilson wrote:
> Quoting Piotr Piorkowski (2018-04-24 08:50:04)
> > From: Piotr Piórkowski <piotr.piorkowski at intel.com>
> > 
> > We doesn't have any tests to verify functionality of
> > i915_guc_log_relay.
> > I think we should test this component.
> > 
> > Let's add simple subtest to debugfs_test, which will test if guc
> > is enabled:
> > - opening the i915_guc_log_relay and creating a file guc_log0,
> > - flushing GuC log buffer,
> > - closing i915_guc_log_relay with removing from debugfs guc_log0.
> > Otherwise, the test will check if the attempt to open the
> > guc_log_relay
> > returns an error ENODEV.
> > 
> > v2:
> > - guc isn't required for run the subtest (Chris)
> > - rewrite subtest as function (Michal Winiarski)
> > - add case when the guc is not enabled
> > 
> > Signed-off-by: Piotr Piórkowski <piotr.piorkowski at intel.com>
> > Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> > Cc: Lukasz Fiedorowicz <lukasz.fiedorowicz at intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > Cc: Michał Winiarski <michal.winiarski at intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  tests/debugfs_test.c | 93
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> > 
> > diff --git a/tests/debugfs_test.c b/tests/debugfs_test.c
> > index 2e87e442..c7a953b1 100644
> > --- a/tests/debugfs_test.c
> > +++ b/tests/debugfs_test.c
> > @@ -26,6 +26,7 @@
> >  #include <fcntl.h>
> >  #include <sys/types.h>
> >  #include <dirent.h>
> > +#include <errno.h>
> >  
> >  static void read_and_discard_sysfs_entries(int path_fd, int
> > indent)
> >  {
> > @@ -87,6 +88,95 @@ static void read_and_discard_sysfs_entries(int
> > path_fd, int indent)
> >         closedir(dir);
> >  }
> >  
> > +static bool is_guc_loaded(int gfx_fd)
> > +{
> > +       bool load;
> > +
> > +       load = igt_debugfs_search(gfx_fd, "i915_guc_load_status",
> > +                               "    status: fetch SUCCESS, load
> > SUCCESS");
> > +       igt_debug("GuC %s loaded\n", load ? "is" : "is not");
> > +
> > +       return load;
> > +}
> > +
> > +#define I915_GUC_LOG_RELAY_FILE  "i915_guc_log_relay"
> > +#define GUC_LOG_FILE  "guc_log0"
> > +
> > +static void check_guc_log_relay(int gfx_fd)
> > +{
> > +
> > +       int page_size = getpagesize();
> > +       int fd_relay = -1, fd_log = -1;
> > +       ssize_t read_size;
> > +       uint8_t *buffer;
> > +       bool guc_is_loaded;
> > +       int debugfs;
> > +
> > +       guc_is_loaded = is_guc_loaded(gfx_fd);
> > +
> > +       debugfs = igt_debugfs_dir(gfx_fd);
> > +       fd_relay = openat(debugfs, I915_GUC_LOG_RELAY_FILE,
> > O_RDWR);
> > +
> > +       if (!guc_is_loaded) {
> > +               igt_debug("Case when GuC is disabled\n");
> 
> Debugging left over? Lots of fluff messages.
> 
> > +               igt_debug("Attempt open file %s return value: %d (
> > %m )\n",
> > +                         I915_GUC_LOG_RELAY_FILE, errno);
> 
> Push this into the failure message.
> 
> > +
> > +               igt_assert(errno == ENODEV);
> 
> errno may have been scribbled over many times, and would have only
> been
> valid if fd_relay is -1.
> 
> Use igt_assert_eq, but you really want igt_assert_f() if you want
> informative error messages rather than igt_debug spam.
> 
> But is this the interface you really want to enforce? If the guc log
> relay is accessible is up to the kernel, not you. If the kernel tells
> you it is not available, skip and move on.

This subtest is to check if guc_log_relay works correctly, no matter if
the guc works or not. The correct behavior of guc_log_relay, when the
guc is disabled, is to return the error ENODEV, and this subtest checks
it (with fix if fd_relay is -1). So do you think that this way to
realize the subtest is wrong, and I should change it? 

Piotr
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3278 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20180424/73647d3f/attachment.bin>