Re: [PATCH 1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2018-02-16 at 08:52 +0000, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:18)
> > i915_gem_obj_pin_to_display() calls frontbuffer_flush with origin set to
> > DIRTYFB. The callers however are at a vantage point to decide if hardware
> > frontbuffer tracking can do the flush for us. For example, legacy cursor
> > updates, like flips, write to MMIO registers, which then triggers PSR flush
> > by the hardware. Moving frontbuffer_flush out will enable us to skip a
> > software initiated flush by setting origin to FLIP. Thanks to Chris for the
> > idea.
> > 
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c      |  9 ++++-----
> >  drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++---
> >  drivers/gpu/drm/i915/intel_fbdev.c   |  6 +++---
> >  drivers/gpu/drm/i915/intel_overlay.c |  1 +
> >  4 files changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index fc68b35854df..22b598e1e0b9 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4071,9 +4071,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> >  }
> >  
> >  /*
> > - * Prepare buffer for display plane (scanout, cursors, etc).
> > - * Can be called from an uninterruptible phase (modesetting) and allows
> > - * any flushes to be pipelined (for pageflips).
> > + * Prepare buffer for display plane (scanout, cursors, etc). Can be called from
> > + * an uninterruptible phase (modesetting) and allows any flushes to be pipelined
> > + * (for pageflips). We only flush the caches while preparing the buffer for
> > + * display, the callers are responsible for frontbuffer flush.
> >   */
> >  struct i915_vma *
> >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > @@ -4139,9 +4140,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >  
> >         vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
> >  
> > -       /* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> >         __i915_gem_object_flush_for_display(obj);
> > -       intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> >  
> >         /* It should now be out of any other write domains, and we can update
> >          * the domain values for our changes.
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 286a9591d179..2b70714ead0f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2806,10 +2806,15 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >         return;
> >  
> >  valid_fb:
> > +       obj = intel_fb_obj(fb);
> >         mutex_lock(&dev->struct_mutex);
> >         intel_state->vma =
> >                 intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
> > +
> > +       if (!IS_ERR(intel_state->vma))
> > +               intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> 
> The fb_obj_flush doesn't need to be inside the lock, so you can do it
> after the normal error testing (or just do it since the flush doesn't
> depend on having the vma bound).

Okay, that answers the question I meant to ask. Decided to err on the
side of caution.
 
> 
> >         mutex_unlock(&dev->struct_mutex);
> > +
> >         if (IS_ERR(intel_state->vma)) {
> >                 DRM_ERROR("failed to pin boot fb on pipe %d: %li\n",
> >                           intel_crtc->pipe, PTR_ERR(intel_state->vma));
> > @@ -12713,10 +12717,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> >                 struct i915_vma *vma;
> >  
> >                 vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> > -               if (!IS_ERR(vma))
> > +               if (!IS_ERR(vma)) {
> >                         to_intel_plane_state(new_state)->vma = vma;
> > -               else
> > +                       intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > +               } else {
> >                         ret =  PTR_ERR(vma);
> 
> Would
> 	if (IS_ERR(vma)) {
> 		ret = PTR_ERR(vma);
> 		break;
There's no loop around this, but this change goes away in [Patch 4/5].


> 	}
> 
> 	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> 	to_intel_plane_state(new_state)->vma = vma;
> 
> look neater here?
> 
> Lgtm,
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux