On Tue, Sep 19, 2023 at 01:52:11PM +0300, Imre Deak wrote:
> At the moment modesetting a stream CRTC will fail if the stream's BW
> along with the current BW of all the other streams on the same MST link
> is above the total BW of the MST link. Make the BW sharing more dynamic
> by trying to reduce the link bpp of one or more streams on the MST link
> in this case.
>
> When selecting a stream to reduce the BW for, take into account which
> link segment in the MST topology ran out of BW and which streams go
> through this link segment. For instance with A,B,C streams in the same
> MST topology A and B may share the BW of a link segment downstream of a
> branch device, stream C not downstream of the branch device, hence not
> affecting this BW. If this link segment's BW runs out one or both of
> stream A/B's BW will be reduced until their total BW is within limits.
>
> While reducing the link bpp for a given stream DSC may need to be
> enabled for it, which requires FEC on the whole MST link. Check for this
> condition and recompute the state for all streams taking the FEC
> overhead into account (on 8b/10b links).
>
> v2:
> - Rebase on s/min_bpp_pipes/min_bpp_reached_pipes/ change.
>
> Signed-off-by: Imre Deak <imre.d...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 5 +-
> drivers/gpu/drm/i915/display/intel_dp.c | 13 +-
> drivers/gpu/drm/i915/display/intel_dp.h | 2 +
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 129 +++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_dp_mst.h | 3 +
> drivers/gpu/drm/i915/display/intel_link_bw.c | 15 ++-
> drivers/gpu/drm/i915/display/intel_link_bw.h | 1 +
> 7 files changed, 159 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 8af22cf9a49de..565a6f20ffbfd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4629,6 +4629,7 @@ intel_modeset_pipe_config(struct intel_atomic_state
> *state,
> if (ret)
> return ret;
>
> + crtc_state->fec_enable = limits->force_fec_pipes & BIT(crtc->pipe);
> 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)) {
> @@ -6435,10 +6436,6 @@ int intel_atomic_check(struct drm_device *dev,
> goto fail;
> }
>
> - ret = drm_dp_mst_atomic_check(&state->base);
> - if (ret)
> - goto fail;
> -
> ret = intel_atomic_check_planes(state);
> if (ret)
> goto fail;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 3d2ede31aa4e8..9b5070d8c8984 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1369,8 +1369,8 @@ static bool intel_dp_source_supports_fec(struct
> intel_dp *intel_dp,
> return false;
> }
>
> -static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
> - const struct intel_crtc_state *pipe_config)
> +bool intel_dp_supports_fec(struct intel_dp *intel_dp,
> + const struct intel_crtc_state *pipe_config)
> {
> return intel_dp_source_supports_fec(intel_dp, pipe_config) &&
> drm_dp_sink_supports_fec(intel_dp->fec_capable);
> @@ -2111,8 +2111,9 @@ int intel_dp_dsc_compute_config(struct intel_dp
> *intel_dp,
> &pipe_config->hw.adjusted_mode;
> int ret;
>
> - pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
> - intel_dp_supports_fec(intel_dp, pipe_config);
> + pipe_config->fec_enable = pipe_config->fec_enable ||
> + (!intel_dp_is_edp(intel_dp) &&
> + intel_dp_supports_fec(intel_dp, pipe_config));
>
> if (!intel_dp_supports_dsc(intel_dp, pipe_config))
> return -EINVAL;
> @@ -2308,6 +2309,10 @@ intel_dp_compute_link_config(struct intel_encoder
> *encoder,
> bool dsc_needed;
> int ret = 0;
>
> + if (pipe_config->fec_enable &&
> + !intel_dp_supports_fec(intel_dp, pipe_config))
> + return -EINVAL;
I wonder, could we just check that, when
we are actually setting fec_enable to true, then
we wouldn't have to care about this here.
Otherwise, with this clarified
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovs...@intel.com>
> +
> if (intel_dp_need_bigjoiner(intel_dp, adjusted_mode->crtc_hdisplay,
> adjusted_mode->crtc_clock))
> pipe_config->bigjoiner_pipes = GENMASK(crtc->pipe + 1,
> crtc->pipe);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> b/drivers/gpu/drm/i915/display/intel_dp.h
> index 2cf3681bac64a..612105a303419 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -135,6 +135,8 @@ static inline unsigned int intel_dp_unused_lane_mask(int
> lane_count)
> return ~((1 << lane_count) - 1) & 0xf;
> }
>
> +bool intel_dp_supports_fec(struct intel_dp *intel_dp,
> + const struct intel_crtc_state *pipe_config);
> u32 intel_dp_mode_to_fec_clock(u32 mode_clock);
> u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp,
> u32 pipe_bpp);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 9681708acaa1a..ce69122fb4e54 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -43,6 +43,7 @@
> #include "intel_dpio_phy.h"
> #include "intel_hdcp.h"
> #include "intel_hotplug.h"
> +#include "intel_link_bw.h"
> #include "intel_vdsc.h"
> #include "skl_scaler.h"
>
> @@ -373,6 +374,10 @@ static int intel_dp_mst_compute_config(struct
> intel_encoder *encoder,
> bool dsc_needed;
> int ret = 0;
>
> + if (pipe_config->fec_enable &&
> + !intel_dp_supports_fec(intel_dp, pipe_config))
> + return -EINVAL;
> +
> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
> return -EINVAL;
>
> @@ -492,6 +497,130 @@ intel_dp_mst_transcoder_mask(struct intel_atomic_state
> *state,
> return transcoders;
> }
>
> +static u8 get_pipes_downstream_of_mst_port(struct intel_atomic_state *state,
> + struct drm_dp_mst_topology_mgr
> *mst_mgr,
> + struct drm_dp_mst_port *parent_port)
> +{
> + const struct intel_digital_connector_state *conn_state;
> + struct intel_connector *connector;
> + u8 mask = 0;
> + int i;
> +
> + for_each_new_intel_connector_in_state(state, connector, conn_state, i) {
> + if (!conn_state->base.crtc)
> + continue;
> +
> + if (&connector->mst_port->mst_mgr != mst_mgr)
> + continue;
> +
> + if (connector->port != parent_port &&
> + !drm_dp_mst_port_downstream_of_parent(mst_mgr,
> + connector->port,
> + parent_port))
> + continue;
> +
> + mask |= BIT(to_intel_crtc(conn_state->base.crtc)->pipe);
> + }
> +
> + return mask;
> +}
> +
> +static int intel_dp_mst_check_fec_change(struct intel_atomic_state *state,
> + struct drm_dp_mst_topology_mgr
> *mst_mgr,
> + struct intel_link_bw_limits *limits)
> +{
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> + struct intel_crtc *crtc;
> + u8 mst_pipe_mask;
> + u8 fec_pipe_mask = 0;
> + int ret;
> +
> + mst_pipe_mask = get_pipes_downstream_of_mst_port(state, mst_mgr, NULL);
> +
> + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, mst_pipe_mask) {
> + struct intel_crtc_state *crtc_state =
> + intel_atomic_get_new_crtc_state(state, crtc);
> +
> + /* Atomic connector check should've added all the MST CRTCs. */
> + if (drm_WARN_ON(&i915->drm, !crtc_state))
> + return -EINVAL;
> +
> + if (crtc_state->fec_enable)
> + fec_pipe_mask |= BIT(crtc->pipe);
> + }
> +
> + if (!fec_pipe_mask || mst_pipe_mask == fec_pipe_mask)
> + return 0;
> +
> + limits->force_fec_pipes |= mst_pipe_mask;
> +
> + ret = intel_modeset_pipes_in_mask_early(state, "MST FEC",
> + mst_pipe_mask);
> +
> + return ret ? : -EAGAIN;
> +}
> +
> +static int intel_dp_mst_check_bw(struct intel_atomic_state *state,
> + struct drm_dp_mst_topology_mgr *mst_mgr,
> + struct drm_dp_mst_topology_state *mst_state,
> + struct intel_link_bw_limits *limits)
> +{
> + struct drm_dp_mst_port *mst_port;
> + u8 mst_port_pipes;
> + int ret;
> +
> + ret = drm_dp_mst_atomic_check_mgr(&state->base, mst_mgr, mst_state,
> &mst_port);
> + if (ret != -ENOSPC)
> + return ret;
> +
> + mst_port_pipes = get_pipes_downstream_of_mst_port(state, mst_mgr,
> mst_port);
> +
> + ret = intel_link_bw_reduce_bpp(state, limits,
> + mst_port_pipes, "MST link BW");
> +
> + return ret ? : -EAGAIN;
> +}
> +
> +/**
> + * intel_dp_mst_atomic_check_link - check all modeset MST link configuration
> + * @state: intel atomic state
> + * @limits: link BW limits
> + *
> + * Check the link configuration for all modeset MST outputs. If the
> + * configuration is invalid @limits will be updated if possible to
> + * reduce the total BW, after which the configuration for all CRTCs in
> + * @state must be recomputed with the updated @limits.
> + *
> + * Returns:
> + * - 0 if the confugration is valid
> + * - %-EAGAIN, if the configuration is invalid and @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_dp_mst_atomic_check_link(struct intel_atomic_state *state,
> + struct intel_link_bw_limits *limits)
> +{
> + struct drm_dp_mst_topology_mgr *mgr;
> + struct drm_dp_mst_topology_state *mst_state;
> + int ret;
> + int i;
> +
> + for_each_new_mst_mgr_in_state(&state->base, mgr, mst_state, i) {
> + ret = intel_dp_mst_check_fec_change(state, mgr, limits);
> + if (ret)
> + return ret;
> +
> + ret = intel_dp_mst_check_bw(state, mgr, mst_state,
> + limits);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int intel_dp_mst_compute_config_late(struct intel_encoder *encoder,
> struct intel_crtc_state *crtc_state,
> struct drm_connector_state
> *conn_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index f1815bb722672..4e836b9ac6061 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -13,6 +13,7 @@ struct intel_crtc;
> struct intel_crtc_state;
> struct intel_digital_port;
> struct intel_dp;
> +struct intel_link_bw_limits;
>
> int intel_dp_mst_encoder_init(struct intel_digital_port *dig_port, int
> conn_id);
> void intel_dp_mst_encoder_cleanup(struct intel_digital_port *dig_port);
> @@ -22,5 +23,7 @@ bool intel_dp_mst_is_slave_trans(const struct
> intel_crtc_state *crtc_state);
> bool intel_dp_mst_source_support(struct intel_dp *intel_dp);
> int intel_dp_mst_add_topology_state_for_crtc(struct intel_atomic_state
> *state,
> struct intel_crtc *crtc);
> +int intel_dp_mst_atomic_check_link(struct intel_atomic_state *state,
> + struct intel_link_bw_limits *limits);
>
> #endif /* __INTEL_DP_MST_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_link_bw.c
> b/drivers/gpu/drm/i915/display/intel_link_bw.c
> index 9d95e4a8478f7..85ceae2a5aaf1 100644
> --- a/drivers/gpu/drm/i915/display/intel_link_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_link_bw.c
> @@ -22,6 +22,7 @@ void intel_link_bw_init_limits(struct drm_i915_private
> *i915, struct intel_link_
> {
> enum pipe pipe;
>
> + limits->force_fec_pipes = 0;
> limits->min_bpp_reached_pipes = 0;
> for_each_pipe(i915, pipe)
> limits->max_bpp_x16[pipe] = INT_MAX;
> @@ -168,6 +169,10 @@ static int check_all_link_config(struct
> intel_atomic_state *state,
> {
> int ret;
>
> + ret = intel_dp_mst_atomic_check_link(state, limits);
> + if (ret)
> + return ret;
> +
> ret = intel_fdi_atomic_check_link(state, limits);
> if (ret)
> return ret;
> @@ -183,6 +188,12 @@ assert_link_limit_change_valid(struct drm_i915_private
> *i915,
> bool bpps_changed = false;
> enum pipe pipe;
>
> + /* FEC can't be forced off after it was forced on. */
> + if (drm_WARN_ON(&i915->drm,
> + (old_limits->force_fec_pipes &
> new_limits->force_fec_pipes) !=
> + old_limits->force_fec_pipes))
> + return false;
> +
> for_each_pipe(i915, pipe) {
> /* The bpp limit can only decrease. */
> if (drm_WARN_ON(&i915->drm,
> @@ -197,7 +208,9 @@ assert_link_limit_change_valid(struct drm_i915_private
> *i915,
>
> /* At least one limit must change. */
> if (drm_WARN_ON(&i915->drm,
> - !bpps_changed))
> + !bpps_changed &&
> + new_limits->force_fec_pipes ==
> + old_limits->force_fec_pipes))
> return false;
>
> return true;
> diff --git a/drivers/gpu/drm/i915/display/intel_link_bw.h
> b/drivers/gpu/drm/i915/display/intel_link_bw.h
> index 0f666c9712f3c..33b9a338beeb6 100644
> --- a/drivers/gpu/drm/i915/display/intel_link_bw.h
> +++ b/drivers/gpu/drm/i915/display/intel_link_bw.h
> @@ -16,6 +16,7 @@ struct intel_atomic_state;
> struct intel_crtc_state;
>
> struct intel_link_bw_limits {
> + u8 force_fec_pipes;
> u8 min_bpp_reached_pipes;
> /* in 1/16 bpp units */
> int max_bpp_x16[I915_MAX_PIPES];
> --
> 2.37.2
>
