| Message ID | 20211021114410.2437099-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/clflush: fixup handling of cache_dirty | expand |
On 10/21/21 13:44, Matthew Auld wrote: > In theory if clflush_work_create() somehow fails here, and we don't yet > have mm.pages populated then we end up resetting cache_dirty, which is > likely wrong, since that will potentially skip the flush-on-acquire, if > it was needed. > > It looks like intel_user_framebuffer_dirty() can arrive here before the > pages are populated. > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_clflush.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c > index f0435c6feb68..d09365b5eb29 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c > @@ -20,6 +20,7 @@ static void __do_clflush(struct drm_i915_gem_object *obj) > { > GEM_BUG_ON(!i915_gem_object_has_pages(obj)); > drm_clflush_sg(obj->mm.pages); > + obj->cache_dirty = false; > I think the guidelines are to avoid updating state in async work if at all possible, so we need to add this after __do_clflush() in the sync path and after dma_fence_work_commit() in the async path. Will that work? /Thomas
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c index f0435c6feb68..d09365b5eb29 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c @@ -20,6 +20,7 @@ static void __do_clflush(struct drm_i915_gem_object *obj) { GEM_BUG_ON(!i915_gem_object_has_pages(obj)); drm_clflush_sg(obj->mm.pages); + obj->cache_dirty = false; i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU); } @@ -115,6 +116,5 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, GEM_BUG_ON(obj->write_domain != I915_GEM_DOMAIN_CPU); } - obj->cache_dirty = false; return true; }
In theory if clflush_work_create() somehow fails here, and we don't yet have mm.pages populated then we end up resetting cache_dirty, which is likely wrong, since that will potentially skip the flush-on-acquire, if it was needed. It looks like intel_user_framebuffer_dirty() can arrive here before the pages are populated. Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_clflush.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)