| Message ID | 20250122093006.405711-6-vinod.govindapillai@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/xe3: FBC Dirty rect feature support | expand |
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. > + > 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. > 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__ */
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. 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__ */ >
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); + 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, } } - 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); + #endif /* __INTEL_FBC_H__ */
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(-)