On Wed, 22 Jan 2025, Vinod Govindapillai <vinod.govindapillai@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
