Re: [PATCH v2 2/2] kms_fbc_crc: Add a CRC based FBC test

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 25, 2013 at 05:08:41PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> kms_fbc_crc will perform various write operations to the scanout buffer
> whilc FBC is enabled. CRC checks will be used to make sure the
> modifcations to scanout buffer are detected.
> 
> The operations include:
>  - page flip
>  - pwrite
>  - GTT mmap
>  - CPU mmap
>  - blit
>  - rendercopy
>  - context switch + rendercopy
>  - combination of a page flip and each operation listed above
> 
> v2: Use gem_sw_finish instead of drmModeDirtyFB after CPU access
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

[snip]

> +	switch (mode) {
> +		void *ptr;
> +	case TEST_PAGE_FLIP:
> +		igt_assert(drmModePageFlip(data->drm_fd, data->crtc_id,
> +					   data->fb_id[1], 0, NULL) == 0);
> +		break;
> +	case TEST_PWRITE:
> +	case TEST_PAGE_FLIP_AND_PWRITE:
> +		gem_write(data->drm_fd, handle, 0, buf, 1);
> +		gem_sw_finish(data->drm_fd, handle);

This one's too much. Like I've said current rules for pwrite is that
a) it's only used for cursors
b) no explicit flushing afterwards.
So the sw_finish here could potentially paper over kernel bugs (but only
for psr since fbc doesn't care about cursors).

> +		break;
> +	case TEST_MMAP_CPU:
> +	case TEST_PAGE_FLIP_AND_MMAP_CPU:
> +		ptr = gem_mmap__cpu(data->drm_fd, handle, 4096, PROT_WRITE);
> +		gem_set_domain(data->drm_fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> +		memset(ptr, 0xff, 4);
> +		gem_set_domain(data->drm_fd, handle, I915_GEM_DOMAIN_GTT, 0);

Nope, that's a bug. There's not set_domain(GTT, 0) after a cpu mmap
operation, just the sw_finish ioctl call. set_domain is only done _before_
mmap access (yeah, a bit a design but in the original gem interface to no
have a begin/end pair).

> +		munmap(ptr, 4096);
> +		gem_sw_finish(data->drm_fd, handle);
> +		break;
> +	case TEST_MMAP_GTT:
> +	case TEST_PAGE_FLIP_AND_MMAP_GTT:
> +		ptr = gem_mmap__gtt(data->drm_fd, handle, 4096, PROT_WRITE);
> +		gem_set_domain(data->drm_fd, handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +		memset(ptr, 0xff, 4);
> +		gem_set_domain(data->drm_fd, handle, I915_GEM_DOMAIN_GTT, 0);

Again no set_domain after the writes have landed.

> +		munmap(ptr, 4096);
> +		break;
> +	case TEST_BLT:
> +	case TEST_PAGE_FLIP_AND_BLT:
> +		fill_blt(data, handle, 0xff);
> +		break;
> +	case TEST_RENDER:
> +	case TEST_CONTEXT:
> +	case TEST_PAGE_FLIP_AND_RENDER:
> +	case TEST_PAGE_FLIP_AND_CONTEXT:
> +		fill_render(data, handle,
> +			    (mode == TEST_CONTEXT || mode == TEST_PAGE_FLIP_AND_CONTEXT) ?
> +			    data->ctx[1] : NULL, 0xff);

busy ioctl call on the render target/frontbuffer still seems to be
missing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch