Re: [PATCH 11/19] drm/i915/dp: Add support for DP tunnel BW allocation
On Wed, Feb 07, 2024 at 01:08:43AM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2024 at 12:28:42PM +0200, Imre Deak wrote:
> > Add support to enable the DP tunnel BW allocation mode. Follow-up
> > patches will call the required helpers added here to prepare for a
> > modeset on a link with DP tunnels, the last change in the patchset
> > actually enabling BWA.
> >
> > With BWA enabled, the driver will expose the full mode list a display
> > supports, regardless of any BW limitation on a shared (Thunderbolt)
> > link. Such BW limits will be checked against only during a modeset, when
> > the driver has the full knowledge of each display's BW requirement.
> >
> > If the link BW changes in a way that a connector's modelist may also
> > change, userspace will get a hotplug notification for all the connectors
> > sharing the same link (so it can adjust the mode used for a display).
> >
> > The BW limitation can change at any point, asynchronously to modesets
> > on a given connector, so a modeset can fail even though the atomic check
> > for it passed. In such scenarios userspace will get a bad link
> > notification and in response is supposed to retry the modeset.
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/Kconfig | 13 +
> > drivers/gpu/drm/i915/Kconfig.debug| 1 +
> > drivers/gpu/drm/i915/Makefile | 3 +
> > drivers/gpu/drm/i915/display/intel_atomic.c | 2 +
> > .../gpu/drm/i915/display/intel_display_core.h | 1 +
> > .../drm/i915/display/intel_display_types.h| 9 +
> > .../gpu/drm/i915/display/intel_dp_tunnel.c| 642 ++
> > .../gpu/drm/i915/display/intel_dp_tunnel.h| 131
> > 8 files changed, 802 insertions(+)
> > create mode 100644 drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > create mode 100644 drivers/gpu/drm/i915/display/intel_dp_tunnel.h
> >
> > diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> > index b5d6e3352071f..4636913c17868 100644
> > --- a/drivers/gpu/drm/i915/Kconfig
> > +++ b/drivers/gpu/drm/i915/Kconfig
> > @@ -155,6 +155,19 @@ config DRM_I915_PXP
> > protected session and manage the status of the alive software session,
> > as well as its life cycle.
> >
> > +config DRM_I915_DP_TUNNEL
> > + bool "Enable DP tunnel support"
> > + depends on DRM_I915
> > + select DRM_DISPLAY_DP_TUNNEL
> > + default y
> > + help
> > + Choose this option to detect DP tunnels and enable the Bandwidth
> > + Allocation mode for such tunnels. This allows using the maximum
> > + resolution allowed by the link BW on all displays sharing the
> > + link BW, for instance on a Thunderbolt link.
> > +
> > + If in doubt, say "Y".
> > +
> > menu "drm/i915 Debugging"
> > depends on DRM_I915
> > depends on EXPERT
> > diff --git a/drivers/gpu/drm/i915/Kconfig.debug
> > b/drivers/gpu/drm/i915/Kconfig.debug
> > index 5b7162076850c..bc18e2d9ea05d 100644
> > --- a/drivers/gpu/drm/i915/Kconfig.debug
> > +++ b/drivers/gpu/drm/i915/Kconfig.debug
> > @@ -28,6 +28,7 @@ config DRM_I915_DEBUG
> > select STACKDEPOT
> > select STACKTRACE
> > select DRM_DP_AUX_CHARDEV
> > + select DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE if DRM_I915_DP_TUNNEL
> > select X86_MSR # used by igt/pm_rpm
> > select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks)
> > select DRM_DEBUG_MM if DRM=y
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index c13f14edb5088..3ef6ed41e62b4 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -369,6 +369,9 @@ i915-y += \
> > display/vlv_dsi.o \
> > display/vlv_dsi_pll.o
> >
> > +i915-$(CONFIG_DRM_I915_DP_TUNNEL) += \
> > + display/intel_dp_tunnel.o
> > +
> > i915-y += \
> > i915_perf.o
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index ec0d5168b5035..96ab37e158995 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -29,6 +29,7 @@
> > * See intel_atomic_plane.c for the plane-specific atomic functionality.
> > */
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -38,6 +39,7 @@
> > #include "intel_atomic.h"
> > #include "intel_cdclk.h"
> > #include "intel_display_types.h"
> > +#include "intel_dp_tunnel.h"
> > #include "intel_global_state.h"
> > #include "intel_hdcp.h"
> > #include "intel_psr.h"
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> > b/drivers/gpu/drm/i915/display/intel_display_core.h
> > index a90f1aa201be8..0993d25a0a686 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > @@ -522,6 +522,7 @@ struct intel_display {
> > } wq;
> >
> > /* Grouping using named structs. Keep sorted. */
> > + struct drm_dp_tunnel_mgr *dp_t
Re: [PATCH 11/19] drm/i915/dp: Add support for DP tunnel BW allocation
On Tue, Jan 23, 2024 at 12:28:42PM +0200, Imre Deak wrote:
> Add support to enable the DP tunnel BW allocation mode. Follow-up
> patches will call the required helpers added here to prepare for a
> modeset on a link with DP tunnels, the last change in the patchset
> actually enabling BWA.
>
> With BWA enabled, the driver will expose the full mode list a display
> supports, regardless of any BW limitation on a shared (Thunderbolt)
> link. Such BW limits will be checked against only during a modeset, when
> the driver has the full knowledge of each display's BW requirement.
>
> If the link BW changes in a way that a connector's modelist may also
> change, userspace will get a hotplug notification for all the connectors
> sharing the same link (so it can adjust the mode used for a display).
>
> The BW limitation can change at any point, asynchronously to modesets
> on a given connector, so a modeset can fail even though the atomic check
> for it passed. In such scenarios userspace will get a bad link
> notification and in response is supposed to retry the modeset.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/Kconfig | 13 +
> drivers/gpu/drm/i915/Kconfig.debug| 1 +
> drivers/gpu/drm/i915/Makefile | 3 +
> drivers/gpu/drm/i915/display/intel_atomic.c | 2 +
> .../gpu/drm/i915/display/intel_display_core.h | 1 +
> .../drm/i915/display/intel_display_types.h| 9 +
> .../gpu/drm/i915/display/intel_dp_tunnel.c| 642 ++
> .../gpu/drm/i915/display/intel_dp_tunnel.h| 131
> 8 files changed, 802 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> create mode 100644 drivers/gpu/drm/i915/display/intel_dp_tunnel.h
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index b5d6e3352071f..4636913c17868 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -155,6 +155,19 @@ config DRM_I915_PXP
> protected session and manage the status of the alive software session,
> as well as its life cycle.
>
> +config DRM_I915_DP_TUNNEL
> + bool "Enable DP tunnel support"
> + depends on DRM_I915
> + select DRM_DISPLAY_DP_TUNNEL
> + default y
> + help
> + Choose this option to detect DP tunnels and enable the Bandwidth
> + Allocation mode for such tunnels. This allows using the maximum
> + resolution allowed by the link BW on all displays sharing the
> + link BW, for instance on a Thunderbolt link.
> +
> + If in doubt, say "Y".
> +
> menu "drm/i915 Debugging"
> depends on DRM_I915
> depends on EXPERT
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug
> b/drivers/gpu/drm/i915/Kconfig.debug
> index 5b7162076850c..bc18e2d9ea05d 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -28,6 +28,7 @@ config DRM_I915_DEBUG
> select STACKDEPOT
> select STACKTRACE
> select DRM_DP_AUX_CHARDEV
> + select DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE if DRM_I915_DP_TUNNEL
> select X86_MSR # used by igt/pm_rpm
> select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks)
> select DRM_DEBUG_MM if DRM=y
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index c13f14edb5088..3ef6ed41e62b4 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -369,6 +369,9 @@ i915-y += \
> display/vlv_dsi.o \
> display/vlv_dsi_pll.o
>
> +i915-$(CONFIG_DRM_I915_DP_TUNNEL) += \
> + display/intel_dp_tunnel.o
> +
> i915-y += \
> i915_perf.o
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> b/drivers/gpu/drm/i915/display/intel_atomic.c
> index ec0d5168b5035..96ab37e158995 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -29,6 +29,7 @@
> * See intel_atomic_plane.c for the plane-specific atomic functionality.
> */
>
> +#include
> #include
> #include
> #include
> @@ -38,6 +39,7 @@
> #include "intel_atomic.h"
> #include "intel_cdclk.h"
> #include "intel_display_types.h"
> +#include "intel_dp_tunnel.h"
> #include "intel_global_state.h"
> #include "intel_hdcp.h"
> #include "intel_psr.h"
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> b/drivers/gpu/drm/i915/display/intel_display_core.h
> index a90f1aa201be8..0993d25a0a686 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -522,6 +522,7 @@ struct intel_display {
> } wq;
>
> /* Grouping using named structs. Keep sorted. */
> + struct drm_dp_tunnel_mgr *dp_tunnel_mgr;
> struct intel_audio audio;
> struct intel_dpll dpll;
> struct intel_fbc *fbc[I915_MAX_FBCS];
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.
Re: [PATCH 11/19] drm/i915/dp: Add support for DP tunnel BW allocation
On Tue, Feb 06, 2024 at 12:47:22AM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2024 at 12:28:42PM +0200, Imre Deak wrote:
> > +static int check_inherited_tunnel_state(struct intel_atomic_state *state,
> > + struct intel_dp *intel_dp,
> > + const struct
> > intel_digital_connector_state *old_conn_state)
> > +{
> > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > + const struct intel_connector *connector =
> > + to_intel_connector(old_conn_state->base.connector);
> > + struct intel_crtc *old_crtc;
> > + const struct intel_crtc_state *old_crtc_state;
> > +
> > + /*
> > +* If a BWA tunnel gets detected only after the corresponding
> > +* connector got enabled already without a BWA tunnel, or a different
> > +* BWA tunnel (which was removed meanwhile) the old CRTC state won't
> > +* contain the state of the current tunnel. This tunnel still has a
> > +* reserved BW, which needs to be released, add the state for such
> > +* inherited tunnels separately only to this atomic state.
> > +*/
> > + if (!intel_dp_tunnel_bw_alloc_is_enabled(intel_dp))
> > + return 0;
> > +
> > + if (!old_conn_state->base.crtc)
> > + return 0;
> > +
> > + old_crtc = to_intel_crtc(old_conn_state->base.crtc);
> > + old_crtc_state = intel_atomic_get_old_crtc_state(state, old_crtc);
> > +
> > + if (!old_crtc_state->hw.active ||
> > + old_crtc_state->dp_tunnel_ref.tunnel == intel_dp->tunnel)
> > + return 0;
> > +
> > + drm_dbg_kms(&i915->drm,
> > + "[DPTUN %s][CONNECTOR:%d:%s][ENCODER:%d:%s][CRTC:%d:%s]
> > Adding state for inherited tunnel %p\n",
> > + drm_dp_tunnel_name(intel_dp->tunnel),
> > + connector->base.base.id,
> > + connector->base.name,
> > + encoder->base.base.id,
> > + encoder->base.name,
> > + old_crtc->base.base.id,
> > + old_crtc->base.name,
> > + intel_dp->tunnel);
> > +
> > + return add_inherited_tunnel_state(state, intel_dp->tunnel, old_crtc);
>
> I still strongly dislike this "tunnels are magically created by detect
> behind our back" approach. IMO in an ideal world we'd only ever create the
> tunnels during modeset/sanitize. What was the reason that didn't work again?
> I think you explained it to me in person at least once already, but can't
> remember anymore...
The tunnel information, describing which group the tunnel belongs to and
so how much BW it can use is needed already during detect time: to
filter the connectors' mode list during connector probing and to
pass/fail an atomic check of connectors that go through a tunnel/group
based on the modes the connectors use, the BW these require vs. the
available BW of the tunnel group.
The atomic state for the tunnel - with the required BW through it - is
only created/added during a modeset.
> --
> Ville Syrjälä
> Intel
Re: [PATCH 11/19] drm/i915/dp: Add support for DP tunnel BW allocation
On Tue, Jan 23, 2024 at 12:28:42PM +0200, Imre Deak wrote:
> +static int check_inherited_tunnel_state(struct intel_atomic_state *state,
> + struct intel_dp *intel_dp,
> + const struct
> intel_digital_connector_state *old_conn_state)
> +{
> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> + const struct intel_connector *connector =
> + to_intel_connector(old_conn_state->base.connector);
> + struct intel_crtc *old_crtc;
> + const struct intel_crtc_state *old_crtc_state;
> +
> + /*
> + * If a BWA tunnel gets detected only after the corresponding
> + * connector got enabled already without a BWA tunnel, or a different
> + * BWA tunnel (which was removed meanwhile) the old CRTC state won't
> + * contain the state of the current tunnel. This tunnel still has a
> + * reserved BW, which needs to be released, add the state for such
> + * inherited tunnels separately only to this atomic state.
> + */
> + if (!intel_dp_tunnel_bw_alloc_is_enabled(intel_dp))
> + return 0;
> +
> + if (!old_conn_state->base.crtc)
> + return 0;
> +
> + old_crtc = to_intel_crtc(old_conn_state->base.crtc);
> + old_crtc_state = intel_atomic_get_old_crtc_state(state, old_crtc);
> +
> + if (!old_crtc_state->hw.active ||
> + old_crtc_state->dp_tunnel_ref.tunnel == intel_dp->tunnel)
> + return 0;
> +
> + drm_dbg_kms(&i915->drm,
> + "[DPTUN %s][CONNECTOR:%d:%s][ENCODER:%d:%s][CRTC:%d:%s]
> Adding state for inherited tunnel %p\n",
> + drm_dp_tunnel_name(intel_dp->tunnel),
> + connector->base.base.id,
> + connector->base.name,
> + encoder->base.base.id,
> + encoder->base.name,
> + old_crtc->base.base.id,
> + old_crtc->base.name,
> + intel_dp->tunnel);
> +
> + return add_inherited_tunnel_state(state, intel_dp->tunnel, old_crtc);
I still strongly dislike this "tunnels are magically created by detect
behind our back" approach. IMO in an ideal world we'd only ever create the
tunnels during modeset/sanitize. What was the reason that didn't work again?
I think you explained it to me in person at least once already, but can't
remember anymore...
--
Ville Syrjälä
Intel
[PATCH 11/19] drm/i915/dp: Add support for DP tunnel BW allocation
Add support to enable the DP tunnel BW allocation mode. Follow-up
patches will call the required helpers added here to prepare for a
modeset on a link with DP tunnels, the last change in the patchset
actually enabling BWA.
With BWA enabled, the driver will expose the full mode list a display
supports, regardless of any BW limitation on a shared (Thunderbolt)
link. Such BW limits will be checked against only during a modeset, when
the driver has the full knowledge of each display's BW requirement.
If the link BW changes in a way that a connector's modelist may also
change, userspace will get a hotplug notification for all the connectors
sharing the same link (so it can adjust the mode used for a display).
The BW limitation can change at any point, asynchronously to modesets
on a given connector, so a modeset can fail even though the atomic check
for it passed. In such scenarios userspace will get a bad link
notification and in response is supposed to retry the modeset.
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/Kconfig | 13 +
drivers/gpu/drm/i915/Kconfig.debug| 1 +
drivers/gpu/drm/i915/Makefile | 3 +
drivers/gpu/drm/i915/display/intel_atomic.c | 2 +
.../gpu/drm/i915/display/intel_display_core.h | 1 +
.../drm/i915/display/intel_display_types.h| 9 +
.../gpu/drm/i915/display/intel_dp_tunnel.c| 642 ++
.../gpu/drm/i915/display/intel_dp_tunnel.h| 131
8 files changed, 802 insertions(+)
create mode 100644 drivers/gpu/drm/i915/display/intel_dp_tunnel.c
create mode 100644 drivers/gpu/drm/i915/display/intel_dp_tunnel.h
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index b5d6e3352071f..4636913c17868 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -155,6 +155,19 @@ config DRM_I915_PXP
protected session and manage the status of the alive software session,
as well as its life cycle.
+config DRM_I915_DP_TUNNEL
+ bool "Enable DP tunnel support"
+ depends on DRM_I915
+ select DRM_DISPLAY_DP_TUNNEL
+ default y
+ help
+ Choose this option to detect DP tunnels and enable the Bandwidth
+ Allocation mode for such tunnels. This allows using the maximum
+ resolution allowed by the link BW on all displays sharing the
+ link BW, for instance on a Thunderbolt link.
+
+ If in doubt, say "Y".
+
menu "drm/i915 Debugging"
depends on DRM_I915
depends on EXPERT
diff --git a/drivers/gpu/drm/i915/Kconfig.debug
b/drivers/gpu/drm/i915/Kconfig.debug
index 5b7162076850c..bc18e2d9ea05d 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -28,6 +28,7 @@ config DRM_I915_DEBUG
select STACKDEPOT
select STACKTRACE
select DRM_DP_AUX_CHARDEV
+ select DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE if DRM_I915_DP_TUNNEL
select X86_MSR # used by igt/pm_rpm
select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks)
select DRM_DEBUG_MM if DRM=y
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index c13f14edb5088..3ef6ed41e62b4 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -369,6 +369,9 @@ i915-y += \
display/vlv_dsi.o \
display/vlv_dsi_pll.o
+i915-$(CONFIG_DRM_I915_DP_TUNNEL) += \
+ display/intel_dp_tunnel.o
+
i915-y += \
i915_perf.o
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
b/drivers/gpu/drm/i915/display/intel_atomic.c
index ec0d5168b5035..96ab37e158995 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -29,6 +29,7 @@
* See intel_atomic_plane.c for the plane-specific atomic functionality.
*/
+#include
#include
#include
#include
@@ -38,6 +39,7 @@
#include "intel_atomic.h"
#include "intel_cdclk.h"
#include "intel_display_types.h"
+#include "intel_dp_tunnel.h"
#include "intel_global_state.h"
#include "intel_hdcp.h"
#include "intel_psr.h"
diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
b/drivers/gpu/drm/i915/display/intel_display_core.h
index a90f1aa201be8..0993d25a0a686 100644
--- a/drivers/gpu/drm/i915/display/intel_display_core.h
+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
@@ -522,6 +522,7 @@ struct intel_display {
} wq;
/* Grouping using named structs. Keep sorted. */
+ struct drm_dp_tunnel_mgr *dp_tunnel_mgr;
struct intel_audio audio;
struct intel_dpll dpll;
struct intel_fbc *fbc[I915_MAX_FBCS];
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
b/drivers/gpu/drm/i915/display/intel_display_types.h
index ae2e8cff9d691..b79db78b27728 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -33,6 +33,7 @@
#include
#include
+#include
#include
#include
#includ
