[RFC PATCH 3/4] drm/i915/xe3: add dirty rect support for FBC
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Nov 22 08:31:23 UTC 2024
On Tue, Nov 19, 2024 at 01:53:24AM +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.
>
> Bspec: 71675, 73424
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai at intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_fbc.c | 130 ++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_fbc.h | 3 +
> .../drm/i915/display/skl_universal_plane.c | 2 +
> 3 files changed, 135 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 1fdb1a3e3fbe..01080171790f 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -42,6 +42,7 @@
> #include <linux/string_helpers.h>
>
> #include <drm/drm_blend.h>
> +#include <drm/drm_damage_helper.h>
> #include <drm/drm_fourcc.h>
>
> #include "gem/i915_gem_stolen.h"
> @@ -58,6 +59,7 @@
> #include "intel_display_trace.h"
> #include "intel_display_types.h"
> #include "intel_display_wa.h"
> +#include "intel_dsb.h"
> #include "intel_fbc.h"
> #include "intel_fbc_regs.h"
> #include "intel_frontbuffer.h"
> @@ -126,6 +128,8 @@ struct intel_fbc {
> */
> struct intel_fbc_state state;
> const char *no_fbc_reason;
> +
> + struct drm_rect dirty_rect;
That belongs in the fbc state.
> };
>
> /* plane stride in pixels */
> @@ -670,6 +674,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 (DISPLAY_VER(display) >= 30)
> + 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);
> }
> @@ -1664,6 +1672,113 @@ void intel_fbc_flush(struct drm_i915_private *i915,
> __intel_fbc_flush(fbc, frontbuffer_bits, origin);
> }
>
> +void
> +intel_fbc_program_dirty_rect(struct intel_dsb *dsb, struct intel_plane *plane)
> +{
> + struct intel_display *display = to_intel_display(plane);
> + struct intel_fbc *fbc = plane->fbc;
> +
> + if (DISPLAY_VER(display) < 30)
> + return;
> +
> + if (!fbc)
> + return;
> +
> + 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));
I suspect the end line needs a -1. But that needs to be confirmed on
actual hardware.
> +}
> +
> +static bool
> +intel_fbc_need_full_region_update(struct intel_plane_state *old_plane_state,
> + struct intel_plane_state *new_plane_state)
> +{
> + const struct drm_framebuffer *old_fb = old_plane_state->hw.fb;
> + const struct drm_framebuffer *new_fb = new_plane_state->hw.fb;
> +
> + if (!old_fb || !new_fb)
> + return true;
> +
> + if (old_fb->format->format != new_fb->format->format)
> + return true;
> +
> + if (old_fb->modifier != new_fb->modifier)
> + return true;
> +
> + if (intel_fbc_plane_stride(old_plane_state) !=
> + intel_fbc_plane_stride(new_plane_state))
> + return true;
> +
> + if (intel_fbc_cfb_stride(old_plane_state) !=
> + intel_fbc_cfb_stride(new_plane_state))
> + return true;
> +
> + if (intel_fbc_cfb_size(old_plane_state) !=
> + intel_fbc_cfb_size(new_plane_state))
> + return true;
> +
> + return false;
> +}
If any of those fail then I think we're going to be
disabling+re-enabling FBC anyway. So we might not need to check
any of this, or if we do we could perhaps just use can_flip_nuke().
> +
> +static void
> +update_dirty_rect_to_full_region(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;
> +
> + dirty_rect->y1 = y_offset;
> + dirty_rect->y2 = y_offset + plane_height;
> +}
> +
> +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;
> +
> + dirty_rect->y1 = clamp(dirty_rect->y1, y_offset, max_endline);
> + dirty_rect->y2 = clamp(dirty_rect->y2, dirty_rect->y1, max_endline);
> +}
> +
> +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,
> + bool need_full_region_update)
> +{
> + struct intel_display *display = to_intel_display(plane);
> + struct intel_fbc *fbc = plane->fbc;
> + struct drm_rect *fbc_dirty_rect = &fbc->dirty_rect;
> +
> + if (need_full_region_update) {
> + drm_dbg_kms(display->drm,
> + "[PLANE:%d:%s] Full region update needed\n",
> + plane->base.base.id, plane->base.name);
> + update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
> + goto out;
> + }
> +
> + if (drm_atomic_helper_damage_merged(&old_plane_state->uapi,
> + &new_plane_state->uapi,
> + fbc_dirty_rect)) {
> + validate_and_clip_dirty_rect(new_plane_state, fbc_dirty_rect);
Your fbc_dirty_rect here will still be in a different coordinate space
(fb relative) than the plane src coordinates (PLANE_SURF relative).
> + } else {
> + drm_dbg_kms(display->drm,
> + "[PLANE:%d:%s] Damage clips merge cal failed. Use full region\n",
> + plane->base.base.id, plane->base.name);
> +
That's going to create excessive spam.
> + /* TODO! if the drm call failed, update full region? */
> + update_dirty_rect_to_full_region(new_plane_state, fbc_dirty_rect);
> + }
> +
> +out:
> + drm_dbg_kms(display->drm,
> + "[PLANE:%d:%s] Dirty rect start line: %d End line: %d\n",
> + plane->base.base.id, plane->base.name, fbc_dirty_rect->y1,
> + fbc_dirty_rect->y2);a
ditto
> +}
> +
> int intel_fbc_atomic_check(struct intel_atomic_state *state)
> {
> struct intel_plane_state __maybe_unused *new_plane_state;
> @@ -1673,11 +1788,26 @@ int intel_fbc_atomic_check(struct intel_atomic_state *state)
>
> for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
> new_plane_state, i) {
> + bool full_region_update;
> int ret;
>
> + if (!plane->fbc)
> + continue;
> +
> ret = intel_fbc_check_plane(state, plane);
> if (ret)
> return ret;
> +
> + if (!new_plane_state->no_fbc_reason)
> + continue;
> +
> + full_region_update =
> + intel_fbc_need_full_region_update(old_plane_state,
> + new_plane_state);
> +
> + intel_fbc_compute_dirty_rect(plane, old_plane_state,
> + new_plane_state,
> + full_region_update);
That belongs in intel_fbc_update_state()
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> index ceae55458e14..073d671ea94d 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,7 @@ 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_program_dirty_rect(struct intel_dsb *dsb,
> + struct intel_plane *plane);
>
> #endif /* __INTEL_FBC_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4c7bcf6806ff..fd3611323ec0 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1510,6 +1510,8 @@ icl_plane_update_noarm(struct intel_dsb *dsb,
> icl_plane_csc_load_black(dsb, plane, crtc_state);
>
> icl_plane_update_sel_fetch_noarm(dsb, plane, crtc_state, plane_state, color_plane);
> +
> + intel_fbc_program_dirty_rect(dsb, plane);
I think we want a completely separate call to FBC around where call
intel_crtc_planes_update_arm().
It's going to be a bit dodgy becase we won't be able take fbc->lock
and thus accessing the fbc state needs to be done locklessly. But
I *think* it should be safe because the fbc state should only be
updated from intel_fbc_{update,disable}() which are done from the
atomic commit path only.
We are going to need some real tests for this:
- update region A -> flip with dirty rect A -> update region B -> flip with dirty rect B
- update region A -> flip with dirty rect A -> update region B -> flip without dirty rect
- update region A -> flip with dirty rect A -> update region B -> dirtyfb
- probably some other ones which involve format changes/etc. as well.
> }
>
> static void icl_plane_update_sel_fetch_arm(struct intel_dsb *dsb,
> --
> 2.34.1
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list