New DP vfuncs get_aux{clock_divider,send_ctl}

New DP vfuncs get_aux{clock_divider,send_ctl}

[Damien Lespiau]

A tiny bit of refactoring to make the code more pluggable:

  1/ separate the get_aux_clock_divider() into multiple functions. I Tested
  this patch on IVB and HSW, the simpler to review that patch is to not look at
  the diff but the before/after version and follow the different code path for
  each platform. This part is optional as it's not very easy to review, but I
  somewhat like the per-platform separation.

  2/ split out a get_aux_send_ctl()

-- 
Damien

[PATCH 1/4] drm/i915: Turn get_aux_clock_divider() into per-product vfuncs

[Damien Lespiau]

A tiny clean-up to allow better code separation between platforms.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 79 ++++++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h |  2 +
 2 files changed, 58 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 885d271..08e8e34 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -353,31 +353,49 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
 	return status;
 }
 
-static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
-				      int index)
+/*
+ * The clock divider is based off the hrawclk, and would like to run at 2MHz.
+ * So, take the hrawclk value and divide by 2 and use that
+ *
+ * Note that PCH attached eDP panels should use a 125MHz input clock divider.
+ */
+
+static uint32_t i9xx_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	/* The clock divider is based off the hrawclk,
-	 * and would like to run at 2MHz. So, take the
-	 * hrawclk value and divide by 2 and use that
-	 *
-	 * Note that PCH attached eDP panels should use a 125MHz input
-	 * clock divider.
-	 */
-	if (IS_VALLEYVIEW(dev)) {
-		return index ? 0 : 100;
-	} else if (intel_dig_port->port == PORT_A) {
-		if (index)
-			return 0;
-		if (HAS_DDI(dev))
-			return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
-		else if (IS_GEN6(dev) || IS_GEN7(dev))
+	return index ? 0 : intel_hrawclk(dev) / 2;
+}
+
+static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+
+	if (index)
+		return 0;
+
+	if (intel_dig_port->port == PORT_A) {
+		if (IS_GEN6(dev) || IS_GEN7(dev))
 			return 200; /* SNB & IVB eDP input clock at 400Mhz */
 		else
 			return 225; /* eDP input clock at 450Mhz */
+	} else {
+		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
+	}
+}
+
+static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (intel_dig_port->port == PORT_A) {
+		if (index)
+			return 0;
+		return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
 	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
 		/* Workaround for non-ULT HSW */
 		switch (index) {
@@ -385,13 +403,16 @@ static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
 		case 1: return 72;
 		default: return 0;
 		}
-	} else if (HAS_PCH_SPLIT(dev)) {
+	} else  {
 		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
-	} else {
-		return index ? 0 :intel_hrawclk(dev) / 2;
 	}
 }
 
+static uint32_t vlv_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
+{
+	return index ? 0 : 100;
+}
+
 static int
 intel_dp_aux_ch(struct intel_dp *intel_dp,
 		uint8_t *send, int send_bytes,
@@ -450,7 +471,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		goto out;
 	}
 
