Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Rodrigo Vivi]

On Wed, Mar 07, 2018 at 10:54:28PM +, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Wed, 2018-03-07 at 14:46 -0800, Rodrigo Vivi wrote:
> > On Tue, Mar 06, 2018 at 07:34:18PM -0800, Dhinakaran Pandiyan wrote:
> > > From: "Pandiyan, Dhinakaran" 
> > > 
> > > 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.
> > > 
> > > v2:
> > > Rebased due to Ville adding intel_plane_pin_fb().
> > > Minor code reordering as fb_obj_flush doesn't need struct_mutex (Chris)
> > > 
> > > Cc: Chris Wilson 
> > > Cc: Ville Syrjälä 
> > > Cc: Paulo Zanoni 
> > > Signed-off-by: Dhinakaran Pandiyan 
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c  | 9 -
> > >  drivers/gpu/drm/i915/intel_display.c | 9 +++--
> > >  drivers/gpu/drm/i915/intel_fbdev.c   | 5 +++--
> > >  drivers/gpu/drm/i915/intel_overlay.c | 1 +
> > >  4 files changed, 15 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > > index a5bd07338b46..e4c5a1a22d8b 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,
> > > @@ -4129,9 +4130,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() */
> > 
> > Although flush by some definition here is invalidate + flush
> > I see flush operations as the operation to be performed at the 
> > "end-of-frame".
> > After the operation was done and whatever had to be modified was modified.
> > 
> > I see you patch changing the flush to the creation and begin of the fb 
> > handling
> > like on creating and init fb functions. I believe by that time we are not 
> > ready
> > to exit PSR yet.
> > 
> > What am I missing?
> 
> 
> The functions that you are referring to call
> i915_gem_object_pin_to_display_plane() which in turn calls frontbuffer
> flush. This patch is just a refactor, doesn't change whether flush() is
> called. I am trying to move it up the call stack so that flush() can be
> selectively removed depending on the call site.

Oh! now it made more sense...

probably this phrase is more clear than the current commit message for a
commit message.

But I checked the code and I agree it is right, so:

Reviewed-by: Rodrigo Vivi 



> 
> 
> 
> 
> > 
> > >   __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 331084082545..91ce8a0522a3 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2858,6 +2858,9 @@ intel_find_initial_plane_obj(struct intel_crtc 
> > > *intel_crtc,
> > >   return;
> > >   }
> > >  
> > > + obj = intel_fb_obj(fb);
> > > + intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > > +
> > >   plane_state->src_x = 0;
> > >   plane_state->src_y = 0;
> > >   plane_state->src_w = fb->width << 16;
> > > @@ -2871,7 +2874,6 @@ intel_find_initial_plane_obj(struct intel_crtc 
> > > *intel_crtc,
> > >   intel_state->base.src = drm_plane_state_src(plane_state);
> > >   intel_state->base.dst = drm_plane_state_dest(plane_state);
> > >  
> > > - obj = intel_fb_obj(fb);
> > >   if (i915_gem_object_is_tiled(obj))
> > >   dev_priv->preserve_bios_swizzle = true;
> > >  

Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Pandiyan, Dhinakaran]

On Wed, 2018-03-07 at 14:46 -0800, Rodrigo Vivi wrote:
> On Tue, Mar 06, 2018 at 07:34:18PM -0800, Dhinakaran Pandiyan wrote:
> > From: "Pandiyan, Dhinakaran" 
> > 
> > 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.
> > 
> > v2:
> > Rebased due to Ville adding intel_plane_pin_fb().
> > Minor code reordering as fb_obj_flush doesn't need struct_mutex (Chris)
> > 
> > Cc: Chris Wilson 
> > Cc: Ville Syrjälä 
> > Cc: Paulo Zanoni 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c  | 9 -
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++--
> >  drivers/gpu/drm/i915/intel_fbdev.c   | 5 +++--
> >  drivers/gpu/drm/i915/intel_overlay.c | 1 +
> >  4 files changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index a5bd07338b46..e4c5a1a22d8b 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,
> > @@ -4129,9 +4130,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() */
> 
> Although flush by some definition here is invalidate + flush
> I see flush operations as the operation to be performed at the "end-of-frame".
> After the operation was done and whatever had to be modified was modified.
> 
> I see you patch changing the flush to the creation and begin of the fb 
> handling
> like on creating and init fb functions. I believe by that time we are not 
> ready
> to exit PSR yet.
> 
> What am I missing?


