[Intel-gfx] [PATCH v2 2/3] drm/i915: Add i915_lpsp_info debugfs

[Jani Nikula]

On Tue, 24 Mar 2020, Anshuman Gupta <anshuman.gupta at intel.com> wrote:
> New i915_pm_lpsp igt solution approach relies on connector specific
> debugfs attribute i915_lpsp_info, it exposes whether an output is
> capable of driving lpsp and exposes lpsp enablement info.
>
> v2:
> - CI fixup.
>
> Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> ---
>  .../drm/i915/display/intel_display_debugfs.c  | 104 ++++++++++++++++++
>  1 file changed, 104 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 424f4e52f783..eb9d88341d48 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -9,6 +9,7 @@
>  #include "i915_debugfs.h"
>  #include "intel_csr.h"
>  #include "intel_display_debugfs.h"
> +#include "intel_display_power.h"
>  #include "intel_display_types.h"
>  #include "intel_dp.h"
>  #include "intel_fbc.h"
> @@ -611,6 +612,95 @@ static void intel_hdcp_info(struct seq_file *m,
>  	seq_puts(m, "\n");
>  }
>  
> +static bool intel_have_embedded_panel(struct drm_connector *connector)
> +{
> +	return connector->connector_type == DRM_MODE_CONNECTOR_DSI ||
> +		connector->connector_type == DRM_MODE_CONNECTOR_eDP;

I know you don't care for this use case, but the function naming leads
one to believe this checks for at least LVDS panels too.

> +}
> +
> +static bool intel_have_gen9_lpsp_panel(struct drm_connector *connector)
> +{
> +	return intel_have_embedded_panel(connector) ||
> +		connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort;
> +}
> +
> +static int
> +intel_lpsp_power_well_enabled(struct drm_i915_private *dev_priv,
> +			      enum i915_power_well_id power_well_id)
> +{
> +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +	struct i915_power_well *power_well = NULL, *pw_itr;
> +	bool is_enabled;
> +
> +	mutex_lock(&power_domains->lock);
> +
> +	for_each_power_well(dev_priv, pw_itr)
> +		if (pw_itr->desc->id == power_well_id) {
> +			power_well = pw_itr;
> +			break;
> +		}

I don't think this code has any business looking inside the guts of the
power well code. I see that intel_hdcp.c does this kind of loop too, and
that *also* shouldn't be doing that.

See intel_display_power_well_is_enabled(). Does what you want, and does
it correctly. So really, the whole intel_lpsp_power_well_enabled()
function shouldn't be added.

> +
> +	if (drm_WARN_ON(&dev_priv->drm, !power_well)) {
> +		mutex_unlock(&power_domains->lock);
> +		/* Assume that BIOS has enabled the power well*/
> +		return true;
> +	}
> +
> +	is_enabled = !!power_well->count;
> +	mutex_unlock(&power_domains->lock);
> +
> +	return is_enabled;
> +}
> +
> +static void
> +intel_lpsp_capable_info(struct seq_file *m, struct drm_connector *connector)
> +{
> +	struct intel_encoder *encoder =
> +			intel_attached_encoder(to_intel_connector(connector));
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +	bool lpsp_capable = false;
> +
> +	if (IS_TIGERLAKE(dev_priv) && encoder->port <= PORT_C) {
> +		lpsp_capable = true;
> +	} else if (INTEL_GEN(dev_priv) >= 11 && intel_have_embedded_panel(connector)) {
> +		lpsp_capable = true;
> +	} else if (INTEL_GEN(dev_priv) >= 9 && (encoder->port == PORT_A &&
> +		   intel_have_gen9_lpsp_panel(connector))) {
> +		lpsp_capable = true;
> +	} else if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
> +		   connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> +		lpsp_capable = true;
> +	} else {
> +		seq_puts(m, "LPSP not supported\n");
> +		return;
> +	}

The whole if ladder above looks suspect, and I'm not sure your helpers
are helping here.

> +
> +	lpsp_capable ? seq_puts(m, "LPSP capable\n") : seq_puts(m, "LPSP incapable\n");

lpsp_capable is always true when you end up here.

> +}
> +
> +static void
> +intel_lpsp_enable_info(struct seq_file *m, struct drm_connector *connector)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +	bool is_lpsp = false;
> +
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		is_lpsp = !intel_lpsp_power_well_enabled(dev_priv,
> +							 ICL_DISP_PW_3);
> +	} else if (INTEL_GEN(dev_priv) >= 9) {
> +		is_lpsp = !intel_lpsp_power_well_enabled(dev_priv,
> +							 SKL_DISP_PW_2);
> +	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> +		is_lpsp = !intel_lpsp_power_well_enabled(dev_priv,
> +							 HSW_DISP_PW_GLOBAL);

The abstraction should probably be to figure out the correct power well
id based on the platform, and then using the generic power well enabled
call.

> +	} else {
> +		seq_puts(m, "LPSP not supported\n");

The "LPSP not supported" case differs from lpsp capable. Huh?

> +		return;
> +	}
> +
> +	is_lpsp ? seq_puts(m, "LPSP enabled\n") : seq_puts(m, "LPSP disabled\n");
> +}
> +
>  static void intel_dp_info(struct seq_file *m,
>  			  struct intel_connector *intel_connector)
>  {
> @@ -1987,6 +2077,17 @@ static int i915_hdcp_sink_capability_show(struct seq_file *m, void *data)
>  }
>  DEFINE_SHOW_ATTRIBUTE(i915_hdcp_sink_capability);
>  
> +static int i915_lpsp_info_show(struct seq_file *m, void *data)
> +{
> +	struct drm_connector *connector = m->private;
> +
> +	intel_lpsp_capable_info(m, connector);
> +	intel_lpsp_enable_info(m, connector);

I'm not sure having these two separate do you any good. Probably would
help *not* separating these.

> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(i915_lpsp_info);
> +
>  static int i915_dsc_fec_support_show(struct seq_file *m, void *data)
>  {
>  	struct drm_connector *connector = m->private;
> @@ -2130,5 +2231,8 @@ int intel_connector_debugfs_add(struct drm_connector *connector)
>  		debugfs_create_file("i915_dsc_fec_support", S_IRUGO, root,
>  				    connector, &i915_dsc_fec_support_fops);
>  
> +	debugfs_create_file("i915_lpsp_info", 0444, root,
> +			    connector, &i915_lpsp_info_fops);

The question is, why are you creating this for connectors that do not
and can not support lpsp to begin with? Your igt test should check for
the existence of the file anyway.

BR,
Jani.


> +
>  	return 0;
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center