Re: [PATCH v2 6/6] drm/i915/alpm: Add debugfs for LOBF

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

On Fri, 12 Apr 2024, Animesh Manna <animesh.manna@intel.com> wrote:
> For validation purpose add debugfs for LOBF.
>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_alpm.c     | 47 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_alpm.h     |  2 +
>  .../drm/i915/display/intel_display_debugfs.c  |  2 +
>  3 files changed, 51 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index ae894c85233c..21dfc06952d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -339,3 +339,50 @@ void intel_alpm_configure(struct intel_dp *intel_dp)
>  {
>  	lnl_alpm_configure(intel_dp);
>  }
> +
> +static int i915_edp_lobf_support_show(struct seq_file *m, void *data)
> +{
> +	struct intel_connector *connector = m->private;
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +
> +	seq_printf(m, "LOBF support: = %s",
> +		   str_yes_no(intel_dp->lobf_supported));

If you have individual debugfs files, where the name tells you what it's
about, what's the point in printing "LOBF support" here?

Moreover, please be more careful, this now prints "LOBF support: =
yes". And you'll want the \n in the end.

> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_support);
> +
> +static int i915_edp_lobf_status_show(struct seq_file *m, void *data)
> +{
> +	struct intel_connector *connector = m->private;
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	const char *status;
> +
> +	if (intel_dp->lobf_enabled)
> +		status = "enabled";
> +	else
> +		status = "disabled";
> +
> +	seq_printf(m, "LOBF: %s\n", status);

Ditto. But there's also str_enabled_disabled().

I mean you could have a read-only info file which prints all of this
info with the prefixes. But if it's one attribute per file, why have the
extra prints? Maybe it should be just alpm info? Idk.

BR,
Jani.

> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_status);
> +
> +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector)
> +{
> +	struct drm_i915_private *i915 = to_i915(connector->base.dev);
> +	struct dentry *root = connector->base.debugfs_entry;
> +
> +	if (DISPLAY_VER(i915) >= 20 &&
> +	    connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> +		return;
> +
> +	debugfs_create_file("i915_edp_lobf_supported", 0444, root,
> +			    connector, &i915_edp_lobf_support_fops);
> +
> +	debugfs_create_file("i915_edp_lobf_status", 0444, root,
> +			    connector, &i915_edp_lobf_status_fops);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h b/drivers/gpu/drm/i915/display/intel_alpm.h
> index c341d2c2b7f7..66e81ed8b2fb 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> @@ -11,6 +11,7 @@
>  struct intel_dp;
>  struct intel_crtc_state;
>  struct drm_connector_state;
> +struct intel_connector;
>  
>  bool intel_dp_get_aux_less_alpm_status(struct intel_dp *intel_dp);
>  bool intel_alpm_compute_params(struct intel_dp *intel_dp,
> @@ -19,5 +20,6 @@ void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
>  				    struct intel_crtc_state *crtc_state,
>  				    struct drm_connector_state *conn_state);
>  void intel_alpm_configure(struct intel_dp *intel_dp);
> +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector);
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 0feffe8d4e45..ba1530149836 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -13,6 +13,7 @@
>  #include "i915_debugfs.h"
>  #include "i915_irq.h"
>  #include "i915_reg.h"
> +#include "intel_alpm.h"
>  #include "intel_crtc.h"
>  #include "intel_de.h"
>  #include "intel_crtc_state_dump.h"
> @@ -1542,6 +1543,7 @@ void intel_connector_debugfs_add(struct intel_connector *connector)
>  	intel_drrs_connector_debugfs_add(connector);
>  	intel_pps_connector_debugfs_add(connector);
>  	intel_psr_connector_debugfs_add(connector);
> +	intel_alpm_lobf_debugfs_add(connector);
>  
>  	if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
>  	    connector_type == DRM_MODE_CONNECTOR_HDMIA ||

-- 
Jani Nikula, Intel