[Intel-gfx] [PATCH v2 0/3 RESEND] i915 lpsp support for lpsp igt

[Intel-gfx] [PATCH v2 0/3 RESEND] i915 lpsp support for lpsp igt

[Anshuman Gupta]

This series adds i915_lpsp_info connector debugfs.

Resend to test with below msgid of igt.

Test-with: 20200324130630.9388-2-anshuman.gupta@intel.com

Anshuman Gupta (3):
  drm/i915: Power well id for ICL PG3
  drm/i915: Add i915_lpsp_info debugfs
  drm/i915: Add connector dbgfs for all connectors

 .../gpu/drm/i915/display/intel_connector.c    |   3 +
 .../drm/i915/display/intel_display_debugfs.c  | 104 ++++++++++++++++++
 .../drm/i915/display/intel_display_power.c    |   6 +-
 .../drm/i915/display/intel_display_power.h    |   2 +-
 drivers/gpu/drm/i915/display/intel_dp.c       |   3 -
 drivers/gpu/drm/i915/display/intel_hdmi.c     |   3 -
 6 files changed, 111 insertions(+), 10 deletions(-)

-- 
2.25.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 1/3] drm/i915: Power well id for ICL PG3

[Anshuman Gupta]

Gen11 onwards PG3 is contains functions for pipe B,
external displays, and VGA. It make sense to add
a power well id with name ICL_DISP_PW_3 rather then
TGL_DISP_PW_3, Also PG3 power well id requires to
know if lpsp is enabled.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_power.c | 6 +++---
 drivers/gpu/drm/i915/display/intel_display_power.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 246e406bb385..15af5177c260 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -943,7 +943,7 @@ static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
 
 	/* Power wells at this level and above must be disabled for DC5 entry */
 	if (INTEL_GEN(dev_priv) >= 12)
-		high_pg = TGL_DISP_PW_3;
+		high_pg = ICL_DISP_PW_3;
 	else
 		high_pg = SKL_DISP_PW_2;
 
@@ -3564,7 +3564,7 @@ static const struct i915_power_well_desc icl_power_wells[] = {
 		.name = "power well 3",
 		.domains = ICL_PW_3_POWER_DOMAINS,
 		.ops = &hsw_power_well_ops,
-		.id = DISP_PW_ID_NONE,
+		.id = ICL_DISP_PW_3,
 		{
 			.hsw.regs = &hsw_power_well_regs,
 			.hsw.idx = ICL_PW_CTL_IDX_PW_3,
@@ -3942,7 +3942,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
 		.name = "power well 3",
 		.domains = TGL_PW_3_POWER_DOMAINS,
 		.ops = &hsw_power_well_ops,
-		.id = TGL_DISP_PW_3,
+		.id = ICL_DISP_PW_3,
 		{
 			.hsw.regs = &hsw_power_well_regs,
 			.hsw.idx = ICL_PW_CTL_IDX_PW_3,
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index da64a5edae7a..56cbae6327b7 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -100,7 +100,7 @@ enum i915_power_well_id {
 	SKL_DISP_PW_MISC_IO,
 	SKL_DISP_PW_1,
 	SKL_DISP_PW_2,
-	TGL_DISP_PW_3,
+	ICL_DISP_PW_3,
 	SKL_DISP_DC_OFF,
 };
 
-- 
2.25.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Anshuman Gupta]

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@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;
+}
+
+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;
+		}
+
+	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;
+	}
+
+	lpsp_capable ? seq_puts(m, "LPSP capable\n") : seq_puts(m, "LPSP incapable\n");
+}
+
+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);
+	} else {
+		seq_puts(m, "LPSP not supported\n");
+		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);
+
+	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);
+
 	return 0;
 }
-- 
2.25.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Tue, 24 Mar 2020, Anshuman Gupta <anshuman.gupta@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Anshuman Gupta]

On 2020-03-24 at 17:53:08 +0200, Jani Nikula wrote:
> On Tue, 24 Mar 2020, Anshuman Gupta <anshuman.gupta@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@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.
Thanks Jani for review comments, actually at v1 version i had used 
used intel_display_power_well_is_enabled,
but i was getting below DMEAG_WARN on a CI run,

"PM wakelock ref not held during HW access"

Apperently it seemed to me intel_display_power_well_is_enabled() is meant for
internal use in intel_display_power.c as it was not declared in header and it
didn't held any PM wakeref.

May be i require below sequence to use intel_display_power_well_is_enabled,

wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
intel_display_power_well_is_enabled()
intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);

