[i-g-t PATCH 0/4] flip_test: support for monotonic timestamps

[i-g-t PATCH 0/4] flip_test: support for monotonic timestamps

[Imre Deak]

The first patch isn't related, just a left-over from the previous
patchset.

Imre Deak (4):
  flip_test: add wf-vblank test for expired sequence
  flip_test: skip check for last_received_ts for the first event
  flip_test: use monotonic time to measure the test duration
  flip_test: switch to using monotonic timestamps

 tests/flip_test.c |  132 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 104 insertions(+), 28 deletions(-)

-- 
1.7.9.5

[i-g-t PATCH 1/4] flip_test: add wf-vblank test for expired sequence

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 tests/flip_test.c |   58 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/tests/flip_test.c b/tests/flip_test.c
index 7fed2f1..3dfce8e 100644
--- a/tests/flip_test.c
+++ b/tests/flip_test.c
@@ -52,6 +52,7 @@
 #define TEST_VBLANK		(1 << 8)
 #define TEST_VBLANK_BLOCK	(1 << 9)
 #define TEST_VBLANK_ABSOLUTE	(1 << 10)
+#define TEST_VBLANK_EXPIRED_SEQ	(1 << 11)
 
 #define EVENT_FLIP		(1 << 0)
 #define EVENT_VBLANK		(1 << 1)
@@ -116,6 +117,16 @@ struct test_output {
 	unsigned int pending_events;
 };
 
