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.syrj...@linux.intel.com>
> > Cc: Jani Nikula <jani.nik...@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Signed-off-by: Jouni Högander <jouni.hogan...@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
>
> Reviewed-by: Maarten Lankhorst <maarten.lankho...@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.
>
