[PATCH 0/5] drm/i915 Avoid long delays when reading EDID on eDP

[PATCH 0/5] drm/i915 Avoid long delays when reading EDID on eDP

[Egbert Eich]

For eDP in the Intel driver pps_lock()/unlock() need to be called before      
initiating an I2C/AUX channel transfer. These operations can be quite         
expensive - especially on values for HZ lower than 1000.                      
It is therefore better to perfrom this locking/unlocking only once,           
ie at the beginning and at the end of the entire I2C transfer.                
The current design of drm_dp_helper.c doesn't allow this.                     
This patchset modifies drm_dp_helper.c and moves the locking/unlocking        
operation to the top.                                                         
This fixes the long delay observed in                                         
   https://bugs.freedesktop.org/show_bug.cgi?id=86201                         

Egbert Eich (4):
  drm/DP: Create pointer to generic DPCD access function
  drm/DP: Export drm_dp_i2c_xfer() DP helper function
  drm/DP: Export drm_dp_dpcd_access() DP helper function
  drm/i915/eDP: Move pps_lock() and edp_panel_vdd_on() to top

Ville Syrjälä (1):
  drm/i915: Try to avoid pps_{lock,unlock}() on DP ports

 drivers/gpu/drm/drm_dp_helper.c  |  11 ++--
 drivers/gpu/drm/i915/intel_dp.c  | 132 +++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h |   5 ++
 include/drm/drm_dp_helper.h      |  14 +++++
 4 files changed, 133 insertions(+), 29 deletions(-)

-- 
1.8.4.5

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

[PATCH 1/5] drm/i915: Try to avoid pps_{lock, unlock}() on DP ports

[Egbert Eich]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 48 ++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 81f959d..a24c8cc7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -807,17 +807,19 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	uint32_t status;
 	int try, clock = 0;
 	bool has_aux_irq = HAS_AUX_IRQ(dev);
-	bool vdd;
+	bool vdd = false;
 
