| Message ID | 20250113154832.1004369-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/xe/dp: Enable DP tunneling | expand |
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. > 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? BR, Jani. > config DRM_XE_FORCE_PROBE > string "Force probe xe for selected Intel hardware IDs" > depends on DRM_XE > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile > index 5c97ad6ed7385..81f63258a7e19 100644 > --- a/drivers/gpu/drm/xe/Makefile > +++ b/drivers/gpu/drm/xe/Makefile > @@ -301,6 +301,9 @@ ifeq ($(CONFIG_DEBUG_FS),y) > i915-display/intel_pipe_crc.o > endif > > +xe-$(CONFIG_DRM_XE_DP_TUNNEL) += \ > + i915-display/intel_dp_tunnel.o > + > obj-$(CONFIG_DRM_XE) += xe.o > obj-$(CONFIG_DRM_XE_KUNIT_TEST) += tests/
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. > BR, > Jani. > > > > config DRM_XE_FORCE_PROBE > > string "Force probe xe for selected Intel hardware IDs" > > depends on DRM_XE > > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile > > index 5c97ad6ed7385..81f63258a7e19 100644 > > --- a/drivers/gpu/drm/xe/Makefile > > +++ b/drivers/gpu/drm/xe/Makefile > > @@ -301,6 +301,9 @@ ifeq ($(CONFIG_DEBUG_FS),y) > > i915-display/intel_pipe_crc.o > > endif > > > > +xe-$(CONFIG_DRM_XE_DP_TUNNEL) += \ > > + i915-display/intel_dp_tunnel.o > > + > > obj-$(CONFIG_DRM_XE) += xe.o > > obj-$(CONFIG_DRM_XE_KUNIT_TEST) += tests/ > > -- > Jani Nikula, Intel
On Mon, Jan 13, 2025 at 05:48:32PM +0200, Imre Deak 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)) > > 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". >+ > config DRM_XE_FORCE_PROBE > string "Force probe xe for selected Intel hardware IDs" > depends on DRM_XE >diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile >index 5c97ad6ed7385..81f63258a7e19 100644 >--- a/drivers/gpu/drm/xe/Makefile >+++ b/drivers/gpu/drm/xe/Makefile >@@ -301,6 +301,9 @@ ifeq ($(CONFIG_DEBUG_FS),y) > i915-display/intel_pipe_crc.o > endif > >+xe-$(CONFIG_DRM_XE_DP_TUNNEL) += \ >+ i915-display/intel_dp_tunnel.o something looks wrong here. We shouldn't build anything in i915-display/ if we don't have DRM_XE_DISPLAY. Are you missing a depends on XE_DISPLAY ? and in fact, with the .config I get: In file included from ../drivers/gpu/drm/i915/display/intel_display_core.h:23, from ../drivers/gpu/drm/i915/display/intel_dp_tunnel.c:9: ../drivers/gpu/drm/i915/display/intel_display_power.h:12:10: fatal error: intel_wakeref.h: No such file or directory 12 | #include "intel_wakeref.h" because we don't setup the right include directories. We used to test in CI a display-disabled build, not sure what happened with that. Lucas De Marchi >+ > obj-$(CONFIG_DRM_XE) += xe.o > obj-$(CONFIG_DRM_XE_KUNIT_TEST) += tests/ > >-- >2.44.2 >
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. Lucas De Marchi
On Thu, Jan 16, 2025 at 02:32:21PM -0600, Lucas De Marchi wrote: > On Mon, Jan 13, 2025 at 05:48:32PM +0200, Imre Deak 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)) > > > > 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". > > + > > config DRM_XE_FORCE_PROBE > > string "Force probe xe for selected Intel hardware IDs" > > depends on DRM_XE > > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile > > index 5c97ad6ed7385..81f63258a7e19 100644 > > --- a/drivers/gpu/drm/xe/Makefile > > +++ b/drivers/gpu/drm/xe/Makefile > > @@ -301,6 +301,9 @@ ifeq ($(CONFIG_DEBUG_FS),y) > > i915-display/intel_pipe_crc.o > > endif > > > > +xe-$(CONFIG_DRM_XE_DP_TUNNEL) += \ > > + i915-display/intel_dp_tunnel.o > > something looks wrong here. We shouldn't build anything in i915-display/ if > we don't have DRM_XE_DISPLAY. Are you missing a > > depends on XE_DISPLAY > ? Yes, thanks for spotting that, I missed the possibility for a non-display build. Will follow up with a fix for this. > and in fact, with the .config I get: > > In file included from > ../drivers/gpu/drm/i915/display/intel_display_core.h:23, > from ../drivers/gpu/drm/i915/display/intel_dp_tunnel.c:9: > ../drivers/gpu/drm/i915/display/intel_display_power.h:12:10: fatal error: > intel_wakeref.h: No such file or directory 12 | #include > "intel_wakeref.h" > > because we don't setup the right include directories. > We used to test in CI a display-disabled build, not sure what happened > with that. > > Lucas De Marchi > > > + > > obj-$(CONFIG_DRM_XE) += xe.o > > obj-$(CONFIG_DRM_XE_KUNIT_TEST) += tests/ > > > > -- > > 2.44.2 > >
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
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@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. 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
On Fri, Jan 17, 2025 at 10:45:56AM -0600, Lucas De Marchi wrote: > [...] > > > > > > 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? All the display options should be configured at one place, but that's only feasible with a separate display module (which is the goal afaik). > Lucas De Marchi
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)) 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". + config DRM_XE_FORCE_PROBE string "Force probe xe for selected Intel hardware IDs" depends on DRM_XE diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index 5c97ad6ed7385..81f63258a7e19 100644 --- a/drivers/gpu/drm/xe/Makefile +++ b/drivers/gpu/drm/xe/Makefile @@ -301,6 +301,9 @@ ifeq ($(CONFIG_DEBUG_FS),y) i915-display/intel_pipe_crc.o endif +xe-$(CONFIG_DRM_XE_DP_TUNNEL) += \ + i915-display/intel_dp_tunnel.o + obj-$(CONFIG_DRM_XE) += xe.o obj-$(CONFIG_DRM_XE_KUNIT_TEST) += tests/
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(-)