+
+static unsigned long gettime_us(void)
+{
+	struct timespec ts;
+
+	clock_gettime(CLOCK_MONOTONIC, &ts);
+
+	return ts.tv_sec * 1000000 + ts.tv_nsec / 1000;
+}
+
 static void emit_dummy_load(struct test_output *o)
 {
 	int i, limit;
@@ -221,13 +232,14 @@ struct vblank_reply {
 	struct timeval ts;
 };
 
-static int do_wait_for_vblank(struct test_output *o, int crtc_idx,
-			      int target_seq, struct vblank_reply *reply)
+static int __wait_for_vblank(unsigned int flags, int crtc_idx,
+			      int target_seq, unsigned long ret_data,
+			      struct vblank_reply *reply)
 {
 	drmVBlank wait_vbl;
 	int ret;
 	unsigned crtc_idx_mask;
-	bool event = !(o->flags & TEST_VBLANK_BLOCK);
+	bool event = !(flags & TEST_VBLANK_BLOCK);
 
 	memset(&wait_vbl, 0, sizeof(wait_vbl));
 
@@ -235,13 +247,13 @@ static int do_wait_for_vblank(struct test_output *o, int crtc_idx,
 	assert(!(crtc_idx_mask & ~DRM_VBLANK_HIGH_CRTC_MASK));
 
 	wait_vbl.request.type = crtc_idx_mask;
-	if (o->flags & TEST_VBLANK_ABSOLUTE)
+	if (flags & TEST_VBLANK_ABSOLUTE)
 		wait_vbl.request.type |= DRM_VBLANK_ABSOLUTE;
 	else
 		wait_vbl.request.type |= DRM_VBLANK_RELATIVE;
 	if (event) {
 		wait_vbl.request.type |= DRM_VBLANK_EVENT;
-		wait_vbl.request.signal = (unsigned long)o;
+		wait_vbl.request.signal = ret_data;
 	}
 	wait_vbl.request.sequence = target_seq;
 
@@ -251,17 +263,25 @@ static int do_wait_for_vblank(struct test_output *o, int crtc_idx,
 		reply->ts.tv_sec = wait_vbl.reply.tval_sec;
 		reply->ts.tv_usec = wait_vbl.reply.tval_usec;
 		reply->sequence = wait_vbl.reply.sequence;
-
-		if (event) {
-			assert(!(o->pending_events & EVENT_VBLANK));
-			o->pending_events |= EVENT_VBLANK;
-		}
 	} else
 		ret = -errno;
 
 	return ret;
 }
 
+static int do_wait_for_vblank(struct test_output *o, int pipe_id,
+			      int target_seq, struct vblank_reply *reply)
+{
+	int ret;
+
+	ret = __wait_for_vblank(o->flags, pipe_id, target_seq, (unsigned long)o,
+				reply);
+	if (ret == 0 && !(o->flags & TEST_VBLANK_BLOCK))
+		set_flag(&o->pending_events, EVENT_VBLANK);
+
+	return ret;
+}
+
 static bool
 analog_tv_connector(struct test_output *o)
 {
@@ -460,6 +480,22 @@ static unsigned int run_test_step(struct test_output *o)
 	o->current_fb_id = !o->current_fb_id;
 	new_fb_id = o->fb_ids[o->current_fb_id];
 
+	if ((o->flags & TEST_VBLANK_EXPIRED_SEQ) &&
+	    !(o->pending_events & EVENT_VBLANK) && o->flip_state.count > 0) {
+		struct vblank_reply reply;
+		unsigned int exp_seq;
+		unsigned long start;
+
+		exp_seq = o->flip_state.current_seq;
+		start = gettime_us();
+		do_or_die(__wait_for_vblank(TEST_VBLANK_ABSOLUTE |
+					    TEST_VBLANK_BLOCK, o->pipe, exp_seq,
+					    0, &reply));
+		assert(gettime_us() - start < 500);
+		assert(reply.sequence == exp_seq);
+		assert(timercmp(&reply.ts, &o->flip_state.last_ts, ==));
+	}
+
 	if (do_flip && (o->flags & TEST_EINVAL) && o->flip_state.count > 0)
 		assert(do_page_flip(o, new_fb_id) == expected_einval);
 
@@ -934,6 +970,8 @@ int main(int argc, char **argv)
 		{ 30, TEST_FLIP | TEST_PAN | TEST_WITH_DUMMY_LOAD, "delayed flip vs panning" },
 		{ 30, TEST_FLIP | TEST_MODESET | TEST_EINVAL, "flip vs modeset" },
 		{ 30, TEST_FLIP | TEST_MODESET | TEST_WITH_DUMMY_LOAD, "delayed flip vs modeset" },
+		{ 5,  TEST_FLIP | TEST_VBLANK_EXPIRED_SEQ,
+					"flip vs. expired vblank" },
 
 		{ 15, TEST_FLIP | TEST_VBLANK | TEST_VBLANK_ABSOLUTE |
 		      TEST_CHECK_TS, "flip vs absolute wf-vblank" },
-- 
1.7.9.5

[i-g-t PATCH 2/4] flip_test: skip check for last_received_ts for the first event

[Imre Deak]

The old way worked too, but just skipping the test for the first event
is more logical/simpler thing to do.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 tests/flip_test.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tests/flip_test.c b/tests/flip_test.c
index 3dfce8e..e2af358 100644
--- a/tests/flip_test.c
+++ b/tests/flip_test.c
@@ -362,6 +362,9 @@ static void check_state(struct test_output *o, struct event_state *es)
 		exit(5);
 	}
 
+	if (es->count == 0)
+		return;
+
 	if (!timercmp(&es->last_received_ts, &es->current_ts, <)) {
 		fprintf(stderr, "%s ts before the %s was issued!\n",
 				es->name, es->name);
@@ -372,9 +375,6 @@ static void check_state(struct test_output *o, struct event_state *es)
 		exit(6);
 	}
 
-	if (es->count == 0)
-		return;
-
 	/* This bounding matches the one in DRM_IOCTL_WAIT_VBLANK. */
 	if (es->current_seq - (es->last_seq + es->seq_step) > 1UL << 23) {
 		fprintf(stderr, "unexpected %s seq %u, should be >= %u\n",
@@ -862,9 +862,6 @@ static void run_test_on_crtc(struct test_output *o, int crtc, int duration)
 	if (o->flags & TEST_CHECK_TS)
 		sleep(1);
 
-	gettimeofday(&o->flip_state.last_ts, NULL);
-	gettimeofday(&o->vblank_state.last_ts, NULL);
-
 	if (do_page_flip(o, o->fb_ids[1])) {
 		fprintf(stderr, "failed to page flip: %s\n", strerror(errno));
 		exit(4);
-- 
1.7.9.5

[i-g-t PATCH 3/4] flip_test: use monotonic time to measure the test duration

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 tests/flip_test.c |   16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/tests/flip_test.c b/tests/flip_test.c
index e2af358..d88f81c 100644
--- a/tests/flip_test.c
+++ b/tests/flip_test.c
@@ -786,15 +786,11 @@ static unsigned int wait_for_events(struct test_output *o)
 /* Returned the ellapsed time in us */
 static unsigned event_loop(struct test_output *o, unsigned duration_sec)
 {
-	struct timeval start, end;
-	struct timeval tv_dur;
+	unsigned long start, end;
 
-	gettimeofday(&start, NULL);
-	end.tv_sec = start.tv_sec + duration_sec;
-	end.tv_usec = start.tv_usec;
+	start = gettime_us();
 
 	while (1) {
-		struct timeval now;
 		unsigned int completed_events;
 
 		completed_events = run_test_step(o);
@@ -803,19 +799,17 @@ static unsigned event_loop(struct test_output *o, unsigned duration_sec)
 		check_all_state(o, completed_events);
 		update_all_state(o, completed_events);
 
-		gettimeofday(&now, NULL);
-		if (!timercmp(&now, &end, <))
+		if ((gettime_us() - start) / 1000000 >= duration_sec)
 			break;
 	}
 
-	gettimeofday(&end, NULL);
-	timersub(&end, &start, &tv_dur);
+	end = gettime_us();
 
 	/* Flush any remaining events */
 	if (o->pending_events)
 		wait_for_events(o);
 
-	return tv_dur.tv_sec * 1000 * 1000 + tv_dur.tv_usec;
+	return end - start;
 }
 
 static void run_test_on_crtc(struct test_output *o, int crtc, int duration)
-- 
1.7.9.5

Re: [i-g-t PATCH 3/4] flip_test: use monotonic time to measure the test duration

[Daniel Vetter]

On Thu, Nov 22, 2012 at 03:25:05PM +0200, Imre Deak wrote:
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Merged patches 1-3, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Imre Deak]

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>
---
 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);
+
+	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));
+
 	/* 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);
+	}
+
 	drm_fd = drm_open_any();
 
 	bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
-- 
1.7.9.5

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

[Daniel Vetter]

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?

> +
>  	/* 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.

> +
>  	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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Imre Deak]

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
> 

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

[Daniel Vetter]

On Thu, Nov 22, 2012 at 2:50 PM, Imre Deak <imre.deak@intel.com> wrote:
>> > +   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.

Oh, I've missed that you write back the desired value. I don't think
we need to test the compat mode if it's not enabled, just test
whatever mode is enabled. That way we also check whether the getcap
interface works.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Imre Deak]

On Thu, 2012-11-22 at 15:04 +0100, Daniel Vetter wrote:
> On Thu, Nov 22, 2012 at 2:50 PM, Imre Deak <imre.deak@intel.com> wrote:
> >> > +   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.
> 
> Oh, I've missed that you write back the desired value. I don't think
> we need to test the compat mode if it's not enabled, just test
> whatever mode is enabled. That way we also check whether the getcap
> interface works.

Ok, will do so. We have to wait though until DRM_CAP_TIMESTAMP_MONOTONIC
is added to libdrm. I've sent already a patch for this.

--Imre

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

[Imre Deak]

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.

v2: drop the tests for the compatibilty mode (Daniel)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 tests/flip_test.c |   33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/tests/flip_test.c b/tests/flip_test.c
index d88f81c..5214da9 100644
--- a/tests/flip_test.c
+++ b/tests/flip_test.c
@@ -57,12 +57,17 @@
 #define EVENT_FLIP		(1 << 0)
 #define EVENT_VBLANK		(1 << 1)
 
+#ifndef DRM_CAP_TIMESTAMP_MONOTONIC
+#define DRM_CAP_TIMESTAMP_MONOTONIC 6
+#endif
+
 drmModeRes *resources;
 int drm_fd;
 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 +301,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;
@@ -933,6 +950,18 @@ static int run_test(int duration, int flags, const char *test_name)
 	return 1;
 }
 
+static void get_timestamp_format(void)
+{
+	uint64_t cap_mono;
+	int ret;
+
+	ret = drmGetCap(drm_fd, DRM_CAP_TIMESTAMP_MONOTONIC, &cap_mono);
+	assert(ret == 0 || errno == EINVAL);
+	monotonic_timestamp = ret == 0 && cap_mono == 1;
+	printf("Using %s timestamps\n",
+		monotonic_timestamp ? "monotonic" : "real");
+}
+
 int main(int argc, char **argv)
 {
 	struct {
@@ -975,6 +1004,8 @@ int main(int argc, char **argv)
 
 	drm_fd = drm_open_any();
 
+	get_timestamp_format();
+
 	bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
 	devid = intel_get_drm_devid(drm_fd);
 	batch = intel_batchbuffer_alloc(bufmgr, devid);
-- 
1.7.9.5

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

[Daniel Vetter]

On Thu, Nov 22, 2012 at 04:46:36PM +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.
> 
> v2: drop the tests for the compatibilty mode (Daniel)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Applied, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] lib/scatterlist: add simple page iterator

[Imre Deak]

Add an iterator to walk through a scatter list a page at a time starting
at a specific page offset. As opposed to the mapping iterator this is
meant to be small, performing well even in simple loops like collecting
all pages on the scatterlist into an array or setting up an iommu table
based on the pages' DMA address.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 include/linux/scatterlist.h |   48 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

[ Resending with proper email addresses. ]

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 4bd6c06..d22851c 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -231,6 +231,54 @@ size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
  */
 #define SG_MAX_SINGLE_ALLOC		(PAGE_SIZE / sizeof(struct scatterlist))
 
+struct sg_page_iter {
+	struct scatterlist *sg;
+	int sg_pgoffset;
+	struct page *page;
+};
+
+static inline int
+sg_page_cnt(struct scatterlist *sg)
+{
+	BUG_ON(sg->offset || sg->length & ~PAGE_MASK);
+
+	return sg->length >> PAGE_SHIFT;
+}
+
+static inline void
+sg_page_iter_next(struct sg_page_iter *iter)
+{
+	while (iter->sg && iter->sg_pgoffset >= sg_page_cnt(iter->sg)) {
+		iter->sg_pgoffset -= sg_page_cnt(iter->sg);
+		iter->sg = sg_next(iter->sg);
+	}
+
+	if (iter->sg) {
+		iter->page = nth_page(sg_page(iter->sg), iter->sg_pgoffset);
+		iter->sg_pgoffset++;
+	}
+}
+
+static inline void
+sg_page_iter_start(struct sg_page_iter *iter, struct scatterlist *sglist,
+		   unsigned long pgoffset)
+{
+	iter->sg = sglist;
+	iter->sg_pgoffset = pgoffset;
+	iter->page = NULL;
+
+	sg_page_iter_next(iter);
+}
+
+/*
+ * Simple sg page iterator, starting off at the given page offset. Each entry
+ * on the sglist must start at offset 0 and can contain only full pages.
+ * iter->page will point to the current page, iter->sg_pgoffset to the page
+ * offset within the sg holding that page.
+ */
+#define for_each_sg_page(sglist, iter, pgoffset)				\
+	for (sg_page_iter_start((iter), (sglist), (pgoffset));		\
+	     (iter)->sg; sg_page_iter_next(iter))
 
 /*
  * Mapping sg iterator
-- 
1.7.9.5

[PATCH v2] lib/scatterlist: add simple page iterator

[Imre Deak]

Add an iterator to walk through a scatter list a page at a time starting
at a specific page offset. As opposed to the mapping iterator this is
meant to be small, performing well even in simple loops like collecting
all pages on the scatterlist into an array or setting up an iommu table
based on the pages' DMA address.

v2:
- In each iteration sg_pgoffset pointed incorrectly at the next page not
  the current one.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 include/linux/scatterlist.h |   50 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 4bd6c06..72578b5 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -231,6 +231,56 @@ size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
  */
 #define SG_MAX_SINGLE_ALLOC		(PAGE_SIZE / sizeof(struct scatterlist))
 
+struct sg_page_iter {
+	struct scatterlist *sg;
+	int sg_pgoffset;
+	struct page *page;
+};
+
+static inline int
+sg_page_cnt(struct scatterlist *sg)
+{
+	BUG_ON(sg->offset || sg->length & ~PAGE_MASK);
+
+	return sg->length >> PAGE_SHIFT;
+}
+
+static inline struct page *
+sg_page_iter_get_page(struct sg_page_iter *iter)
+{
+	while (iter->sg && iter->sg_pgoffset >= sg_page_cnt(iter->sg)) {
+		iter->sg_pgoffset -= sg_page_cnt(iter->sg);
+		iter->sg = sg_next(iter->sg);
+	}
+
+	return iter->sg ? nth_page(sg_page(iter->sg), iter->sg_pgoffset) : NULL;
+}
+
+static inline void
+sg_page_iter_next(struct sg_page_iter *iter)
+{
+	iter->sg_pgoffset++;
+	iter->page = sg_page_iter_get_page(iter);
+}
+
+static inline void
+sg_page_iter_start(struct sg_page_iter *iter, struct scatterlist *sglist,
+		   unsigned long pgoffset)
+{
+	iter->sg = sglist;
+	iter->sg_pgoffset = pgoffset;
+	iter->page = sg_page_iter_get_page(iter);
+}
+
+/*
+ * Simple sg page iterator, starting off at the given page offset. Each entry
+ * on the sglist must start at offset 0 and can contain only full pages.
+ * iter->page will point to the current page, iter->sg_pgoffset to the page
+ * offset within the sg holding that page.
+ */
+#define for_each_sg_page(sglist, iter, pgoffset)				\
+	for (sg_page_iter_start((iter), (sglist), (pgoffset));		\
+	     (iter)->page; sg_page_iter_next(iter))
 
 /*
  * Mapping sg iterator
-- 
1.7.9.5

Re: [PATCH v2] lib/scatterlist: add simple page iterator

[Andrew Morton]

On Mon, 11 Feb 2013 20:50:04 +0200
Imre Deak <imre.deak@intel.com> wrote:

> Add an iterator to walk through a scatter list a page at a time starting
> at a specific page offset. As opposed to the mapping iterator this is

What is "the mapping iterator"?

> meant to be small, performing well even in simple loops like collecting
> all pages on the scatterlist into an array or setting up an iommu table
> based on the pages' DMA address.

Where will this new macro be used?  What is driving this effort?

> v2:
> - In each iteration sg_pgoffset pointed incorrectly at the next page not
>   the current one.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  include/linux/scatterlist.h |   50 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 4bd6c06..72578b5 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -231,6 +231,56 @@ size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
>   */
>  #define SG_MAX_SINGLE_ALLOC		(PAGE_SIZE / sizeof(struct scatterlist))
>  
> +struct sg_page_iter {
> +	struct scatterlist *sg;
> +	int sg_pgoffset;
> +	struct page *page;
> +};

Some documentation wouldn't hurt.   What it's used for, why it exists.

> +static inline int
> +sg_page_cnt(struct scatterlist *sg)

unneeded newline here.

A more typical name would be "sg_page_count".  Stripping words of their
vowels makes the symbols harder to remember.

> +{
> +	BUG_ON(sg->offset || sg->length & ~PAGE_MASK);
> +
> +	return sg->length >> PAGE_SHIFT;
> +}
> +
> +static inline struct page *
> +sg_page_iter_get_page(struct sg_page_iter *iter)
> +{
> +	while (iter->sg && iter->sg_pgoffset >= sg_page_cnt(iter->sg)) {
> +		iter->sg_pgoffset -= sg_page_cnt(iter->sg);
> +		iter->sg = sg_next(iter->sg);
> +	}
> +
> +	return iter->sg ? nth_page(sg_page(iter->sg), iter->sg_pgoffset) : NULL;
> +}
> +
> +static inline void
> +sg_page_iter_next(struct sg_page_iter *iter)
> +{
> +	iter->sg_pgoffset++;
> +	iter->page = sg_page_iter_get_page(iter);
> +}
> +
> +static inline void
> +sg_page_iter_start(struct sg_page_iter *iter, struct scatterlist *sglist,
> +		   unsigned long pgoffset)
> +{
> +	iter->sg = sglist;
> +	iter->sg_pgoffset = pgoffset;
> +	iter->page = sg_page_iter_get_page(iter);
> +}

All the above are undocumented also.  I guess that's acceptable if they
are only ever to be used by for_each_sg_page().  Although if that's the
case then perhaps the identifiers should be a bit more obscure-looking.
Usually we prefix them with "__" to say "this is in internal thing".

> +/*
> + * Simple sg page iterator, starting off at the given page offset. Each entry
> + * on the sglist must start at offset 0 and can contain only full pages.
> + * iter->page will point to the current page, iter->sg_pgoffset to the page
> + * offset within the sg holding that page.
> + */
> +#define for_each_sg_page(sglist, iter, pgoffset)				\
> +	for (sg_page_iter_start((iter), (sglist), (pgoffset));		\
> +	     (iter)->page; sg_page_iter_next(iter))

Because all the helper functions are inlined, this will expand to a
quite large amount of code.  And large code can be slow code due to
I-cache eviction.

I don't know *how* big this thing will be because the patch didn't
include a caller and I can't be bothered writing my own.  (And the lack
of any caller means that the code will not be tested).

