Re: [Intel-gfx] [RFC PATCH] drm/i915: Remove all frontbuffer tracking calls from the gem code

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Thu, 2022-01-27 at 09:49 -0500, Rodrigo Vivi wrote:
> On Thu, Jan 27, 2022 at 09:20:14AM +0100, Maarten Lankhorst wrote:
> > Op 26-01-2022 om 14:09 schreef Jouni Högander:
> > > We should now rely on userspace doing dirtyfb. There is no
> > > need to have separate frontbuffer tracking hooks in gem code.
> > > 
> > > This patch is removing all frontbuffer tracking calls from the
> > > gem
> > > code.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_overlay.c |  2 --
> > >  drivers/gpu/drm/i915/gem/i915_gem_clflush.c  |  2 --
> > >  drivers/gpu/drm/i915/gem/i915_gem_domain.c   |  5 ----
> > >  drivers/gpu/drm/i915/gem/i915_gem_object.c   | 24 --------------
> > > ------
> > >  drivers/gpu/drm/i915/gem/i915_gem_object.h   | 16 -------------
> > >  drivers/gpu/drm/i915/gem/i915_gem_phys.c     |  7 ------
> > >  drivers/gpu/drm/i915/i915_gem.c              |  5 ----
> > >  7 files changed, 61 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c
> > > b/drivers/gpu/drm/i915/display/intel_overlay.c
> > > index 5358f03b52db..fc2691dac278 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> > > @@ -809,8 +809,6 @@ static int intel_overlay_do_put_image(struct
> > > intel_overlay *overlay,
> > >  		goto out_pin_section;
> > >  	}
> > >  
> > > -	i915_gem_object_flush_frontbuffer(new_bo, ORIGIN_DIRTYFB);
> > > -
> > >  	if (!overlay->active) {
> > >  		const struct intel_crtc_state *crtc_state =
> > >  			overlay->crtc->config;
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
> > > index 8a248003dfae..115e6d877e38 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
> > > @@ -20,8 +20,6 @@ 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);
> > > -
> > > -	i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU);
> > >  }
> > >  
> > >  static void clflush_work(struct dma_fence_work *base)
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > index 26532c07d467..ab1fc2d930e1 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > @@ -63,7 +63,6 @@ flush_write_domain(struct drm_i915_gem_object
> > > *obj, unsigned int flush_domains)
> > >  		}
> > >  		spin_unlock(&obj->vma.lock);
> > >  
> > > -		i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU);
> > >  		break;
> > >  
> > >  	case I915_GEM_DOMAIN_WC:
> > > @@ -615,9 +614,6 @@ i915_gem_set_domain_ioctl(struct drm_device
> > > *dev, void *data,
> > >  out_unlock:
> > >  	i915_gem_object_unlock(obj);
> > >  
> > > -	if (!err && write_domain)
> > > -		i915_gem_object_invalidate_frontbuffer(obj,
> > > ORIGIN_CPU);
> > > -
> > >  out:
> > >  	i915_gem_object_put(obj);
> > >  	return err;
> > > @@ -728,7 +724,6 @@ int i915_gem_object_prepare_write(struct
> > > drm_i915_gem_object *obj,
> > >  	}
> > >  
> > >  out:
> > > -	i915_gem_object_invalidate_frontbuffer(obj, ORIGIN_CPU);
> > >  	obj->mm.dirty = true;
> > >  	/* return with the pages pinned */
> > >  	return 0;
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > index 1a9e1f940a7d..f7ba66deb923 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > @@ -394,30 +394,6 @@ static void i915_gem_free_object(struct
> > > drm_gem_object *gem_obj)
> > >  		queue_delayed_work(i915->wq, &i915->mm.free_work, 0);
> > >  }
> > >  
> > > -void __i915_gem_object_flush_frontbuffer(struct
> > > drm_i915_gem_object *obj,
> > > -					 enum fb_op_origin origin)
> > > -{
> > > -	struct intel_frontbuffer *front;
> > > -
> > > -	front = __intel_frontbuffer_get(obj);
> > > -	if (front) {
> > > -		intel_frontbuffer_flush(front, origin);
> > > -		intel_frontbuffer_put(front);
> > > -	}
> > > -}
> > > -
> > > -void __i915_gem_object_invalidate_frontbuffer(struct
> > > drm_i915_gem_object *obj,
> > > -					      enum fb_op_origin origin)
> > > -{
> > > -	struct intel_frontbuffer *front;
> > > -
> > > -	front = __intel_frontbuffer_get(obj);
> > > -	if (front) {
> > > -		intel_frontbuffer_invalidate(front, origin);
> > > -		intel_frontbuffer_put(front);
> > > -	}
> > > -}
> > > -
> > >  static void
> > >  i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object
> > > *obj, u64 offset, void *dst, int size)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > > b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > > index 02c37fe4a535..d7a08172b239 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > > @@ -578,22 +578,6 @@ void
> > > __i915_gem_object_flush_frontbuffer(struct drm_i915_gem_object
> > > *obj,
> > >  void __i915_gem_object_invalidate_frontbuffer(struct
> > > drm_i915_gem_object *obj,
> > >  					      enum fb_op_origin
> > > origin);
> > >  
> > > -static inline void
> > > -i915_gem_object_flush_frontbuffer(struct drm_i915_gem_object
> > > *obj,
> > > -				  enum fb_op_origin origin)
> > > -{
> > > -	if (unlikely(rcu_access_pointer(obj->frontbuffer)))
> > > -		__i915_gem_object_flush_frontbuffer(obj, origin);
> > > -}
> > > -
> > > -static inline void
> > > -i915_gem_object_invalidate_frontbuffer(struct
> > > drm_i915_gem_object *obj,
> > > -				       enum fb_op_origin origin)
> > > -{
> > > -	if (unlikely(rcu_access_pointer(obj->frontbuffer)))
> > > -		__i915_gem_object_invalidate_frontbuffer(obj, origin);
> > > -}
> > > -
> > >  int i915_gem_object_read_from_page(struct drm_i915_gem_object
> > > *obj, u64 offset, void *dst, int size);
> > >  
> > >  bool i915_gem_object_is_shmem(const struct drm_i915_gem_object
> > > *obj);
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> > > index ca6faffcc496..e98a9884cf5a 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> > > @@ -151,19 +151,12 @@ int i915_gem_object_pwrite_phys(struct
> > > drm_i915_gem_object *obj,
> > >  	if (err)
> > >  		return err;
> > >  
> > > -	/*
> > > -	 * We manually control the domain here and pretend that it
> > > -	 * remains coherent i.e. in the GTT domain, like shmem_pwrite.
> > > -	 */
> > > -	i915_gem_object_invalidate_frontbuffer(obj, ORIGIN_CPU);
> > > -
> > >  	if (copy_from_user(vaddr, user_data, args->size))
> > >  		return -EFAULT;
> > >  
> > >  	drm_clflush_virt_range(vaddr, args->size);
> > >  	intel_gt_chipset_flush(to_gt(i915));
> > >  
> > > -	i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU);
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > > index e3a2c2a0e156..60838209f9cd 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -563,8 +563,6 @@ i915_gem_gtt_pwrite_fast(struct
> > > drm_i915_gem_object *obj,
> > >  		goto out_rpm;
> > >  	}
> > >  
> > > -	i915_gem_object_invalidate_frontbuffer(obj, ORIGIN_CPU);
> > > -
> > >  	user_data = u64_to_user_ptr(args->data_ptr);
> > >  	offset = args->offset;
> > >  	remain = args->size;
> > > @@ -607,7 +605,6 @@ i915_gem_gtt_pwrite_fast(struct
> > > drm_i915_gem_object *obj,
> > >  	}
> > >  
> > >  	intel_gt_flush_ggtt_writes(ggtt->vm.gt);
> > > -	i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU);
> > >  
> > >  	i915_gem_gtt_cleanup(obj, &node, vma);
> > >  out_rpm:
> > > @@ -694,8 +691,6 @@ i915_gem_shmem_pwrite(struct
> > > drm_i915_gem_object *obj,
> > >  		offset = 0;
> > >  	}
> > >  
> > > -	i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU);
> > > -
> > >  	i915_gem_object_unpin_pages(obj);
> > >  	return ret;
> > >  
> > 
> > We should get rid of frontbuffer tracking completely still, this
> > should definitely happen. I've looked at it before, but at that the
> > time we didn't do it yet. Mostly out of concerns of breaking old
> > userspace.
> > 
> > The people you cc'd are not part of the cc here. I added them.
> > 
> > I see i915_pm_dc failing on dc5-psr, something to look into?
> 
> probably...  frontbuffer tracking was a hammer needed on bad psr
> corner cases :(
> 
> +Jose

This is actually triggered by my patch. I can reproduce it by running
same sequence of testcases as in CI. Dc5-psr alone is passing.

> 
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > With the caveat that you wil need someone else to ack the changes,
> > as the last time I proposed this, there was pushback out of fear of
> > breaking userspace.
> >