Re: [PATCH 10/10] drm/i915: Add force link bpp debugfs entry to connectors

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 09 Apr 2025, Imre Deak <imre.deak@intel.com> wrote:
> Add the debugfs entry to force a link bpp to all relevant connectors:
> all DP connectors and on an FDI link CRT/SDVO/LVDS/HDMI connectors.

This deviates from the current approach of intel_connector_register()
calling intel_connector_debugfs_add() which checks for connector types
and other conditions before registering debugfs files.

In many cases intel_connector_debugfs_add() unconditionally calls
feature specific debugfs functions such as
intel_hdcp_connector_debugfs_add() which then check the connector type.

I understand the motivation in this patch, being more object oriented
and all, but it's still a deviation. I prefer the same approach for
all. Currently it's obvious where all connector debugfs files get
registered. After this patch, it's not, and it's no longer clear cut
where connector debugfs files should be created.

Please add the connector type checks in
intel_link_bw_connector_debugfs_add() and call it from
intel_connector_debugfs_add().


BR,
Jani.


>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_crt.c      | 20 ++++++++++++++++++-
>  .../drm/i915/display/intel_display_device.h   |  1 +
>  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |  2 ++
>  drivers/gpu/drm/i915/display/intel_hdmi.c     |  8 +++++++-
>  drivers/gpu/drm/i915/display/intel_lvds.c     | 20 ++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_sdvo.c     | 20 ++++++++++++++++++-
>  7 files changed, 70 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> index cca22d2402e88..69831d6f68912 100644
> --- a/drivers/gpu/drm/i915/display/intel_crt.c
> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> @@ -43,6 +43,7 @@
>  #include "intel_ddi.h"
>  #include "intel_ddi_buf_trans.h"
>  #include "intel_de.h"
> +#include "intel_display_device.h"
>  #include "intel_display_driver.h"
>  #include "intel_display_types.h"
>  #include "intel_fdi.h"
> @@ -51,6 +52,7 @@
>  #include "intel_gmbus.h"
>  #include "intel_hotplug.h"
>  #include "intel_hotplug_irq.h"
> +#include "intel_link_bw.h"
>  #include "intel_load_detect.h"
>  #include "intel_pch_display.h"
>  #include "intel_pch_refclk.h"
> @@ -986,13 +988,29 @@ void intel_crt_reset(struct drm_encoder *encoder)
>  
>  }
>  
> +static int intel_crt_connector_register(struct drm_connector *_connector)
> +{
> +	struct intel_connector *connector = to_intel_connector(_connector);
> +	struct intel_display *display = to_intel_display(connector);
> +	int err;
> +
> +	err = intel_connector_register(&connector->base);
> +	if (err)
> +		return err;
> +
> +	if (HAS_FDI(display))
> +		intel_link_bw_connector_debugfs_add(connector);
> +
> +	return 0;
> +}
> +
>  /*
>   * Routines for controlling stuff on the analog port
>   */
>  
>  static const struct drm_connector_funcs intel_crt_connector_funcs = {
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.late_register = intel_connector_register,
> +	.late_register = intel_crt_connector_register,
>  	.early_unregister = intel_connector_unregister,
>  	.destroy = intel_connector_destroy,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 368b0d3417c26..a84bdc83417f1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -171,6 +171,7 @@ struct intel_display_platforms {
>  #define HAS_GMBUS_BURST_READ(__display)	(DISPLAY_VER(__display) >= 10 || (__display)->platform.kabylake)
>  #define HAS_GMBUS_IRQ(__display)	(DISPLAY_VER(__display) >= 4)
>  #define HAS_GMCH(__display)		(DISPLAY_INFO(__display)->has_gmch)
> +#define HAS_FDI(__display)		(IS_DISPLAY_VER((__display), 5, 8) && !HAS_GMCH(__display))
>  #define HAS_HOTPLUG(__display)		(DISPLAY_INFO(__display)->has_hotplug)
>  #define HAS_HW_SAGV_WM(__display)	(DISPLAY_VER(__display) >= 13 && !(__display)->platform.dgfx)
>  #define HAS_IPC(__display)		(DISPLAY_INFO(__display)->has_ipc)
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 8ca33ebedce27..0b19a9b5adda5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -80,6 +80,7 @@
>  #include "intel_hdmi.h"
>  #include "intel_hotplug.h"
>  #include "intel_hotplug_irq.h"
> +#include "intel_link_bw.h"
>  #include "intel_lspcon.h"
>  #include "intel_lvds.h"
>  #include "intel_modeset_lock.h"
> @@ -5890,6 +5891,8 @@ intel_dp_connector_register(struct drm_connector *_connector)
>  	if (ret)
>  		return ret;
>  
> +	intel_link_bw_connector_debugfs_add(connector);
> +
>  	drm_dbg_kms(display->drm, "registering %s bus for %s\n",
>  		    intel_dp->aux.name, connector->base.kdev->kobj.name);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 35214d9a8c781..7508aa4e3695f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1445,6 +1445,8 @@ mst_connector_late_register(struct drm_connector *_connector)
>  	if (ret < 0)
>  		drm_dp_mst_connector_early_unregister(&connector->base, connector->mst.port);
>  
> +	intel_link_bw_connector_debugfs_add(connector);
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 8f2cef36bdf79..0747ef8d6c0ca 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -52,6 +52,7 @@
>  #include "intel_cx0_phy.h"
>  #include "intel_ddi.h"
>  #include "intel_de.h"
> +#include "intel_display_device.h"
>  #include "intel_display_driver.h"
>  #include "intel_display_types.h"
>  #include "intel_dp.h"
> @@ -60,6 +61,7 @@
>  #include "intel_hdcp_regs.h"
>  #include "intel_hdcp_shim.h"
>  #include "intel_hdmi.h"
> +#include "intel_link_bw.h"
>  #include "intel_lspcon.h"
>  #include "intel_panel.h"
>  #include "intel_pfit.h"
> @@ -2611,13 +2613,17 @@ static int
>  intel_hdmi_connector_register(struct drm_connector *_connector)
>  {
>  	struct intel_connector *connector = to_intel_connector(_connector);
> +	struct intel_display *display = to_intel_display(connector);
>  	int ret;
>  
>  	ret = intel_connector_register(&connector->base);
>  	if (ret)
>  		return ret;
>  
> -	return ret;
> +	if (HAS_FDI(display))
> +		intel_link_bw_connector_debugfs_add(connector);
> +
> +	return 0;
>  }
>  
>  static void intel_hdmi_connector_unregister(struct drm_connector *_connector)
> diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
> index 89d26913e2539..3ac6aaa025434 100644
> --- a/drivers/gpu/drm/i915/display/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/display/intel_lvds.c
> @@ -45,10 +45,12 @@
>  #include "intel_backlight.h"
>  #include "intel_connector.h"
>  #include "intel_de.h"
> +#include "intel_display_device.h"
>  #include "intel_display_types.h"
>  #include "intel_dpll.h"
>  #include "intel_fdi.h"
>  #include "intel_gmbus.h"
> +#include "intel_link_bw.h"
>  #include "intel_lvds.h"
>  #include "intel_lvds_regs.h"
>  #include "intel_panel.h"
> @@ -501,6 +503,22 @@ static int intel_lvds_get_modes(struct drm_connector *_connector)
>  	return intel_panel_get_modes(connector);
>  }
>  
> +static int intel_lvds_connector_register(struct drm_connector *_connector)
> +{
> +	struct intel_connector *connector = to_intel_connector(_connector);
> +	struct intel_display *display = to_intel_display(connector);
> +	int err;
> +
> +	err = intel_connector_register(&connector->base);
> +	if (err)
> +		return err;
> +
> +	if (HAS_FDI(display))
> +		intel_link_bw_connector_debugfs_add(connector);
> +
> +	return 0;
> +}
> +
>  static const struct drm_connector_helper_funcs intel_lvds_connector_helper_funcs = {
>  	.get_modes = intel_lvds_get_modes,
>  	.mode_valid = intel_lvds_mode_valid,
> @@ -512,7 +530,7 @@ static const struct drm_connector_funcs intel_lvds_connector_funcs = {
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.atomic_get_property = intel_digital_connector_atomic_get_property,
>  	.atomic_set_property = intel_digital_connector_atomic_set_property,
> -	.late_register = intel_connector_register,
> +	.late_register = intel_lvds_connector_register,
>  	.early_unregister = intel_connector_unregister,
>  	.destroy = intel_connector_destroy,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 757b9ce7e3b1c..ab7caaa4f287f 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -45,6 +45,7 @@
>  #include "intel_connector.h"
>  #include "intel_crtc.h"
>  #include "intel_de.h"
> +#include "intel_display_device.h"
>  #include "intel_display_driver.h"
>  #include "intel_display_types.h"
>  #include "intel_fdi.h"
> @@ -52,6 +53,7 @@
>  #include "intel_gmbus.h"
>  #include "intel_hdmi.h"
>  #include "intel_hotplug.h"
> +#include "intel_link_bw.h"
>  #include "intel_panel.h"
>  #include "intel_sdvo.h"
>  #include "intel_sdvo_regs.h"
> @@ -2502,12 +2504,28 @@ intel_sdvo_connector_duplicate_state(struct drm_connector *connector)
>  	return &state->base.base;
>  }
>  
> +static int intel_sdvo_connector_register(struct drm_connector *_connector)
> +{
> +	struct intel_connector *connector = to_intel_connector(_connector);
> +	struct intel_display *display = to_intel_display(connector);
> +	int err;
> +
> +	err = intel_connector_register(&connector->base);
> +	if (err)
> +		return err;
> +
> +	if (HAS_FDI(display))
> +		intel_link_bw_connector_debugfs_add(connector);
> +
> +	return 0;
> +}
> +
>  static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
>  	.detect = intel_sdvo_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.atomic_get_property = intel_sdvo_connector_atomic_get_property,
>  	.atomic_set_property = intel_sdvo_connector_atomic_set_property,
> -	.late_register = intel_connector_register,
> +	.late_register = intel_sdvo_connector_register,
>  	.early_unregister = intel_connector_unregister,
>  	.destroy = intel_connector_destroy,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,

-- 
Jani Nikula, Intel