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

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

On Wed, Apr 09, 2025 at 05:00:18PM +0300, Jani Nikula wrote:
> On Wed, 09 Apr 2025, Imre Deak <imre.deak@intel.com> wrote:
> > On Wed, Apr 09, 2025 at 11:16:58AM +0300, Jani Nikula wrote:
> >> 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.
> >
> > I think the better approach is each connector adding the debugfs entries
> > relevant to them, even for the existing hdcp, pps, psr etc. entries. That
> > would avoid all the connector_type checks, replicated now in all the
> > *_connector_debugfs_add() functions. It's also odd to recheck the
> > connector type on a code path the connector type is already known to the
> > caller.
> 
> I'm not saying that's not a valid argument. And I said I understand the
> motivation.
> 
> I just don't want this done for a single debugfs entry in a series about
> something completely different, essentially leaving behind a mix of two
> entirely different approaches.
> 
> In the past switching to your proposed approach wasn't really even an
> option because everything was still in intel_display_debugfs.c. We've
> gradually moved away from that. And arguably the work of moving the
> debugfs next to the functionality should be completed first, before
> starting another refactoring. intel_connector_debugfs_add() should just
> be a function calling intel_*_connector_debugfs_add() functions instead
> of having inlined debugfs creation.

I don't agree that the new entry added in this patch couldn't be added
in the correct way to begin with, also making it easier to move adding
the rest of the entries the same way as well; but will change it.

> BR,
> Jani.
> 
> 
> 
> >
> > This approach would also match how the connector specific properties are
> > added.
> >
> >> 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
> 
> -- 
> Jani Nikula, Intel