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

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

On Mon, 20 May 2024, Animesh Manna <animesh.manna@intel.com> wrote:
> For validation purpose add debugfs for LOBF.
>
> v1: Initial version.
> v2: Add aux-wake/less info along with lobf status. [Jouni]
>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_alpm.c     | 49 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_alpm.h     |  2 +
>  .../drm/i915/display/intel_display_debugfs.c  |  2 +
>  3 files changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 8f4da817ef55..843ffb5fcb7a 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -360,3 +360,52 @@ void intel_alpm_configure(struct intel_dp *intel_dp,
>  {
>  	lnl_alpm_configure(intel_dp, crtc_state);
>  }
> +
> +static int i915_edp_lobf_info_show(struct seq_file *m, void *data)
> +{
> +	struct intel_connector *connector = m->private;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct drm_crtc *crtc;
> +	struct intel_crtc_state *crtc_state;
> +	enum transcoder cpu_transcoder;
> +	u32 alpm_ctl;
> +	int ret;
> +
> +	ret = drm_modeset_lock_single_interruptible(&dev_priv->drm.mode_config.connection_mutex);
> +	if (ret)
> +		return ret;
> +
> +	crtc = connector->base.state->crtc;
> +	if (connector->base.status != connector_status_connected || !crtc) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	crtc_state = to_intel_crtc_state(crtc->state);
> +	cpu_transcoder = crtc_state->cpu_transcoder;
> +	alpm_ctl = intel_de_read(dev_priv, ALPM_CTL(dev_priv, cpu_transcoder));
> +	seq_printf(m, "LOBF status: %s\n", str_enabled_disabled(alpm_ctl & ALPM_CTL_LOBF_ENABLE));
> +	seq_printf(m, "Aux-wake alpm status: %s\n",
> +		   str_enabled_disabled(!(alpm_ctl & ALPM_CTL_ALPM_AUX_LESS_ENABLE)));
> +	seq_printf(m, "Aux-less alpm status: %s\n",
> +		   str_enabled_disabled(alpm_ctl & ALPM_CTL_ALPM_AUX_LESS_ENABLE));
> +out:
> +	drm_modeset_unlock(&dev_priv->drm.mode_config.connection_mutex);
> +
> +	return ret;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_info);
> +
> +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector)

This file is about alpm, and might add more alpm related debugfs files
later. There's no need to encode lobf in the name here.

> +{
> +	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_info", 0444, root,

Why does the filename need to include edp? The connector debugfs files
for psr don't include that either.

> +			    connector, &i915_edp_lobf_info_fops);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h b/drivers/gpu/drm/i915/display/intel_alpm.h
> index fd9be8aa876c..0dab2068164a 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;
>  
>  void intel_alpm_get_capability(struct intel_dp *intel_dp);
>  bool intel_alpm_compute_params(struct intel_dp *intel_dp,
> @@ -20,4 +21,5 @@ void intel_alpm_compute_lobf_config(struct intel_dp *intel_dp,
>  				    struct drm_connector_state *conn_state);
>  void intel_alpm_configure(struct intel_dp *intel_dp,
>  			  const struct intel_crtc_state *crtc_state);
> +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 35f9f86ef70f..86d9900c40af 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"
> @@ -1515,6 +1516,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);

All the others are intel_foo_connector_debugfs_add(). So should this.

>  
>  	if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
>  	    connector_type == DRM_MODE_CONNECTOR_HDMIA ||

-- 
Jani Nikula, Intel