On Fri, Jul 31, 2015 at 05:42:23PM -0300, Tiago Vignatti wrote:

For now we're opting out devices that don't have the LLC CPU cache (mostly "Atom" devices). Alternatively, we could build up a path to mmap them through GTT WC (and ignore the fact that will be dead-slow for reading). Or, an even more complex work I believe, would involve on setting up dma-buf ioctls to allow userspace flush, controlling manually the synchronization via begin{,end}_cpu_access.

Signed-off-by: Tiago Vignatti tiago.vignatti@intel.com

drivers/gpu/drm/i915/i915_gem_dmabuf.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index e9c2bfd..e6cb402 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -193,7 +193,26 @@ static void i915_gem_dmabuf_kunmap(struct dma_buf *dma_buf, unsigned long page_n

static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma) {

• return -EINVAL;

• struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
• struct drm_device *dev = obj->base.dev;
• int ret;
• if (obj->base.size < vma->vm_end - vma->vm_start)

• return -EINVAL;
  

• /* On non-LLC machines we'd need to be careful cause CPU and GPU don't

• * share the CPU's L3 cache and coherency may hurt when CPU mapping. */
  

• if (!HAS_LLC(dev))

• return -EINVAL;
  

The first problem is that llc does not guarrantee that the buffer is cache coherent with all aspects of the GPU. For scanout and similar writes need to be WC.

if (obj->has_framebuffer_references) would at least catch where the fb is made before the mmap.

Equally this buffer could then be shared with other devices and exposing a CPU mmap to userspace (and no flush/set-domain protocol) will result in corruption.

• if (!obj->base.filp)

• return -EINVAL;
  

• ret = obj->base.filp->f_op->mmap(obj->base.filp, vma);
• fput(vma->vm_file);
• vma->vm_file = get_file(obj->base.filp);

Transfer owenership even if the ->mmap() fails? -Chris

On Tue, Aug 04, 2015 at 06:30:25PM -0300, Tiago Vignatti wrote:

On 07/31/2015 06:02 PM, Chris Wilson wrote:

...

The first problem is that llc does not guarrantee that the buffer is cache coherent with all aspects of the GPU. For scanout and similar writes need to be WC.

if (obj->has_framebuffer_references) would at least catch where the fb is made before the mmap.

Equally this buffer could then be shared with other devices and exposing a CPU mmap to userspace (and no flush/set-domain protocol) will result in corruption.

I've built an igt test to catch this corruption but it's not really falling there in my IvyBridge. If what you described is right (and so what I coded) then this test should write in the mapped buffer but not update the screen.

Any idea what's going on?

https://github.com/tiagovignatti/intel-gpu-tools/commit/3e130ac2b274f1a3f688...

From 3e130ac2b274f1a3f68855559c78cb72d0673ca2 Mon Sep 17 00:00:00 2001 From: Tiago Vignatti tiago.vignatti@intel.com Date: Tue, 4 Aug 2015 13:38:09 -0300 Subject: [PATCH] tests: Add prime_crc for cache coherency

This program can be used to detect when the writes don't land in scanout, due cache incoherency.

Run it like ./prime_crc --interactive-debug=crc

Signed-off-by: Tiago Vignatti tiago.vignatti@intel.com

tests/.gitignore | 1 + tests/Makefile.sources | 1 + tests/prime_crc.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 203 insertions(+) create mode 100644 tests/prime_crc.c

diff --git a/tests/.gitignore b/tests/.gitignore index 5bc4a58..96dbf57 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -160,6 +160,7 @@ pm_rc6_residency pm_rpm pm_rps pm_sseu +prime_crc prime_nv_api prime_nv_pcopy prime_nv_test diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 5b2072e..c05b5a7 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -90,6 +90,7 @@ TESTS_progs_M = \ pm_rps \ pm_rc6_residency \ pm_sseu \

• prime_crc \ prime_mmap \ prime_self_import \ template \

diff --git a/tests/prime_crc.c b/tests/prime_crc.c new file mode 100644 index 0000000..3474cc9 --- /dev/null +++ b/tests/prime_crc.c @@ -0,0 +1,201 @@ +/*

• • Copyright © 2015 Intel Corporation
• • Permission is hereby granted, free of charge, to any person obtaining a
• • copy of this software and associated documentation files (the

"Software"),

• • to deal in the Software without restriction, including without

limitation

• • the rights to use, copy, modify, merge, publish, distribute, sublicense,
• • and/or sell copies of the Software, and to permit persons to whom the
• • Software is furnished to do so, subject to the following conditions:
• • The above copyright notice and this permission notice (including the

next

• • paragraph) shall be included in all copies or substantial portions of

the

• • Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS

OR

• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
• • FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
• • THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR

OTHER

• • LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
• • FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER

DEALINGS

• • IN THE SOFTWARE.
• • Authors:
• • Tiago Vignatti <tiago.vignatti at intel.com>
• */

+/* This program can detect when the writes don't land in scanout, due cache

• • incoherency. */

+#include "drmtest.h" +#include "igt_debugfs.h" +#include "igt_kms.h"

+#define MAX_CONNECTORS 32

+struct modeset_params {

• uint32_t crtc_id;
• uint32_t connector_id;
• drmModeModeInfoPtr mode;

+};

+int drm_fd; +drmModeResPtr drm_res; +drmModeConnectorPtr drm_connectors[MAX_CONNECTORS]; +drm_intel_bufmgr *bufmgr; +igt_pipe_crc_t *pipe_crc;

+struct modeset_params ms;

+static void find_modeset_params(void) +{

• int i;
• uint32_t connector_id = 0, crtc_id;
• drmModeModeInfoPtr mode = NULL;
• for (i = 0; i < drm_res->count_connectors; i++) {

• drmModeConnectorPtr c = drm_connectors[i];
  

• if (c->count_modes) {
  

• 	connector_id = c->connector_id;
  

• 	mode = &c->modes[0];
  

• 	break;
  

• }
  

• }
• igt_require(connector_id);
• crtc_id = drm_res->crtcs[0];
• igt_assert(crtc_id);
• igt_assert(mode);
• ms.connector_id = connector_id;
• ms.crtc_id = crtc_id;
• ms.mode = mode;

+}

+#define BO_SIZE (16*1024)

+char pattern[] = {0xff, 0x00, 0x00, 0x00,

• 0x00, 0xff, 0x00, 0x00,
• 0x00, 0x00, 0xff, 0x00,
• 0x00, 0x00, 0x00, 0xff};

+static void mess_with_coherency(char *ptr) +{

• off_t i;
• for (i = 0; i < BO_SIZE; i+=sizeof(pattern)) {

• memcpy(ptr + i, pattern, sizeof(pattern));
  

• }

+// munmap(ptr, BO_SIZE); +// close(dma_buf_fd); +}

+static char *dmabuf_mmap_framebuffer(struct igt_fb *fb) +{

• int dma_buf_fd;
• char *ptr = NULL;
• dma_buf_fd = prime_handle_to_fd(drm_fd, fb->gem_handle);
• igt_assert(errno == 0);
• ptr = mmap(NULL, BO_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, dma_buf_fd,

0);

• igt_assert(ptr != MAP_FAILED);
• return ptr;

+}

+static void get_method_crc(uint64_t tiling, igt_crc_t *crc, bool mess) +{

• struct igt_fb fb;
• int rc;
• char *ptr;
• igt_create_fb(drm_fd, ms.mode->hdisplay, ms.mode->vdisplay,

•       DRM_FORMAT_XRGB8888, tiling, &fb);
  

• if (mess)

• ptr = dmabuf_mmap_framebuffer(&fb);
  

• rc = drmModeSetCrtc(drm_fd, ms.crtc_id, fb.fb_id, 0, 0,

• 	    &ms.connector_id, 1, ms.mode);
  

• igt_assert(rc == 0);
• if (mess)

• mess_with_coherency(ptr);
  

• igt_pipe_crc_collect_crc(pipe_crc, crc);
• kmstest_unset_all_crtcs(drm_fd, drm_res);
• igt_remove_fb(drm_fd, &fb);

+}

+static void draw_method_subtest(uint64_t tiling) +{

• igt_crc_t reference_crc, crc;
• kmstest_unset_all_crtcs(drm_fd, drm_res);
• find_modeset_params();
• get_method_crc(tiling, &reference_crc, false);
• get_method_crc(tiling, &crc, true);
• // XXX: IIUC if we mess up with the scanout device, through a dma-buf

mmap'ed

• // pointer, then both the reference crc and the messed up one should be

equal

• // because the latter wasn't flushed. That's the theory, but it's not

what's

• // happening and the following is not passing.

Nah they don't have to be equal since the problem isn't that nothing goes out to memory where the display can see it, but usually only parts of it. I.e. you need to change your test to - draw black screen (it starts that way so nothing to do really), grab crtc - draw white screen and make sure you flush correctly, don't bother with crc (we can't test for inequality because collisions are too easy) - draw black screen again without flushing, grab crc

Then assert that your two crc will be inequal (which they shouldn't be because some cachelines will still be stuck). Maybe also add a delay somewhere so you can see the cacheline dirt pattern, it's very characteristic. -Daniel

• igt_assert_crc_equal(&reference_crc, &crc);

+}

+static void setup_environment(void) +{

• int i;
• drm_fd = drm_open_any_master();
• igt_require(drm_fd >= 0);
• drm_res = drmModeGetResources(drm_fd);
• igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
• for (i = 0; i < drm_res->count_connectors; i++)

• drm_connectors[i] = drmModeGetConnector(drm_fd,
  

• 					drm_res->connectors[i]);
  

• kmstest_set_vt_graphics_mode();
• bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
• igt_assert(bufmgr);
• drm_intel_bufmgr_gem_enable_reuse(bufmgr);
• pipe_crc = igt_pipe_crc_new(0, INTEL_PIPE_CRC_SOURCE_AUTO);

+}

+static void teardown_environment(void) +{

• int i;
• igt_pipe_crc_free(pipe_crc);
• drm_intel_bufmgr_destroy(bufmgr);
• for (i = 0; i < drm_res->count_connectors; i++)

• drmModeFreeConnector(drm_connectors[i]);
  

• drmModeFreeResources(drm_res);
• close(drm_fd);

+}

+igt_main +{

• igt_fixture

• setup_environment();
  

• igt_subtest_f("draw-method-tiled")

• draw_method_subtest(LOCAL_I915_FORMAT_MOD_X_TILED);
  

• igt_fixture

• teardown_environment();
  

+}

On Wed, Aug 05, 2015 at 05:10:17PM -0300, Tiago Vignatti wrote:

On 08/05/2015 04:08 AM, Daniel Vetter wrote:

...

On Tue, Aug 04, 2015 at 06:30:25PM -0300, Tiago Vignatti wrote: Nah they don't have to be equal since the problem isn't that nothing goes out to memory where the display can see it, but usually only parts of it. I.e. you need to change your test to

• draw black screen (it starts that way so nothing to do really), grab crtc
• draw white screen and make sure you flush correctly, don't bother with crc (we can't test for inequality because collisions are too easy)
• draw black screen again without flushing, grab crc

Then assert that your two crc will be inequal (which they shouldn't be because some cachelines will still be stuck). Maybe also add a delay somewhere so you can see the cacheline dirt pattern, it's very characteristic.

Cool, I've got it now. The test below makes the cachelines dirt, requiring them to get flushed correctly -- I'll work on it now. Should we add that kind of test somewhere in igt BTW?

Yeah if you expect me to merge dma-buf mmap with the begin/end stuff I'll ask for an igt for it ;-)

PS: I had an issue with the original kms_pwrite_crc which returns frequent fails. Paulo helped though and showed me that pwrite is currently broken: https://bugs.freedesktop.org/show_bug.cgi?id=86422

If you do dma-buf mmap with begin/end it should work, since in there we'll just to manual range-based clflushing. -Daniel

Tiago

diff --git a/tests/kms_pwrite_crc.c b/tests/kms_pwrite_crc.c index 05b9e38..419b46d 100644 --- a/tests/kms_pwrite_crc.c +++ b/tests/kms_pwrite_crc.c @@ -50,6 +50,20 @@ typedef struct { uint32_t devid; } data_t;

+static char *dmabuf_mmap_framebuffer(int drm_fd, struct igt_fb *fb) +{

• int dma_buf_fd;
• char *ptr = NULL;
• dma_buf_fd = prime_handle_to_fd(drm_fd, fb->gem_handle);
• igt_assert(errno == 0);
• ptr = mmap(NULL, fb->size, PROT_READ | PROT_WRITE, MAP_SHARED, dma_buf_fd,

0);

• igt_assert(ptr != MAP_FAILED);
• return ptr;

+}

static void test(data_t *data) { igt_display_t *display = &data->display; @@ -57,6 +71,7 @@ static void test(data_t *data) struct igt_fb *fb = &data->fb[1]; drmModeModeInfo *mode; cairo_t *cr;

• char *ptr; uint32_t caching; void *buf; igt_crc_t crc;

@@ -67,6 +82,8 @@ static void test(data_t *data) igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay, DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, fb);

• ptr = dmabuf_mmap_framebuffer(data->drm_fd, fb);
• cr = igt_get_cairo_ctx(data->drm_fd, fb); igt_paint_test_pattern(cr, fb->width, fb->height); cairo_destroy(cr);

@@ -83,11 +100,11 @@ static void test(data_t *data) caching = gem_get_caching(data->drm_fd, fb->gem_handle); igt_assert(caching == I915_CACHING_NONE || caching == I915_CACHING_DISPLAY);

• /* use pwrite to make the other fb all white too */

• /* use dmabuf pointer to make the other fb all white too */ buf = malloc(fb->size); igt_assert(buf != NULL); memset(buf, 0xff, fb->size);

• gem_write(data->drm_fd, fb->gem_handle, 0, buf, fb->size);

• memcpy(ptr, buf, fb->size); free(buf);

  /* and flip to it */