On Wed, 2025-01-22 at 12:47 +0200, Jani Nikula wrote:
> On Wed, 22 Jan 2025, Vinod Govindapillai <vinod.govindapillai@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
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__ */
>