Please correct me if i am wrong here.
> 
> > +
> > +	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?
My bad while fixing regression on legacy platform i messed it,
i will fix this.
> 
> > +		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.
igt checks for file existence and it will assert the subtest but this
will also fail the test as earlier legacy platform were passing non-lpsp
i915_pm_rpm test considering all non-edp paneld are non-lpsp
https://patchwork.freedesktop.org/patch/358652/?series=74647&rev=4
May be igt lib function i915_output_is_lpsp_capable() should return
false for lgeacy platform and legacy connector,
Please correct me if there is any other better way to handle that. 
> 
> BR,
> Jani.
> 
> 
> > +
> >  	return 0;
> >  }
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 3/3] drm/i915: Add connector dbgfs for all connectors

[Anshuman Gupta]

Add connector debugfs attributes for each intel
connector which is getting register.

v2:
- adding connector debugfs for each connector in
  intel_connector_register() to fix CI failure for legacy connectors.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_connector.c | 3 +++
 drivers/gpu/drm/i915/display/intel_dp.c        | 3 ---
 drivers/gpu/drm/i915/display/intel_hdmi.c      | 3 ---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index 903e49659f56..0cf5fe326a0b 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -33,6 +33,7 @@
 
 #include "i915_drv.h"
 #include "intel_connector.h"
+#include "intel_display_debugfs.h"
 #include "intel_display_types.h"
 #include "intel_hdcp.h"
 
@@ -123,6 +124,8 @@ int intel_connector_register(struct drm_connector *connector)
 		goto err_backlight;
 	}
 
+	intel_connector_debugfs_add(connector);
+
 	return 0;
 
 err_backlight:
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 7f1a4e55cda1..c4352d013c29 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -48,7 +48,6 @@
 #include "intel_audio.h"
 #include "intel_connector.h"
 #include "intel_ddi.h"
-#include "intel_display_debugfs.h"
 #include "intel_display_types.h"
 #include "intel_dp.h"
 #include "intel_dp_link_training.h"
@@ -6204,8 +6203,6 @@ intel_dp_connector_register(struct drm_connector *connector)
 	if (ret)
 		return ret;
 
-	intel_connector_debugfs_add(connector);
-
 	DRM_DEBUG_KMS("registering %s bus for %s\n",
 		      intel_dp->aux.name, connector->kdev->kobj.name);
 
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 39930232b253..2d4dced7143e 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -44,7 +44,6 @@
 #include "intel_audio.h"
 #include "intel_connector.h"
 #include "intel_ddi.h"
-#include "intel_display_debugfs.h"
 #include "intel_display_types.h"
 #include "intel_dp.h"
 #include "intel_dpio_phy.h"
@@ -2813,8 +2812,6 @@ intel_hdmi_connector_register(struct drm_connector *connector)
 	if (ret)
 		return ret;
 
-	intel_connector_debugfs_add(connector);
-
 	intel_hdmi_create_i2c_symlink(connector);
 
 	return ret;
-- 
2.25.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Add connector dbgfs for all connectors

[Jani Nikula]

On Tue, 24 Mar 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> Add connector debugfs attributes for each intel
> connector which is getting register.

Okay, so this is a good idea, and for that,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> v2:
> - adding connector debugfs for each connector in
>   intel_connector_register() to fix CI failure for legacy connectors.

However, it's *not* a good idea for the purpose of registering the lpsp
debugfs file for every connector out there. There's no point in that, at
all. You need to register the file only when it makes sense in
intel_connector_debugfs_add(), and not for every conceivable connector
out there.

And your igt test absolutely *must* check if the debugfs is there or
not, and handle it gracefully.

BR,
Jani.