-	while ((aux_clock_divider = get_aux_clock_divider(intel_dp, clock++))) {
+	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
 		/* Must try at least 3 times according to DP spec */
 		for (try = 0; try < 5; try++) {
 			/* Load the send data into the aux channel data registers */
@@ -1613,10 +1634,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t aux_clock_divider = get_aux_clock_divider(intel_dp, 0);
+	uint32_t aux_clock_divider;
 	int precharge = 0x3;
 	int msg_size = 5;       /* Header(4) + Message(1) */
 
+	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
+
 	/* Enable PSR in sink */
 	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
 		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
@@ -3637,6 +3660,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	const char *name = NULL;
 	int type, error;
 
+	/* intel_dp vfuncs */
+	if (IS_VALLEYVIEW(dev))
+		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
+	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
+	else if (HAS_PCH_SPLIT(dev))
+		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
+	else
+		intel_dp->get_aux_clock_divider = i9xx_get_aux_clock_divider;
+
 	/* Preserve the current hw state. */
 	intel_dp->DP = I915_READ(intel_dp->output_reg);
 	intel_dp->attached_connector = intel_connector;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2b72b1d..6aeb62a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -492,6 +492,8 @@ struct intel_dp {
 	unsigned long last_backlight_off;
 	bool psr_setup_done;
 	struct intel_connector *attached_connector;
+
+	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
 };
 
 struct intel_digital_port {
-- 
1.8.3.1

Re: [PATCH 1/4] drm/i915: Turn get_aux_clock_divider() into per-product vfuncs

[Jani Nikula]

On Mon, 20 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> A tiny clean-up to allow better code separation between platforms.

Should it also say "per-platform" instead of "per-product" in the
subject?

> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 79 ++++++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h |  2 +
>  2 files changed, 58 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 885d271..08e8e34 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -353,31 +353,49 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>  	return status;
>  }
>  
> -static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
> -				      int index)
> +/*
> + * The clock divider is based off the hrawclk, and would like to run at 2MHz.
> + * So, take the hrawclk value and divide by 2 and use that

This part of the comment clearly belongs above
i9xx_get_aux_clock_divider.

> + *
> + * Note that PCH attached eDP panels should use a 125MHz input clock divider.
> + */

But I'm not sure where this should go. Is this even valid anymore?

With that comment moved and possible fixed, or just nuked,

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

> +
> +static uint32_t i9xx_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	/* The clock divider is based off the hrawclk,
> -	 * and would like to run at 2MHz. So, take the
> -	 * hrawclk value and divide by 2 and use that
> -	 *
> -	 * Note that PCH attached eDP panels should use a 125MHz input
> -	 * clock divider.
> -	 */
> -	if (IS_VALLEYVIEW(dev)) {
> -		return index ? 0 : 100;
> -	} else if (intel_dig_port->port == PORT_A) {
> -		if (index)
> -			return 0;
> -		if (HAS_DDI(dev))
> -			return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
> -		else if (IS_GEN6(dev) || IS_GEN7(dev))
> +	return index ? 0 : intel_hrawclk(dev) / 2;
> +}
> +
> +static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +
> +	if (index)
> +		return 0;
> +
> +	if (intel_dig_port->port == PORT_A) {
> +		if (IS_GEN6(dev) || IS_GEN7(dev))
>  			return 200; /* SNB & IVB eDP input clock at 400Mhz */
>  		else
>  			return 225; /* eDP input clock at 450Mhz */
> +	} else {
> +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> +	}
> +}
> +
> +static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (intel_dig_port->port == PORT_A) {
> +		if (index)
> +			return 0;
> +		return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
>  	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
>  		/* Workaround for non-ULT HSW */
>  		switch (index) {
> @@ -385,13 +403,16 @@ static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
>  		case 1: return 72;
>  		default: return 0;
>  		}
> -	} else if (HAS_PCH_SPLIT(dev)) {
> +	} else  {
>  		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> -	} else {
> -		return index ? 0 :intel_hrawclk(dev) / 2;
>  	}
>  }
>  
> +static uint32_t vlv_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> +{
> +	return index ? 0 : 100;
> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		uint8_t *send, int send_bytes,
> @@ -450,7 +471,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		goto out;
>  	}
>  
> -	while ((aux_clock_divider = get_aux_clock_divider(intel_dp, clock++))) {
> +	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
>  		/* Must try at least 3 times according to DP spec */
>  		for (try = 0; try < 5; try++) {
>  			/* Load the send data into the aux channel data registers */
> @@ -1613,10 +1634,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint32_t aux_clock_divider = get_aux_clock_divider(intel_dp, 0);
> +	uint32_t aux_clock_divider;
>  	int precharge = 0x3;
>  	int msg_size = 5;       /* Header(4) + Message(1) */
>  
> +	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
> +
>  	/* Enable PSR in sink */
>  	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
>  		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> @@ -3637,6 +3660,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	const char *name = NULL;
>  	int type, error;
>  
> +	/* intel_dp vfuncs */
> +	if (IS_VALLEYVIEW(dev))
> +		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
> +	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
> +	else if (HAS_PCH_SPLIT(dev))
> +		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
> +	else
> +		intel_dp->get_aux_clock_divider = i9xx_get_aux_clock_divider;
> +
>  	/* Preserve the current hw state. */
>  	intel_dp->DP = I915_READ(intel_dp->output_reg);
>  	intel_dp->attached_connector = intel_connector;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2b72b1d..6aeb62a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -492,6 +492,8 @@ struct intel_dp {
>  	unsigned long last_backlight_off;
>  	bool psr_setup_done;
>  	struct intel_connector *attached_connector;
> +
> +	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>  };
>  
>  struct intel_digital_port {
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 1/4 v2] drm/i915: Turn get_aux_clock_divider() into per-platform vfuncs

[Damien Lespiau]

A tiny clean-up to allow better code separation between platforms.

v2: Fix comment placement (put in in i9xx_get_aux_clock_divider()) and
    nuke the outdated PCH eDP comment (Jani Nikula)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 74 ++++++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 2 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 885d271..8a4ed4a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -353,31 +353,46 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
 	return status;
 }
 
-static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
-				      int index)
+static uint32_t i9xx_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	/* The clock divider is based off the hrawclk,
-	 * and would like to run at 2MHz. So, take the
-	 * hrawclk value and divide by 2 and use that
-	 *
-	 * Note that PCH attached eDP panels should use a 125MHz input
-	 * clock divider.
+	/*
+	 * The clock divider is based off the hrawclk, and would like to run at
+	 * 2MHz.  So, take the hrawclk value and divide by 2 and use that
 	 */
-	if (IS_VALLEYVIEW(dev)) {
-		return index ? 0 : 100;
-	} else if (intel_dig_port->port == PORT_A) {
-		if (index)
-			return 0;
-		if (HAS_DDI(dev))
-			return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
-		else if (IS_GEN6(dev) || IS_GEN7(dev))
+	return index ? 0 : intel_hrawclk(dev) / 2;
+}
+
+static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+
+	if (index)
+		return 0;
+
+	if (intel_dig_port->port == PORT_A) {
+		if (IS_GEN6(dev) || IS_GEN7(dev))
 			return 200; /* SNB & IVB eDP input clock at 400Mhz */
 		else
 			return 225; /* eDP input clock at 450Mhz */
+	} else {
+		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
+	}
+}
+
+static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (intel_dig_port->port == PORT_A) {
+		if (index)
+			return 0;
+		return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
 	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
 		/* Workaround for non-ULT HSW */
 		switch (index) {
@@ -385,13 +400,16 @@ static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
 		case 1: return 72;
 		default: return 0;
 		}
-	} else if (HAS_PCH_SPLIT(dev)) {
+	} else  {
 		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
-	} else {
-		return index ? 0 :intel_hrawclk(dev) / 2;
 	}
 }
 
+static uint32_t vlv_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
+{
+	return index ? 0 : 100;
+}
+
 static int
 intel_dp_aux_ch(struct intel_dp *intel_dp,
 		uint8_t *send, int send_bytes,
@@ -450,7 +468,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		goto out;
 	}
 
