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 at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
