Re: [RFC PATCH 3/4] drm/i915/xe3: add dirty rect support for FBC

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

On Tue, Nov 19, 2024 at 01:53:24AM +0200, Vinod Govindapillai wrote:
> Dirty rectangle feature allows FBC to recompress a subsection
> of a frame. When this feature is enabled, display will read
> the scan lines between dirty rectangle start line and dirty
> rectangle end line in subsequent frames.
> 
> Bspec: 71675, 73424
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c      | 130 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_fbc.h      |   3 +
>  .../drm/i915/display/skl_universal_plane.c    |   2 +
>  3 files changed, 135 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 1fdb1a3e3fbe..01080171790f 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -42,6 +42,7 @@
>  #include <linux/string_helpers.h>
>  
>  #include <drm/drm_blend.h>
> +#include <drm/drm_damage_helper.h>
>  #include <drm/drm_fourcc.h>
>  
>  #include "gem/i915_gem_stolen.h"
> @@ -58,6 +59,7 @@
>  #include "intel_display_trace.h"
>  #include "intel_display_types.h"
>  #include "intel_display_wa.h"
> +#include "intel_dsb.h"
>  #include "intel_fbc.h"
>  #include "intel_fbc_regs.h"
>  #include "intel_frontbuffer.h"
> @@ -126,6 +128,8 @@ struct intel_fbc {
>  	 */
>  	struct intel_fbc_state state;
>  	const char *no_fbc_reason;
> +
> +	struct drm_rect dirty_rect;

That belongs in the fbc state.

>  };
>  
>  /* plane stride in pixels */
> @@ -670,6 +674,10 @@ static void ivb_fbc_activate(struct intel_fbc *fbc)
>  	if (DISPLAY_VER(display) >= 20)
>  		intel_de_write(display, ILK_DPFC_CONTROL(fbc->id), dpfc_ctl);
>  
> +	if (DISPLAY_VER(display) >= 30)
> +		intel_de_write(display, XE3_FBC_DIRTY_CTL(fbc->id),
> +			       FBC_DIRTY_RECT_EN);
> +
>  	intel_de_write(display, ILK_DPFC_CONTROL(fbc->id),
>  		       DPFC_CTL_EN | dpfc_ctl);
>  }
> @@ -1664,6 +1672,113 @@ void intel_fbc_flush(struct drm_i915_private *i915,
>  		__intel_fbc_flush(fbc, frontbuffer_bits, origin);
>  }
>  
> +void
> +intel_fbc_program_dirty_rect(struct intel_dsb *dsb, struct intel_plane *plane)
> +{
> +	struct intel_display *display = to_intel_display(plane);
> +	struct intel_fbc *fbc = plane->fbc;
> +
> +	if (DISPLAY_VER(display) < 30)
> +		return;
> +
> +	if (!fbc)
> +		return;
> +
> +	intel_de_write_dsb(display, dsb, XE3_FBC_DIRTY_RECT(fbc->id),
> +			   FBC_DIRTY_RECT_START_LINE(fbc->dirty_rect.y1) |
> +			   FBC_DIRTY_RECT_END_LINE(fbc->dirty_rect.y2));

I suspect the end line needs a -1. But that needs to be confirmed on
actual hardware.

> +}
> +
> +static bool
> +intel_fbc_need_full_region_update(struct intel_plane_state *old_plane_state,
> +				  struct intel_plane_state *new_plane_state)
> +{
> +	const struct drm_framebuffer *old_fb = old_plane_state->hw.fb;
> +	const struct drm_framebuffer *new_fb = new_plane_state->hw.fb;
> +
> +	if (!old_fb || !new_fb)
> +		return true;
> +
> +	if (old_fb->format->format != new_fb->format->format)
> +		return true;
> +
> +	if (old_fb->modifier != new_fb->modifier)
> +		return true;
> +
> +	if (intel_fbc_plane_stride(old_plane_state) !=
> +	    intel_fbc_plane_stride(new_plane_state))
> +		return true;
> +
> +	if (intel_fbc_cfb_stride(old_plane_state) !=
> +	    intel_fbc_cfb_stride(new_plane_state))
> +		return true;
> +
> +	if (intel_fbc_cfb_size(old_plane_state) !=
> +	    intel_fbc_cfb_size(new_plane_state))
> +		return true;
> +
> +	return false;
> +}

If any of those fail then I think we're going to be
disabling+re-enabling FBC anyway. So we might not need to check
any of this, or if we do we could perhaps just use can_flip_nuke().

