On Mon, Sep 18, 2023 at 09:25:05PM +0300, Imre Deak wrote:
> At the moment a modeset fails if the config computation of a pipe can't
> fit its required BW to the available link BW even though the limitation
> may be resolved by reducing the BW requirement of other pipes.
>
> To improve the above this patch adds helper functions checking the
> overall BW limits after all CRTC states have been computed. If the check
> fails the maximum link bpp for a selected pipe will be reduced and all
> the CRTC states will be recomputed until either the overall BW limit
> check passes, or further bpp reduction is not possible (because all
> pipes/encoders sharing the link BW reached their minimum link bpp).
>
> Atm, the MST encoder allocates twice the required BW for YUV420 format
> streams. A follow-up patchset will fix that, add a code comment about
> this.
>
> This change prepares for upcoming patches enabling the above BW
> management on FDI and MST links.
>
> v2:
> - Rename intel_crtc_state::max_link_bpp to max_link_bpp_x16 and
> intel_link_bw_limits::max_bpp to max_bpp_x16. (Jani)
> v3:
> - Add the helper functions in a separate patch. (Ville)
> - Add the functions to intel_link_bw.c instead of intel_atomic.c (Ville)
> - Return -ENOSPC instead of -EINVAL to userspace in case of a link BW
> limit failure.
> v4:
> - Make intel_atomic_check_config() static.
> v5: (Ville)
> - Rename intel_link_bw_limits::min_bpp_pipes to min_bpp_reached_pipes
> and intel_link_bw_reset_pipe_limit_to_min() to
> intel_link_bw_set_min_bpp_for_pipe().
> - Rename pipe_bpp to link_bpp in intel_link_bw_reduce_bpp().
> - Add FIXME: comment about MST encoder's YUV420 BW allocation and
> tracking the link bpp limit accordingly.
>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/display/intel_crtc.c | 1 +
> drivers/gpu/drm/i915/display/intel_display.c | 65 ++++-
> .../drm/i915/display/intel_display_types.h | 3 +-
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 +
> drivers/gpu/drm/i915/display/intel_link_bw.c | 232 ++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_link_bw.h | 38 +++
> 7 files changed, 339 insertions(+), 5 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/display/intel_link_bw.c
> create mode 100644 drivers/gpu/drm/i915/display/intel_link_bw.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 1b2e02e9d92cb..de4967c141f00 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -268,6 +268,7 @@ i915-y += \
> display/intel_hotplug.o \
> display/intel_hotplug_irq.o \
> display/intel_hti.o \
> + display/intel_link_bw.o \
> display/intel_load_detect.o \
> display/intel_lpe_audio.o \
> display/intel_modeset_lock.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 182c6dd64f47c..1eda6a9f19aa8 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -175,6 +175,7 @@ void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
> crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> crtc_state->scaler_state.scaler_id = -1;
> crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> + crtc_state->max_link_bpp_x16 = INT_MAX;
> }
>
> static struct intel_crtc *intel_crtc_alloc(void)
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 3bdc338a22e19..537884035304c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -87,6 +87,7 @@
> #include "intel_frontbuffer.h"
> #include "intel_hdmi.h"
> #include "intel_hotplug.h"
> +#include "intel_link_bw.h"
> #include "intel_lvds.h"
> #include "intel_lvds_regs.h"
> #include "intel_modeset_setup.h"
> @@ -4596,7 +4597,8 @@ intel_crtc_prepare_cleared_state(struct intel_atomic_state *state,
>
> static int
> intel_modeset_pipe_config(struct intel_atomic_state *state,
> - struct intel_crtc *crtc)
> + struct intel_crtc *crtc,
> + const struct intel_link_bw_limits *limits)
> {
> struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> struct intel_crtc_state *crtc_state =
> @@ -4628,6 +4630,15 @@ intel_modeset_pipe_config(struct intel_atomic_state *state,
> if (ret)
> return ret;
>
> + crtc_state->max_link_bpp_x16 = limits->max_bpp_x16[crtc->pipe];
> +
> + if (crtc_state->pipe_bpp > to_bpp_int(crtc_state->max_link_bpp_x16)) {
> + drm_dbg_kms(&i915->drm,
> + "[CRTC:%d:%s] Link bpp limited to " BPP_X16_FMT "\n",
> + crtc->base.base.id, crtc->base.name,
> + BPP_X16_ARGS(crtc_state->max_link_bpp_x16));
> + }
> +
> base_bpp = crtc_state->pipe_bpp;
>
> /*
> @@ -6218,7 +6229,9 @@ static int intel_bigjoiner_add_affected_crtcs(struct intel_atomic_state *state)
> return 0;
> }
>
> -static int intel_atomic_check_config(struct intel_atomic_state *state)
> +static int intel_atomic_check_config(struct intel_atomic_state *state,
> + struct intel_link_bw_limits *limits,
> + enum pipe *failed_pipe)
> {
> struct drm_i915_private *i915 = to_i915(state->base.dev);
> struct intel_crtc_state *new_crtc_state;
> @@ -6226,6 +6239,8 @@ static int intel_atomic_check_config(struct intel_atomic_state *state)
> int ret;
> int i;
>
> + *failed_pipe = INVALID_PIPE;
> +
> ret = intel_bigjoiner_add_affected_crtcs(state);
> if (ret)
> return ret;
> @@ -6251,7 +6266,7 @@ static int intel_atomic_check_config(struct intel_atomic_state *state)
> if (!new_crtc_state->hw.enable)
> continue;
>
> - ret = intel_modeset_pipe_config(state, crtc);
> + ret = intel_modeset_pipe_config(state, crtc, limits);
> if (ret)
> break;
>
> @@ -6260,9 +6275,51 @@ static int intel_atomic_check_config(struct intel_atomic_state *state)
> break;
> }
>
> + if (ret)
> + *failed_pipe = crtc->pipe;
> +
> return ret;
> }
>
> +static int intel_atomic_check_config_and_link(struct intel_atomic_state *state)
> +{
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> + struct intel_link_bw_limits new_limits;
> + struct intel_link_bw_limits old_limits;
> + int ret;
> +
> + intel_link_bw_init_limits(i915, &new_limits);
> + old_limits = new_limits;
> +
> + while (true) {
> + enum pipe failed_pipe;
> +
> + ret = intel_atomic_check_config(state, &new_limits,
> + &failed_pipe);
> + if (ret) {
> + /*
> + * The bpp limit for a pipe is below the minimum it supports, set the
> + * limit to the minimum and recalculate the config.
> + */
> + if (ret == -EINVAL &&
> + intel_link_bw_set_min_bpp_for_pipe(state,
> + &old_limits,
> + &new_limits,
> + failed_pipe))
> + continue;
> +
> + break;
> + }
> +
> + old_limits = new_limits;
> +
> + ret = intel_link_bw_atomic_check(state, &new_limits);
> + if (ret != -EAGAIN)
> + break;
> + }
> +
> + return ret;
> +}
> /**
> * intel_atomic_check - validate state object
> * @dev: drm device
> @@ -6307,7 +6364,7 @@ int intel_atomic_check(struct drm_device *dev,
> return ret;
> }
>
> - ret = intel_atomic_check_config(state);
> + ret = intel_atomic_check_config_and_link(state);
> if (ret)
> goto fail;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 50a22261f5eec..4d7948fa4bbba 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1189,7 +1189,8 @@ struct intel_crtc_state {
> u32 ctrl, div;
> } dsi_pll;
>
> - int pipe_bpp;
> + int max_link_bpp_x16; /* in 1/16 bpp units */
> + int pipe_bpp; /* in 1 bpp units */
> struct intel_link_m_n dp_m_n;
>
> /* m2_n2 for eDP downclock */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index a914d83ab3dde..f26c2eecb2778 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -157,6 +157,10 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> int slots = -EINVAL;
> int link_bpp;
>
> + /*
> + * FIXME: allocate the BW according to link_bpp, which in the case of
> + * YUV420 is only half of the pipe bpp value.
> + */
> slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state,
> to_bpp_int(limits->link.max_bpp_x16),
> to_bpp_int(limits->link.min_bpp_x16),
> diff --git a/drivers/gpu/drm/i915/display/intel_link_bw.c b/drivers/gpu/drm/i915/display/intel_link_bw.c
> new file mode 100644
> index 0000000000000..9b6d0891345d7
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_link_bw.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +
> +#include "intel_atomic.h"
> +#include "intel_display_types.h"
> +#include "intel_dp_mst.h"
> +#include "intel_fdi.h"
> +#include "intel_link_bw.h"
> +
> +/**
> + * intel_link_bw_init_limits - initialize BW limits
> + * @i915: device instance
> + * @limits: link BW limits
> + *
> + * Initialize @limits.
> + */
> +void intel_link_bw_init_limits(struct drm_i915_private *i915, struct intel_link_bw_limits *limits)
> +{
> + enum pipe pipe;
> +
> + limits->min_bpp_reached_pipes = 0;
> + for_each_pipe(i915, pipe)
> + limits->max_bpp_x16[pipe] = INT_MAX;
> +}
> +
> +/**
> + * intel_link_bw_compute_pipe_bpp - compute pipe bpp limited by max link bpp
> + * @crtc_state: the crtc state
> + *
> + * Compute the pipe bpp limited by the CRTC's maximum link bpp. Encoders can
> + * call this function during state computation in the simple case where the
> + * link bpp will always match the pipe bpp. This is the case for all non-DP
> + * encoders, while DP encoders will use a link bpp lower than pipe bpp in case
> + * of DSC compression.
> + *
> + * Returns %true in case of success, %false if pipe bpp would need to be
> + * reduced below its valid range.
> + */
> +bool intel_link_bw_compute_pipe_bpp(struct intel_crtc_state *crtc_state)
> +{
> + int pipe_bpp = min(crtc_state->pipe_bpp,
> + to_bpp_int(crtc_state->max_link_bpp_x16));
> +
> + pipe_bpp = rounddown(pipe_bpp, 2 * 3);
> +
> + if (pipe_bpp < 6 * 3)
> + return false;
> +
> + crtc_state->pipe_bpp = pipe_bpp;
> +
> + return true;
> +}
Hmm. The fact that you can't use this for DP makes this rather
non-generic. Maybe it should just live in intel_fdi.c or something?
Or are we going to use it for something else as well?
> +
> +/**
> + * intel_link_bw_reduce_bpp - reduce maximum link bpp for a selected pipe
> + * @state: atomic state
> + * @limits: link BW limits
> + * @pipe_mask: mask of pipes to select from
> + * @reason: explanation of why bpp reduction is needed
> + *
> + * Select the pipe from @pipe_mask with the biggest link bpp value and set the
> + * maximum of link bpp in @limits below this value. Modeset the selected pipe,
> + * so that its state will get recomputed.
> + *
> + * This function can be called to resolve a link's BW overallocation by reducing
> + * the link bpp of one pipe on the link and hence reducing the total link BW.
> + *
> + * Returns
> + * - 0 in case of success
> + * - %-ENOSPC if no pipe can further reduce its link bpp
> + * - Other negative error, if modesetting the selected pipe failed
> + */
> +int intel_link_bw_reduce_bpp(struct intel_atomic_state *state,
> + struct intel_link_bw_limits *limits,
> + u8 pipe_mask,
> + const char *reason)
> +{
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> + enum pipe max_bpp_pipe = INVALID_PIPE;
> + struct intel_crtc *crtc;
> + int max_bpp = 0;
> +
> + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) {
> + struct intel_crtc_state *crtc_state;
> + int link_bpp;
> +
> + if (limits->min_bpp_reached_pipes & BIT(crtc->pipe))
> + continue;
> +
> + crtc_state = intel_atomic_get_crtc_state(&state->base,
> + crtc);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> +
> + if (crtc_state->dsc.compression_enable)
> + link_bpp = crtc_state->dsc.compressed_bpp;
> + else
> + /*
> + * TODO: for YUV420 the actual link bpp is only half
> + * of the pipe bpp value. The MST encoder's BW allocation
> + * is based on the pipe bpp value, set the actual link bpp
> + * limit here once the MST BW allocation is fixed.
> + */
> + link_bpp = crtc_state->pipe_bpp;
Not quite sure how we should handle all this in the end. IIRC the current
SST logic will attempt both 4:4:4 and 4:2:0 immediately. Dunno if that
makes sense or if we should try to stick to 4:4:4 a bit more aggressively
before allowing 4:2:0 fallback. But I guess that's more or less the
same kind of problem as the DSC vs. no DSC issue.
I was also pondering if the respect_downstream_limits stuff could
also matter here, but I think we use that just for some TMDS link
stuff so there should be no wider implications from it.
> +
> + if (link_bpp > max_bpp) {
> + max_bpp = link_bpp;
> + max_bpp_pipe = crtc->pipe;
> + }
> + }
> +
> + if (max_bpp_pipe == INVALID_PIPE)
> + return -ENOSPC;
> +
> + limits->max_bpp_x16[max_bpp_pipe] = to_bpp_x16(max_bpp) - 1;
> +
> + return intel_modeset_pipes_in_mask_early(state, reason,
> + BIT(max_bpp_pipe));
> +}
> +
> +/**
> + * intel_link_bw_set_min_bpp_for_pipe - set link bpp limit for a pipe to its minimum
> + * @state: atomic state
> + * @old_limits: link BW limits
> + * @new_limits: link BW limits
> + * @pipe: pipe
> + *
> + * Set the link bpp limit for @pipe in @new_limits to its value in
> + * @old_limits and mark this limit as the minimum. This function must be
> + * called after a pipe's compute config function failed, @old_limits
> + * containing the bpp limit with which compute config previously passed.
> + *
> + * The function will fail if setting a minimum is not possible, either
> + * because the old and new limits match (and so would lead to a pipe compute
> + * config failure) or the limit is already at the minimum.
> + *
> + * Returns %true in case of success.
> + */
> +bool
> +intel_link_bw_set_min_bpp_for_pipe(struct intel_atomic_state *state,
> + const struct intel_link_bw_limits *old_limits,
> + struct intel_link_bw_limits *new_limits,
> + enum pipe pipe)
> +{
> + if (pipe == INVALID_PIPE)
> + return false;
> +
> + if (new_limits->min_bpp_reached_pipes & BIT(pipe))
> + return false;
I suppose this check is a bit redundant and it should also be
caught by the == check below. But I suppose no harm in having this
too.
The naming is still bugging me though. The "min_bpp" might end up
being a bit confusing since we have a min_bpp member in the limits
but we're setting the max_bpp here. Maybe s/min_bpp/bpp_limit/ or
something like that would be a bit better?
> +
> + if (new_limits->max_bpp_x16[pipe] ==
> + old_limits->max_bpp_x16[pipe])
> + return false;
> +
> + new_limits->max_bpp_x16[pipe] =
> + old_limits->max_bpp_x16[pipe];
> + new_limits->min_bpp_reached_pipes |= BIT(pipe);
> +
> + return true;
> +}
> +
> +static int check_all_link_config(struct intel_atomic_state *state,
> + struct intel_link_bw_limits *limits)
> +{
> + /* TODO: Check all shared display link configurations like FDI */
> + return 0;
> +}
> +
> +static bool
> +assert_link_limit_change_valid(struct drm_i915_private *i915,
> + const struct intel_link_bw_limits *old_limits,
> + const struct intel_link_bw_limits *new_limits)
> +{
> + bool bpps_changed = false;
> + enum pipe pipe;
> +
> + for_each_pipe(i915, pipe) {
> + /* The bpp limit can only decrease. */
> + if (drm_WARN_ON(&i915->drm,
> + new_limits->max_bpp_x16[pipe] >
> + old_limits->max_bpp_x16[pipe]))
> + return false;
> +
> + if (new_limits->max_bpp_x16[pipe] <
> + old_limits->max_bpp_x16[pipe])
> + bpps_changed = true;
> + }
> +
> + /* At least one limit must change. */
> + if (drm_WARN_ON(&i915->drm,
> + !bpps_changed))
> + return false;
> +
> + return true;
> +}
> +
> +/**
> + * intel_link_bw_atomic_check - check display link states and set a fallback config if needed
> + * @state: atomic state
> + * @new_limits: link BW limits
> + *
> + * Check the configuration of all shared display links in @state and set new BW
> + * limits in @new_limits if there is a BW limitation.
> + *
> + * Returns:
> + * - 0 if the confugration is valid
> + * - %-EAGAIN, if the configuration is invalid and @new_limits got updated
> + * with fallback values with which the configuration of all CRTCs
> + * in @state must be recomputed
> + * - Other negative error, if the configuration is invalid without a
> + * fallback possibility, or the check failed for another reason
> + */
> +int intel_link_bw_atomic_check(struct intel_atomic_state *state,
> + struct intel_link_bw_limits *new_limits)
> +{
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> + struct intel_link_bw_limits old_limits = *new_limits;
> + int ret;
> +
> + ret = check_all_link_config(state, new_limits);
> + if (ret != -EAGAIN)
> + return ret;
> +
> + if (!assert_link_limit_change_valid(i915, &old_limits, new_limits))
> + return -EINVAL;
> +
> + return -EAGAIN;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_link_bw.h b/drivers/gpu/drm/i915/display/intel_link_bw.h
> new file mode 100644
> index 0000000000000..0f666c9712f3c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_link_bw.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __INTEL_LINK_BW_H__
> +#define __INTEL_LINK_BW_H__
> +
> +#include <linux/types.h>
> +
> +#include "intel_display_limits.h"
> +
> +struct drm_i915_private;
> +
> +struct intel_atomic_state;
> +struct intel_crtc_state;
> +
> +struct intel_link_bw_limits {
> + u8 min_bpp_reached_pipes;
> + /* in 1/16 bpp units */
> + int max_bpp_x16[I915_MAX_PIPES];
> +};
> +
> +void intel_link_bw_init_limits(struct drm_i915_private *i915,
> + struct intel_link_bw_limits *limits);
> +bool intel_link_bw_compute_pipe_bpp(struct intel_crtc_state *crtc_state);
> +int intel_link_bw_reduce_bpp(struct intel_atomic_state *state,
> + struct intel_link_bw_limits *limits,
> + u8 pipe_mask,
> + const char *reason);
> +bool intel_link_bw_set_min_bpp_for_pipe(struct intel_atomic_state *state,
> + const struct intel_link_bw_limits *old_limits,
> + struct intel_link_bw_limits *new_limits,
> + enum pipe pipe);
> +int intel_link_bw_atomic_check(struct intel_atomic_state *state,
> + struct intel_link_bw_limits *new_limits);
> +
> +#endif
> --
> 2.37.2
--
Ville Syrjälä
Intel
