On Tue, Jan 13, 2015 at 10:23:55PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 13, 2015 at 01:32:52PM +0000, Chris Wilson wrote:
> > Currently we are hitting the WARN inside
> > i915_gem_object_set_cache_level() as we can now have an unbound object
> > in the GTT write domain (due to 43566dedde54f9 "drm/i915: Broaden
> > application of set-domain(GTT)"). To avoid the warning, we need to track
> > when we elided the clflush on a cacheable object and then evict the
> > cache for the object when we move the object out of a cacheable domain.
> >
> > Reported-by: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++-----------------------
> > 2 files changed, 10 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index bf18a5238887..7070482000cd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1998,6 +1998,7 @@ struct drm_i915_gem_object {
> > */
> > unsigned long gt_ro:1;
> > unsigned int cache_level:3;
> > + unsigned int cache_dirty:1;
> >
> > unsigned int has_dma_mapping:1;
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 9bafe50d3df7..aa089e7c31bf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3639,11 +3639,14 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> > * snooping behaviour occurs naturally as the result of our domain
> > * tracking.
> > */
> > - if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
> > + if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
> > + obj->cache_dirty = true;
>
> Hmm. This would mark the cache dirty even when just moving to the CPU read
> domain. A subsequent set_cache_level might then flush a bit needlessly.
> But maybe that's rare enoguh to ignore?
Right, we err on the side of doing an extra clflush on cache-level
transitions. I thought keeping the rule as simple as always do the
clflush if we have skipped any clflushes was a good idea. And yes,
moving dirty objects into uncached is rare, roughly once every 10s on a
bad day.
> I suppose we could follow this up with another patch to fix
> kms_pwrite_crc using cache_dirty as well. But actually that would
> require doing the clflush even when the cache_level didn't change,
> which might aggravate the needless flushing issue somewhat. Or
> perhaps we should just add another cache_dirty dependent flush to
> i915_gem_object_pin_to_display_plane() to deal with that particular
> problem?
I like the idea. I think it will help document the requirements if done
well.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
