Re: [igt-dev] [PATCH i-g-t] tests/kms_lease: exercise uevent

[Lyude Paul <lyude@redhat.com>]

On Thu, 2019-02-21 at 00:48 +0100, Daniel Vetter wrote:
> On Wed, Feb 20, 2019 at 06:42:43PM -0500, Lyude Paul wrote:
> > On Wed, 2019-02-20 at 22:17 +0100, Daniel Vetter wrote:
> > > And make sure we get the LEASE=1 value, indicating a lessee change.
> > > 
> > > v2: Apparently netlink reading can leak EAGAIN out through
> > > udev_monitor_receive_device. No idea what's going on there, so let's
> > > wrap some duct tape around it.
> > > 
> > > v3: Lyude reported that we might get a few udev events on startup of
> > > the test. Drain those first.
> > > 
> > > v4: Use the igt hotplug library functions, they already take care of
> > > all the uevent special cases.
> > > 
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Keith Packard <keithp@keithp.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  lib/igt_kms.c     | 42 +++++++++++++++++++++++++++++++-----------
> > >  lib/igt_kms.h     |  2 ++
> > >  tests/kms_lease.c | 36 ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 69 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > index 85a911e11060..8f0b79d7727b 100644
> > > --- a/lib/igt_kms.c
> > > +++ b/lib/igt_kms.c
> > > @@ -4019,16 +4019,8 @@ struct udev_monitor *igt_watch_hotplug(void)
> > >  	return mon;
> > >  }
> > >  
> > > -/**
> > > - * igt_hotplug_detected:
> > > - * @mon: A udev monitor initialized with #igt_watch_hotplug
> > > - * @timeout_secs: How long to wait for a hotplug event to occur.
> > > - *
> > > - * Assert that a hotplug event was received since we last checked the
> > > monitor.
> > > - *
> > > - * Returns: true if a sysfs hotplug event was received, false if we
> > > timed
> > > out
> > > - */
> > > -bool igt_hotplug_detected(struct udev_monitor *mon, int timeout_secs)
> > > +static bool event_detected(struct udev_monitor *mon, int timeout_secs,
> > > +			   const char *property)
> > >  {
> > >  	struct udev_device *dev;
> > >  	const char *hotplug_val;
> > > @@ -4046,7 +4038,7 @@ bool igt_hotplug_detected(struct udev_monitor
> > > *mon,
> > > int timeout_secs)
> > >  	while (!hotplug_received && poll(&fd, 1, timeout_secs * 1000)) {
> > >  		dev = udev_monitor_receive_device(mon);
> > >  
> > > -		hotplug_val = udev_device_get_property_value(dev, "HOTPLUG");
> > > +		hotplug_val = udev_device_get_property_value(dev, property);
> > >  		if (hotplug_val && atoi(hotplug_val) == 1)
> > >  			hotplug_received = true;
> > >  
> > > @@ -4056,6 +4048,34 @@ bool igt_hotplug_detected(struct udev_monitor
> > > *mon,
> > > int timeout_secs)
> > >  	return hotplug_received;
> > >  }
> > >  
> > > +/**
> > > + * igt_hotplug_detected:
> > > + * @mon: A udev monitor initialized with #igt_watch_hotplug
> > > + * @timeout_secs: How long to wait for a hotplug event to occur.
> > > + *
> > > + * Assert that a hotplug event was received since we last checked the
> > > monitor.
> > > + *
> > > + * Returns: true if a sysfs hotplug event was received, false if we
> > > timed
> > > out
> > > + */
> > > +bool igt_hotplug_detected(struct udev_monitor *mon, int timeout_secs)
> > > +{
> > > +	return event_detected(mon, timeout_secs, "HOTPLUG");
> > > +}
> > > +
> > > +/**
> > > + * igt_lease_change_detected:
> > > + * @mon: A udev monitor initialized with #igt_watch_hotplug
> > > + * @timeout_secs: How long to wait for a lease change event to occur.
> > > + *
> > > + * Assert that a lease change event was received since we last checked
> > > the
> > > monitor.
> > > + *
> > > + * Returns: true if a sysfs lease change event was received, false if
> > > we
> > > timed out
> > > + */
> > > +bool igt_lease_change_detected(struct udev_monitor *mon, int
> > > timeout_secs)
> > > +{
> > > +	return event_detected(mon, timeout_secs, "LEASE");
> > > +}
> > > +
> > >  /**
> > >   * igt_flush_hotplugs:
> > >   * @mon: A udev monitor initialized with #igt_watch_hotplug
> > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > > index 679d4e84fab8..5755b160d13e 100644
> > > --- a/lib/igt_kms.h
> > > +++ b/lib/igt_kms.h
> > > @@ -749,6 +749,8 @@ const unsigned char* igt_kms_get_alt_edid(void);
> > >  struct udev_monitor *igt_watch_hotplug(void);
> > >  bool igt_hotplug_detected(struct udev_monitor *mon,
> > >  			  int timeout_secs);
> > > +bool igt_lease_change_detected(struct udev_monitor *mon,
> > > +			       int timeout_secs);
> > >  void igt_flush_hotplugs(struct udev_monitor *mon);
> > >  void igt_cleanup_hotplug(struct udev_monitor *mon);
> > >  
> > > diff --git a/tests/kms_lease.c b/tests/kms_lease.c
> > > index d082a7988eaa..600f6779561d 100644
> > > --- a/tests/kms_lease.c
> > > +++ b/tests/kms_lease.c
> > > @@ -40,6 +40,8 @@
> > >  #include <sys/time.h>
> > >  #include <sys/wait.h>
> > >  
> > > +#include <libudev.h>
> > > +
> > >  #include <drm.h>
> > >  
> > >  IGT_TEST_DESCRIPTION("Test of CreateLease.");
> > > @@ -976,6 +978,37 @@ static void implicit_plane_lease(data_t *data)
> > >  	drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> > >  }
> > >  
> > > +static void lease_uevent(data_t *data)
> > > +{
> > > +	int lease_fd;
> > > +	struct local_drm_mode_list_lessees mll;
> > > +	struct udev_monitor *uevent_monitor;
> > > +
> > > +	uevent_monitor = igt_watch_hotplug();
> > > +
> > > +	igt_flush_hotplugs(uevent_monitor);
> > Shouldn't we rename
> > igt_watch_hotplug()/igt_flush_hotplugs()/igt_cleanup_hotplug()/etc. as
> > well to
> > reflect that these are used for more then udev hotplug events now?
> 
> I was feeling unmotivated :-)
> 
> Slightly more seriously, the excuse is that I expect all testcases except
> this one will want hotplug uevents, nothing else. So the naming makes
> some sense to keep, leaving kms_lease here as the special case.
> 
> Originally drm leases even use HOTPLUG=1 as their uevent too, but we fixed
> that before it became uapi. -modesetting still doesn't differentiate.
> -Daniel

Makes sense. In that case:

Reviewed-by: Lyude Paul <lyude@redhat.com>

> 
> > Otherwise, looks good to me
> > 
> > > +
> > > +	lease_fd = create_simple_lease(data->master.fd, data);
> > > +
> > > +	igt_assert(!igt_lease_change_detected(uevent_monitor, 1));
> > > +
> > > +	mll.pad = 0;
> > > +	mll.count_lessees = 0;
> > > +	mll.lessees_ptr = 0;
> > > +	igt_assert_eq(list_lessees(data->master.fd, &mll), 0);
> > > +	igt_assert_eq(mll.count_lessees, 1);
> > > +
> > > +	close(lease_fd);
> > > +
> > > +	igt_assert(igt_lease_change_detected(uevent_monitor, 1));
> > > +
> > > +	mll.lessees_ptr = 0;
> > > +	igt_assert_eq(list_lessees(data->master.fd, &mll), 0);
> > > +	igt_assert_eq(mll.count_lessees, 0);
> > > +
> > > +	igt_cleanup_hotplug(uevent_monitor);
> > > +}
> > > +
> > >  igt_main
> > >  {
> > >  	data_t data;
> > > @@ -1023,4 +1056,7 @@ igt_main
> > >  
> > >  	igt_subtest("implicit-plane-lease")
> > >  		implicit_plane_lease(&data);
> > > +
> > > +	igt_subtest("lease-uevent")
> > > +		lease_uevent(&data);
> > >  }
> > -- 
> > Cheers,
> > 	Lyude Paul
> > 
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev