On Wed, 22 Jan 2025, "Govindapillai, Vinod" <vinod.govindapillai@xxxxxxxxx> wrote:
> 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.
Try with:
scripts/checkpatch.pl -q --emacs --strict --show-types --max-line-length=100 --ignore=BIT_MACRO,SPLIT_STRING,LONG_LINE_STRING,BOOL_MEMBER
>
> 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__ */
>>
>
--
Jani Nikula, Intel