> +
> +static void
> +update_dirty_rect_to_full_region(struct intel_plane_state *plane_state,
> +				 struct drm_rect *dirty_rect)
> +{
> +	int y_offset = plane_state->view.color_plane[0].y;
> +	int plane_height = drm_rect_height(&plane_state->uapi.src) >> 16;
> +
> +	dirty_rect->y1 = y_offset;
> +	dirty_rect->y2 = y_offset + plane_height;
> +}
> +
> +static void validate_and_clip_dirty_rect(struct intel_plane_state *plane_state,
> +					 struct drm_rect *dirty_rect)
> +{
> +	int y_offset = plane_state->view.color_plane[0].y;
> +	int plane_height = drm_rect_height(&plane_state->uapi.src) >> 16;
> +	int max_endline = y_offset + plane_height;
> +
> +	dirty_rect->y1 = clamp(dirty_rect->y1, y_offset, max_endline);
> +	dirty_rect->y2 = clamp(dirty_rect->y2, dirty_rect->y1, max_endline);
> +}
> +
> +static void intel_fbc_compute_dirty_rect(struct intel_plane *plane,
> +					struct intel_plane_state *old_plane_state,
> +					struct intel_plane_state *new_plane_state,
> +					bool need_full_region_update)
> +{
> +	struct intel_display *display = to_intel_display(plane);
> +	struct intel_fbc *fbc = plane->fbc;
> +	struct drm_rect *fbc_dirty_rect = &fbc->dirty_rect;
> +
> +	if (need_full_region_update) {
> +		drm_dbg_kms(display->drm,
> +			    "[PLANE:%d:%s] Full region update needed\n",
> +			    plane->base.base.id, plane->base.name);
> +		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
> +		goto out;
> +	}
> +
> +	if (drm_atomic_helper_damage_merged(&old_plane_state->uapi,
> +					    &new_plane_state->uapi,
> +					    fbc_dirty_rect)) {
> +		validate_and_clip_dirty_rect(new_plane_state, fbc_dirty_rect);

Your fbc_dirty_rect here will still be in a different coordinate space
(fb relative) than the plane src coordinates (PLANE_SURF relative).

> +	} else {
> +		drm_dbg_kms(display->drm,
> +			    "[PLANE:%d:%s] Damage clips merge cal failed. Use full region\n",
> +			    plane->base.base.id, plane->base.name);
> +

That's going to create excessive spam.

> +		/* TODO! if the drm call failed, update full region? */
> +		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
> +	}
> +
> +out:
> +	drm_dbg_kms(display->drm,
> +		    "[PLANE:%d:%s] Dirty rect start line: %d End line: %d\n",
> +		    plane->base.base.id, plane->base.name, fbc_dirty_rect->y1,
> +		    fbc_dirty_rect->y2);a

ditto

> +}
> +
>  int intel_fbc_atomic_check(struct intel_atomic_state *state)
>  {
>  	struct intel_plane_state __maybe_unused *new_plane_state;
> @@ -1673,11 +1788,26 @@ int intel_fbc_atomic_check(struct intel_atomic_state *state)
>  
>  	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
>  					     new_plane_state, i) {
> +		bool full_region_update;
>  		int ret;
>  
> +		if (!plane->fbc)
> +			continue;
> +
>  		ret = intel_fbc_check_plane(state, plane);
>  		if (ret)
>  			return ret;
> +
> +		if (!new_plane_state->no_fbc_reason)
> +			continue;
> +
> +		full_region_update =
> +			intel_fbc_need_full_region_update(old_plane_state,
> +							  new_plane_state);
> +
> +		intel_fbc_compute_dirty_rect(plane, old_plane_state,
> +					     new_plane_state,
> +					     full_region_update);

That belongs in intel_fbc_update_state()

>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> index ceae55458e14..073d671ea94d 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> @@ -14,6 +14,7 @@ struct intel_atomic_state;
>  struct intel_crtc;
>  struct intel_crtc_state;
>  struct intel_display;
> +struct intel_dsb;
>  struct intel_fbc;
>  struct intel_plane;
>  struct intel_plane_state;
> @@ -48,5 +49,7 @@ void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display);
>  void intel_fbc_reset_underrun(struct intel_display *display);
>  void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc);
>  void intel_fbc_debugfs_register(struct intel_display *display);
> +void intel_fbc_program_dirty_rect(struct intel_dsb *dsb,
> +				  struct intel_plane *plane);
>  
>  #endif /* __INTEL_FBC_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4c7bcf6806ff..fd3611323ec0 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1510,6 +1510,8 @@ icl_plane_update_noarm(struct intel_dsb *dsb,
>  		icl_plane_csc_load_black(dsb, plane, crtc_state);
>  
>  	icl_plane_update_sel_fetch_noarm(dsb, plane, crtc_state, plane_state, color_plane);
> +
> +	intel_fbc_program_dirty_rect(dsb, plane);

I think we want a completely separate call to FBC around where call
intel_crtc_planes_update_arm().

It's going to be a bit dodgy becase we won't be able take fbc->lock
and thus accessing the fbc state needs to be done locklessly. But
I *think* it should be safe because the fbc state should only be
updated from intel_fbc_{update,disable}() which are done from the
atomic commit path only.

We are going to need some real tests for this:
- update region A -> flip with dirty rect A -> update region B -> flip with dirty rect B
- update region A -> flip with dirty rect A -> update region B -> flip without dirty rect
- update region A -> flip with dirty rect A -> update region B -> dirtyfb
- probably some other ones which involve format changes/etc. as well.

>  }
>  
>  static void icl_plane_update_sel_fetch_arm(struct intel_dsb *dsb,
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel