So based on your comments on the previous patch and this one, let me conclude the changes to be done
here!
1. As part of the previous patch, we collect the fbc_dirty rect from the damage_helper call. Make
damage empty if the plane not visible.
2. Update the FBC dirty rect registes. This do two things
2.1 Prepare the collected FBC dirty and copy it to the correct fbc_state -
intel_fbc_prepare_dirty_rect()
2.2 Write to the registers - intel_fbc_dirty_rect_update_noarm()
2.1 AT the beginning of the atomic_commit_tail() within the DSB block handling, calls
intel_fbc_prepare_dirty_rect() to populate fbc_state's fbc_dirty_rect structs
In the intel_fbc_prepare_dirty_rect(),
iterate through each plane in the atomic_state
check the correct plane, pipe etc. Then,
if (drm_rect_visible(damage_merged)
update fbc_state's fbc dirty rect
else
Make at it atleast one scan line and update the fbc_state's fbc_dirty rect
2.2 Then call intel_fbc_dirty_rect_update_noarm() form intel_plane_update_noarm()
And within in intel_fbc_dirty_rect_update_noarm() just update the FBC dirty registers with
the values from corresponding fbc_state->fbc_dirty
I think last time when we discussed, we decided keep that valid flag to track
intel_fbc_can_flip_nuke() case. So I guess that is not required anymore
Regarding the async flip cases, I thought nothing specific need to be done for the fbc dirty rect.
Atleast the some the tests I tried were passing with dirty rect enabled.
I will add the locks in these new functions.
BR
Vinod
On Wed, 2025-02-05 at 15:31 +0200, Ville Syrjälä wrote:
> On Thu, Jan 30, 2025 at 11:00:24PM +0200, Vinod Govindapillai wrote:
> > Dirty rectangle feature allows FBC to recompress a subsection
> > of a frame. When this feature is enabled, display will read
> > the scan lines between dirty rectangle start line and dirty
> > rectangle end line in subsequent frames.
> >
> > Use the merged damage clip stored in the plane state to
> > configure the FBC dirty rect areas.
> >
> > v2: - Move dirty rect handling to fbc state (Ville)
> >
> > v3: - Use intel_fbc_dirty_rect_update_noarm (Ville)
> > - Split plane damage collection and dirty rect preparation
> > - Handle case where dirty rect fall outside the visible region
> >
> > v4: - A state variable to check if we need to update dirty rect
> > registers in case intel_fbc_can_flip_nuke() (Ville)
> >
> > Bspec: 68881, 71675, 73424
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 7 +
> > drivers/gpu/drm/i915/display/intel_fbc.c | 132 +++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_fbc.h | 6 +
> > 3 files changed, 145 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7d68d652c1bc..33277f892279 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7263,6 +7263,8 @@ static void intel_update_crtc(struct intel_atomic_state *state,
> >
> > commit_pipe_pre_planes(state, crtc);
> >
> > + intel_fbc_dirty_rect_update_noarm(NULL, state, crtc);
> > +
>
> That is not the right place for _noarm() stuff.
>
> Hmm, maybe we should just do this as part of intel_plane_update_noarm()?
> How does the dirty rect interact with async flips?
Ok.
I thought no need to handle anything specific to dirty_rect in case of async flips.
Atleast the the tests that I tried were ok!
>
> > intel_crtc_planes_update_arm(NULL, state, crtc);
> >
> > commit_pipe_post_planes(state, crtc);
> > @@ -7723,6 +7725,8 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
> > new_crtc_state);
> > intel_crtc_planes_update_noarm(new_crtc_state->dsb_commit,
> > state, crtc);
> > + intel_fbc_dirty_rect_update_noarm(new_crtc_state->dsb_commit,
> > + state, crtc);
> >
> > intel_dsb_vblank_evade(state, new_crtc_state->dsb_commit);
> >
> > @@ -7769,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_prepare_dirty_rect(state, crtc);
> > +
> > 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 df05904bac8a..24736c7a79a6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -88,6 +88,10 @@ struct intel_fbc_state {
> > u16 override_cfb_stride;
> > u16 interval;
> > s8 fence_id;
> > + struct {
> > + struct drm_rect rect;
> > + bool valid;
> > + } fbc_dirty_rect;
> > };
> >
> > struct intel_fbc {
> > @@ -527,6 +531,9 @@ static void ilk_fbc_deactivate(struct intel_fbc *fbc)
> > struct intel_display *display = fbc->display;
> > u32 dpfc_ctl;
> >
> > + if (HAS_FBC_DIRTY_RECT(display))
> > + intel_de_write(display, XE3_FBC_DIRTY_CTL(fbc->id), 0);
> > +
> > /* Disable compression */
> > dpfc_ctl = intel_de_read(display, ILK_DPFC_CONTROL(fbc->id));
> > if (dpfc_ctl & DPFC_CTL_EN) {
> > @@ -670,6 +677,10 @@ static void ivb_fbc_activate(struct intel_fbc *fbc)
> > if (DISPLAY_VER(display) >= 20)
> > intel_de_write(display, ILK_DPFC_CONTROL(fbc->id), dpfc_ctl);
> >
> > + if (HAS_FBC_DIRTY_RECT(display))
> > + intel_de_write(display, XE3_FBC_DIRTY_CTL(fbc->id),
> > + FBC_DIRTY_RECT_EN);
> > +
> > intel_de_write(display, ILK_DPFC_CONTROL(fbc->id),
> > DPFC_CTL_EN | dpfc_ctl);
> > }
> > @@ -1203,6 +1214,127 @@ 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);
>
> You should put the code into a place that doesn't need this extra
> declaration.
>
> > +
> > +static void
> > +intel_fbc_dirty_rect_update(struct intel_dsb *dsb, struct intel_plane *plane)
> > +{
> > + struct intel_display *display = to_intel_display(plane);
> > + struct intel_fbc *fbc = plane->fbc;
> > + struct intel_fbc_state *fbc_state = &fbc->state;
> > + struct drm_rect *fbc_dirty_rect = &fbc_state->fbc_dirty_rect.rect;
> > +
> > + if (fbc_state->plane != plane)
> > + return;
> > +
> > + if (!fbc_state->fbc_dirty_rect.valid)
> > + return;
>
> I think that extra flag can be thrown out and you can just
> check if the rect is visible here.
> > +
> > + intel_de_write_dsb(display, dsb, XE3_FBC_DIRTY_RECT(fbc->id),
> > + FBC_DIRTY_RECT_START_LINE(fbc_dirty_rect->y1) |
> > + FBC_DIRTY_RECT_END_LINE(fbc_dirty_rect->y2));
> > +
> > + fbc_state->fbc_dirty_rect.valid = false;
>
> Why?
>
> > +}
> > +
> > +void
> > +intel_fbc_dirty_rect_update_noarm(struct intel_dsb *dsb,
> > + struct intel_atomic_state *state,
> > + struct intel_crtc *crtc)
> > +{
> > + struct intel_display *display = to_intel_display(state);
> > + struct intel_plane_state *plane_state;
>
> const, although it seems to me we can just rely on the rect being valid
> or not and then the plane visiblility is also already covered.
>
> > + struct intel_plane *plane;
> > + int i;
> > +
> > + if (!HAS_FBC_DIRTY_RECT(display))
> > + return;
> > +
> > + for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
> > + struct intel_fbc *fbc = plane->fbc;
> > +
> > + if (!fbc || plane->pipe != crtc->pipe)
> > + continue;
> > +
> > + if (!plane_state->uapi.visible)
> > + continue;
>
> That check would also disappear.
>
> This does need to check if the plane is the one using FBC
> currently. Oh, I see you put that check inside
> intel_fbc_dirty_rect_update(). I think the check belongs
> here because that is how we do it everwhere else.
>
> I was pondering if we need to keep this lockless or not.
> I guess we don't since we are doing it from the noarm()
> section (assuming we're not planing to implement mailbox
> flips tomorrow). So for now I think we can just do the proper
> locking here too. If we do need to make this lockless in
> then we need to explain what makes it safe/correct.
>
> > +
> > + intel_fbc_dirty_rect_update(dsb, plane);
> > + }
> > +}
> > +
> > +static void
> > +validate_and_clip_dirty_rect(struct intel_plane_state *plane_state,
> > + struct drm_rect *dirty_rect)
> > +{
> > + int y_offset = plane_state->view.color_plane[0].y;
> > + int plane_height = drm_rect_height(&plane_state->uapi.src) >> 16;
> > + int max_endline = y_offset + plane_height - 1;
> > +
> > + dirty_rect->y1 = clamp(dirty_rect->y1, y_offset, max_endline);
> > + dirty_rect->y2 = clamp(dirty_rect->y2, dirty_rect->y1, max_endline);
> > +}
>
> This is not needed.
>
> > +
> > +static void
> > +__intel_fbc_prepare_dirty_rect(struct intel_plane *plane,
> > + struct intel_plane_state *plane_state,
> > + bool fbc_dirty_rect_valid)
> > +{
> > + struct intel_fbc_state *fbc_state = &plane->fbc->state;
> > + struct drm_rect *fbc_dirty_rect = &fbc_state->fbc_dirty_rect.rect;
> > + struct drm_rect *damage_merged = &plane_state->damage_merged;
> > + int y_offset = plane_state->view.color_plane[0].y;
> > +
> > + fbc_state->fbc_dirty_rect.valid = fbc_dirty_rect_valid;
> > +
> > + if (drm_rect_visible(damage_merged)) {
> > + fbc_dirty_rect->y1 = damage_merged->y1;
> > + fbc_dirty_rect->y2 = damage_merged->y2;
> > + } else {
> > + /* If not visible, we need to set one single line */
> > + fbc_dirty_rect->y1 = y_offset;
> > + fbc_dirty_rect->y2 = y_offset;
> > + }
> > + fbc_dirty_rect->x1 = damage_merged->x1;
> > + fbc_dirty_rect->x2 = damage_merged->x2;
> > +
> > + validate_and_clip_dirty_rect(plane_state, fbc_dirty_rect);
> > +}
> > +
> > +void
> > +intel_fbc_prepare_dirty_rect(struct intel_atomic_state *state,
> > + struct intel_crtc *crtc)
> > +{
> > + struct intel_display *display = to_intel_display(state);
> > + struct intel_plane_state *plane_state;
> > + struct intel_plane *plane;
> > + int i;
> > +
> > + if (!HAS_FBC_DIRTY_RECT(display))
> > + return;
> > +
> > + for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
> > + struct intel_fbc *fbc = plane->fbc;
> > +
> > + if (!fbc || plane->pipe != crtc->pipe)
> > + continue;
> > +
> > + if (!fbc->state.plane)
> > + continue;
>
> fbc->state.plane != plane
>
> Also needs locking.
>
> > +
> > + /* If plane not visible, dirty rect might have invalid coordinates */
> > + if (!plane_state->uapi.visible)
> > + continue;
>
> Seems like a redundant check.
>
> > +
> > + __intel_fbc_prepare_dirty_rect(plane, plane_state,
> > + intel_fbc_can_flip_nuke(state,
> > + crtc,
> > + plane));
> > + }
> > +}
> > +
> > static void intel_fbc_update_state(struct intel_atomic_state *state,
> > struct intel_crtc *crtc,
> > struct intel_plane *plane)
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> > index ceae55458e14..8f1b11c6f4d2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> > @@ -14,6 +14,7 @@ struct intel_atomic_state;
> > struct intel_crtc;
> > struct intel_crtc_state;
> > struct intel_display;
> > +struct intel_dsb;
> > struct intel_fbc;
> > struct intel_plane;
> > struct intel_plane_state;
> > @@ -48,5 +49,10 @@ 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_prepare_dirty_rect(struct intel_atomic_state *state,
> > + struct intel_crtc *crtc);
> > +void intel_fbc_dirty_rect_update_noarm(struct intel_dsb *dsb,
> > + struct intel_atomic_state *state,
> > + struct intel_crtc *crtc);
> >
> > #endif /* __INTEL_FBC_H__ */
> > --
> > 2.43.0
>
