Re: [PATCH] drm/xe/dp: Enable DP tunneling

[Imre Deak <imre.deak@intel.com>]

On Thu, Jan 16, 2025 at 02:38:34PM -0600, Lucas De Marchi wrote:
> On Mon, Jan 13, 2025 at 07:40:59PM +0200, Imre Deak wrote:
> > On Mon, Jan 13, 2025 at 06:38:34PM +0200, Jani Nikula wrote:
> > > On Mon, 13 Jan 2025, Imre Deak <imre.deak@intel.com> wrote:
> > > > Enable the DP tunneling functionality in the xe driver.
> > > >
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp_tunnel.h |  5 +++--
> > > >  drivers/gpu/drm/xe/Kconfig                     | 14 ++++++++++++++
> > > >  drivers/gpu/drm/xe/Makefile                    |  3 +++
> > > >  3 files changed, 20 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_tunnel.h b/drivers/gpu/drm/i915/display/intel_dp_tunnel.h
> > > > index e9314cf25a193..7a91b4945eb8d 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_tunnel.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_tunnel.h
> > > > @@ -20,7 +20,8 @@ struct intel_dp;
> > > >  struct intel_encoder;
> > > >  struct intel_link_bw_limits;
> > > >
> > > > -#if IS_ENABLED(CONFIG_DRM_I915_DP_TUNNEL) && defined(I915)
> > > > +#if (defined(CONFIG_DRM_I915_DP_TUNNEL) && defined(I915)) || \
> > > > +	(defined(CONFIG_DRM_XE_DP_TUNNEL) && !defined(I915))
> > > 
> > > Please retain IS_ENABLED for checking kconfig symbols.
> > 
> > Ok, missed that, will change it.
> > 
> > > >  int intel_dp_tunnel_detect(struct intel_dp *intel_dp, struct drm_modeset_acquire_ctx *ctx);
> > > >  void intel_dp_tunnel_disconnect(struct intel_dp *intel_dp);
> > > > @@ -127,6 +128,6 @@ intel_dp_tunnel_mgr_init(struct intel_display *display)
> > > >
> > > >  static inline void intel_dp_tunnel_mgr_cleanup(struct intel_display *display) {}
> > > >
> > > > -#endif /* CONFIG_DRM_I915_DP_TUNNEL */
> > > > +#endif /* CONFIG_DRM_I915_DP_TUNNEL || CONFIG_DRM_XE_DP_TUNNEL */
> > > >
> > > >  #endif /* __INTEL_DP_TUNNEL_H__ */
> > > > diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> > > > index b51a2bde73e29..50cf80df51900 100644
> > > > --- a/drivers/gpu/drm/xe/Kconfig
> > > > +++ b/drivers/gpu/drm/xe/Kconfig
> > > > @@ -59,6 +59,20 @@ config DRM_XE_DISPLAY
> > > >  	help
> > > >  	  Disable this option only if you want to compile out display support.
> > > >
> > > > +config DRM_XE_DP_TUNNEL
> > > > +	bool "Enable DP tunnel support"
> > > > +	depends on DRM_XE
> > > > +	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".
> > > > +
> > > 
> > > I'm sort of wondering why we have this (and the i915 one) as
> > > user-selectable config options at all. Is it ever reasonable for the
> > > user to disable this if USB4 is enabled?
> > 
> > On platforms that don't support DP tunneling, while supporting other
> > USB4 functionality (or for systems w/o any TypeC/DP connectors) it would
> > make sense to disable this option.
> 
> isn't this too fine grained? if we expose every single functionality of
> the driver like this we will bury distros on configs and exponentially
> explode the testing combination. And yes, this broke the build for me.

The tunneling functionality depends on USB4, BW allocation could fail
without that. The option being user selectable also makes sense to me,
as it has a size (~30kB) and runtime overhead (detecting tunnels and
allocating/freeing BW), only required if the user has a dock/multiple
displays.

> Lucas De Marchi