[PATCH v2 10/13] drm/i915/vrr: Pause DC balancing for DSB commits
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Wed Apr 23 07:21:43 UTC 2025
On 4/21/2025 9:18 PM, Mitul Golani wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Pause the DMC DC balancing for the remainder of the
> commit so that vmin/vmax won't change after we've baked
> them into the DSB vblank evasion commands.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani at intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 13 +++++++
> drivers/gpu/drm/i915/display/intel_vrr.c | 38 +++++++++++++------
> drivers/gpu/drm/i915/display/intel_vrr_regs.h | 12 ++++++
> 3 files changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 33c09999c42e..274d01552ccf 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7194,6 +7194,17 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
> }
>
> if (new_crtc_state->use_dsb) {
> + /*
> + * Pause the DMC DC balancing for the remainder of the
> + * commit so that vmin/vmax won't change after we've baked
> + * them into the DSB vblank evasion commands.
> + *
> + * FIXME maybe need a small delay here to make sure DMC has
> + * finished updating the values? Or we need a better DMC<->driver
> + * protocol that gives is real guarantees about that...
> + */
> + intel_pipedmc_dcb_disable(NULL, crtc);
> +
> if (intel_crtc_needs_color_update(new_crtc_state))
> intel_color_commit_noarm(new_crtc_state->dsb_commit,
> new_crtc_state);
> @@ -7230,6 +7241,8 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
> intel_vrr_send_push(new_crtc_state->dsb_commit, new_crtc_state);
> intel_dsb_wait_vblank_delay(state, new_crtc_state->dsb_commit);
> intel_vrr_check_push_sent(new_crtc_state->dsb_commit, new_crtc_state);
> + if (new_crtc_state->vrr.dc_balance.enable)
> + intel_pipedmc_dcb_enable(new_crtc_state->dsb_commit, crtc);
> intel_dsb_interrupt(new_crtc_state->dsb_commit);
> }
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 383024dc2784..fb96d03bbf03 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -9,6 +9,7 @@
> #include "i915_reg.h"
> #include "intel_de.h"
> #include "intel_display_types.h"
> +#include "intel_dmc.h"
> #include "intel_dp.h"
> #include "intel_dmc_regs.h"
> #include "intel_vrr.h"
> @@ -598,7 +599,9 @@ bool intel_vrr_always_use_vrr_tg(struct intel_display *display)
> void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
> {
> struct intel_display *display = to_intel_display(crtc_state);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> + u32 ctl;
>
> if (!crtc_state->vrr.enable)
> return;
> @@ -609,20 +612,19 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
> crtc_state->vrr.vmax - 1);
> intel_de_write(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder),
> crtc_state->vrr.flipline - 1);
> + if (!intel_vrr_always_use_vrr_tg(display))
> + intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), TRANS_PUSH_EN);
This doesn't make any sense. Perhaps a typo.
>
> intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
> TRANS_PUSH_EN);
>
> - if (!intel_vrr_always_use_vrr_tg(display)) {
> - if (crtc_state->cmrr.enable) {
> - intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> - VRR_CTL_VRR_ENABLE | VRR_CTL_CMRR_ENABLE |
> - trans_vrr_ctl(crtc_state));
> - } else {
> - intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> - VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
> - }
> - }
> + ctl = VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state);
> + if (crtc_state->cmrr.enable)
> + ctl |= VRR_CTL_CMRR_ENABLE;
> + if (crtc_state->vrr.dc_balance.enable)
> + ctl |= VRR_CTL_DCB_ADJ_ENABLE;
This seems to be doing couple of things at a time. Is it possible to
break this into 2 patches?
Can the setting of VRR_CTL_DCB_ADJ_ENABLE bit and setting
ADAPTIVE_SYNC_COUNTER_EN in reg TRANS_ADAPTIVE_SYNC_DCB_CTL be a
separate patch?
Regards,
Ankit
> +
> + intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), ctl);
>
> if (crtc_state->vrr.dc_balance.enable && HAS_DC_BALANCE(display)) {
> intel_de_write(display, PIPEDMC_DCB_VMIN(display, cpu_transcoder),
> @@ -639,18 +641,26 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
> crtc_state->vrr.dc_balance.slope);
> intel_de_write(display, PIPEDMC_DCB_VBLANK(display, cpu_transcoder),
> crtc_state->vrr.dc_balance.vblank_target);
> + /* FIXME reset counters? */
> + intel_de_write(display, TRANS_ADAPTIVE_SYNC_DCB_CTL(display, cpu_transcoder),
> + ADAPTIVE_SYNC_COUNTER_EN);
> + /* FIMXE configure pipedmc DC balance parameters somewhere */
> + intel_pipedmc_dcb_enable(NULL, crtc);
> }
> }
>
> void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
> {
> struct intel_display *display = to_intel_display(old_crtc_state);
> + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
> + u32 ctl;
>
> if (!old_crtc_state->vrr.enable)
> return;
>
> if (old_crtc_state->vrr.dc_balance.enable && HAS_DC_BALANCE(display)) {
> + intel_pipedmc_dcb_disable(NULL, crtc);
> intel_de_write(display, TRANS_ADAPTIVE_SYNC_DCB_CTL(display, cpu_transcoder), 0);
> intel_de_write(display, PIPEDMC_DCB_VMIN(display, cpu_transcoder), 0);
> intel_de_write(display, PIPEDMC_DCB_VMAX(display, cpu_transcoder), 0);
> @@ -661,9 +671,13 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
> intel_de_write(display, PIPEDMC_DCB_VBLANK(display, cpu_transcoder), 0);
> }
>
> + ctl = trans_vrr_ctl(old_crtc_state);
> + if (intel_vrr_always_use_vrr_tg(display))
> + ctl |= VRR_CTL_VRR_ENABLE;
> +
> + intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), ctl);
> +
> if (!intel_vrr_always_use_vrr_tg(display)) {
> - intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> - trans_vrr_ctl(old_crtc_state));
> intel_de_wait_for_clear(display,
> TRANS_VRR_STATUS(display, cpu_transcoder),
> VRR_STATUS_VRR_EN_LIVE, 1000);
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr_regs.h b/drivers/gpu/drm/i915/display/intel_vrr_regs.h
> index 2db477325c83..a88fcd69111f 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vrr_regs.h
> @@ -63,6 +63,17 @@
> trans, \
> _TRANS_ADAPTIVE_SYNC_DCB_CTL_A)
>
> +#define _TRANS_ADAPTIVE_SYNC_DCB_CTL_A 0x604C0
> +#define _TRANS_ADAPTIVE_SYNC_DCB_CTL_B 0x614C0
> +#define _TRANS_ADAPTIVE_SYNC_DCB_CTL_C 0x624C0
> +#define _TRANS_ADAPTIVE_SYNC_DCB_CTL_D 0x634C0
> +#define _TRANS_ADAPTIVE_SYNC_DCB_CTL_E 0x6B4C0
> +#define _TRANS_ADAPTIVE_SYNC_DCB_CTL_F 0x6C4C0
> +#define TRANS_ADAPTIVE_SYNC_DCB_CTL(dev_priv, trans) _MMIO_TRANS2(dev_priv, \
> + trans, \
> + _TRANS_ADAPTIVE_SYNC_DCB_CTL_A)
> +#define ADAPTIVE_SYNC_COUNTER_EN REG_BIT(31)
> +
> #define _TRANS_VRR_CTL_A 0x60420
> #define _TRANS_VRR_CTL_B 0x61420
> #define _TRANS_VRR_CTL_C 0x62420
> @@ -71,6 +82,7 @@
> #define VRR_CTL_VRR_ENABLE REG_BIT(31)
> #define VRR_CTL_IGN_MAX_SHIFT REG_BIT(30)
> #define VRR_CTL_FLIP_LINE_EN REG_BIT(29)
> +#define VRR_CTL_DCB_ADJ_ENABLE REG_BIT(28)
> #define VRR_CTL_PIPELINE_FULL_MASK REG_GENMASK(10, 3)
> #define VRR_CTL_PIPELINE_FULL(x) REG_FIELD_PREP(VRR_CTL_PIPELINE_FULL_MASK, (x))
> #define VRR_CTL_PIPELINE_FULL_OVERRIDE REG_BIT(0)
More information about the Intel-xe
mailing list