On Wed, Jan 21, 2015 at 03:55:18PM +0100, Daniel Vetter wrote:
> On Wed, Jan 21, 2015 at 04:12:07PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 21, 2015 at 02:53:49PM +0100, Daniel Vetter wrote:
> > > Both places check the cpu domain and through some indirection
> > > obj->pin_display and whether cpu access is coherent. And then flush
> > > both cpu caches and the chipset cache. Which is what
> > > flush_cpu_write_domain does.
> > >
> > > Only difference is that on top we get a tracepoint, frontbuffer
> > > tracking and and domain tracking updates. Which doesn't matter since
> > > both paths are slowpaths.
> > >
> > > So lets go with more shared code for clearer idioms.
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
> > > 1 file changed, 4 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 783d1040bf83..b48c39230b80 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1019,11 +1019,8 @@ out:
> > > * cachelines in-line while writing and the object moved
> > > * out of the cpu write domain while we've dropped the lock.
> > > */
> > > - if (!needs_clflush_after &&
> > > - obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
> > > - if (i915_gem_clflush_object(obj, obj->pin_display))
> > > - i915_gem_chipset_flush(dev);
> > > - }
> > > + if (!needs_clflush_after)
> > > + i915_gem_object_flush_cpu_write_domain(obj);
> >
> > Nack. You've just inverted the write domain check.
>
> Indeed, brain must have been offline. And thinking about this more this
> really is a low-level clflush from a function playing tricks, so doesn't
> make sense to use higher level functions anyway.
>
> I'll respin with just the 2nd hunk and try to explain a bit better why
> it's ok, like Chris requested.
The 2nd hunk has the same problem.
> -Daniel
>
> >
> > > }
> > >
> > > if (needs_clflush_after)
> > > @@ -3825,12 +3822,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > > vma->node.color = cache_level;
> > > obj->cache_level = cache_level;
> > >
> > > - if (obj->cache_dirty &&
> > > - obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> > > - cpu_write_needs_clflush(obj)) {
> > > - if (i915_gem_clflush_object(obj, true))
> > > - i915_gem_chipset_flush(obj->base.dev);
> > > - }
> > > + if (obj->cache_dirty)
> > > + i915_gem_object_flush_cpu_write_domain(obj);
> > >
> > > return 0;
> > > }
> > > --
> > > 2.1.4
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
