[Intel-gfx] [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[chris at chris-wilson.co.uk]

On Thu, Mar 24, 2016 at 09:03:59PM +0000, chris at chris-wilson.co.uk wrote:
> On Thu, Mar 24, 2016 at 08:53:21PM +0000, Zanoni, Paulo R wrote:
> > Em Qui, 2016-03-24 às 19:31 +0000, Chris Wilson escreveu:
> > > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> > > > 
> > > > FBC and the frontbuffer tracking infrastructure were designed
> > > > assuming
> > > > that user space applications would follow a specific set of rules
> > > > regarding frontbuffer management and mmapping. I recently
> > > > discovered
> > > > that current user space is not exactly following these rules: my
> > > > investigation led me to the conclusion that the generic backend
> > > > from
> > > > SNA - used by SKL and the other new platforms without a specific
> > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using the
> > > > CPU
> > > > and WC mmaps. I discovered this when running lightdm: I would type
> > > > the
> > > > password and nothing would appear on the screen unless I moved the
> > > > mouse over the place where the letters were supposed to appear.
> > > Yes, that is a kernel bug. The protocol we said the kernel would
> > > follow
> > > is to disable FBC/WC when userspace marks the object for writing by
> > > the
> > > CPU and would only reestablish FBC/WC upon dirtyfb.
> > 
> > But on WC mmaps we mark the object for writing by the GTT instead of
> > the CPU, and while the tracking engine is able to see "normal" GTT mmap
> > writes, it's not able to see WC mmap writes, so we established that
> > we'd call dirtyfb after frontbuffer drawing through WC mmaps, which is
> > something that the DDX never implemented. This was discussed on #intel-
> > gfx on Nov 5 2014, and also possibly other places, but I can't find the
> > logs. Daniel also confirmed this to me again on private IRC on Jun 16
> > 2015. So I still don't understand why this is a Kernel bug instead of a
> > DDX bug.
> 
> Because we said that once invalidated, it would not be restored until
> dirtyfb. The kernel is not doing that. Your patch does not do that. To
> be even close, you should be setting the origin flag based on the existence
> of wc mmaping for the object inside set-to-gtt-domain. Otherwise, you
> are not implementing even close to the protocol you say you are. That is
> invalidate on set-domain, flush on dirtyfb.
> 
> The kernel's bug is that is not cancelling FBC. Userspace's bug is not
> signalling when to reenable it.

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8dec2e8..0314346 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2145,6 +2145,7 @@ struct drm_i915_gem_object {
        unsigned int cache_dirty:1;
 
        unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
+       unsigned int has_wc_mmap:1;
 
        /** Count of VMA actually bound by this object */
        unsigned int bind_count;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 05ae706..29ca96d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1310,6 +1310,13 @@ unlock:
        return ret;
 }
 
+static enum fb_op_origin
+write_origin(struct drm_i915_gem_object *obj, unsigned domain)
+{
+       return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
+             ORIGIN_GTT : ORIGIN_CPU;
+}
+
 /**
  * Called when user space prepares to use an object with the CPU, either
  * through the mmap ioctl's mapping or a GTT mapping.
@@ -1363,9 +1370,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
                ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
 
        if (write_domain != 0)
-               intel_fb_obj_invalidate(obj,
-                                       write_domain == I915_GEM_DOMAIN_GTT ?
-                                       ORIGIN_GTT : ORIGIN_CPU);
+               intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
 
 unref:
        drm_gem_object_unreference(&obj->base);
@@ -1466,6 +1471,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
                else
                        addr = -ENOMEM;
                up_write(&mm->mmap_sem);
+
+               /* This may race, but that's ok, it only gets set */
+               to_intel_bo(obj)->has_wc_mmap = true;
        }

-- 
Chris Wilson, Intel Open Source Technology Centre