-	while ((aux_clock_divider = get_aux_clock_divider(intel_dp, clock++))) {
+	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
 		/* Must try at least 3 times according to DP spec */
 		for (try = 0; try < 5; try++) {
 			/* Load the send data into the aux channel data registers */
@@ -1613,10 +1631,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t aux_clock_divider = get_aux_clock_divider(intel_dp, 0);
+	uint32_t aux_clock_divider;
 	int precharge = 0x3;
 	int msg_size = 5;       /* Header(4) + Message(1) */
 
+	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
+
 	/* Enable PSR in sink */
 	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
 		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
@@ -3637,6 +3657,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	const char *name = NULL;
 	int type, error;
 
+	/* intel_dp vfuncs */
+	if (IS_VALLEYVIEW(dev))
+		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
+	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
+	else if (HAS_PCH_SPLIT(dev))
+		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
+	else
+		intel_dp->get_aux_clock_divider = i9xx_get_aux_clock_divider;
+
 	/* Preserve the current hw state. */
 	intel_dp->DP = I915_READ(intel_dp->output_reg);
 	intel_dp->attached_connector = intel_connector;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2b72b1d..6aeb62a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -492,6 +492,8 @@ struct intel_dp {
 	unsigned long last_backlight_off;
 	bool psr_setup_done;
 	struct intel_connector *attached_connector;
+
+	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
 };
 
 struct intel_digital_port {
-- 
1.8.3.1

Re: [PATCH 1/4 v2] drm/i915: Turn get_aux_clock_divider() into per-platform vfuncs

[Jani Nikula]

On Tue, 21 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> A tiny clean-up to allow better code separation between platforms.
>
> v2: Fix comment placement (put in in i9xx_get_aux_clock_divider()) and
>     nuke the outdated PCH eDP comment (Jani Nikula)

I'll take your word nothing else changed,

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


> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 74 ++++++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  2 files changed, 54 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 885d271..8a4ed4a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -353,31 +353,46 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>  	return status;
>  }
>  
> -static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
> -				      int index)
> +static uint32_t i9xx_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	/* The clock divider is based off the hrawclk,
> -	 * and would like to run at 2MHz. So, take the
> -	 * hrawclk value and divide by 2 and use that
> -	 *
> -	 * Note that PCH attached eDP panels should use a 125MHz input
> -	 * clock divider.
> +	/*
> +	 * The clock divider is based off the hrawclk, and would like to run at
> +	 * 2MHz.  So, take the hrawclk value and divide by 2 and use that
>  	 */
> -	if (IS_VALLEYVIEW(dev)) {
> -		return index ? 0 : 100;
> -	} else if (intel_dig_port->port == PORT_A) {
> -		if (index)
> -			return 0;
> -		if (HAS_DDI(dev))
> -			return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
> -		else if (IS_GEN6(dev) || IS_GEN7(dev))
> +	return index ? 0 : intel_hrawclk(dev) / 2;
> +}
> +
> +static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +
> +	if (index)
> +		return 0;
> +
> +	if (intel_dig_port->port == PORT_A) {
> +		if (IS_GEN6(dev) || IS_GEN7(dev))
>  			return 200; /* SNB & IVB eDP input clock at 400Mhz */
>  		else
>  			return 225; /* eDP input clock at 450Mhz */
> +	} else {
> +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> +	}
> +}
> +
> +static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (intel_dig_port->port == PORT_A) {
> +		if (index)
> +			return 0;
> +		return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
>  	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
>  		/* Workaround for non-ULT HSW */
>  		switch (index) {
> @@ -385,13 +400,16 @@ static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
>  		case 1: return 72;
>  		default: return 0;
>  		}
> -	} else if (HAS_PCH_SPLIT(dev)) {
> +	} else  {
>  		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> -	} else {
> -		return index ? 0 :intel_hrawclk(dev) / 2;
>  	}
>  }
>  
> +static uint32_t vlv_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> +{
> +	return index ? 0 : 100;
> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		uint8_t *send, int send_bytes,
> @@ -450,7 +468,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		goto out;
>  	}
>  
> -	while ((aux_clock_divider = get_aux_clock_divider(intel_dp, clock++))) {
> +	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
>  		/* Must try at least 3 times according to DP spec */
>  		for (try = 0; try < 5; try++) {
>  			/* Load the send data into the aux channel data registers */
> @@ -1613,10 +1631,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint32_t aux_clock_divider = get_aux_clock_divider(intel_dp, 0);
> +	uint32_t aux_clock_divider;
>  	int precharge = 0x3;
>  	int msg_size = 5;       /* Header(4) + Message(1) */
>  
> +	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
> +
>  	/* Enable PSR in sink */
>  	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
>  		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> @@ -3637,6 +3657,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	const char *name = NULL;
>  	int type, error;
>  
> +	/* intel_dp vfuncs */
> +	if (IS_VALLEYVIEW(dev))
> +		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
> +	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
> +	else if (HAS_PCH_SPLIT(dev))
> +		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
> +	else
> +		intel_dp->get_aux_clock_divider = i9xx_get_aux_clock_divider;
> +
>  	/* Preserve the current hw state. */
>  	intel_dp->DP = I915_READ(intel_dp->output_reg);
>  	intel_dp->attached_connector = intel_connector;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2b72b1d..6aeb62a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -492,6 +492,8 @@ struct intel_dp {
>  	unsigned long last_backlight_off;
>  	bool psr_setup_done;
>  	struct intel_connector *attached_connector;
> +
> +	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>  };
>  
>  struct intel_digital_port {
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 2/4] drm/i915: Factor out a function returning the AUX_CTL value to start a send

[Damien Lespiau]

Also, move that computation outside of the for loop that tries 5 times,
this value doesn't change between tries.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 08e8e34..a9a05d2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -413,6 +413,36 @@ static uint32_t vlv_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 	return index ? 0 : 100;
 }
 
+static uint32_t i9xx_get_aux_send_ctl(struct intel_dp *intel_dp,
+				      bool has_aux_irq,
+				      int send_bytes,
+				      uint32_t aux_clock_divider)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	uint32_t precharge, timeout;
+
+	if (IS_GEN6(dev))
+		precharge = 3;
+	else
+		precharge = 5;
+
+	if (IS_BROADWELL(dev) && intel_dp->aux_ch_ctl_reg == DPA_AUX_CH_CTL)
+		timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
+	else
+		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
+
+	return DP_AUX_CH_CTL_SEND_BUSY |
+	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
+	       timeout |
+	       (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
+	       (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
+	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT) |
+	       DP_AUX_CH_CTL_DONE |
+	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
+	       DP_AUX_CH_CTL_RECEIVE_ERROR;
+}
+
 static int
 intel_dp_aux_ch(struct intel_dp *intel_dp,
 		uint8_t *send, int send_bytes,
@@ -426,9 +456,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	uint32_t aux_clock_divider;
 	int i, ret, recv_bytes;
 	uint32_t status;
-	int try, precharge, clock = 0;
+	int try, clock = 0;
 	bool has_aux_irq = true;
-	uint32_t timeout;
 
 	/* dp aux is extremely sensitive to irq latency, hence request the
 	 * lowest possible wakeup latency and so prevent the cpu from going into
@@ -438,16 +467,6 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	intel_dp_check_edp(intel_dp);
 
-	if (IS_GEN6(dev))
-		precharge = 3;
-	else
-		precharge = 5;
-
-	if (IS_BROADWELL(dev) && ch_ctl == DPA_AUX_CH_CTL)
-		timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
-	else
-		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
-
 	intel_aux_display_runtime_get(dev_priv);
 
 	/* Try to wait for any previous AUX channel activity */
@@ -472,6 +491,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	}
 
 	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
+		u32 send_ctl = i9xx_get_aux_send_ctl(intel_dp,
+						     has_aux_irq,
+						     send_bytes,
+						     aux_clock_divider);
+
 		/* Must try at least 3 times according to DP spec */
 		for (try = 0; try < 5; try++) {
 			/* Load the send data into the aux channel data registers */
@@ -480,16 +504,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 					   pack_aux(send + i, send_bytes - i));
 
 			/* Send the command and wait for it to complete */
-			I915_WRITE(ch_ctl,
-				   DP_AUX_CH_CTL_SEND_BUSY |
-				   (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
-				   timeout |
-				   (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
-				   (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
-				   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT) |
-				   DP_AUX_CH_CTL_DONE |
-				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				   DP_AUX_CH_CTL_RECEIVE_ERROR);
+			I915_WRITE(ch_ctl, send_ctl);
 
 			status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
 
-- 
1.8.3.1

Re: [PATCH 2/4] drm/i915: Factor out a function returning the AUX_CTL value to start a send

[Jani Nikula]

On Mon, 20 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> Also, move that computation outside of the for loop that tries 5 times,
> this value doesn't change between tries.

Some general bikeshedding...

I really wish everyone would write commit messages that work independent
of the patch subject. Just like a newspaper article should make sense
and tell a story also in the absence of a headline.

I know, Daniel usually churns out commit messages that are prime
examples of the *opposite* of this, so *sigh*. And,

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

on the patch.

> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 08e8e34..a9a05d2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -413,6 +413,36 @@ static uint32_t vlv_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  	return index ? 0 : 100;
>  }
>  
> +static uint32_t i9xx_get_aux_send_ctl(struct intel_dp *intel_dp,
> +				      bool has_aux_irq,
> +				      int send_bytes,
> +				      uint32_t aux_clock_divider)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	uint32_t precharge, timeout;
> +
> +	if (IS_GEN6(dev))
> +		precharge = 3;
> +	else
> +		precharge = 5;
> +
> +	if (IS_BROADWELL(dev) && intel_dp->aux_ch_ctl_reg == DPA_AUX_CH_CTL)
> +		timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
> +	else
> +		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
> +
> +	return DP_AUX_CH_CTL_SEND_BUSY |
> +	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> +	       timeout |
> +	       (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> +	       (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> +	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT) |
> +	       DP_AUX_CH_CTL_DONE |
> +	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
> +	       DP_AUX_CH_CTL_RECEIVE_ERROR;
> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		uint8_t *send, int send_bytes,
> @@ -426,9 +456,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	uint32_t aux_clock_divider;
>  	int i, ret, recv_bytes;
>  	uint32_t status;
> -	int try, precharge, clock = 0;
> +	int try, clock = 0;
>  	bool has_aux_irq = true;
> -	uint32_t timeout;
>  
>  	/* dp aux is extremely sensitive to irq latency, hence request the
>  	 * lowest possible wakeup latency and so prevent the cpu from going into
> @@ -438,16 +467,6 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	intel_dp_check_edp(intel_dp);
>  
> -	if (IS_GEN6(dev))
> -		precharge = 3;
> -	else
> -		precharge = 5;
> -
> -	if (IS_BROADWELL(dev) && ch_ctl == DPA_AUX_CH_CTL)
> -		timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
> -	else
> -		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
> -
>  	intel_aux_display_runtime_get(dev_priv);
>  
>  	/* Try to wait for any previous AUX channel activity */
> @@ -472,6 +491,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	}
>  
>  	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
> +		u32 send_ctl = i9xx_get_aux_send_ctl(intel_dp,
> +						     has_aux_irq,
> +						     send_bytes,
> +						     aux_clock_divider);
> +
>  		/* Must try at least 3 times according to DP spec */
>  		for (try = 0; try < 5; try++) {
>  			/* Load the send data into the aux channel data registers */
> @@ -480,16 +504,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  					   pack_aux(send + i, send_bytes - i));
>  
>  			/* Send the command and wait for it to complete */
> -			I915_WRITE(ch_ctl,
> -				   DP_AUX_CH_CTL_SEND_BUSY |
> -				   (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> -				   timeout |
> -				   (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> -				   (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> -				   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT) |
> -				   DP_AUX_CH_CTL_DONE |
> -				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
> -				   DP_AUX_CH_CTL_RECEIVE_ERROR);
> +			I915_WRITE(ch_ctl, send_ctl);
>  
>  			status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
>  
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/4] drm/i915: Factor out a function returning the AUX_CTL value to start a send

[Damien Lespiau]

On Tue, Jan 21, 2014 at 12:07:23PM +0200, Jani Nikula wrote:
> 
> On Mon, 20 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> > Also, move that computation outside of the for loop that tries 5 times,
> > this value doesn't change between tries.
> 
> Some general bikeshedding...
> 
> I really wish everyone would write commit messages that work independent
> of the patch subject. Just like a newspaper article should make sense
> and tell a story also in the absence of a headline.

Will keep this in mind next time (one could argue we're in deep
bikeshedding water here!).

-- 
Damien

Re: [PATCH 2/4] drm/i915: Factor out a function returning the AUX_CTL value to start a send

[Jani Nikula]

On Tue, 21 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> On Tue, Jan 21, 2014 at 12:07:23PM +0200, Jani Nikula wrote:
>> Some general bikeshedding...
>> 
>> I really wish everyone would write commit messages that work independent
>> of the patch subject. Just like a newspaper article should make sense
>> and tell a story also in the absence of a headline.
>
> Will keep this in mind next time (one could argue we're in deep
> bikeshedding water here!).

Well, yeah. Nothing personal, Damien, times_this_has_annoyed_me++ just
grew beyond some threshold at this patch. ;)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 3/4] drm/i915: Reorder the AUX_CTL bits in descending order

[Damien Lespiau]

So it's easier to compare what we program with the documentation, not
having to jump at all.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a9a05d2..ae9902a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -433,14 +433,14 @@ static uint32_t i9xx_get_aux_send_ctl(struct intel_dp *intel_dp,
 		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
 
 	return DP_AUX_CH_CTL_SEND_BUSY |
+	       DP_AUX_CH_CTL_DONE |
 	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
+	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
 	       timeout |
+	       DP_AUX_CH_CTL_RECEIVE_ERROR |
 	       (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
 	       (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
-	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT) |
-	       DP_AUX_CH_CTL_DONE |
-	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
-	       DP_AUX_CH_CTL_RECEIVE_ERROR;
+	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT);
 }
 
 static int
-- 
1.8.3.1

Re: [PATCH 3/4] drm/i915: Reorder the AUX_CTL bits in descending order

[Jani Nikula]

On Mon, 20 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> So it's easier to compare what we program with the documentation, not
> having to jump at all.

This could be squashed into the previous patch just as well, but either
way,

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

> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a9a05d2..ae9902a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -433,14 +433,14 @@ static uint32_t i9xx_get_aux_send_ctl(struct intel_dp *intel_dp,
>  		timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
>  
>  	return DP_AUX_CH_CTL_SEND_BUSY |
> +	       DP_AUX_CH_CTL_DONE |
>  	       (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> +	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
>  	       timeout |
> +	       DP_AUX_CH_CTL_RECEIVE_ERROR |
>  	       (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
>  	       (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> -	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT) |
> -	       DP_AUX_CH_CTL_DONE |
> -	       DP_AUX_CH_CTL_TIME_OUT_ERROR |
> -	       DP_AUX_CH_CTL_RECEIVE_ERROR;
> +	       (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT);
>  }
>  
>  static int
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/4] drm/i915: Reorder the AUX_CTL bits in descending order

[Damien Lespiau]

On Tue, Jan 21, 2014 at 12:09:18PM +0200, Jani Nikula wrote:
> On Mon, 20 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> > So it's easier to compare what we program with the documentation, not
> > having to jump at all.
> 
> This could be squashed into the previous patch just as well, but either
> way,

I think it's a bit better to have separate patches to factor out code,
and modify it. It generates easier diffs to review, rather than having
one moving and changing the code at the same time.

-- 
Damien

[PATCH 4/4] drm/i915: Introduce a get_aux_send_ctl() vfunc

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 10 ++++++----
 drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ae9902a..95147ac 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -491,10 +491,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	}
 
 	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
-		u32 send_ctl = i9xx_get_aux_send_ctl(intel_dp,
-						     has_aux_irq,
-						     send_bytes,
-						     aux_clock_divider);
+		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
+							  has_aux_irq,
+							  send_bytes,
+							  aux_clock_divider);
 
 		/* Must try at least 3 times according to DP spec */
 		for (try = 0; try < 5; try++) {
@@ -3685,6 +3685,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	else
 		intel_dp->get_aux_clock_divider = i9xx_get_aux_clock_divider;
 
+	intel_dp->get_aux_send_ctl = i9xx_get_aux_send_ctl;
+
 	/* Preserve the current hw state. */
 	intel_dp->DP = I915_READ(intel_dp->output_reg);
 	intel_dp->attached_connector = intel_connector;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6aeb62a..85bda0e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -494,6 +494,14 @@ struct intel_dp {
 	struct intel_connector *attached_connector;
 
 	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
+	/*
+	 * This function returns the value we have to program the AUX_CTL
+	 * register with to kick off an AUX transaction.
+	 */
+	uint32_t (*get_aux_send_ctl)(struct intel_dp *dp,
+				     bool has_aux_irq,
+				     int send_bytes,
+				     uint32_t aux_clock_divider);
 };
 
 struct intel_digital_port {
-- 
1.8.3.1

Re: [PATCH 4/4] drm/i915: Introduce a get_aux_send_ctl() vfunc

[Jani Nikula]

On Mon, 20 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Might be worth a mention this is prep work for something, as alone this
looks like a vfunc for the sake of vfunc...

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 10 ++++++----
>  drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ae9902a..95147ac 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -491,10 +491,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	}
>  
>  	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
> -		u32 send_ctl = i9xx_get_aux_send_ctl(intel_dp,
> -						     has_aux_irq,
> -						     send_bytes,
> -						     aux_clock_divider);
> +		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> +							  has_aux_irq,
> +							  send_bytes,
> +							  aux_clock_divider);
>  
>  		/* Must try at least 3 times according to DP spec */
>  		for (try = 0; try < 5; try++) {
> @@ -3685,6 +3685,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	else
>  		intel_dp->get_aux_clock_divider = i9xx_get_aux_clock_divider;
>  
> +	intel_dp->get_aux_send_ctl = i9xx_get_aux_send_ctl;
> +
>  	/* Preserve the current hw state. */
>  	intel_dp->DP = I915_READ(intel_dp->output_reg);
>  	intel_dp->attached_connector = intel_connector;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6aeb62a..85bda0e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -494,6 +494,14 @@ struct intel_dp {
>  	struct intel_connector *attached_connector;
>  
>  	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
> +	/*
> +	 * This function returns the value we have to program the AUX_CTL
> +	 * register with to kick off an AUX transaction.
> +	 */
> +	uint32_t (*get_aux_send_ctl)(struct intel_dp *dp,
> +				     bool has_aux_irq,
> +				     int send_bytes,
> +				     uint32_t aux_clock_divider);
>  };
>  
>  struct intel_digital_port {
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 4/4 v2] drm/i915: Introduce a get_aux_send_ctl() vfunc

[Damien Lespiau]

We need a bit more flexibility here in the future, bits get shuffled
around.

v2: more descriptive commit message (Jani Nikula)

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 10 ++++++----
 drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7faf364..8936298 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -488,10 +488,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	}
 
 	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
-		u32 send_ctl = i9xx_get_aux_send_ctl(intel_dp,
-						     has_aux_irq,
-						     send_bytes,
-						     aux_clock_divider);
+		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
+							  has_aux_irq,
+							  send_bytes,
+							  aux_clock_divider);
 
 		/* Must try at least 3 times according to DP spec */
 		for (try = 0; try < 5; try++) {
@@ -3682,6 +3682,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	else
 		intel_dp->get_aux_clock_divider = i9xx_get_aux_clock_divider;
 
+	intel_dp->get_aux_send_ctl = i9xx_get_aux_send_ctl;
+
 	/* Preserve the current hw state. */
 	intel_dp->DP = I915_READ(intel_dp->output_reg);
 	intel_dp->attached_connector = intel_connector;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6aeb62a..85bda0e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -494,6 +494,14 @@ struct intel_dp {
 	struct intel_connector *attached_connector;
 
 	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
+	/*
+	 * This function returns the value we have to program the AUX_CTL
+	 * register with to kick off an AUX transaction.
+	 */
+	uint32_t (*get_aux_send_ctl)(struct intel_dp *dp,
+				     bool has_aux_irq,
+				     int send_bytes,
+				     uint32_t aux_clock_divider);
 };
 
 struct intel_digital_port {
-- 
1.8.3.1

Re: [PATCH 4/4 v2] drm/i915: Introduce a get_aux_send_ctl() vfunc

[Daniel Vetter]

On Tue, Jan 21, 2014 at 01:37:15PM +0000, Damien Lespiau wrote:
> We need a bit more flexibility here in the future, bits get shuffled
> around.
> 
> v2: more descriptive commit message (Jani Nikula)
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Merged all to dinq, thanks for patches and review.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 10 ++++++----
>  drivers/gpu/drm/i915/intel_drv.h |  8 ++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7faf364..8936298 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -488,10 +488,10 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	}
>  
>  	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
> -		u32 send_ctl = i9xx_get_aux_send_ctl(intel_dp,
> -						     has_aux_irq,
> -						     send_bytes,
> -						     aux_clock_divider);
> +		u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> +							  has_aux_irq,
> +							  send_bytes,
> +							  aux_clock_divider);
>  
>  		/* Must try at least 3 times according to DP spec */
>  		for (try = 0; try < 5; try++) {
> @@ -3682,6 +3682,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	else
>  		intel_dp->get_aux_clock_divider = i9xx_get_aux_clock_divider;
>  
> +	intel_dp->get_aux_send_ctl = i9xx_get_aux_send_ctl;
> +
>  	/* Preserve the current hw state. */
>  	intel_dp->DP = I915_READ(intel_dp->output_reg);
>  	intel_dp->attached_connector = intel_connector;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6aeb62a..85bda0e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -494,6 +494,14 @@ struct intel_dp {
>  	struct intel_connector *attached_connector;
>  
>  	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
> +	/*
> +	 * This function returns the value we have to program the AUX_CTL
> +	 * register with to kick off an AUX transaction.
> +	 */
> +	uint32_t (*get_aux_send_ctl)(struct intel_dp *dp,
> +				     bool has_aux_irq,
> +				     int send_bytes,
> +				     uint32_t aux_clock_divider);
>  };
>  
>  struct intel_digital_port {
> -- 
> 1.8.3.1
> 
> _______________________________________________
> 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