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

Wed Mar 7 22:46:30 UTC 2018

On Tue, Mar 06, 2018 at 07:34:18PM -0800, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan at intel.com>
> 
> 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 <chris at chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> ---
>  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 = &ifbdev->fb->base;
> +	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> +
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		DRM_ERROR("Failed to allocate fb_info\n");
> @@ -230,8 +233,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  
>  	info->par = helper;
>  
> -	fb = &ifbdev->fb->base;
> -
>  	ifbdev->helper.fb = fb;
>  
>  	strcpy(info->fix.id, "inteldrmfb");
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 36671a937fa4..c2f10d899329 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -807,6 +807,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  		ret = PTR_ERR(vma);
>  		goto out_pin_section;
>  	}
> +	intel_fb_obj_flush(new_bo, ORIGIN_DIRTYFB);
>  
>  	ret = i915_vma_put_fence(vma);
>  	if (ret)
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx