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, "Govindapillai, Vinod" <vinod.govindapillai@intel.com> wrote:
> On Wed, 2025-01-22 at 12:47 +0200, Jani Nikula wrote:
>> 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.
>
> Well.. actually I dont need this call at all. I don't need to split this between compute_dirt_rect
> and program_dirty_rect. Instead I can directly call program_dirty rect which internally gets the
> dirty rects. I will update that
>
>> 
>> > +
>> >  	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.
>
> Yeah.. not really intentional. I will update!
> My local checkpatch didn't catch that though! But the CI checkpatch did.

Try with:

scripts/checkpatch.pl -q --emacs --strict --show-types --max-line-length=100 --ignore=BIT_MACRO,SPLIT_STRING,LONG_LINE_STRING,BOOL_MEMBER


>
> BR
> Vinod
>
>> 
>> >  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