So, exactly how big is this thing, and how do we know it's better this
way than if we were to uninline some/all of the helpers?

Re: [PATCH v2] lib/scatterlist: add simple page iterator

[Imre Deak]

On Mon, 2013-02-11 at 12:54 -0800, Andrew Morton wrote:
> On Mon, 11 Feb 2013 20:50:04 +0200
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > Add an iterator to walk through a scatter list a page at a time starting
> > at a specific page offset. As opposed to the mapping iterator this is
> 
> What is "the mapping iterator"?

It's the one implemented by sg_miter_{start,stop} in scatterlist.c. It
also iterates through a scatterlist a page at a time, but it also kmaps
these pages. Since in our use case we don't need to map the pages we
needed a solution without this overhead.

> > meant to be small, performing well even in simple loops like collecting
> > all pages on the scatterlist into an array or setting up an iommu table
> > based on the pages' DMA address.
> 
> Where will this new macro be used?  What is driving this effort?

At the moment the only user of the macro would be the i915 driver, see
[1] for the patches that takes it into use. In the patchset the macro
was added as a DRM specific macro, but since it might be useful in the
future for other drivers too (anything using dma-buf) I'd like to add it
to a more generic place.

> > v2:
> > - In each iteration sg_pgoffset pointed incorrectly at the next page not
> >   the current one.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  include/linux/scatterlist.h |   50 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> > 
> > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> > index 4bd6c06..72578b5 100644
> > --- a/include/linux/scatterlist.h
> > +++ b/include/linux/scatterlist.h
> > @@ -231,6 +231,56 @@ size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
> >   */
> >  #define SG_MAX_SINGLE_ALLOC		(PAGE_SIZE / sizeof(struct scatterlist))
> >  
> > +struct sg_page_iter {
> > +	struct scatterlist *sg;
> > +	int sg_pgoffset;
> > +	struct page *page;
> > +};
> 
> Some documentation wouldn't hurt.   What it's used for, why it exists.

