Re: [PATCH v4 5/6] drm/i915/xe3: handle dirty rect update within the scope of DSB

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 22 Jan 2025, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> Programming of the dirty rectangle coordinates should happen
> within the scope of DSB prepare and finish calls. So call the
> compute and programming of dirty rectangle related routines
> early within the DSB programming window. Some of the FBC state
> handling is done later as part of pre-plane or post-plane
> updates. So enabling / disabling / hw activate will happen
> later but it should handle the sequence without any issue.
>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  3 ++
>  drivers/gpu/drm/i915/display/intel_fbc.c     | 47 ++++++++++++++++----
>  drivers/gpu/drm/i915/display/intel_fbc.h     |  3 ++
>  3 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index d154fcd0e77a..e6e017e65da6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7773,6 +7773,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  
>  	intel_atomic_prepare_plane_clear_colors(state);
>  
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
> +		intel_fbc_compute_dirty_rect(state, crtc);

"compute" is a fairly loaded word in our driver. The immediate
association is "it's compute config, but missing the config part". And
doing anything "compute" seems completely out of place in
intel_atomic_commit_tail(), where we've long since passed the compute
config stage.

> +
>  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
>  		intel_atomic_dsb_finish(state, crtc);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 963fbe2c7361..033eb4a3eab0 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -1213,6 +1213,10 @@ static bool tiling_is_valid(const struct intel_plane_state *plane_state)
>  		return i8xx_fbc_tiling_valid(plane_state);
>  }
>  
> +static bool intel_fbc_can_flip_nuke(struct intel_atomic_state *state,
> +				    struct intel_crtc *crtc,
> +				    struct intel_plane *plane);
> +
>  static void
>  __intel_fbc_program_dirty_rect(struct intel_dsb *dsb, struct intel_plane *plane)
>  {
> @@ -1251,7 +1255,6 @@ intel_fbc_program_dirty_rect(struct intel_dsb *dsb,
>  	}
>  }
>  
> -

The previous patch added a superfluous newline here, and this one
removes it. Please just don't add it in the first place.

>  static void
>  update_dirty_rect_to_full_region(struct intel_plane_state *plane_state,
>  				 struct drm_rect *dirty_rect)
> @@ -1276,9 +1279,9 @@ validate_and_clip_dirty_rect(struct intel_plane_state *plane_state,
>  }
>  
>  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)
> +__intel_fbc_compute_dirty_rect(struct intel_plane *plane,
> +			       struct intel_plane_state *old_plane_state,
> +			       struct intel_plane_state *new_plane_state)
>  {
>  	struct intel_fbc *fbc = plane->fbc;
>  	struct intel_fbc_state *fbc_state = &fbc->state;
> @@ -1292,6 +1295,37 @@ intel_fbc_compute_dirty_rect(struct intel_plane *plane,
>  		update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
>  }
>  
> +void
> +intel_fbc_compute_dirty_rect(struct intel_atomic_state *state,
> +			     struct intel_crtc *crtc)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct intel_plane_state *new_plane_state;
> +	struct intel_plane_state *old_plane_state;
> +	struct intel_plane *plane;
> +	int i;
> +
> +	if (DISPLAY_VER(display) < 30)
> +		return;
> +
> +	for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> +		struct intel_fbc *fbc = plane->fbc;
> +
> +		if (!fbc || plane->pipe != crtc->pipe)
> +			continue;
> +
> +		/* If plane not visible, dirty rect might have invalid coordinates */
> +		if (!new_plane_state->uapi.visible)
> +			continue;
> +
> +		/* If FBC to be disabled, then no need to update dirty rect */
> +		if (!intel_fbc_can_flip_nuke(state, crtc, plane))
> +			continue;
> +
> +		__intel_fbc_compute_dirty_rect(plane, old_plane_state, new_plane_state);
> +	}
> +}
> +
>  static void intel_fbc_update_state(struct intel_atomic_state *state,
>  				   struct intel_crtc *crtc,
>  				   struct intel_plane *plane)
> @@ -1301,8 +1335,6 @@ static void intel_fbc_update_state(struct intel_atomic_state *state,
>  		intel_atomic_get_new_crtc_state(state, crtc);
>  	struct intel_plane_state *plane_state =
>  		intel_atomic_get_new_plane_state(state, plane);
> -	struct intel_plane_state *old_plane_state =
> -		intel_atomic_get_old_plane_state(state, plane);
>  	struct intel_fbc *fbc = plane->fbc;
>  	struct intel_fbc_state *fbc_state = &fbc->state;
>  
> @@ -1327,9 +1359,6 @@ static void intel_fbc_update_state(struct intel_atomic_state *state,
>  	fbc_state->cfb_stride = intel_fbc_cfb_stride(plane_state);
>  	fbc_state->cfb_size = intel_fbc_cfb_size(plane_state);
>  	fbc_state->override_cfb_stride = intel_fbc_override_cfb_stride(plane_state);
> -
> -	if (DISPLAY_VER(display) >= 30)
> -		intel_fbc_compute_dirty_rect(plane, old_plane_state, plane_state);
>  }
>  
>  static bool intel_fbc_is_fence_ok(const struct intel_plane_state *plane_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> index acaebe15f312..87be5653db0f 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> @@ -49,8 +49,11 @@ 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_compute_dirty_rect(struct intel_atomic_state *state,
> +				  struct intel_crtc *crtc);
>  void intel_fbc_program_dirty_rect(struct intel_dsb *dsb,
>  				  struct intel_atomic_state *state,
>  				  struct intel_crtc *crtc);
>  
> +

Superfluous newline.

>  #endif /* __INTEL_FBC_H__ */

-- 
Jani Nikula, Intel