Re: [i-g-t PATCH 4/4] flip_test: switch to using monotonic timestamps

[Imre Deak <imre.deak@intel.com>]

On Thu, 2012-11-22 at 14:46 +0100, Daniel Vetter wrote:
> On Thu, Nov 22, 2012 at 03:25:06PM +0200, Imre Deak wrote:
> > Since monotonic timestamps are now the preferred time format, change
> > timestamps checks to compare against monotonic instead of real time.
> > Also add two tests for DRM's compatibility mode where it returns real
> > timestamps.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> A few comments below.
> -Daniel
> 
> > ---
> >  tests/flip_test.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/flip_test.c b/tests/flip_test.c
> > index d88f81c..258d727 100644
> > --- a/tests/flip_test.c
> > +++ b/tests/flip_test.c
> > @@ -53,6 +53,7 @@
> >  #define TEST_VBLANK_BLOCK	(1 << 9)
> >  #define TEST_VBLANK_ABSOLUTE	(1 << 10)
> >  #define TEST_VBLANK_EXPIRED_SEQ	(1 << 11)
> > +#define TEST_TIMESTAMP_REAL	(1 << 12)
> >  
> >  #define EVENT_FLIP		(1 << 0)
> >  #define EVENT_VBLANK		(1 << 1)
> > @@ -63,6 +64,7 @@ static drm_intel_bufmgr *bufmgr;
> >  struct intel_batchbuffer *batch;
> >  uint32_t devid;
> >  int test_time = 3;
> > +static bool monotonic_timestamp;
> >  
> >  uint32_t *fb_ptr;
> >  
> > @@ -296,7 +298,19 @@ analog_tv_connector(struct test_output *o)
> >  static void event_handler(struct event_state *es, unsigned int frame,
> >  			  unsigned int sec, unsigned int usec)
> >  {
> > -	gettimeofday(&es->current_received_ts, NULL);
> > +	struct timeval now;
> > +
> > +	if (monotonic_timestamp) {
> > +		struct timespec ts;
> > +
> > +		clock_gettime(CLOCK_MONOTONIC, &ts);
> > +		now.tv_sec = ts.tv_sec;
> > +		now.tv_usec = ts.tv_nsec / 1000;
> > +	} else {
> > +		gettimeofday(&now, NULL);
> > +	}
> > +	es->current_received_ts = now;
> > +
> >  	es->current_ts.tv_sec = sec;
> >  	es->current_ts.tv_usec = usec;
> >  	es->current_seq = frame;
> > @@ -899,6 +913,28 @@ static int get_pipe_from_crtc_id(int crtc_id)
> >  	return pfci.pipe;
> >  }
> >  
> > +static void enable_monotonic_timestamp(bool enable)
> > +{
> > +	static const char *sysfs_mono_path =
> > +		"/sys/module/drm/parameters/timestamp_monotonic";
> > +	int mono_ts_enabled;
> > +	int scanned;
> > +	FILE *f;
> > +
> > +	f = fopen(sysfs_mono_path, "r+");
> > +	assert(f);
> > +
> > +	scanned = fscanf(f, "%d", &mono_ts_enabled);
> > +	assert(scanned == 1 && (mono_ts_enabled == 1 || mono_ts_enabled == 0));
> > +
> > +	if (mono_ts_enabled != enable)
> > +		fprintf(f, "%d", enable);
> 
> I think it'd be better to use the drm getcap ioctl here (and default to
> disabled if it returns -EINVAL), since that's the officially blessed
> interface to figure this out.
> 
> 
> > +
> > +	fclose(f);
> > +
> > +	monotonic_timestamp = enable;
> > +}
> > +
> >  static int run_test(int duration, int flags, const char *test_name)
> >  {
> >  	struct test_output o;
> > @@ -911,6 +947,8 @@ static int run_test(int duration, int flags, const char *test_name)
> >  		exit(5);
> >  	}
> >  
> > +	enable_monotonic_timestamp(!(flags & TEST_TIMESTAMP_REAL));
> 
> I don't follow why we should enable monotonic timestamps for some tests
> and not for some others? Shouldn't all tests with TEST_CHECK_TS just use
> the same clock the kernel uses?

The idea was to also test the compatibility mode, where we get real
timestamps.

> 
> > +
> >  	/* Find any connected displays */
> >  	for (c = 0; c < resources->count_connectors; c++) {
> >  		for (i = 0; i < resources->count_crtcs; i++) {
> > @@ -941,6 +979,8 @@ int main(int argc, char **argv)
> >  		const char *name;
> >  	} tests[] = {
> >  		{ 15, TEST_VBLANK | TEST_CHECK_TS, "wf-vblank" },
> > +		{ 15, TEST_VBLANK | TEST_TIMESTAMP_REAL | TEST_CHECK_TS,
> > +					"wf-vblank with real timestamps" },
> >  		{ 15, TEST_VBLANK | TEST_VBLANK_BLOCK | TEST_CHECK_TS,
> >  					"blocking wf-vblank" },
> >  		{ 5,  TEST_VBLANK | TEST_VBLANK_ABSOLUTE,
> > @@ -955,6 +995,8 @@ int main(int argc, char **argv)
> >  					"delayed wf-vblank vs modeset" },
> >  
> >  		{ 15, TEST_FLIP | TEST_CHECK_TS | TEST_EBUSY , "plain flip" },
> > +		{ 5,  TEST_FLIP | TEST_TIMESTAMP_REAL | TEST_CHECK_TS,
> > +					"flip with real timestamps" },
> >  		{ 30, TEST_FLIP | TEST_DPMS | TEST_EINVAL, "flip vs dpms" },
> >  		{ 30, TEST_FLIP | TEST_DPMS | TEST_WITH_DUMMY_LOAD, "delayed flip vs dpms" },
> >  		{ 5,  TEST_FLIP | TEST_PAN, "flip vs panning" },
> > @@ -973,6 +1015,11 @@ int main(int argc, char **argv)
> >  	};
> >  	int i;
> >  
> > +	if (geteuid() != 0) {
> > +		fprintf(stderr, "you must run this as root\n");
> > +		exit(EXIT_FAILURE);
> > +	}
> 
> If you want this, I think we should have a common function to check for
> master capability ... Imo you can drop this.

I added this early check since accessing timestamp_monotonic needs it.
Otherwise you get some error only later when switching to compatibility
mode, which I thought was confusing.

> 
> > +
> >  	drm_fd = drm_open_any();
> >  
> >  	bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
> > -- 
> > 1.7.9.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>