On Mon, Apr 25, 2016 at 09:20:05AM +0100, Chris Wilson wrote:
> On Mon, Apr 25, 2016 at 10:15:11AM +0200, Daniel Vetter wrote:
> > On Mon, Apr 04, 2016 at 06:17:17PM -0300, Paulo Zanoni wrote:
> > > From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > >
> > > ... instead of the previous ORIGIN_GTT. This should actually
> > > invalidate FBC once something is written on the frontbuffer using WC
> > > mmaps. The problem with ORIGIN_GTT is that the automatic hardware
> > > tracking is not able to detect the WC writes as it can detect the GTT
> > > writes.
> > >
> > > This should help fix the SKL bug where nothing happens when you type
> > > your username/password on lightdm.
> > >
> > > This patch was originally pasted on an email by Chris and converted to
> > > an actual git patch by Paulo.
> > >
> > > Cc: <stable@xxxxxxxxxxxxxxx> # 4.6
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >
> > This is From: Chris but lacks his sob. Can't merge as-is, pls ask him for
> > his sob on irc.
> >
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > > drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
> > > 2 files changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index dd18772..17b42a8 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2157,6 +2157,7 @@ struct drm_i915_gem_object {
> > > unsigned int cache_dirty:1;
> > >
> > > unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> > > + unsigned int has_wc_mmap:1;
> > >
> > > unsigned int pin_display;
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 40f90c7..bfafa07 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1608,6 +1608,13 @@ static struct intel_rps_client *to_rps_client(struct drm_file *file)
> > > return &fpriv->rps;
> > > }
> > >
> > > +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.
> > > @@ -1661,9 +1668,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);
> > > @@ -1761,6 +1766,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 */
> >
> > This comment doesn't mesh with the
> >
> > unsigned int has_wc_mmap:1;
> >
> > above. If you depend upon atomicity of writes at the hw level, your struct
> > member _must_ be a full machine word. Which in practice means
> >
> > /*
> > * IMPORTANT: We have unlocked access to this, this must be a
> > * stand-alone bool.
> > */
> > bool has_wc_mmap;
> >
> > You can only fold together different bitfields into one if they're _all_
> > protected by the same lock. gcc needs to generate read-modify-write cycles
> > to write them, which means if any of them are accessed through a separate
> > lock or lock-lessly, there's a real race.
> >
> > With that fixed (and assuming the ABI fix has still Chris' ack/sob):
>
> Yup, we can do better and use WRITE_ONCE() as well. That would make GCC
> complain about it being bogus.
+1 for WRITE_ONCE(). Great suggestion, totally forgot about it.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html