-	pps_lock(intel_dp);
+	if (is_edp(intel_dp)) {
+		pps_lock(intel_dp);
 
-	/*
-	 * We will be called with VDD already enabled for dpcd/edid/oui reads.
-	 * In such cases we want to leave VDD enabled and it's up to upper layers
-	 * to turn it off. But for eg. i2c-dev access we need to turn it on/off
-	 * ourselves.
-	 */
-	vdd = edp_panel_vdd_on(intel_dp);
+		/*
+		 * We will be called with VDD already enabled for dpcd/edid/oui reads.
+		 * In such cases we want to leave VDD enabled and it's up to upper layers
+		 * to turn it off. But for eg. i2c-dev access we need to turn it on/off
+		 * ourselves.
+		 */
+		vdd = edp_panel_vdd_on(intel_dp);
+	}
 
 	/* dp aux is extremely sensitive to irq latency, hence request the
 	 * lowest possible wakeup latency and so prevent the cpu from going into
@@ -924,10 +926,12 @@ out:
 	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
 	intel_aux_display_runtime_put(dev_priv);
 
-	if (vdd)
-		edp_panel_vdd_off(intel_dp, false);
+	if (is_edp(intel_dp)) {
+		if (vdd)
+			edp_panel_vdd_off(intel_dp, false);
 
-	pps_unlock(intel_dp);
+		pps_unlock(intel_dp);
+	}
 
 	return ret;
 }
@@ -2291,18 +2295,22 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	if (WARN_ON(dp_reg & DP_PORT_EN))
 		return;
 
-	pps_lock(intel_dp);
+	if (is_edp(intel_dp)) {
+		pps_lock(intel_dp);
 
-	if (IS_VALLEYVIEW(dev))
-		vlv_init_panel_power_sequencer(intel_dp);
+		if (IS_VALLEYVIEW(dev))
+			vlv_init_panel_power_sequencer(intel_dp);
 
-	intel_dp_enable_port(intel_dp);
+		intel_dp_enable_port(intel_dp);
 
-	edp_panel_vdd_on(intel_dp);
-	edp_panel_on(intel_dp);
-	edp_panel_vdd_off(intel_dp, true);
+		edp_panel_vdd_on(intel_dp);
+		edp_panel_on(intel_dp);
+		edp_panel_vdd_off(intel_dp, true);
 
-	pps_unlock(intel_dp);
+		pps_unlock(intel_dp);
+	} else {
+		intel_dp_enable_port(intel_dp);
+	}
 
 	if (IS_VALLEYVIEW(dev))
 		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
-- 
1.8.4.5

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

[PATCH 2/5] drm/DP: Create pointer to generic DPCD access function

[Egbert Eich]

This way a driver can replace this function with its own version.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_dp_helper.c | 5 +++--
 include/drm/drm_dp_helper.h     | 8 ++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 959e207..d132838 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -238,7 +238,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 			 void *buffer, size_t size)
 {
-	return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
+	return aux->dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
 				  size);
 }
 EXPORT_SYMBOL(drm_dp_dpcd_read);
@@ -260,7 +260,7 @@ EXPORT_SYMBOL(drm_dp_dpcd_read);
 ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
 			  void *buffer, size_t size)
 {
-	return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer,
+	return aux->dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer,
 				  size);
 }
 EXPORT_SYMBOL(drm_dp_dpcd_write);
@@ -552,6 +552,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
 int drm_dp_aux_register(struct drm_dp_aux *aux)
 {
 	mutex_init(&aux->hw_mutex);
+	aux->dpcd_access = drm_dp_dpcd_access;
 
 	aux->ddc.algo = &drm_dp_i2c_algo;
 	aux->ddc.algo_data = aux;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 11f8c84..cd7b464 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -497,6 +497,8 @@ struct drm_dp_aux_msg {
  * @dev: pointer to struct device that is the parent for this AUX channel
  * @hw_mutex: internal mutex used for locking transfers
  * @transfer: transfers a message representing a single AUX transaction
+ * @dp_dpcd_access: hook to optionally wrap or replace high level dpcd message
+ *                  function.
  *
  * The .dev field should be set to a pointer to the device that implements
  * the AUX channel.
@@ -519,6 +521,10 @@ struct drm_dp_aux_msg {
  * transactions. The drm_dp_aux_register_i2c_bus() function registers an
  * I2C adapter that can be passed to drm_probe_ddc(). Upon removal, drivers
  * should call drm_dp_aux_unregister_i2c_bus() to remove the I2C adapter.
+ * dpcd_access() can be used by the driver to wrap or replace the generic
+ * dpcd access function in case certain operations should be performed before
+ * a dpcd transaction is started if this cannot reasonably performed in
+ * .transfer(). It is initialized to its default value by drm_dp_aux_register().
  *
  * Note that the aux helper code assumes that the .transfer() function
  * only modifies the reply field of the drm_dp_aux_msg structure.  The
@@ -531,6 +537,8 @@ struct drm_dp_aux {
 	struct mutex hw_mutex;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
+	int (*dpcd_access)(struct drm_dp_aux *aux, u8 request,
+			     unsigned int offset, void *buffer, size_t size);
 	unsigned i2c_nack_count, i2c_defer_count;
 };
 
-- 
1.8.4.5

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

[PATCH 3/5] drm/DP: Export drm_dp_i2c_xfer() DP helper function

[Egbert Eich]

It may be required to wrap the generic DP I2C transfer function to
perfrom certain operations before of after this function is called.
Make this function available to the driver.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_dp_helper.c | 3 ++-
 include/drm/drm_dp_helper.h     | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index d132838..f17ac01 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -481,7 +481,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	return -EREMOTEIO;
 }
 
-static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
+int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 			   int num)
 {
 	struct drm_dp_aux *aux = adapter->algo_data;
@@ -537,6 +537,7 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 
 	return err;
 }
+EXPORT_SYMBOL(drm_dp_i2c_xfer);
 
 static const struct i2c_algorithm drm_dp_i2c_algo = {
 	.functionality = drm_dp_i2c_functionality,
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index cd7b464..0103b7f 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -423,6 +423,8 @@ void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
 
 u8 drm_dp_link_rate_to_bw_code(int link_rate);
 int drm_dp_bw_code_to_link_rate(u8 link_bw);
+int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
+		    int num);
 
 struct edp_sdp_header {
 	u8 HB0; /* Secondary Data Packet ID */
-- 
1.8.4.5

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

[PATCH 4/5] drm/DP: Export drm_dp_dpcd_access() DP helper function

[Egbert Eich]

It may be required to wrap the generic DP DPCD transfer function to
perfrom certain operations before of after this function is called.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/drm_dp_helper.c | 3 ++-
 include/drm/drm_dp_helper.h     | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index f17ac01..2a731e6 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -171,7 +171,7 @@ EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
  * Both native and I2C-over-AUX transactions are supported.
  */
 
