Re: [PATCH v2 12/21] drm/i915/dp: Add support for DP tunnel BW allocation
On Fri, Feb 23, 2024 at 11:37:41PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 20, 2024 at 11:18:32PM +0200, Imre Deak wrote:
> > +static void queue_retry_work(struct intel_atomic_state *state,
> > +struct drm_dp_tunnel *tunnel,
> > +const struct intel_crtc_state *crtc_state)
> > +{
> > + struct drm_i915_private *i915 = to_i915(state->base.dev);
> > + struct intel_encoder *encoder;
> > +
> > + encoder = intel_get_crtc_new_encoder(state, crtc_state);
>
> I was pondering what happens if we have no encoder here?
That is when the crtc is disabled.
> But I guess crtc_state->tunnel_ref.tunnel should be NULL in
> that case and so we should not end up here.
Yes, in case crtc is disabled tunnel should be NULL, so
queue_retry_work() is not called.
> > +
> > + if (!intel_digital_port_connected(encoder))
> > + return;
> > +
> > + drm_dbg_kms(>drm,
> > + "[DPTUN %s][ENCODER:%d:%s] BW allocation failed on a
> > connected sink\n",
> > + drm_dp_tunnel_name(tunnel),
> > + encoder->base.base.id,
> > + encoder->base.name);
> > +
> > + intel_dp_queue_modeset_retry_for_link(state, encoder, crtc_state);
> > +}
> > +
>
> --
> Ville Syrjälä
> Intel
RE: [PATCH v2 12/21] drm/i915/dp: Add support for DP tunnel BW allocation
> -Original Message- > From: Intel-gfx On Behalf Of Imre > Deak > Sent: Wednesday, February 21, 2024 2:49 AM > To: intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org > Cc: Ville Syrjälä > Subject: [PATCH v2 12/21] 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. > > v2: > - Fix old vs. new connector state in intel_dp_tunnel_atomic_check_state(). > (Ville) > - Fix propagating the error from > intel_dp_tunnel_atomic_compute_stream_bw(). (Ville) > - Move tunnel==NULL checks from driver to DRM core helpers. (Ville) > - Simplify return flow from intel_dp_tunnel_detect(). (Ville) > - s/dp_tunnel_state/inherited_dp_tunnels (Ville) > - Simplify struct intel_dp_tunnel_inherited_state. (Ville) > - Unconstify object pointers (vs. states) where possible. (Ville) > - Init crtc_state while declaring it in check_group_state(). (Ville) > - Join obj->base.id, obj->name arg lines in debug prints to reduce LOC. > (Ville) > - Add/rework intel_dp_tunnel_atomic_alloc_bw() to prepare for moving the > BW allocation from encoder hooks up to intel_atomic_commit_tail() > later in the patchset. > - Disable BW alloc mode during system suspend. > - Allocate the required BW for all tunnels during system resume. > - Add intel_dp_tunnel_atomic_clear_stream_bw() instead of the open-coded > sequence in a follow-up patch. > - Add function documentation to all exported functions. > - Add CONFIG_USB4 dependency to CONFIG_DRM_I915_DP_TUNNEL. Changes look good to me. Reviewed-by: Uma Shankar > Cc: Ville Syrjälä > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/Kconfig | 14 + > drivers/gpu/drm/i915/Kconfig.debug| 1 + > drivers/gpu/drm/i915/Makefile | 3 + > drivers/gpu/drm/i915/display/intel_atomic.c | 2 + > drivers/gpu/drm/i915/display/intel_crtc.c | 25 + > drivers/gpu/drm/i915/display/intel_crtc.h | 1 + > .../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| 815 ++ > .../gpu/drm/i915/display/intel_dp_tunnel.h| 133 +++ > 10 files changed, 1004 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 > 3089029abba48..5932024f8f954 100644 > --- a/drivers/gpu/drm/i915/Kconfig > +++ b/drivers/gpu/drm/i915/Kconfig > @@ -155,6 +155,20 @@ 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 > + depends on USB4 > + 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 > sel
Re: [PATCH v2 12/21] drm/i915/dp: Add support for DP tunnel BW allocation
On Tue, Feb 20, 2024 at 11:18:32PM +0200, Imre Deak wrote:
> +static void queue_retry_work(struct intel_atomic_state *state,
> + struct drm_dp_tunnel *tunnel,
> + const struct intel_crtc_state *crtc_state)
> +{
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> + struct intel_encoder *encoder;
> +
> + encoder = intel_get_crtc_new_encoder(state, crtc_state);
I was pondering what happens if we have no encoder here?
But I guess crtc_state->tunnel_ref.tunnel should be NULL in
that case and so we should not end up here.
> +
> + if (!intel_digital_port_connected(encoder))
> + return;
> +
> + drm_dbg_kms(>drm,
> + "[DPTUN %s][ENCODER:%d:%s] BW allocation failed on a
> connected sink\n",
> + drm_dp_tunnel_name(tunnel),
> + encoder->base.base.id,
> + encoder->base.name);
> +
> + intel_dp_queue_modeset_retry_for_link(state, encoder, crtc_state);
> +}
> +
--
Ville Syrjälä
Intel
[PATCH v2 12/21] 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. v2: - Fix old vs. new connector state in intel_dp_tunnel_atomic_check_state(). (Ville) - Fix propagating the error from intel_dp_tunnel_atomic_compute_stream_bw(). (Ville) - Move tunnel==NULL checks from driver to DRM core helpers. (Ville) - Simplify return flow from intel_dp_tunnel_detect(). (Ville) - s/dp_tunnel_state/inherited_dp_tunnels (Ville) - Simplify struct intel_dp_tunnel_inherited_state. (Ville) - Unconstify object pointers (vs. states) where possible. (Ville) - Init crtc_state while declaring it in check_group_state(). (Ville) - Join obj->base.id, obj->name arg lines in debug prints to reduce LOC. (Ville) - Add/rework intel_dp_tunnel_atomic_alloc_bw() to prepare for moving the BW allocation from encoder hooks up to intel_atomic_commit_tail() later in the patchset. - Disable BW alloc mode during system suspend. - Allocate the required BW for all tunnels during system resume. - Add intel_dp_tunnel_atomic_clear_stream_bw() instead of the open-coded sequence in a follow-up patch. - Add function documentation to all exported functions. - Add CONFIG_USB4 dependency to CONFIG_DRM_I915_DP_TUNNEL. Cc: Ville Syrjälä Signed-off-by: Imre Deak --- drivers/gpu/drm/i915/Kconfig | 14 + drivers/gpu/drm/i915/Kconfig.debug| 1 + drivers/gpu/drm/i915/Makefile | 3 + drivers/gpu/drm/i915/display/intel_atomic.c | 2 + drivers/gpu/drm/i915/display/intel_crtc.c | 25 + drivers/gpu/drm/i915/display/intel_crtc.h | 1 + .../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| 815 ++ .../gpu/drm/i915/display/intel_dp_tunnel.h| 133 +++ 10 files changed, 1004 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 3089029abba48..5932024f8f954 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -155,6 +155,20 @@ 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 + depends on USB4 + 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 +++