Ok, will add it.

> 
> > +static inline int
> > +sg_page_cnt(struct scatterlist *sg)
> 
> unneeded newline here.
> 
> A more typical name would be "sg_page_count".  Stripping words of their
> vowels makes the symbols harder to remember.

Ok, will fix this.

> > +{
> > +	BUG_ON(sg->offset || sg->length & ~PAGE_MASK);
> > +
> > +	return sg->length >> PAGE_SHIFT;
> > +}
> > +
> > +static inline struct page *
> > +sg_page_iter_get_page(struct sg_page_iter *iter)
> > +{
> > +	while (iter->sg && iter->sg_pgoffset >= sg_page_cnt(iter->sg)) {
> > +		iter->sg_pgoffset -= sg_page_cnt(iter->sg);
> > +		iter->sg = sg_next(iter->sg);
> > +	}
> > +
> > +	return iter->sg ? nth_page(sg_page(iter->sg), iter->sg_pgoffset) : NULL;
> > +}
> > +
> > +static inline void
> > +sg_page_iter_next(struct sg_page_iter *iter)
> > +{
> > +	iter->sg_pgoffset++;
> > +	iter->page = sg_page_iter_get_page(iter);
> > +}
> > +
> > +static inline void
> > +sg_page_iter_start(struct sg_page_iter *iter, struct scatterlist *sglist,
> > +		   unsigned long pgoffset)
> > +{
> > +	iter->sg = sglist;
> > +	iter->sg_pgoffset = pgoffset;
> > +	iter->page = sg_page_iter_get_page(iter);
> > +}
> 
> All the above are undocumented also.  I guess that's acceptable if they
> are only ever to be used by for_each_sg_page().  Although if that's the
> case then perhaps the identifiers should be a bit more obscure-looking.
> Usually we prefix them with "__" to say "this is in internal thing".

Yes, they are meant to be used only internally, so I'll add the __
prefix.

> > +/*
> > + * Simple sg page iterator, starting off at the given page offset. Each entry
> > + * on the sglist must start at offset 0 and can contain only full pages.
> > + * iter->page will point to the current page, iter->sg_pgoffset to the page
> > + * offset within the sg holding that page.
> > + */
> > +#define for_each_sg_page(sglist, iter, pgoffset)				\
> > +	for (sg_page_iter_start((iter), (sglist), (pgoffset));		\
> > +	     (iter)->page; sg_page_iter_next(iter))
> 
> Because all the helper functions are inlined, this will expand to a
> quite large amount of code.  And large code can be slow code due to
> I-cache eviction.
> 
> I don't know *how* big this thing will be because the patch didn't
> include a caller and I can't be bothered writing my own.  (And the lack
> of any caller means that the code will not be tested).
> 
> So, exactly how big is this thing, and how do we know it's better this
> way than if we were to uninline some/all of the helpers?

I admit I only hoped compiler optimization would keep the inlined parts
at a minimum, but now I actually checked (on Intel CPU). I applied the
patchset from [1] and uninlined sg_page_iter_start as it's not
significant for speed:

 size drivers/gpu/drm/i915/i915.ko
 514855	  15996	    272	 531123	  81ab3	drivers/gpu/drm/i915/i915.ko

Then uninlined all helpers:
 size drivers/gpu/drm/i915/i915.ko
 513447	  15996	    272	 529715	  81533	drivers/gpu/drm/i915/i915.ko

Since there are 8 invocations of the macro, the overhead for a single
invocation is about (531123 - 529715) / 8 = 191 bytes.

For speed, I benchmarked a simple loop which was basically:

page = vmalloc(sizeof(*page) * 1000, GFP_KERNEL);
for_each_sg_page(sglist, iter, 0)
	*page++ = iter.page;

where each entry on the sglist contained 16 consecutive pages. This
takes ~10% more time for the uninlined version to run. This is a rather
artificial test and I couldn't come up with something more real-life
using only the i915 driver's ioctl interface that would show a
significant change in speed.

So at least for now I'm ok with just uninlining all the helpers.

Thanks for the review,
Imre

[1]
http://lists.freedesktop.org/archives/intel-gfx/2013-February/024589.html

Re: [PATCH v2] lib/scatterlist: add simple page iterator

[Tejun Heo]

Hello,

On Tue, Feb 12, 2013 at 07:07:20PM +0200, Imre Deak wrote:
> It's the one implemented by sg_miter_{start,stop} in scatterlist.c. It
> also iterates through a scatterlist a page at a time, but it also kmaps
> these pages. Since in our use case we don't need to map the pages we
> needed a solution without this overhead.

I'm not against having non-mapping iterator but please consider that
kmaps are no-ops on many configurations.  It matters only for archs w/
high memory.

> where each entry on the sglist contained 16 consecutive pages. This
> takes ~10% more time for the uninlined version to run. This is a rather
> artificial test and I couldn't come up with something more real-life
> using only the i915 driver's ioctl interface that would show a
> significant change in speed.
> 
> So at least for now I'm ok with just uninlining all the helpers.

