Re: [igt-dev] [PATCH i-g-t v2 1/2] lib/igt_debugfs: set provision to ignore long HPDs

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

Hi Kamil,

Thanks for the comments. A new version sent after addressing your comments.

Regarding resetting the value, it is not explicitly done when leaving the subtest. As this mainly
targets CI execution in systems with panels which generate such spurious HPDs and CI is expected to
set the env variable IGT_KMS_IGNORE_HPD all the time. In some HPD tests using chamelion etc., this
flag is unset separately by such tests after the igt_display_require call is completed.

BR
vinod

On Wed, 2023-03-08 at 19:36 +0100, Kamil Konieczny wrote:
> Hi Vinod,
> 
> On 2023-02-14 at 11:57:23 +0200, Vinod Govindapillai wrote:
> > Add provision to ignore long HPDs as some displays generate
> > long HPDs even while connected. For some CI systems with
> > such displays it might be usefull to ignore such spurious
> > HPDs while executing testcases.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > ---
> >  lib/igt_debugfs.c | 22 ++++++++++++++++++++++
> >  lib/igt_debugfs.h |  1 +
> >  2 files changed, 23 insertions(+)
> > 
> > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > index 6de178d6..8d2d3144 100644
> > --- a/lib/igt_debugfs.c
> > +++ b/lib/igt_debugfs.c
> > @@ -411,6 +411,28 @@ bool igt_debugfs_search(int device, const char *filename, const char
> > *substring)
> >         return matched;
> >  }
> >  
> > +/**
> > + * igt_ignore_long_hpd:
> 
> Add also description of parameters here.
> 
> > + *
> > + * Set / unset ignore long HPD events from the panels. Some panels
> > + * generate long HPDs even while connected to the ports causing
> > + * unexpected CI execution issues. Set this to ignore such unexpected
> > + * long HPDs where we dont expect to disconnect the displays.
> > + */
> > +void igt_ignore_long_hpd(int drm_fd, bool enable)
> -- ^
> imho bool would be better.
> 
> > +{
> > +       int fd = igt_debugfs_open(drm_fd, "i915_ignore_long_hpd", O_WRONLY);
> > +
> > +       if (fd < 0) {
> > +               igt_debug("couldn't open ignore long hpd file\n");
> > +               return;
> 
> Then you can
>         return false;
> here.
> 
> > +       }
> > +
> > +       igt_assert_eq(write(fd, enable ? "1" : "0", 1), 1);
> 
> Asserts in libs should be avoided, so maybe:
>         ret = write(fd, enable ? "1" : "0", 1);
> 
> > +
> > +       close(fd);
> 
> and then:
>         return ret == 1;
> 
> Btw regarding your second patch, did you reset this to old value
> before leaving subtest ?
> 
> Regards,
> Kamil
> 
> > +}
> > +
> >  static void igt_hpd_storm_exit_handler(int sig)
> >  {
> >         int fd = drm_open_driver(DRIVER_INTEL);
> > diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
> > index a883e2d4..fe23a5ef 100644
> > --- a/lib/igt_debugfs.h
> > +++ b/lib/igt_debugfs.h
> > @@ -72,6 +72,7 @@ void igt_hpd_storm_set_threshold(int fd, unsigned int threshold);
> >  void igt_hpd_storm_reset(int fd);
> >  bool igt_hpd_storm_detected(int fd);
> >  void igt_require_hpd_storm_ctl(int fd);
> > +void igt_ignore_long_hpd(int fd, bool enable);
> >  
> >  /*
> >   * Drop caches
> > -- 
> > 2.34.1
> >