Re: [Intel-gfx] [PATCH v2 3/9] drm/i915/display: Drop unnecessary frontbuffer flushes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 29, 2021 at 05:14:03PM -0700, José Roberto de Souza wrote:
> This unnecessary flushes are hurting power-savings are it causes
> features like PSR, FBC and DRRS to disable it self to handle
> frontbuffer rendering, below some explanation of why each removed
> call is not necessary.
> 
> The flush in intel_prepare_plane_fb() is not required as framebuffer
> will be flipped and power-saving features do the proper flip handling
> in hardware.
> 
> intel_find_initial_plane_obj() flush is not required because it is
> only executed during driver load and at this point the power-saving
> features are not even enabled.
> 
> And the last one intelfb_create(), is also not required as at this
> point the fbdev was just allocated, userspace will draw on
> it what will trigger frontbuffer invalidates and flushes later on.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Yeah, I think these are not right. We might have to think a bit
more about page flips vs. frontbuffer tracking at some point.
But at least FBC doesn't want these, and for drrs I think
you added something to the flip path (also drrs is kinda semi
borked anyway atm and needs some real fixing).

So lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 3 ---
>  drivers/gpu/drm/i915/display/intel_fbdev.c   | 2 --
>  2 files changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 4ce80ca7751f0..9e9db1b0a907b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1711,8 +1711,6 @@ intel_find_initial_plane_obj(struct intel_crtc *crtc,
>  	plane_state->uapi.crtc = &crtc->base;
>  	intel_plane_copy_uapi_to_hw_state(plane_state, plane_state, crtc);
>  
> -	intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
> -
>  	atomic_or(plane->frontbuffer_bit, &to_intel_frontbuffer(fb)->bits);
>  }
>  
> @@ -10755,7 +10753,6 @@ intel_prepare_plane_fb(struct drm_plane *_plane,
>  		return ret;
>  
>  	i915_gem_object_wait_priority(obj, 0, &attr);
> -	i915_gem_object_flush_frontbuffer(obj, ORIGIN_DIRTYFB);
>  
>  	if (!new_plane_state->uapi.fence) { /* implicit fencing */
>  		struct dma_fence *fence;
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 60d3ded270476..53484267b2a4a 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -230,8 +230,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  		goto out_unlock;
>  	}
>  
> -	intel_frontbuffer_flush(to_frontbuffer(ifbdev), ORIGIN_DIRTYFB);
> -
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		drm_err(&dev_priv->drm, "Failed to allocate fb_info\n");
> -- 
> 2.33.0

-- 
Ville Syrjälä
Intel