Can we reimplement mapping iters using the new ones?

Thanks.

-- 
tejun

Re: [PATCH v2] lib/scatterlist: add simple page iterator

[Imre Deak]

On Tue, 2013-02-12 at 09:13 -0800, Tejun Heo wrote:
> Hello,
> 
> On Tue, Feb 12, 2013 at 07:07:20PM +0200, Imre Deak wrote:
> > It's the one implemented by sg_miter_{start,stop} in scatterlist.c. It
> > also iterates through a scatterlist a page at a time, but it also kmaps
> > these pages. Since in our use case we don't need to map the pages we
> > needed a solution without this overhead.
> 
> I'm not against having non-mapping iterator but please consider that
> kmaps are no-ops on many configurations.  It matters only for archs w/
> high memory.

Ok, I haven't thought about that. But in any case we care about those
archs too and would like to avoid the mapping there as well.

> > where each entry on the sglist contained 16 consecutive pages. This
> > takes ~10% more time for the uninlined version to run. This is a rather
> > artificial test and I couldn't come up with something more real-life
> > using only the i915 driver's ioctl interface that would show a
> > significant change in speed.
> > 
> > So at least for now I'm ok with just uninlining all the helpers.
> 
> Can we reimplement mapping iters using the new ones?

Yes I think it's a good idea, I will follow up with a new patchset
addressing this and Andrew's comments.

Thanks,
Imre

Re: [PATCH v2] lib/scatterlist: add simple page iterator

[Andrew Morton]

On Tue, 12 Feb 2013 19:07:20 +0200
Imre Deak <imre.deak@intel.com> wrote:

> > So, exactly how big is this thing, and how do we know it's better this
> > way than if we were to uninline some/all of the helpers?
> 
> I admit I only hoped compiler optimization would keep the inlined parts
> at a minimum, but now I actually checked (on Intel CPU). I applied the
> patchset from [1] and uninlined sg_page_iter_start as it's not
> significant for speed:
> 
>  size drivers/gpu/drm/i915/i915.ko
>  514855	  15996	    272	 531123	  81ab3	drivers/gpu/drm/i915/i915.ko
> 
> Then uninlined all helpers:
>  size drivers/gpu/drm/i915/i915.ko
>  513447	  15996	    272	 529715	  81533	drivers/gpu/drm/i915/i915.ko
> 
> Since there are 8 invocations of the macro, the overhead for a single
> invocation is about (531123 - 529715) / 8 = 191 bytes.
> 
> For speed, I benchmarked a simple loop which was basically:
> 
> page = vmalloc(sizeof(*page) * 1000, GFP_KERNEL);
> for_each_sg_page(sglist, iter, 0)
> 	*page++ = iter.page;
> 
> where each entry on the sglist contained 16 consecutive pages. This
> takes ~10% more time for the uninlined version to run. This is a rather
> artificial test and I couldn't come up with something more real-life
> using only the i915 driver's ioctl interface that would show a
> significant change in speed.

10% for the function call overhead sounds reasonable.  Of course, that
test is biased in one direction.  A test which was biased in the other
direction would exercise all eight of the macro's callsites and would
investigate the performance impact of a 1kbyte increase in L1 cache
utilisation.

And I must say, it would need to be a pretty damn carefully crafted
test case to be able to trigger enough cache thrashing to cause a 10%
hit.

> So at least for now I'm ok with just uninlining all the helpers.

OK.

[PATCH v3 1/2] lib/scatterlist: add simple page iterator

[Imre Deak]

Add an iterator to walk through a scatter list a page at a time starting
at a specific page offset. As opposed to the mapping iterator this is
meant to be small, performing well even in simple loops like collecting
all pages on the scatterlist into an array or setting up an iommu table
based on the pages' DMA address.

v2:
- sg_pgoffset pointed incorrectly at the next page not the current one.

v3:
- formatting fixes, documentation (Andrew)
- uninline helper functions, as they are too big (Andrew)
- support for terminating the iteration after a maximum number
  sglist->nents entries, needed by the next patch

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 include/linux/scatterlist.h |   35 +++++++++++++++++++++++++++++++++++
 lib/scatterlist.c           |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 4bd6c06..788a853 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -231,6 +231,41 @@ size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
  */
 #define SG_MAX_SINGLE_ALLOC		(PAGE_SIZE / sizeof(struct scatterlist))
 
+/*
+ * sg page iterator
+ *
+ * Iterates over sg entries page-by-page.  On each successful iteration,
+ * @piter->page points to the current page, @piter->sg to the sg holding this
+ * page and @piter->sg_pgoffset to the page's page offset within the sg. The
+ * iteration will stop either when a maximum number of sg entries was reached
+ * or a terminating sg (sg_last(sg) == true) was reached.
+ */
+struct sg_page_iter {
+	struct page		*page;		/* current page */
+	struct scatterlist	*sg;		/* sg holding the page */
+	unsigned int		sg_pgoffset;	/* page offset within the sg */
+
+	/* these are internal states, keep away */
+	unsigned int		__nents;	/* remaining sg entries */
+	int			__pg_advance;	/* nr pages to advance at the
+						 * next step */
+};
+
+bool __sg_page_iter_next(struct sg_page_iter *piter);
+void __sg_page_iter_start(struct sg_page_iter *piter,
+			  struct scatterlist *sglist, unsigned int nents,
+			  unsigned long pgoffset);
+
+/**
+ * for_each_sg_page - iterate over the pages of the given sg list
+ * @sglist:	sglist to iterate over
+ * @piter:	page iterator to hold current page, sg, sg_pgoffset
+ * @nents:	maximum number of sg entries to iterate over
+ * @pgoffset:	starting page offset
+ */
+#define for_each_sg_page(sglist, piter, nents, pgoffset)		   \
+	for (__sg_page_iter_start((piter), (sglist), (nents), (pgoffset)); \
+	     __sg_page_iter_next(piter);)
 
 /*
  * Mapping sg iterator
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 7874b01..a1d1564 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -394,6 +394,44 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
 }
 EXPORT_SYMBOL(sg_alloc_table_from_pages);
 
+void __sg_page_iter_start(struct sg_page_iter *piter,
+			  struct scatterlist *sglist, unsigned int nents,
+			  unsigned long pgoffset)
+{
+	piter->__pg_advance = 0;
+	piter->__nents = nents;
+
+	piter->page = NULL;
+	piter->sg = sglist;
+	piter->sg_pgoffset = pgoffset;
+}
+EXPORT_SYMBOL(__sg_page_iter_start);
+
+static int sg_page_count(struct scatterlist *sg)
+{
+	return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
+}
+
+bool __sg_page_iter_next(struct sg_page_iter *piter)
+{
+	if (!piter->__nents || !piter->sg)
+		return false;
+
+	piter->sg_pgoffset += piter->__pg_advance;
+	piter->__pg_advance = 1;
+
+	while (piter->sg_pgoffset >= sg_page_count(piter->sg)) {
+		piter->sg_pgoffset -= sg_page_count(piter->sg);
+		piter->sg = sg_next(piter->sg);
+		if (!--piter->__nents || !piter->sg)
+			return false;
+	}
+	piter->page = nth_page(sg_page(piter->sg), piter->sg_pgoffset);
+
+	return true;
+}
+EXPORT_SYMBOL(__sg_page_iter_next);
+
 /**
  * sg_miter_start - start mapping iteration over a sg list
  * @miter: sg mapping iter to be started
-- 
1.7.10.4

[PATCH v3 2/2] lib/scatterlist: use page iterator in the mapping iterator

[Imre Deak]

For better code reuse use the newly added page iterator to iterate
through the pages. The offset, length within the page is still
calculated by the mapping iterator as well as the actual mapping.
Idea from Tejun Heo <tj@kernel.org>.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 include/linux/scatterlist.h |    6 +++---
 lib/scatterlist.c           |   46 ++++++++++++++++++++-----------------------
 2 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 788a853..a6cd692 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -295,9 +295,9 @@ struct sg_mapping_iter {
 	size_t			consumed;	/* number of consumed bytes */
 
 	/* these are internal states, keep away */
