Re: [Intel-gfx] [PATCH v2 7/8] drm/i915/display/skl+: Drop frontbuffer rendering support

[Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>]

On 8/25/21 3:58 AM, José Roberto de Souza wrote:
> By now all the userspace applications should have migrated to atomic
> or at least be calling DRM_IOCTL_MODE_DIRTYFB.
> 
> With that we can kill frontbuffer rendering support in i915 for
> modern platforms.
> 
> So here converting legacy APIs into atomic commits so it can be
> properly handled by driver i915.
> 
> Several IGT tests will fail with this changes, because some tests
> were stressing those frontbuffer rendering scenarios that no userspace
> should be using by now, fixes to IGT should be sent soon.
> 
> v2:
> - return earlier to not set fb_tracking.busy/flip_bits
> - added a warn on to make sure we are not setting the busy/flip_bits
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_cursor.c    |  6 ++----
>   drivers/gpu/drm/i915/display/intel_fb.c        |  8 +++++++-
>   .../gpu/drm/i915/display/intel_frontbuffer.c   | 18 ++++++++++++++++++
>   drivers/gpu/drm/i915/i915_drv.h                |  2 ++
>   4 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index c7618fef01439..5aa996c3b7980 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   			   u32 src_w, u32 src_h,
>   			   struct drm_modeset_acquire_ctx *ctx)
>   {
> +	struct drm_i915_private *i915 = to_i915(_crtc->dev);
>   	struct intel_plane *plane = to_intel_plane(_plane);
>   	struct intel_crtc *crtc = to_intel_crtc(_crtc);
>   	struct intel_plane_state *old_plane_state =
> @@ -633,12 +634,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   	 * PSR2 selective fetch also requires the slow path as
>   	 * PSR2 plane and transcoder registers can only be updated during
>   	 * vblank.
> -	 *
> -	 * FIXME bigjoiner fastpath would be good
>   	 */
>   	if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) ||
> -	    crtc_state->update_pipe || crtc_state->bigjoiner ||
> -	    crtc_state->enable_psr2_sel_fetch)
> +	    crtc_state->update_pipe || !HAS_FRONTBUFFER_RENDERING(i915))
>   		goto slow;
>   
>   	/*
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index e4b8602ec0cd2..3eb60785c9f29 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -3,6 +3,7 @@
>    * Copyright © 2021 Intel Corporation
>    */
>   
> +#include <drm/drm_damage_helper.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_modeset_helper.h>
>   
> @@ -1235,10 +1236,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
>   					unsigned int num_clips)
>   {
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>   
>   	i915_gem_object_flush_if_display(obj);
> -	intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
>   
> +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> +		return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips,
> +						 num_clips);
Hi,
Even if the userspace informs us of the dirty (damage) region of the 
front buffer being used, GEN9 to GEN12 still uses the HW Tracking 
function for PSR and FBC.
And if you look at the description of the intel_psr_flush() function, 
you can see that there are the following restrictions.

"Since the hardware frontbuffer tracking has gaps we need to integrate 
with the software frontbuffer tracking."

If this restriction is still valid from GEN9 to GEN12, even if the 
existing frontbuffer tracking function is not used, when 
intel_user_framebuffer_dirty() is called, in the case of PSR, 
psr_force_hw_tracking_exit() is called or intel_psr_exit() and 
schedule_work(psr.work) seems to be required.

In the case of FBC, it seems that calls to FBC deactive / FBC activate 
should be added.

If GEN9 to GEN12 do not have the above restrictions, please ignore this 
comment.

G.G.
> +
> +	intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> index 0492446cd04ad..3860f87dac31c 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> @@ -112,6 +112,9 @@ static void frontbuffer_flush(struct drm_i915_private *i915,
>   void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915,
>   				    unsigned frontbuffer_bits)
>   {
> +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> +		return;
> +
>   	spin_lock(&i915->fb_tracking.lock);
>   	i915->fb_tracking.flip_bits |= frontbuffer_bits;
>   	/* Remove stale busy bits due to the old buffer. */
> @@ -132,6 +135,12 @@ void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915,
>   void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
>   				     unsigned frontbuffer_bits)
>   {
> +	if (!HAS_FRONTBUFFER_RENDERING(i915)) {
> +		drm_WARN_ON_ONCE(&i915->drm, i915->fb_tracking.flip_bits |
> +					     i915->fb_tracking.busy_bits);
> +		return;
> +	}
> +
>   	spin_lock(&i915->fb_tracking.lock);
>   	/* Mask any cancelled flips. */
>   	frontbuffer_bits &= i915->fb_tracking.flip_bits;
> @@ -156,6 +165,9 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
>   void intel_frontbuffer_flip(struct drm_i915_private *i915,
>   			    unsigned frontbuffer_bits)
>   {
> +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> +		return;
> +
>   	spin_lock(&i915->fb_tracking.lock);
>   	/* Remove stale busy bits due to the old buffer. */
>   	i915->fb_tracking.busy_bits &= ~frontbuffer_bits;
> @@ -170,6 +182,9 @@ void __intel_fb_invalidate(struct intel_frontbuffer *front,
>   {
>   	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
>   
> +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> +		return;
> +
>   	if (origin == ORIGIN_CS) {
>   		spin_lock(&i915->fb_tracking.lock);
>   		i915->fb_tracking.busy_bits |= frontbuffer_bits;
> @@ -191,6 +206,9 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>   {
>   	struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
>   
> +	if (!HAS_FRONTBUFFER_RENDERING(i915))
> +		return;
> +
>   	if (origin == ORIGIN_CS) {
>   		spin_lock(&i915->fb_tracking.lock);
>   		/* Filter out new bits since rendering started. */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 91453f7dbd656..20c135a2bba33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1721,6 +1721,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>   
>   #define HAS_ASYNC_FLIPS(i915)		(DISPLAY_VER(i915) >= 5)
>   
> +#define HAS_FRONTBUFFER_RENDERING(i915)	(DISPLAY_VER(i915) < 9)
> +
>   /* Only valid when HAS_DISPLAY() is true */
>   #define INTEL_DISPLAY_ENABLED(dev_priv) \
>   	(drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), !(dev_priv)->params.disable_display)
>