Re: [Intel-gfx] [PATCH v4 09/25] drm/i915: Add helpers for BW management on shared display links
On Fri, Sep 15, 2023 at 10:11:38PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 15, 2023 at 03:33:54AM +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).
> >
> > 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.
> >
> > Cc: Jani Nikula
> > Cc: Ville Syrjälä
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > drivers/gpu/drm/i915/display/intel_crtc.c | 1 +
> > drivers/gpu/drm/i915/display/intel_display.c | 61 -
> > .../drm/i915/display/intel_display_types.h| 3 +-
> > drivers/gpu/drm/i915/display/intel_link_bw.c | 226 ++
> > drivers/gpu/drm/i915/display/intel_link_bw.h | 38 +++
> > 6 files changed, 325 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;
>
> Are we sure we won't end up doing random arithmetic with that which could
> overflow?
It's only used limiting values to an upper bound, so can't see at least
where it could overflow.
> > }
> >
> > 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 fe3b6844e063d..0f30723a68cc0 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;
> >
> > /*
> > @@ -6195,7 +6206,9 @@ static int intel_bigjoine
Re: [Intel-gfx] [PATCH v4 09/25] drm/i915: Add helpers for BW management on shared display links
On Fri, Sep 15, 2023 at 03:33:54AM +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).
>
> 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.
>
> Cc: Jani Nikula
> Cc: Ville Syrjälä
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/display/intel_crtc.c | 1 +
> drivers/gpu/drm/i915/display/intel_display.c | 61 -
> .../drm/i915/display/intel_display_types.h| 3 +-
> drivers/gpu/drm/i915/display/intel_link_bw.c | 226 ++
> drivers/gpu/drm/i915/display/intel_link_bw.h | 38 +++
> 6 files changed, 325 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;
Are we sure we won't end up doing random arithmetic with that which could
overflow?
> }
>
> 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 fe3b6844e063d..0f30723a68cc0 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;
>
> /*
> @@ -6195,7 +6206,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,
> +
[Intel-gfx] [PATCH v4 09/25] drm/i915: Add helpers for BW management on shared display links
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).
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.
Cc: Jani Nikula
Cc: Ville Syrjälä
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/display/intel_crtc.c | 1 +
drivers/gpu/drm/i915/display/intel_display.c | 61 -
.../drm/i915/display/intel_display_types.h| 3 +-
drivers/gpu/drm/i915/display/intel_link_bw.c | 226 ++
drivers/gpu/drm/i915/display/intel_link_bw.h | 38 +++
6 files changed, 325 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 fe3b6844e063d..0f30723a68cc0 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;
/*
@@ -6195,7 +6206,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;
@@ -6203,6 +6216,8 @@ static int intel_atomic_check_config(struct
intel_atomic_state *state)
int ret;
int i;
+ *failed_pipe = INVALID_PIP
