On Wed, Apr 09, 2025 at 11:16:58AM +0300, Jani Nikula wrote: > On Wed, 09 Apr 2025, Imre Deak <[email protected]> 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. 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 <[email protected]> > > --- > > 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