The functions that you are referring to call
i915_gem_object_pin_to_display_plane() which in turn calls frontbuffer
flush. This patch is just a refactor, doesn't change whether flush() is
called. I am trying to move it up the call stack so that flush() can be
selectively removed depending on the call site.




> 
> > __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 331084082545..91ce8a0522a3 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2858,6 +2858,9 @@ intel_find_initial_plane_obj(struct intel_crtc 
> > *intel_crtc,
> > return;
> > }
> >  
> > +   obj = intel_fb_obj(fb);
> > +   intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > +
> > plane_state->src_x = 0;
> > plane_state->src_y = 0;
> > plane_state->src_w = fb->width << 16;
> > @@ -2871,7 +2874,6 @@ intel_find_initial_plane_obj(struct intel_crtc 
> > *intel_crtc,
> > intel_state->base.src = drm_plane_state_src(plane_state);
> > intel_state->base.dst = drm_plane_state_dest(plane_state);
> >  
> > -   obj = intel_fb_obj(fb);
> > if (i915_gem_object_is_tiled(obj))
> > dev_priv->preserve_bios_swizzle = true;
> >  
> > @@ -12785,6 +12787,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> > if (ret)
> > return ret;
> >  
> > +   intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > +
> > if (!new_state->fence) { /* implicit fencing */
> > struct dma_fence *fence;
> >  
> > @@ -13172,8 +13176,9 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > if (ret)
> > goto out_unlock;
> >  
> > -   old_fb = old_plane_state->fb;
> > +   intel_fb_obj_flush(intel_fb_obj(fb), 

Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Rodrigo Vivi]

On Tue, Mar 06, 2018 at 07:34:18PM -0800, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" 
> 
> 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.
> 
> v2:
> Rebased due to Ville adding intel_plane_pin_fb().
> Minor code reordering as fb_obj_flush doesn't need struct_mutex (Chris)
> 
> Cc: Chris Wilson 
> Cc: Ville Syrjälä 
> Cc: Paulo Zanoni 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/i915_gem.c  | 9 -
>  drivers/gpu/drm/i915/intel_display.c | 9 +++--
>  drivers/gpu/drm/i915/intel_fbdev.c   | 5 +++--
>  drivers/gpu/drm/i915/intel_overlay.c | 1 +
>  4 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a5bd07338b46..e4c5a1a22d8b 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,
> @@ -4129,9 +4130,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() */

Although flush by some definition here is invalidate + flush
I see flush operations as the operation to be performed at the "end-of-frame".
After the operation was done and whatever had to be modified was modified.

I see you patch changing the flush to the creation and begin of the fb handling
like on creating and init fb functions. I believe by that time we are not ready
to exit PSR yet.

What am I missing?

>   __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 331084082545..91ce8a0522a3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2858,6 +2858,9 @@ intel_find_initial_plane_obj(struct intel_crtc 
> *intel_crtc,
>   return;
>   }
>  
> + obj = intel_fb_obj(fb);
> + intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> +
>   plane_state->src_x = 0;
>   plane_state->src_y = 0;
>   plane_state->src_w = fb->width << 16;
> @@ -2871,7 +2874,6 @@ intel_find_initial_plane_obj(struct intel_crtc 
> *intel_crtc,
>   intel_state->base.src = drm_plane_state_src(plane_state);
>   intel_state->base.dst = drm_plane_state_dest(plane_state);
>  
> - obj = intel_fb_obj(fb);
>   if (i915_gem_object_is_tiled(obj))
>   dev_priv->preserve_bios_swizzle = true;
>  
> @@ -12785,6 +12787,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>   if (ret)
>   return ret;
>  
> + intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> +
>   if (!new_state->fence) { /* implicit fencing */
>   struct dma_fence *fence;
>  
> @@ -13172,8 +13176,9 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>   if (ret)
>   goto out_unlock;
>  
> - old_fb = old_plane_state->fb;
> + intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
>  
> + old_fb = old_plane_state->fb;
>   i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
> intel_plane->frontbuffer_bit);
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6f12adc06365..65a3313723c9 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -221,6 +221,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   goto out_unlock;
>   }
>  
> + fb = >fb->base;
> +