diff mbox series

drm/i915/lspcon: enter standby mode to enhance power saving

Message ID 20201116135913.20782-1-shawn.c.lee@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/lspcon: enter standby mode to enhance power saving | expand

Commit Message

Lee, Shawn C Nov. 16, 2020, 1:59 p.m. UTC
After system boot up, LSPCON will be configured as PCON mode.
But it never go into power saving state. Source driver can
do the following. Then LSPCON can enter standby mode
automatically to save more power.

1. At PCON mode, source driver write 0x2 to DPCD 600h.
2. At LS mode, try to disable DP_DUAL_MODE_TMDS_OEN.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>
Cc: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c     | 7 ++++++-
 drivers/gpu/drm/i915/display/intel_lspcon.c | 8 ++++++++
 drivers/gpu/drm/i915/display/intel_lspcon.h | 1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

Comments

Ville Syrjälä Nov. 17, 2020, 4:29 p.m. UTC | #1
On Mon, Nov 16, 2020 at 09:59:13PM +0800, Lee Shawn C wrote:
> After system boot up, LSPCON will be configured as PCON mode.
> But it never go into power saving state. Source driver can
> do the following. Then LSPCON can enter standby mode
> automatically to save more power.
> 
> 1. At PCON mode, source driver write 0x2 to DPCD 600h.
> 2. At LS mode, try to disable DP_DUAL_MODE_TMDS_OEN.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
> Cc: Khaled Almahallawy <khaled.almahallawy@intel.com>
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     | 7 ++++++-
>  drivers/gpu/drm/i915/display/intel_lspcon.c | 8 ++++++++
>  drivers/gpu/drm/i915/display/intel_lspcon.h | 1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index ec8359f03aaf..7dd16d6bd5ba 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6536,6 +6536,7 @@ intel_dp_detect(struct drm_connector *connector,
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>  	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>  	struct intel_encoder *encoder = &dig_port->base;
>  	enum drm_connector_status status;
>  
> @@ -6632,9 +6633,13 @@ intel_dp_detect(struct drm_connector *connector,
>  	intel_dp_check_service_irq(intel_dp);
>  
>  out:
> -	if (status != connector_status_connected && !intel_dp->is_mst)
> +	if (status != connector_status_connected && !intel_dp->is_mst) {
>  		intel_dp_unset_edid(intel_dp);
>  
> +		if (lspcon && lspcon->active)
> +			lspcon_standby(dp_to_dig_port(intel_dp));

We should proably just do this for all DP devices. But I'm not sure if
we can just do it uncoditionally like this. We should perhaps first check
that the connector is not in use. So doing this sort of stuff as part
of the normal init/resume sanitization process might be the better option.

> +	}
> +
>  	/*
>  	 * Make sure the refs for power wells enabled during detect are
>  	 * dropped to avoid a new detect cycle triggered by HPD polling.
> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c b/drivers/gpu/drm/i915/display/intel_lspcon.c
> index e37d45e531df..4913ff20d7b4 100644
> --- a/drivers/gpu/drm/i915/display/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
> @@ -550,6 +550,14 @@ static bool lspcon_init(struct intel_digital_port *dig_port)
>  	return true;
>  }
>  
> +void lspcon_standby(struct intel_digital_port *dig_port)
> +{
> +	struct intel_dp *dp = &dig_port->dp;
> +
> +	if (drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D3) <= 0)
> +		DRM_DEBUG_KMS("Failed to write EDID checksum\n");

Eh?

> +}
> +
>  void lspcon_resume(struct intel_digital_port *dig_port)
>  {
>  	struct intel_lspcon *lspcon = &dig_port->lspcon;
> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.h b/drivers/gpu/drm/i915/display/intel_lspcon.h
> index b03dcb7076d8..658a2e5b22db 100644
> --- a/drivers/gpu/drm/i915/display/intel_lspcon.h
> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.h
> @@ -16,6 +16,7 @@ struct intel_encoder;
>  struct intel_lspcon;
>  
>  void lspcon_resume(struct intel_digital_port *dig_port);
> +void lspcon_standby(struct intel_digital_port *dig_port);
>  void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
>  void lspcon_write_infoframe(struct intel_encoder *encoder,
>  			    const struct intel_crtc_state *crtc_state,
> -- 
> 2.17.1
Lee, Shawn C Nov. 18, 2020, 3:10 a.m. UTC | #2
On Tue, Nov. 17, 2020, 4:29 p.m., Ville Syrjälä wrote:
>On Mon, Nov 16, 2020 at 09:59:13PM +0800, Lee Shawn C wrote:
>> After system boot up, LSPCON will be configured as PCON mode.
>> But it never go into power saving state. Source driver can do the 
>> following. Then LSPCON can enter standby mode automatically to save 
>> more power.
>> 
>> 1. At PCON mode, source driver write 0x2 to DPCD 600h.
>> 2. At LS mode, try to disable DP_DUAL_MODE_TMDS_OEN.
>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Uma Shankar <uma.shankar@intel.com>
>> Cc: Cooper Chiou <cooper.chiou@intel.com>
>> Cc: Khaled Almahallawy <khaled.almahallawy@intel.com>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp.c     | 7 ++++++-
>>  drivers/gpu/drm/i915/display/intel_lspcon.c | 8 ++++++++  
>> drivers/gpu/drm/i915/display/intel_lspcon.h | 1 +
>>  3 files changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index ec8359f03aaf..7dd16d6bd5ba 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -6536,6 +6536,7 @@ intel_dp_detect(struct drm_connector *connector,
>>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>  	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
>>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> +	struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>>  	struct intel_encoder *encoder = &dig_port->base;
>>  	enum drm_connector_status status;
>>  
>> @@ -6632,9 +6633,13 @@ intel_dp_detect(struct drm_connector *connector,
>>  	intel_dp_check_service_irq(intel_dp);
>>  
>>  out:
>> -	if (status != connector_status_connected && !intel_dp->is_mst)
>> +	if (status != connector_status_connected && !intel_dp->is_mst) {
>>  		intel_dp_unset_edid(intel_dp);
>>  
>> +		if (lspcon && lspcon->active)
>> +			lspcon_standby(dp_to_dig_port(intel_dp));
>
>We should proably just do this for all DP devices. But I'm not sure if we can just do it uncoditionally like this. We should perhaps first check that the connector is not in use. So doing this sort of stuff as part of the normal init/resume sanitization process might be the better option.
>

Source driver already set sink power state to D3 in intel_ddi_post_disable_dp().
We learned from LSPCON vendor that configure power state to D3 must be the latest AUX command.
Any aux communication after that, LSPCON will be wake up until source set it to D3 next time.

Some user space application (ex: chrome os) used IOCTL to monitor external display's connection.
intel_dp_detect() will be called and try to read sink's info via AUX. In this case, LSPCON will
be active again. If we found there is LSPCON on this port and no more sink connected.
Source driver set power state to D3 here to make LSPCON enter standby mode.

>> +	}
>> +
>>  	/*
>>  	 * Make sure the refs for power wells enabled during detect are
>>  	 * dropped to avoid a new detect cycle triggered by HPD polling.
>> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c 
>> b/drivers/gpu/drm/i915/display/intel_lspcon.c
>> index e37d45e531df..4913ff20d7b4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_lspcon.c
>> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
>> @@ -550,6 +550,14 @@ static bool lspcon_init(struct intel_digital_port *dig_port)
>>  	return true;
>>  }
>>  
>> +void lspcon_standby(struct intel_digital_port *dig_port) {
>> +	struct intel_dp *dp = &dig_port->dp;
>> +
>> +	if (drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D3) <= 0)
>> +		DRM_DEBUG_KMS("Failed to write EDID checksum\n");
>
>Eh?
>

Sorry I miss to modify the error messages here. I will fix it later.

>> +}
>> +
>>  void lspcon_resume(struct intel_digital_port *dig_port)  {
>>  	struct intel_lspcon *lspcon = &dig_port->lspcon; diff --git 
>> a/drivers/gpu/drm/i915/display/intel_lspcon.h 
>> b/drivers/gpu/drm/i915/display/intel_lspcon.h
>> index b03dcb7076d8..658a2e5b22db 100644
>> --- a/drivers/gpu/drm/i915/display/intel_lspcon.h
>> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.h
>> @@ -16,6 +16,7 @@ struct intel_encoder;  struct intel_lspcon;
>>  
>>  void lspcon_resume(struct intel_digital_port *dig_port);
>> +void lspcon_standby(struct intel_digital_port *dig_port);
>>  void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);  void 
>> lspcon_write_infoframe(struct intel_encoder *encoder,
>>  			    const struct intel_crtc_state *crtc_state,
>> --
>> 2.17.1
>
>--
>Ville Syrjälä
>Intel
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index ec8359f03aaf..7dd16d6bd5ba 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -6536,6 +6536,7 @@  intel_dp_detect(struct drm_connector *connector,
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
 	struct intel_encoder *encoder = &dig_port->base;
 	enum drm_connector_status status;
 
@@ -6632,9 +6633,13 @@  intel_dp_detect(struct drm_connector *connector,
 	intel_dp_check_service_irq(intel_dp);
 
 out:
-	if (status != connector_status_connected && !intel_dp->is_mst)
+	if (status != connector_status_connected && !intel_dp->is_mst) {
 		intel_dp_unset_edid(intel_dp);
 
+		if (lspcon && lspcon->active)
+			lspcon_standby(dp_to_dig_port(intel_dp));
+	}
+
 	/*
 	 * Make sure the refs for power wells enabled during detect are
 	 * dropped to avoid a new detect cycle triggered by HPD polling.
diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c b/drivers/gpu/drm/i915/display/intel_lspcon.c
index e37d45e531df..4913ff20d7b4 100644
--- a/drivers/gpu/drm/i915/display/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
@@ -550,6 +550,14 @@  static bool lspcon_init(struct intel_digital_port *dig_port)
 	return true;
 }
 
+void lspcon_standby(struct intel_digital_port *dig_port)
+{
+	struct intel_dp *dp = &dig_port->dp;
+
+	if (drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D3) <= 0)
+		DRM_DEBUG_KMS("Failed to write EDID checksum\n");
+}
+
 void lspcon_resume(struct intel_digital_port *dig_port)
 {
 	struct intel_lspcon *lspcon = &dig_port->lspcon;
diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.h b/drivers/gpu/drm/i915/display/intel_lspcon.h
index b03dcb7076d8..658a2e5b22db 100644
--- a/drivers/gpu/drm/i915/display/intel_lspcon.h
+++ b/drivers/gpu/drm/i915/display/intel_lspcon.h
@@ -16,6 +16,7 @@  struct intel_encoder;
 struct intel_lspcon;
 
 void lspcon_resume(struct intel_digital_port *dig_port);
+void lspcon_standby(struct intel_digital_port *dig_port);
 void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
 void lspcon_write_infoframe(struct intel_encoder *encoder,
 			    const struct intel_crtc_state *crtc_state,