Re: [PATCH v2 12/21] drm/i915/dp: Add support for DP tunnel BW allocation

2024-02-26 Thread Imre Deak
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

2024-02-26 Thread Shankar, Uma


> -Original Message-
> From: Intel-gfx  On Behalf Of Imre
> Deak
> Sent: Wednesday, February 21, 2024 2:49 AM
> To: intel-...@lists.freedesktop.org; dri-devel@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
>   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 

Re: [PATCH v2 12/21] drm/i915/dp: Add support for DP tunnel BW allocation

2024-02-23 Thread Ville Syrjälä
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