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@xxxxxxxxxxxxxxx>
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxx>
> >
> > 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.
> >