-	struct scatterlist	*__sg;		/* current entry */
-	unsigned int		__nents;	/* nr of remaining entries */
-	unsigned int		__offset;	/* offset within sg */
+	unsigned int		__offset;	/* offset within page */
+	struct sg_page_iter	__piter;	/* page iterator */
+	unsigned int		__remaining;	/* remaining bytes on page */
 	unsigned int		__flags;
 };
 
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index a1d1564..4e4974a 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -449,9 +449,7 @@ void sg_miter_start(struct sg_mapping_iter *miter, struct scatterlist *sgl,
 {
 	memset(miter, 0, sizeof(struct sg_mapping_iter));
 
-	miter->__sg = sgl;
-	miter->__nents = nents;
-	miter->__offset = 0;
+	__sg_page_iter_start(&miter->__piter, sgl, nents, 0);
 	WARN_ON(!(flags & (SG_MITER_TO_SG | SG_MITER_FROM_SG)));
 	miter->__flags = flags;
 }
@@ -476,36 +474,33 @@ EXPORT_SYMBOL(sg_miter_start);
  */
 bool sg_miter_next(struct sg_mapping_iter *miter)
 {
-	unsigned int off, len;
-
-	/* check for end and drop resources from the last iteration */
-	if (!miter->__nents)
-		return false;
-
 	sg_miter_stop(miter);
 
-	/* get to the next sg if necessary.  __offset is adjusted by stop */
-	while (miter->__offset == miter->__sg->length) {
-		if (--miter->__nents) {
-			miter->__sg = sg_next(miter->__sg);
-			miter->__offset = 0;
-		} else
+	/*
+	 * Get to the next page if necessary.
+	 * __remaining, __offset is adjusted by sg_miter_stop
+	 */
+	if (!miter->__remaining) {
+		struct scatterlist *sg;
+		unsigned long pgoffset;
+
+		if (!__sg_page_iter_next(&miter->__piter))
 			return false;
-	}
 
-	/* map the next page */
-	off = miter->__sg->offset + miter->__offset;
-	len = miter->__sg->length - miter->__offset;
+		sg = miter->__piter.sg;
+		pgoffset = miter->__piter.sg_pgoffset;
 
-	miter->page = nth_page(sg_page(miter->__sg), off >> PAGE_SHIFT);
-	off &= ~PAGE_MASK;
-	miter->length = min_t(unsigned int, len, PAGE_SIZE - off);
-	miter->consumed = miter->length;
+		miter->__offset = pgoffset ? 0 : sg->offset;
+		miter->__remaining = sg->offset + sg->length -
+				(pgoffset << PAGE_SHIFT) - miter->__offset;
+	}
+	miter->page = miter->__piter.page;
+	miter->consumed = miter->length = miter->__remaining;
 
 	if (miter->__flags & SG_MITER_ATOMIC)
-		miter->addr = kmap_atomic(miter->page) + off;
+		miter->addr = kmap_atomic(miter->page) + miter->__offset;
 	else
-		miter->addr = kmap(miter->page) + off;
+		miter->addr = kmap(miter->page) + miter->__offset;
 
 	return true;
 }
@@ -532,6 +527,7 @@ void sg_miter_stop(struct sg_mapping_iter *miter)
 	/* drop resources from the last iteration */
 	if (miter->addr) {
 		miter->__offset += miter->consumed;
+		miter->__remaining -= miter->consumed;
 
 		if (miter->__flags & SG_MITER_TO_SG)
 			flush_kernel_dcache_page(miter->page);
-- 
1.7.10.4

[PATCH v4] lib/scatterlist: use page iterator in the mapping iterator

[Imre Deak]

For better code reuse use the newly added page iterator to iterate through
the pages.  The offset, length within the page is still calculated by the
mapping iterator as well as the actual mapping.  Idea from Tejun Heo.

v1-v3:
- original version

v4:
- The dw_mmc driver used sg_mapping_iter::__sg, which was marked as internal
  and moved by this patch to the new sg_page_iter struct. This caused a
  compile time breakage for the driver. Fix this by making
  sg_mapping_iter::piter a public interface and making the driver use
  sg_mapping_iter::piter.sg instead of sg_mapping_iter::__sg.
  Thanks to James Hogan <james.hogan@imgtec.com> for pointing this out.

Signed-off-by: Imre Deak <imre.deak@intel.com>
Cc: Maxim Levitsky <maximlevitsky@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/mmc/host/dw_mmc.c   |    4 ++--
 include/linux/scatterlist.h |    6 +++---
 lib/scatterlist.c           |   46 ++++++++++++++++++++-----------------------
 3 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 323c502..6b89fde 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1445,7 +1445,7 @@ static void dw_mci_read_data_pio(struct dw_mci *host)
 		if (!sg_miter_next(sg_miter))
 			goto done;
 
-		host->sg = sg_miter->__sg;
+		host->sg = sg_miter->piter.sg;
 		buf = sg_miter->addr;
 		remain = sg_miter->length;
 		offset = 0;
@@ -1500,7 +1500,7 @@ static void dw_mci_write_data_pio(struct dw_mci *host)
 		if (!sg_miter_next(sg_miter))
 			goto done;
 
-		host->sg = sg_miter->__sg;
+		host->sg = sg_miter->piter.sg;
 		buf = sg_miter->addr;
 		remain = sg_miter->length;
 		offset = 0;
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 788a853..2d8bdae 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -293,11 +293,11 @@ struct sg_mapping_iter {
 	void			*addr;		/* pointer to the mapped area */
 	size_t			length;		/* length of the mapped area */
 	size_t			consumed;	/* number of consumed bytes */
+	struct sg_page_iter	piter;		/* page iterator */
 
 	/* these are internal states, keep away */
-	struct scatterlist	*__sg;		/* current entry */
-	unsigned int		__nents;	/* nr of remaining entries */
-	unsigned int		__offset;	/* offset within sg */
+	unsigned int		__offset;	/* offset within page */
+	unsigned int		__remaining;	/* remaining bytes on page */
 	unsigned int		__flags;
 };
 
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index a1d1564..2645acf 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -449,9 +449,7 @@ void sg_miter_start(struct sg_mapping_iter *miter, struct scatterlist *sgl,
 {
 	memset(miter, 0, sizeof(struct sg_mapping_iter));
 
-	miter->__sg = sgl;
-	miter->__nents = nents;
-	miter->__offset = 0;
+	__sg_page_iter_start(&miter->piter, sgl, nents, 0);
 	WARN_ON(!(flags & (SG_MITER_TO_SG | SG_MITER_FROM_SG)));
 	miter->__flags = flags;
 }
@@ -476,36 +474,33 @@ EXPORT_SYMBOL(sg_miter_start);
  */
 bool sg_miter_next(struct sg_mapping_iter *miter)
 {
-	unsigned int off, len;
-
-	/* check for end and drop resources from the last iteration */
-	if (!miter->__nents)
-		return false;
-
 	sg_miter_stop(miter);
 
-	/* get to the next sg if necessary.  __offset is adjusted by stop */
-	while (miter->__offset == miter->__sg->length) {
-		if (--miter->__nents) {
-			miter->__sg = sg_next(miter->__sg);
-			miter->__offset = 0;
-		} else
+	/*
+	 * Get to the next page if necessary.
+	 * __remaining, __offset is adjusted by sg_miter_stop
+	 */
+	if (!miter->__remaining) {
+		struct scatterlist *sg;
+		unsigned long pgoffset;
+
+		if (!__sg_page_iter_next(&miter->piter))
 			return false;
-	}
 
-	/* map the next page */
-	off = miter->__sg->offset + miter->__offset;
-	len = miter->__sg->length - miter->__offset;
+		sg = miter->piter.sg;
+		pgoffset = miter->piter.sg_pgoffset;
 
-	miter->page = nth_page(sg_page(miter->__sg), off >> PAGE_SHIFT);
-	off &= ~PAGE_MASK;
-	miter->length = min_t(unsigned int, len, PAGE_SIZE - off);
-	miter->consumed = miter->length;
+		miter->__offset = pgoffset ? 0 : sg->offset;
+		miter->__remaining = sg->offset + sg->length -
+				(pgoffset << PAGE_SHIFT) - miter->__offset;
+	}
+	miter->page = miter->piter.page;
+	miter->consumed = miter->length = miter->__remaining;
 
 	if (miter->__flags & SG_MITER_ATOMIC)
-		miter->addr = kmap_atomic(miter->page) + off;
+		miter->addr = kmap_atomic(miter->page) + miter->__offset;
 	else
-		miter->addr = kmap(miter->page) + off;
+		miter->addr = kmap(miter->page) + miter->__offset;
 
 	return true;
 }
@@ -532,6 +527,7 @@ void sg_miter_stop(struct sg_mapping_iter *miter)
 	/* drop resources from the last iteration */
 	if (miter->addr) {
 		miter->__offset += miter->consumed;
+		miter->__remaining -= miter->consumed;
 
 		if (miter->__flags & SG_MITER_TO_SG)
 			flush_kernel_dcache_page(miter->page);
-- 
1.7.10.4

[PATCH v5] lib/scatterlist: use page iterator in the mapping iterator

[Imre Deak]

For better code reuse use the newly added page iterator to iterate through
the pages.  The offset, length within the page is still calculated by the
mapping iterator as well as the actual mapping.  Idea from Tejun Heo.

v1-v3:
- original version

v4:
- The dw_mmc driver used sg_mapping_iter::__sg, which was marked as internal
  and moved by this patch to the new sg_page_iter struct. This caused a
  compile time breakage for the driver. Fix this by making
  sg_mapping_iter::piter a public interface and making the driver use
  sg_mapping_iter::piter.sg instead of sg_mapping_iter::__sg.
  Thanks to James Hogan for pointing this out.

v5:
- Fix the mapping size to be page-size limited. Thanks to Stephen Warren
  for tracking down the bug.

Signed-off-by: Imre Deak <imre.deak@intel.com>
Cc: Maxim Levitsky <maximlevitsky@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/mmc/host/dw_mmc.c   |    4 ++--
 include/linux/scatterlist.h |    6 +++---
 lib/scatterlist.c           |   48 +++++++++++++++++++++----------------------
 3 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 323c502..6b89fde 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1445,7 +1445,7 @@ static void dw_mci_read_data_pio(struct dw_mci *host)
 		if (!sg_miter_next(sg_miter))
 			goto done;
 
-		host->sg = sg_miter->__sg;
+		host->sg = sg_miter->piter.sg;
 		buf = sg_miter->addr;
 		remain = sg_miter->length;
 		offset = 0;
@@ -1500,7 +1500,7 @@ static void dw_mci_write_data_pio(struct dw_mci *host)
 		if (!sg_miter_next(sg_miter))
 			goto done;
 
-		host->sg = sg_miter->__sg;
+		host->sg = sg_miter->piter.sg;
 		buf = sg_miter->addr;
 		remain = sg_miter->length;
 		offset = 0;
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 788a853..2d8bdae 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -293,11 +293,11 @@ struct sg_mapping_iter {
 	void			*addr;		/* pointer to the mapped area */
 	size_t			length;		/* length of the mapped area */
 	size_t			consumed;	/* number of consumed bytes */
+	struct sg_page_iter	piter;		/* page iterator */
 
 	/* these are internal states, keep away */
-	struct scatterlist	*__sg;		/* current entry */
-	unsigned int		__nents;	/* nr of remaining entries */
-	unsigned int		__offset;	/* offset within sg */
+	unsigned int		__offset;	/* offset within page */
+	unsigned int		__remaining;	/* remaining bytes on page */
 	unsigned int		__flags;
 };
 
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index a1d1564..b83c144 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -449,9 +449,7 @@ void sg_miter_start(struct sg_mapping_iter *miter, struct scatterlist *sgl,
 {
 	memset(miter, 0, sizeof(struct sg_mapping_iter));
 
-	miter->__sg = sgl;
-	miter->__nents = nents;
-	miter->__offset = 0;
+	__sg_page_iter_start(&miter->piter, sgl, nents, 0);
 	WARN_ON(!(flags & (SG_MITER_TO_SG | SG_MITER_FROM_SG)));
 	miter->__flags = flags;
 }
@@ -476,36 +474,35 @@ EXPORT_SYMBOL(sg_miter_start);
  */
 bool sg_miter_next(struct sg_mapping_iter *miter)
 {
-	unsigned int off, len;
-
-	/* check for end and drop resources from the last iteration */
-	if (!miter->__nents)
-		return false;
-
 	sg_miter_stop(miter);
 
-	/* get to the next sg if necessary.  __offset is adjusted by stop */
-	while (miter->__offset == miter->__sg->length) {
-		if (--miter->__nents) {
-			miter->__sg = sg_next(miter->__sg);
-			miter->__offset = 0;
-		} else
+	/*
+	 * Get to the next page if necessary.
+	 * __remaining, __offset is adjusted by sg_miter_stop
+	 */
+	if (!miter->__remaining) {
+		struct scatterlist *sg;
+		unsigned long pgoffset;
+
+		if (!__sg_page_iter_next(&miter->piter))
 			return false;
-	}
 
-	/* map the next page */
-	off = miter->__sg->offset + miter->__offset;
-	len = miter->__sg->length - miter->__offset;
+		sg = miter->piter.sg;
+		pgoffset = miter->piter.sg_pgoffset;
 
-	miter->page = nth_page(sg_page(miter->__sg), off >> PAGE_SHIFT);
-	off &= ~PAGE_MASK;
-	miter->length = min_t(unsigned int, len, PAGE_SIZE - off);
-	miter->consumed = miter->length;
+		miter->__offset = pgoffset ? 0 : sg->offset;
+		miter->__remaining = sg->offset + sg->length -
+				(pgoffset << PAGE_SHIFT) - miter->__offset;
+		miter->__remaining = min_t(unsigned long, miter->__remaining,
+					   PAGE_SIZE - miter->__offset);
+	}
+	miter->page = miter->piter.page;
+	miter->consumed = miter->length = miter->__remaining;
 
 	if (miter->__flags & SG_MITER_ATOMIC)
-		miter->addr = kmap_atomic(miter->page) + off;
+		miter->addr = kmap_atomic(miter->page) + miter->__offset;
 	else
-		miter->addr = kmap(miter->page) + off;
+		miter->addr = kmap(miter->page) + miter->__offset;
 
 	return true;
 }
@@ -532,6 +529,7 @@ void sg_miter_stop(struct sg_mapping_iter *miter)
 	/* drop resources from the last iteration */
 	if (miter->addr) {
 		miter->__offset += miter->consumed;
+		miter->__remaining -= miter->consumed;
 
 		if (miter->__flags & SG_MITER_TO_SG)
 			flush_kernel_dcache_page(miter->page);
-- 
1.7.10.4

Re: [PATCH v5] lib/scatterlist: use page iterator in the mapping iterator

[Stephen Warren]

On 02/24/2013 04:05 AM, Imre Deak wrote:
> For better code reuse use the newly added page iterator to iterate through
> the pages.  The offset, length within the page is still calculated by the
> mapping iterator as well as the actual mapping.  Idea from Tejun Heo.

Just for completeness, this updated combined patch,
Tested-by: Stephen Warren <swarren@wwwdotorg.org>

Re: [PATCH v3 2/2] lib/scatterlist: use page iterator in the mapping iterator

[Stephen Warren]

On 02/13/2013 08:10 AM, Imre Deak wrote:
> For better code reuse use the newly added page iterator to iterate
> through the pages. The offset, length within the page is still
> calculated by the mapping iterator as well as the actual mapping.
> Idea from Tejun Heo <tj@kernel.org>.

This patch appears in linux-next since next-20130220. It breaks mounting
a root filesystem on an SD card on the Raspberry Pi ARM platform, with
errors such as those shown below.

next-20130222 with just this patch reverted works fine.

> [    0.708426] VFS: Mounted root (ext4 filesystem) on device 179:2.
> [    0.723742] devtmpfs: mounted
> [    0.733064] Freeing init memory: 204K
> [    0.777992] EXT4-fs error (device mmcblk0p2): ext4_iget:3814: inode #4259: comm swapper: bad extra_isize (57200 != 256)
> [    0.815172] EXT4-fs error (device mmcblk0p2): ext4_lookup:1428: inode #8198: comm swapper: deleted inode referenced: 487
> [    0.826179] Kernel panic - not syncing: No init found.  Try passing init= option to kernel. See Linux Documentation/init.txt for guidance.

> [    0.719365] VFS: Mounted root (ext4 filesystem) on device 179:2.
> [    0.740918] devtmpfs: mounted
> [    0.745219] Freeing init memory: 204K
> ERROR: ld.so: object '/usr/lib/arm-linux-gnueabihf/libcofi_rpi.so' from /etc/ld.so.preload cannot be preloaded: ignored.
> [    0.906840] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

> [    0.724018] VFS: Mounted root (ext4 filesystem) on device 179:2.
> [    0.739404] devtmpfs: mounted
> [    0.748741] Freeing init memory: 204K
> [    0.793603] EXT4-fs error (device mmcblk0p2): ext4_iget:3814: inode #4259: comm swapper: bad extra_isize (57200 != 256)
> [    0.822138] Kernel panic - not syncing: No init found.  Try passing init= option to kernel. See Linux Documentation/init.txt for guidance.

Re: [PATCH v3 2/2] lib/scatterlist: use page iterator in the mapping iterator

[Imre Deak]

On Fri, 2013-02-22 at 21:29 -0700, Stephen Warren wrote:
> On 02/13/2013 08:10 AM, Imre Deak wrote:
> > For better code reuse use the newly added page iterator to iterate
> > through the pages. The offset, length within the page is still
> > calculated by the mapping iterator as well as the actual mapping.
> > Idea from Tejun Heo <tj@kernel.org>.
> 
> This patch appears in linux-next since next-20130220. It breaks mounting
> a root filesystem on an SD card on the Raspberry Pi ARM platform, with
> errors such as those shown below.
> 
> next-20130222 with just this patch reverted works fine.

Thanks for tracking this down. I noticed now an obvious mistake I've
made, not limiting the mapping size to page size :/ I didn't hit it
since it only causes a problem when the user of miter modifies
miter->consumed and I think nothing does this on my machine. Since the
sdhci driver on Raspberry Pi does this the following might fix the
problem you saw. Could you give it a try? It applies on top of
v4 of the patch [1]:

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 2645acf..b83c144 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -493,6 +493,8 @@ bool sg_miter_next(struct sg_mapping_iter *miter)
 		miter->__offset = pgoffset ? 0 : sg->offset;
 		miter->__remaining = sg->offset + sg->length -
 				(pgoffset << PAGE_SHIFT) - miter->__offset;
+		miter->__remaining = min_t(unsigned long, miter->__remaining,
+					   PAGE_SIZE - miter->__offset);
 	}
 	miter->page = miter->piter.page;
 	miter->consumed = miter->length = miter->__remaining;


--Imre

[1]
http://lists.linaro.org/pipermail/linaro-mm-sig/2013-February/003069.html

Re: [PATCH v3 2/2] lib/scatterlist: use page iterator in the mapping iterator

[Stephen Warren]

On Sat, 23 Feb 2013 22:04:06 +0200, Imre Deak wrote:
> On Fri, 2013-02-22 at 21:29 -0700, Stephen Warren wrote:
>> On 02/13/2013 08:10 AM, Imre Deak wrote:
>>> For better code reuse use the newly added page iterator to iterate
>>> through the pages. The offset, length within the page is still
>>> calculated by the mapping iterator as well as the actual mapping.
>>> Idea from Tejun Heo <tj@kernel.org>.
>>
>> This patch appears in linux-next since next-20130220. It breaks mounting
>> a root filesystem on an SD card on the Raspberry Pi ARM platform, with
>> errors such as those shown below.
>>
>> next-20130222 with just this patch reverted works fine.
> 
> ...
> the following might fix the
> problem you saw. Could you give it a try? It applies on top of
> v4 of the patch [1]:

Yes, thanks very much. That incremental patch you gave solves the
problem perfectly.