Re: [PATCH v2 i-g-t] kms_atomic: Measure speed of some plane ioctls

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 04/27/2015 10:20 PM, Chris Wilson wrote:
> On Mon, Apr 27, 2015 at 04:34:05PM +0100, Tvrtko Ursulin wrote:
>> +static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
>> +			 igt_plane_t *plane)
>> +{
>> +	drmModeModeInfo *mode;
>> +	igt_display_t *display = &data->display;
>> +	int fb_id, fb_modeset_id;
>> +	unsigned int w, h;
>> +	uint64_t tiling = LOCAL_DRM_FORMAT_MOD_NONE;
>> +	uint32_t pixel_format = DRM_FORMAT_XRGB8888;
>> +	enum igt_commit_style commit = COMMIT_LEGACY;
>> +	igt_plane_t *primary;
>> +
>> +	igt_output_set_pipe(output, pipe);
>> +
>> +	mode = igt_output_get_mode(output);
>> +
>> +	w = mode->hdisplay;
>> +	h = mode->vdisplay;
>> +
>> +	fb_modeset_id = igt_create_fb(data->gfx_fd,
>> +				      w, h,
>> +				      pixel_format,
>> +				      tiling,
>> +				      &data->fb_modeset);
>> +	igt_assert(fb_modeset_id);
>> +
>> +	/*
>> +	 * With igt_display_commit2 and COMMIT_UNIVERSAL, we call just the
>> +	 * setplane without a modeset. So, to be able to call
>> +	 * igt_display_commit and ultimately setcrtc to do the first modeset,
>> +	 * we create an fb covering the crtc and call commit
>> +	 */
>> +
>> +	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
>> +	igt_plane_set_fb(primary, &data->fb_modeset);
>> +	igt_display_commit(display);
>> +
>> +	fb_id = igt_create_fb(data->gfx_fd,
>> +			      w, h,
>> +			      pixel_format,
>> +			      tiling,
>> +			      &data->fb);
>> +	igt_assert(fb_id);
>> +
>> +	igt_plane_set_fb(plane, &data->fb);
>> +
>
> Please do commit = COMMIT_LEGACY here so I don't have to scan backwads.

Yeah makes sense, too much copy&paste.

>> +	if (plane->is_primary || plane->is_cursor) {
>> +		igt_require(data->display.has_universal_planes);
>> +		commit = COMMIT_UNIVERSAL;
>> +	}
>> +
>> +	igt_display_commit2(display, commit);
>> +}
>> +
>> +static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane)
>> +{
>> +	igt_display_t *display = &data->display;
>> +
>> +	igt_remove_fb(data->gfx_fd, &data->fb);
>> +	igt_remove_fb(data->gfx_fd, &data->fb_modeset);
>> +
>> +	/* XXX: see the note in prepare_crtc() */
>> +	if (!plane->is_primary) {
>> +		igt_plane_t *primary;
>> +
>> +		primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
>> +		igt_plane_set_fb(primary, NULL);
>> +	}
>> +
>> +	igt_plane_set_fb(plane, NULL);
>> +	igt_output_set_pipe(output, PIPE_ANY);
>> +
>> +	igt_display_commit(display);
>> +}
>> +
>> +static double elapsed(const struct timeval *start,
>> +		      const struct timeval *end,
>> +		      int loop)
>> +{
>> +	return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_usec - start->tv_usec))/loop;
>> +}
>> +
>> +static void test_commit_speed(data_t *data, enum igt_plane plane_type)
>> +{
>> +	igt_display_t *display = &data->display;
>> +	igt_output_t *output;
>> +	enum pipe pipe;
>> +	enum igt_commit_style commit = COMMIT_LEGACY;
>> +	unsigned int i;
>> +	const unsigned int loops = 10000;
>> +	struct timeval start, end;
>> +	double e;
>> +
>
> Same comment for commit = COMMIT_LEGACY here, mainly now for consistency
> with before.

Here we want to be able to choose between commit types if we want to 
keep two subtests.

>> +	if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
>> +		igt_require(data->display.has_universal_planes);
>> +		commit = COMMIT_UNIVERSAL;
>> +	}
>> +
>> +	for_each_connected_output(display, output) {
>> +		for_each_pipe(display, pipe) {
>> +			igt_plane_t *plane;
>> +
>> +			igt_output_set_pipe(output, pipe);
>> +			plane = igt_output_get_plane(output, plane_type);
>> +
>> +			prepare_crtc(data, output, pipe, plane);
>> +
>> +			igt_display_commit2(display, commit);
>> +
>> +			gettimeofday(&start, NULL);
>> +			for (i = loops; i > 0; i--) {
>> +				plane->position_changed = true;
>> +				igt_plane_commit(plane, output, commit, true);
>> +			}
>> +			gettimeofday(&end, NULL);
>> +			e = elapsed(&start, &end, loops);
>> +			igt_info("Pipe: %u Plane: %u Time: %7.3fµs\n",
>> +				 pipe, plane->index, e);
>> +			igt_require(e < 1000); /* 1ms, just so. */
>> +
>> +			kmstest_restore_vt_mode();
>> +			kmstest_set_vt_graphics_mode();
>
> Do you really mean to switch VT between text/gfx on every display/pipe?
> Why?

Because copy&paste is so easy. :)

>> +
>> +			cleanup_crtc(data, output, plane);
>> +		}
>> +	}
>> +}
>
> Looks like magic to me. Is this enough variation to flush out future
> bugs?

On which part does this apply?

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx