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

[Lucas De Marchi]

On Fri, Jan 17, 2025 at 05:55:57PM +0200, Imre Deak wrote:
>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 at intel.com> wrote:
>> > > > Enable the DP tunneling functionality in the xe driver.
>> > > >
>> > > > Signed-off-by: Imre Deak <imre.deak at 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.

I will leave this up to the display maintainers - I still think it's too
fine grained to have this option as user selectable and worse, in 2
drivers.... does the user have to know which driver officially support
that hardware to enable one and disable the other? 

Lucas De Marchi