-static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
+int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 			      unsigned int offset, void *buffer, size_t size)
 {
 	struct drm_dp_aux_msg msg;
@@ -220,6 +220,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 	DRM_DEBUG_KMS("too many retries, giving up\n");
 	return -EIO;
 }
+EXPORT_SYMBOL(drm_dp_dpcd_access);
 
 /**
  * drm_dp_dpcd_read() - read a series of bytes from the DPCD
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 0103b7f..ad04f64 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -405,6 +405,8 @@
 #define MODE_I2C_READ	4
 #define MODE_I2C_STOP	8
 
+struct drm_dp_aux;
+
 #define DP_LINK_STATUS_SIZE	   6
 bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
 			  int lane_count);
@@ -423,6 +425,8 @@ void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
 
 u8 drm_dp_link_rate_to_bw_code(int link_rate);
 int drm_dp_bw_code_to_link_rate(u8 link_bw);
+int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
+		       unsigned int offset, void *buffer, size_t size);
 int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 		    int num);
 
-- 
1.8.4.5

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

[PATCH 5/5] drm/i915/eDP: Move pps_lock() and edp_panel_vdd_on() to top

[Egbert Eich]

On eDP each low level transfer is wrapped by pps_lock()/unlock()
and edp_panel_vdd_on()/off(). Since PPS locking can be quite expensive
and each call to master_xfer() or dpcd_access() may call these low level
transfer functions many times it may introduce a considerable delay.
Move locking/VDD enablement to the top so that it is performed only
once per master_xfer() or dpcd_access().
This fixes https://bugs.freedesktop.org/show_bug.cgi?id=86201

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/intel_dp.c  | 116 ++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |   5 ++
 2 files changed, 100 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a24c8cc7..164f80e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -807,19 +807,6 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	uint32_t status;
 	int try, clock = 0;
 	bool has_aux_irq = HAS_AUX_IRQ(dev);
-	bool vdd = false;
-
-	if (is_edp(intel_dp)) {
-		pps_lock(intel_dp);
-
-		/*
-		 * We will be called with VDD already enabled for dpcd/edid/oui reads.
-		 * In such cases we want to leave VDD enabled and it's up to upper layers
-		 * to turn it off. But for eg. i2c-dev access we need to turn it on/off
-		 * ourselves.
-		 */
-		vdd = edp_panel_vdd_on(intel_dp);
-	}
 
 	/* dp aux is extremely sensitive to irq latency, hence request the
 	 * lowest possible wakeup latency and so prevent the cpu from going into
@@ -926,13 +913,6 @@ out:
 	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
 	intel_aux_display_runtime_put(dev_priv);
 
-	if (is_edp(intel_dp)) {
-		if (vdd)
-			edp_panel_vdd_off(intel_dp, false);
-
-		pps_unlock(intel_dp);
-	}
-
 	return ret;
 }
 
@@ -1001,6 +981,97 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	return ret;
 }
 
+int intel_edp_dpcd_access(struct drm_dp_aux *aux, u8 request,
+			      unsigned int offset, void *buffer, size_t size)
+{
+	struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
+	bool vdd = false;
+	int ret;
+
+	pps_lock(intel_dp);
+	/*
+	 * We will be called with VDD already enabled for dpcd/edid/oui reads.
+	 * In such cases we want to leave VDD enabled and it's up to upper
+	 * layers to turn it off. But for eg. i2c-dev access we need to turn
+	 * it on/off ourselves.
+	 */
+	vdd = edp_panel_vdd_on(intel_dp);
+
+	ret = drm_dp_dpcd_access(aux, request, offset, buffer, size);
+
+	if (vdd)
+		edp_panel_vdd_off(intel_dp, false);
+
+	pps_unlock(intel_dp);
+
+	return ret;
+}
+
+static int
+intel_edp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
+			  int num)
+{
+	struct drm_dp_aux *aux = adapter->algo_data;
+	struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
+	bool vdd = false;
+	int ret;
+
+	pps_lock(intel_dp);
+	/*
+	 * We will be called with VDD already enabled for dpcd/edid/oui reads.
+	 * In such cases we want to leave VDD enabled and it's up to upper
+	 * layers to turn it off. But for eg. i2c-dev access we need to turn
+	 * it on/off ourselves.
+	 */
+	vdd = edp_panel_vdd_on(intel_dp);
+
+	ret = drm_dp_i2c_xfer(adapter, msgs, num);
+
+	if (vdd)
+		edp_panel_vdd_off(intel_dp, false);
+
+	pps_unlock(intel_dp);
+
+	return ret;
+}
+
+static u32 intel_edp_i2c_functionality(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+	       I2C_FUNC_SMBUS_READ_BLOCK_DATA |
+	       I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
+	       I2C_FUNC_10BIT_ADDR;
+}
+
+static const struct i2c_algorithm intel_edp_i2c_algo = {
+	.functionality = intel_edp_i2c_functionality,
+	.master_xfer = intel_edp_i2c_xfer,
+};
+
+/*
+ * intel_edp_aux_register() - Intel replacement for
+ *                           drm_dp_aux_register().
+ */
+static int intel_edp_aux_register(struct drm_dp_aux *aux)
+{
+	mutex_init(&aux->hw_mutex);
+	aux->dpcd_access = intel_edp_dpcd_access;
+
+	aux->ddc.algo = &intel_edp_i2c_algo;
+	aux->ddc.algo_data = aux;
+	aux->ddc.retries = 3;
+
+	aux->ddc.class = I2C_CLASS_DDC;
+	aux->ddc.owner = THIS_MODULE;
+	aux->ddc.dev.parent = aux->dev;
+	aux->ddc.dev.of_node = aux->dev->of_node;
+
+	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
+		sizeof(aux->ddc.name));
+
+	return i2c_add_adapter(&aux->ddc);
+}
+
 static void
 intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
 {
@@ -1050,7 +1121,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
 	DRM_DEBUG_KMS("registering %s bus for %s\n", name,
 		      connector->base.kdev->kobj.name);
 
-	ret = drm_dp_aux_register(&intel_dp->aux);
+	if (!is_edp(intel_dp))
+		ret = drm_dp_aux_register(&intel_dp->aux);
+	else
+		ret = intel_edp_aux_register(&intel_dp->aux);
 	if (ret < 0) {
 		DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
 			  name, ret);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 44b153c5..783261a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -589,6 +589,11 @@ struct intel_dp {
 	uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
 	struct drm_dp_aux aux;
+	struct i2c_algorithm ddc_algo;
+	int (*aux_master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			   int num);
+	int (*dpcd_access)(struct drm_dp_aux *aux, u8 request,
+			   unsigned int offset, void *buffer, size_t size);
 	uint8_t train_set[4];
 	int panel_power_up_delay;
 	int panel_power_down_delay;
-- 
1.8.4.5

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

Re: [PATCH 1/5] drm/i915: Try to avoid pps_{lock, unlock}() on DP ports

[Jani Nikula]

On Mon, 24 Nov 2014, Egbert Eich <eich@suse.de> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 48 ++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 81f959d..a24c8cc7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -807,17 +807,19 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	uint32_t status;
>  	int try, clock = 0;
>  	bool has_aux_irq = HAS_AUX_IRQ(dev);
> -	bool vdd;
> +	bool vdd = false;
>  
> -	pps_lock(intel_dp);
> +	if (is_edp(intel_dp)) {
> +		pps_lock(intel_dp);

Why not add is_edp() into pps_lock()? The lockdep stuff in
edp_panel_vdd_on() could be moved after the is_edp() check.

BR,
Jani.

>  
> -	/*
> -	 * We will be called with VDD already enabled for dpcd/edid/oui reads.
> -	 * In such cases we want to leave VDD enabled and it's up to upper layers
> -	 * to turn it off. But for eg. i2c-dev access we need to turn it on/off
> -	 * ourselves.
> -	 */
> -	vdd = edp_panel_vdd_on(intel_dp);
> +		/*
> +		 * We will be called with VDD already enabled for dpcd/edid/oui reads.
> +		 * In such cases we want to leave VDD enabled and it's up to upper layers
> +		 * to turn it off. But for eg. i2c-dev access we need to turn it on/off
> +		 * ourselves.
> +		 */
> +		vdd = edp_panel_vdd_on(intel_dp);
> +	}
>  
>  	/* dp aux is extremely sensitive to irq latency, hence request the
>  	 * lowest possible wakeup latency and so prevent the cpu from going into
> @@ -924,10 +926,12 @@ out:
>  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>  	intel_aux_display_runtime_put(dev_priv);
>  
> -	if (vdd)
> -		edp_panel_vdd_off(intel_dp, false);
> +	if (is_edp(intel_dp)) {
> +		if (vdd)
> +			edp_panel_vdd_off(intel_dp, false);
>  
> -	pps_unlock(intel_dp);
> +		pps_unlock(intel_dp);
> +	}
>  
>  	return ret;
>  }
> @@ -2291,18 +2295,22 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>  	if (WARN_ON(dp_reg & DP_PORT_EN))
>  		return;
>  
> -	pps_lock(intel_dp);
> +	if (is_edp(intel_dp)) {
> +		pps_lock(intel_dp);
>  
> -	if (IS_VALLEYVIEW(dev))
> -		vlv_init_panel_power_sequencer(intel_dp);
> +		if (IS_VALLEYVIEW(dev))
> +			vlv_init_panel_power_sequencer(intel_dp);
>  
> -	intel_dp_enable_port(intel_dp);
> +		intel_dp_enable_port(intel_dp);
>  
> -	edp_panel_vdd_on(intel_dp);
> -	edp_panel_on(intel_dp);
> -	edp_panel_vdd_off(intel_dp, true);
> +		edp_panel_vdd_on(intel_dp);
> +		edp_panel_on(intel_dp);
> +		edp_panel_vdd_off(intel_dp, true);
>  
> -	pps_unlock(intel_dp);
> +		pps_unlock(intel_dp);
> +	} else {
> +		intel_dp_enable_port(intel_dp);
> +	}
>  
>  	if (IS_VALLEYVIEW(dev))
>  		vlv_wait_port_ready(dev_priv, dp_to_dig_port(intel_dp));
> -- 
> 1.8.4.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 0/5] drm/i915 Avoid long delays when reading EDID on eDP

[Jani Nikula]

On Mon, 24 Nov 2014, Egbert Eich <eich@suse.de> wrote:
> For eDP in the Intel driver pps_lock()/unlock() need to be called before      
> initiating an I2C/AUX channel transfer. These operations can be quite         
> expensive - especially on values for HZ lower than 1000.                      
> It is therefore better to perfrom this locking/unlocking only once,           
> ie at the beginning and at the end of the entire I2C transfer.                
> The current design of drm_dp_helper.c doesn't allow this.                     
> This patchset modifies drm_dp_helper.c and moves the locking/unlocking        
> operation to the top.                                                         
> This fixes the long delay observed in                                         
>    https://bugs.freedesktop.org/show_bug.cgi?id=86201                         
>
> Egbert Eich (4):
>   drm/DP: Create pointer to generic DPCD access function
>   drm/DP: Export drm_dp_i2c_xfer() DP helper function
>   drm/DP: Export drm_dp_dpcd_access() DP helper function

These three need to be sent to dri-devel, with cc: Thierry Reding
<thierry.reding@gmail.com>.

BR,
Jani.

>   drm/i915/eDP: Move pps_lock() and edp_panel_vdd_on() to top
>
> Ville Syrjälä (1):
>   drm/i915: Try to avoid pps_{lock,unlock}() on DP ports
>
>  drivers/gpu/drm/drm_dp_helper.c  |  11 ++--
>  drivers/gpu/drm/i915/intel_dp.c  | 132 +++++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h |   5 ++
>  include/drm/drm_dp_helper.h      |  14 +++++
>  4 files changed, 133 insertions(+), 29 deletions(-)
>
> -- 
> 1.8.4.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 0/5] drm/i915 Avoid long delays when reading EDID on eDP

[Daniel Vetter]

On Mon, Nov 24, 2014 at 06:16:22PM +0100, Egbert Eich wrote:
> For eDP in the Intel driver pps_lock()/unlock() need to be called before      
> initiating an I2C/AUX channel transfer. These operations can be quite         
> expensive - especially on values for HZ lower than 1000.                      
> It is therefore better to perfrom this locking/unlocking only once,           
> ie at the beginning and at the end of the entire I2C transfer.                
> The current design of drm_dp_helper.c doesn't allow this.                     
> This patchset modifies drm_dp_helper.c and moves the locking/unlocking        
> operation to the top.                                                         
> This fixes the long delay observed in                                         
>    https://bugs.freedesktop.org/show_bug.cgi?id=86201                         
> 
> Egbert Eich (4):
>   drm/DP: Create pointer to generic DPCD access function
>   drm/DP: Export drm_dp_i2c_xfer() DP helper function
>   drm/DP: Export drm_dp_dpcd_access() DP helper function
>   drm/i915/eDP: Move pps_lock() and edp_panel_vdd_on() to top
> 
> Ville Syrjälä (1):
>   drm/i915: Try to avoid pps_{lock,unlock}() on DP ports

Imo this approach with overwrite all the entry points won't scale since
besides i2c and dpcd there will be more sooner or later (oui, dp mst, some
debugfs userspace dp aux tools, ...).

I think what we need is the same as in the i2c layer has with the
xfer_pre/post functions. To make this as painless as possible we should
probably refcount that in the dp helper, protected by aux->hw_mutex. That
way normal dp reads could just do the xfer_pre/post around the call to
aux->transfer while i2c could do it around the entire i2c transaction.
-Daniel

> 
>  drivers/gpu/drm/drm_dp_helper.c  |  11 ++--
>  drivers/gpu/drm/i915/intel_dp.c  | 132 +++++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h |   5 ++
>  include/drm/drm_dp_helper.h      |  14 +++++
>  4 files changed, 133 insertions(+), 29 deletions(-)
> 
> -- 
> 1.8.4.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/5] drm/i915 Avoid long delays when reading EDID on eDP

[Egbert Eich]

Daniel Vetter writes:

 > Imo this approach with overwrite all the entry points won't scale since
 > besides i2c and dpcd there will be more sooner or later (oui, dp mst, some
 > debugfs userspace dp aux tools, ...).
 > 
 > I think what we need is the same as in the i2c layer has with the
 > xfer_pre/post functions. To make this as painless as possible we should
 > probably refcount that in the dp helper, protected by aux->hw_mutex. That
 > way normal dp reads could just do the xfer_pre/post around the call to
 > aux->transfer while i2c could do it around the entire i2c transaction.

I agree with your idea of having xfer_pre/post functions. 
I'm not sure though if I understand your idea about reference counters:
are you trying to protect from xfer_pre/post being called at different
levels here? 
With my proposal entire transactions (I2C) are serialized thru the pps_mutex 
lock. This is a side effect that's probably unwanted. Is this what you are
trying to avoid by using ref counters?

Cheers,
	Egbert.

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

Re: [PATCH 0/5] drm/i915 Avoid long delays when reading EDID on eDP

[Daniel Vetter]

On Tue, Nov 25, 2014 at 06:20:17PM +0100, Egbert Eich wrote:
> Daniel Vetter writes:
> 
>  > Imo this approach with overwrite all the entry points won't scale since
>  > besides i2c and dpcd there will be more sooner or later (oui, dp mst, some
>  > debugfs userspace dp aux tools, ...).
>  > 
>  > I think what we need is the same as in the i2c layer has with the
>  > xfer_pre/post functions. To make this as painless as possible we should
>  > probably refcount that in the dp helper, protected by aux->hw_mutex. That
>  > way normal dp reads could just do the xfer_pre/post around the call to
>  > aux->transfer while i2c could do it around the entire i2c transaction.
> 
> I agree with your idea of having xfer_pre/post functions. 
> I'm not sure though if I understand your idea about reference counters:
> are you trying to protect from xfer_pre/post being called at different
> levels here? 
> With my proposal entire transactions (I2C) are serialized thru the pps_mutex 
> lock. This is a side effect that's probably unwanted. Is this what you are
> trying to avoid by using ref counters?

Yeah I want to protect entire transactions at the highest levels with
xfer_pre/post. The problem is that things nest and we can't grab
sufficient locking to prevent that nesting. E.g. for i2c we should do the
dp_aux xfer_pre/post calls from i2c xfer_pre/post (so that the entire edid
read only has one pre/post, assuming no retries). But the i2c lock which
protects that doesn't protect against concurrent dp aux transactions from
someone else (e.g. dp mst is done without modeset locks, or if we ever get
a userspace interface for debuggin db issues). So direct dp aux again
needs to do xfer_pre/post. Same for dp mst, we might want to wrap an
entire dp mst transaction with this, but the low-level code might not now.

Hence why I think we shold have a refcount for pre/post plus small helpers
(atm just static, we can export once we need them) like this

drm_dp_aux_xfer_pre(dp_aux)
{
	mutex_lock(dp_aux->hw_lock)
	if (dp_aux->xfer_count++ == 0)
		dp_aux->xfer_pre();
	mutex_unlock(dp_aux->hw_lock)
}

Same for xfer_post. Then we could wire that up for i2c-over-dp and for the
low-level dp xfer functions and it should all Just Work. And if there's a
new perf critical case where we want to do this over an entire series of
transactinos we can just call the two xfer_pre/post wrappers and done.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx