Re: [PATCH v4 6/6] drm/i915/xe3: introduce a dirty rectangle state variable

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

On Wed, 22 Jan 2025, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> To avoid programming garbage to dirty rectangle registers,
> introduce a state variable to track the validity of the
> dirty rectangle update scenarios. Program the dirty rectangle
> coordinate only if this state variable is valid.
>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 033eb4a3eab0..ab8acb1cc090 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -91,6 +91,7 @@ struct intel_fbc_state {
>  	u16 interval;
>  	s8 fence_id;
>  	struct drm_rect dirty_rect;
> +	bool dr_valid;

Please let's try to avoid inventing new acronyms for every little
thing. I don't think we want to be guessing what "dr_valid" means.

Adding context would help:

	struct {
		struct drm_rect rect;
		bool valid;
	} dirty_rect;

Or similar.


>  };
>  
>  struct intel_fbc {
> @@ -1227,6 +1228,9 @@ __intel_fbc_program_dirty_rect(struct intel_dsb *dsb, struct intel_plane *plane)
>  	if (fbc_state->plane != plane)
>  		return;
>  
> +	if (!fbc_state->dr_valid)
> +		return;
> +
>  	intel_de_write_dsb(display, dsb, XE3_FBC_DIRTY_RECT(fbc->id),
>  			   FBC_DIRTY_RECT_START_LINE(fbc_state->dirty_rect.y1) |
>  			   FBC_DIRTY_RECT_END_LINE(fbc_state->dirty_rect.y2));
> @@ -1314,6 +1318,8 @@ intel_fbc_compute_dirty_rect(struct intel_atomic_state *state,
>  		if (!fbc || plane->pipe != crtc->pipe)
>  			continue;
>  
> +		fbc->state.dr_valid = false;
> +
>  		/* If plane not visible, dirty rect might have invalid coordinates */
>  		if (!new_plane_state->uapi.visible)
>  			continue;
> @@ -1323,6 +1329,8 @@ intel_fbc_compute_dirty_rect(struct intel_atomic_state *state,
>  			continue;
>  
>  		__intel_fbc_compute_dirty_rect(plane, old_plane_state, new_plane_state);
> +
> +		fbc->state.dr_valid = true;
>  	}
>  }

-- 
Jani Nikula, Intel