>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_connector.c | 3 +++
>  drivers/gpu/drm/i915/display/intel_dp.c        | 3 ---
>  drivers/gpu/drm/i915/display/intel_hdmi.c      | 3 ---
>  3 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
> index 903e49659f56..0cf5fe326a0b 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.c
> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> @@ -33,6 +33,7 @@
>  
>  #include "i915_drv.h"
>  #include "intel_connector.h"
> +#include "intel_display_debugfs.h"
>  #include "intel_display_types.h"
>  #include "intel_hdcp.h"
>  
> @@ -123,6 +124,8 @@ int intel_connector_register(struct drm_connector *connector)
>  		goto err_backlight;
>  	}
>  
> +	intel_connector_debugfs_add(connector);
> +
>  	return 0;
>  
>  err_backlight:
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 7f1a4e55cda1..c4352d013c29 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -48,7 +48,6 @@
>  #include "intel_audio.h"
>  #include "intel_connector.h"
>  #include "intel_ddi.h"
> -#include "intel_display_debugfs.h"
>  #include "intel_display_types.h"
>  #include "intel_dp.h"
>  #include "intel_dp_link_training.h"
> @@ -6204,8 +6203,6 @@ intel_dp_connector_register(struct drm_connector *connector)
>  	if (ret)
>  		return ret;
>  
> -	intel_connector_debugfs_add(connector);
> -
>  	DRM_DEBUG_KMS("registering %s bus for %s\n",
>  		      intel_dp->aux.name, connector->kdev->kobj.name);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 39930232b253..2d4dced7143e 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -44,7 +44,6 @@
>  #include "intel_audio.h"
>  #include "intel_connector.h"
>  #include "intel_ddi.h"
> -#include "intel_display_debugfs.h"
>  #include "intel_display_types.h"
>  #include "intel_dp.h"
>  #include "intel_dpio_phy.h"
> @@ -2813,8 +2812,6 @@ intel_hdmi_connector_register(struct drm_connector *connector)
>  	if (ret)
>  		return ret;
>  
> -	intel_connector_debugfs_add(connector);
> -
>  	intel_hdmi_create_i2c_symlink(connector);
>  
>  	return ret;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BUILD: failure for i915 lpsp support for lpsp igt (rev4)

[Patchwork]

== Series Details ==

Series: i915 lpsp support for lpsp igt (rev4)
URL   : https://patchwork.freedesktop.org/series/74648/
State : failure

== Summary ==

Patch is empty.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 0/3] i915 lpsp support for lpsp igt

[Anshuman Gupta]

This series adds i915_lpsp_info connector debugfs.

Test-with: 20200323063248.5261-2-anshuman.gupta@intel.com

Anshuman Gupta (3):
  drm/i915: Power well id for ICL PG3
  drm/i915: Add i915_lpsp_info debugfs
  drm/i915: Add connector dbgfs for all connectors

 .../gpu/drm/i915/display/intel_connector.c    |   3 +
 .../drm/i915/display/intel_display_debugfs.c  | 102 ++++++++++++++++++
 .../drm/i915/display/intel_display_power.c    |   6 +-
 .../drm/i915/display/intel_display_power.h    |   2 +-
 drivers/gpu/drm/i915/display/intel_dp.c       |   3 -
 drivers/gpu/drm/i915/display/intel_hdmi.c     |   3 -
 6 files changed, 109 insertions(+), 10 deletions(-)

-- 
2.25.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 3/3] drm/i915: Add connector dbgfs for all connectors

[Anshuman Gupta]

Add connector debugfs attributes for each intel
connector which is getting register.

v2:
- adding connector debugfs for each connector in
  intel_connector_register() to fix CI failure for legacy connectors.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_connector.c | 3 +++
 drivers/gpu/drm/i915/display/intel_dp.c        | 3 ---
 drivers/gpu/drm/i915/display/intel_hdmi.c      | 3 ---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c
index 903e49659f56..0cf5fe326a0b 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -33,6 +33,7 @@
 
 #include "i915_drv.h"
 #include "intel_connector.h"
+#include "intel_display_debugfs.h"
 #include "intel_display_types.h"
 #include "intel_hdcp.h"
 
@@ -123,6 +124,8 @@ int intel_connector_register(struct drm_connector *connector)
 		goto err_backlight;
 	}
 
+	intel_connector_debugfs_add(connector);
+
 	return 0;
 
 err_backlight:
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 49d01d33de69..db90322a6315 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -48,7 +48,6 @@
 #include "intel_audio.h"
 #include "intel_connector.h"
 #include "intel_ddi.h"
-#include "intel_display_debugfs.h"
 #include "intel_display_types.h"
 #include "intel_dp.h"
 #include "intel_dp_link_training.h"
@@ -6204,8 +6203,6 @@ intel_dp_connector_register(struct drm_connector *connector)
 	if (ret)
 		return ret;
 
-	intel_connector_debugfs_add(connector);
-
 	DRM_DEBUG_KMS("registering %s bus for %s\n",
 		      intel_dp->aux.name, connector->kdev->kobj.name);
 
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 39930232b253..2d4dced7143e 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -44,7 +44,6 @@
 #include "intel_audio.h"
 #include "intel_connector.h"
 #include "intel_ddi.h"
-#include "intel_display_debugfs.h"
 #include "intel_display_types.h"
 #include "intel_dp.h"
 #include "intel_dpio_phy.h"
@@ -2813,8 +2812,6 @@ intel_hdmi_connector_register(struct drm_connector *connector)
 	if (ret)
 		return ret;
 
-	intel_connector_debugfs_add(connector);
-
 	intel_hdmi_create_i2c_symlink(connector);
 
 	return ret;
-- 
2.25.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx