[PATCH 0/6] drm/i915: switch to drm core dp aux helpers

[PATCH 0/6] drm/i915: switch to drm core dp aux helpers

[Jani Nikula]

Switch over to using Thierry's drm dp aux helpers.

The previous RFC series (from before the helpers were merged) is at
[1]. I specifically decided to dodge the sleep state issues pointed out
by Ville [2] for now; this series does not make us better or worse in
this regard.

Only briefly tested on SNB, but seems to work.

Patch 1 has been posted and reviewed on dri-devel already [3], and is
included here for completeness.

(These patches demonstrate that git format-patch needs a "function
rewrite" option... the diffs in patches 4 and 6 turned out quite hideous
with mixed + and - lines.)



BR,
Jani.


[1] http://mid.gmane.org/cover.1391520164.git.jani.nikula@intel.com
[2] http://mid.gmane.org/20140204142053.GH3891@intel.com
[3] http://mid.gmane.org/1394020227-16738-1-git-send-email-jani.nikula@intel.com

Jani Nikula (6):
  drm/dp: let drivers specify the name of the I2C-over-AUX adapter
  drm/i915/dp: split edp_panel_vdd_on() for reuse
  drm/i915/dp: move edp vdd enable/disable at a lower level in
    i2c-over-aux
  drm/i915/dp: use the new drm helpers for dp aux
  drm/i915/dp: move dp aux ch register init to aux init
  drm/i915/dp: use the new drm helpers for dp i2c-over-aux

 drivers/gpu/drm/drm_dp_helper.c  |    3 +-
 drivers/gpu/drm/i915/intel_dp.c  |  513 ++++++++++++++------------------------
 drivers/gpu/drm/i915/intel_drv.h |    3 +-
 include/drm/drm_dp_helper.h      |    4 +
 4 files changed, 189 insertions(+), 334 deletions(-)

-- 
1.7.9.5

[PATCH 1/6] drm/dp: let drivers specify the name of the I2C-over-AUX adapter

[Jani Nikula]

Let the drivers specify the name of the I2C-over-AUX adapter to maintain
backwards compatibility in the sysfs when converting to the new
I2C-over-AUX helper infrastructure.

The i915 driver currently uses DPDDC-A to DPDDC-D as names for the DP
i2c adapters. These names show up in the i2c sysfs name attribute. We'd
like to be able to maintain that when switching over to the new helpers.

Due to i2c device and connector cleanup ordering issues we also recently
made the drm device (instead of connector) the parent of the i2c
adapters:

commit 80f65de3c9b8101c1613fa82df500ba6a099a11c
Author: Imre Deak <imre.deak@intel.com>
Date:   Tue Feb 11 17:12:49 2014 +0200

    drm/i915: dp: fix order of dp aux i2c device cleanup

With the name picked up from the adapter parent using dev_name(), it
would be the same for all i2c adapters with the current I2C-over-AUX
helpers.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Thierry Reding <treding@nvidia.com>
---
 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 35251af3b14e..17832d048147 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -726,7 +726,8 @@ int drm_dp_aux_register_i2c_bus(struct drm_dp_aux *aux)
 	aux->ddc.dev.parent = aux->dev;
 	aux->ddc.dev.of_node = aux->dev->of_node;
 
-	strncpy(aux->ddc.name, dev_name(aux->dev), sizeof(aux->ddc.name));
+	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
+		sizeof(aux->ddc.name));
 
 	return i2c_add_adapter(&aux->ddc);
 }
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 42947566e755..b4f58914bf7d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -438,6 +438,9 @@ struct drm_dp_aux_msg {
  * The .dev field should be set to a pointer to the device that implements
  * the AUX channel.
  *
+ * The .name field may be used to specify the name of the I2C adapter. If set to
+ * NULL, dev_name() of .dev will be used.
+ *
  * Drivers provide a hardware-specific implementation of how transactions
  * are executed via the .transfer() function. A pointer to a drm_dp_aux_msg
  * structure describing the transaction is passed into this function. Upon
@@ -455,6 +458,7 @@ struct drm_dp_aux_msg {
  * should call drm_dp_aux_unregister_i2c_bus() to remove the I2C adapter.
  */
 struct drm_dp_aux {
+	const char *name;
 	struct i2c_adapter ddc;
 	struct device *dev;
 
-- 
1.7.9.5

Re: [PATCH 1/6] drm/dp: let drivers specify the name of the I2C-over-AUX adapter

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

//Although I know it has been already reviewed ;)

On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> Let the drivers specify the name of the I2C-over-AUX adapter to maintain
> backwards compatibility in the sysfs when converting to the new
> I2C-over-AUX helper infrastructure.
>
> The i915 driver currently uses DPDDC-A to DPDDC-D as names for the DP
> i2c adapters. These names show up in the i2c sysfs name attribute. We'd
> like to be able to maintain that when switching over to the new helpers.
>
> Due to i2c device and connector cleanup ordering issues we also recently
> made the drm device (instead of connector) the parent of the i2c
> adapters:
>
> commit 80f65de3c9b8101c1613fa82df500ba6a099a11c
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Tue Feb 11 17:12:49 2014 +0200
>
>     drm/i915: dp: fix order of dp aux i2c device cleanup
>
> With the name picked up from the adapter parent using dev_name(), it
> would be the same for all i2c adapters with the current I2C-over-AUX
> helpers.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
> ---
>  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 35251af3b14e..17832d048147 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -726,7 +726,8 @@ int drm_dp_aux_register_i2c_bus(struct drm_dp_aux *aux)
>         aux->ddc.dev.parent = aux->dev;
>         aux->ddc.dev.of_node = aux->dev->of_node;
>
> -       strncpy(aux->ddc.name, dev_name(aux->dev), sizeof(aux->ddc.name));
> +       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
> +               sizeof(aux->ddc.name));
>
>         return i2c_add_adapter(&aux->ddc);
>  }
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 42947566e755..b4f58914bf7d 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -438,6 +438,9 @@ struct drm_dp_aux_msg {
>   * The .dev field should be set to a pointer to the device that implements
>   * the AUX channel.
>   *
> + * The .name field may be used to specify the name of the I2C adapter. If set to
> + * NULL, dev_name() of .dev will be used.
> + *
>   * Drivers provide a hardware-specific implementation of how transactions
>   * are executed via the .transfer() function. A pointer to a drm_dp_aux_msg
>   * structure describing the transaction is passed into this function. Upon
> @@ -455,6 +458,7 @@ struct drm_dp_aux_msg {
>   * should call drm_dp_aux_unregister_i2c_bus() to remove the I2C adapter.
>   */
>  struct drm_dp_aux {
> +       const char *name;
>         struct i2c_adapter ddc;
>         struct device *dev;
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 2/6] drm/i915/dp: split edp_panel_vdd_on() for reuse

[Jani Nikula]

Introduce _edp_panel_vdd_on() that returns true if the call enabled vdd,
and a matching disable is needed. Keep edp_panel_vdd_on() as a helper
for when it is expected the vdd is off.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bb66f9301cd9..94e7577da484 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -91,6 +91,7 @@ static struct intel_dp *intel_attached_dp(struct drm_connector *connector)
 }
 
 static void intel_dp_link_down(struct intel_dp *intel_dp);
+static bool _edp_panel_vdd_on(struct intel_dp *intel_dp);
 static void edp_panel_vdd_on(struct intel_dp *intel_dp);
 static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 
@@ -1162,23 +1163,21 @@ static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
 	return control;
 }
 
-static void edp_panel_vdd_on(struct intel_dp *intel_dp)
+static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 pp;
 	u32 pp_stat_reg, pp_ctrl_reg;
+	bool need_to_disable = !intel_dp->want_panel_vdd;
 
 	if (!is_edp(intel_dp))
-		return;
-
-	WARN(intel_dp->want_panel_vdd,
-	     "eDP VDD already requested on\n");
+		return false;
 
 	intel_dp->want_panel_vdd = true;
 
 	if (edp_have_panel_vdd(intel_dp))
-		return;
+		return need_to_disable;
 
 	intel_runtime_pm_get(dev_priv);
 
@@ -1204,6 +1203,17 @@ static void edp_panel_vdd_on(struct intel_dp *intel_dp)
 		DRM_DEBUG_KMS("eDP was not running\n");
 		msleep(intel_dp->panel_power_up_delay);
 	}
+
+	return need_to_disable;
+}
+
+static void edp_panel_vdd_on(struct intel_dp *intel_dp)
+{
+	if (is_edp(intel_dp)) {
+		bool vdd = _edp_panel_vdd_on(intel_dp);
+
+		WARN(!vdd, "eDP VDD already requested on\n");
+	}
 }
 
 static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
-- 
1.7.9.5

Re: [PATCH 2/6] drm/i915/dp: split edp_panel_vdd_on() for reuse

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> Introduce _edp_panel_vdd_on() that returns true if the call enabled vdd,
> and a matching disable is needed. Keep edp_panel_vdd_on() as a helper
> for when it is expected the vdd is off.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bb66f9301cd9..94e7577da484 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -91,6 +91,7 @@ static struct intel_dp *intel_attached_dp(struct drm_connector *connector)
>  }
>
>  static void intel_dp_link_down(struct intel_dp *intel_dp);
> +static bool _edp_panel_vdd_on(struct intel_dp *intel_dp);
>  static void edp_panel_vdd_on(struct intel_dp *intel_dp);
>  static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>
> @@ -1162,23 +1163,21 @@ static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
>         return control;
>  }
>
> -static void edp_panel_vdd_on(struct intel_dp *intel_dp)
> +static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
>  {
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 pp;
>         u32 pp_stat_reg, pp_ctrl_reg;
> +       bool need_to_disable = !intel_dp->want_panel_vdd;
>
>         if (!is_edp(intel_dp))
> -               return;
> -
> -       WARN(intel_dp->want_panel_vdd,
> -            "eDP VDD already requested on\n");
> +               return false;
>
>         intel_dp->want_panel_vdd = true;
>
>         if (edp_have_panel_vdd(intel_dp))
> -               return;
> +               return need_to_disable;
>
>         intel_runtime_pm_get(dev_priv);
>
> @@ -1204,6 +1203,17 @@ static void edp_panel_vdd_on(struct intel_dp *intel_dp)
>                 DRM_DEBUG_KMS("eDP was not running\n");
>                 msleep(intel_dp->panel_power_up_delay);
>         }
> +
> +       return need_to_disable;
> +}
> +
> +static void edp_panel_vdd_on(struct intel_dp *intel_dp)
> +{
> +       if (is_edp(intel_dp)) {
> +               bool vdd = _edp_panel_vdd_on(intel_dp);
> +
> +               WARN(!vdd, "eDP VDD already requested on\n");
> +       }
>  }
>
>  static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 2/6] drm/i915/dp: split edp_panel_vdd_on() for reuse

[Rodrigo Vivi]

forgot to say that this patch needs a rebase on -nightly

On Mon, Mar 17, 2014 at 11:09 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
> On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>> Introduce _edp_panel_vdd_on() that returns true if the call enabled vdd,
>> and a matching disable is needed. Keep edp_panel_vdd_on() as a helper
>> for when it is expected the vdd is off.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c |   22 ++++++++++++++++------
>>  1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index bb66f9301cd9..94e7577da484 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -91,6 +91,7 @@ static struct intel_dp *intel_attached_dp(struct drm_connector *connector)
>>  }
>>
>>  static void intel_dp_link_down(struct intel_dp *intel_dp);
>> +static bool _edp_panel_vdd_on(struct intel_dp *intel_dp);
>>  static void edp_panel_vdd_on(struct intel_dp *intel_dp);
>>  static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>>
>> @@ -1162,23 +1163,21 @@ static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
>>         return control;
>>  }
>>
>> -static void edp_panel_vdd_on(struct intel_dp *intel_dp)
>> +static bool _edp_panel_vdd_on(struct intel_dp *intel_dp)
>>  {
>>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>         u32 pp;
>>         u32 pp_stat_reg, pp_ctrl_reg;
>> +       bool need_to_disable = !intel_dp->want_panel_vdd;
>>
>>         if (!is_edp(intel_dp))
>> -               return;
>> -
>> -       WARN(intel_dp->want_panel_vdd,
>> -            "eDP VDD already requested on\n");
>> +               return false;
>>
>>         intel_dp->want_panel_vdd = true;
>>
>>         if (edp_have_panel_vdd(intel_dp))
>> -               return;
>> +               return need_to_disable;
>>
>>         intel_runtime_pm_get(dev_priv);
>>
>> @@ -1204,6 +1203,17 @@ static void edp_panel_vdd_on(struct intel_dp *intel_dp)
>>                 DRM_DEBUG_KMS("eDP was not running\n");
>>                 msleep(intel_dp->panel_power_up_delay);
>>         }
>> +
>> +       return need_to_disable;
>> +}
>> +
>> +static void edp_panel_vdd_on(struct intel_dp *intel_dp)
>> +{
>> +       if (is_edp(intel_dp)) {
>> +               bool vdd = _edp_panel_vdd_on(intel_dp);
>> +
>> +               WARN(!vdd, "eDP VDD already requested on\n");
>> +       }
>>  }
>>
>>  static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 3/6] drm/i915/dp: move edp vdd enable/disable at a lower level in i2c-over-aux

[Jani Nikula]

This is prep work for conversion to generic drm i2c-over-aux helpers
where we won't have the function to do this at.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 94e7577da484..22134edc03dd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -461,6 +461,9 @@ 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;
+
+	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
@@ -566,6 +569,9 @@ 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);
+
 	return ret;
 }
 
@@ -678,8 +684,6 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 	int reply_bytes;
 	int ret;
 
-	edp_panel_vdd_on(intel_dp);
-	intel_dp_check_edp(intel_dp);
 	/* Set up the command byte */
 	if (mode & MODE_I2C_READ)
 		msg[0] = DP_AUX_I2C_READ << 4;
@@ -781,7 +785,6 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 	ret = -EREMOTEIO;
 
 out:
-	edp_panel_vdd_off(intel_dp, false);
 	return ret;
 }
 
-- 
1.7.9.5

Re: [PATCH 3/6] drm/i915/dp: move edp vdd enable/disable at a lower level in i2c-over-aux

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> This is prep work for conversion to generic drm i2c-over-aux helpers
> where we won't have the function to do this at.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 94e7577da484..22134edc03dd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -461,6 +461,9 @@ 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;
> +
> +       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
> @@ -566,6 +569,9 @@ 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);
> +
>         return ret;
>  }
>
> @@ -678,8 +684,6 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
>         int reply_bytes;
>         int ret;
>
> -       edp_panel_vdd_on(intel_dp);
> -       intel_dp_check_edp(intel_dp);
>         /* Set up the command byte */
>         if (mode & MODE_I2C_READ)
>                 msg[0] = DP_AUX_I2C_READ << 4;
> @@ -781,7 +785,6 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
>         ret = -EREMOTEIO;
>
>  out:
> -       edp_panel_vdd_off(intel_dp, false);
>         return ret;
>  }
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 4/6] drm/i915/dp: use the new drm helpers for dp aux

[Jani Nikula]

Functionality remains largely the same as before.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |  257 +++++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 116 insertions(+), 142 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 22134edc03dd..24cbf4bd36c4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -575,97 +575,77 @@ out:
 	return ret;
 }
 
-/* Write data to the aux channel in native mode */
-static int
-intel_dp_aux_native_write(struct intel_dp *intel_dp,
-			  uint16_t address, uint8_t *send, int send_bytes)
+#define HEADER_SIZE	4
+static ssize_t
+intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
+	struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
+	uint8_t txbuf[20], rxbuf[20];
+	size_t txsize, rxsize;
 	int ret;
-	uint8_t	msg[20];
-	int msg_bytes;
-	uint8_t	ack;
-	int retry;
 
-	if (WARN_ON(send_bytes > 16))
-		return -E2BIG;
+	txbuf[0] = msg->request << 4;
+	txbuf[1] = msg->address >> 8;
+	txbuf[2] = msg->address & 0xff;
+	txbuf[3] = msg->size - 1;
 
-	intel_dp_check_edp(intel_dp);
-	msg[0] = DP_AUX_NATIVE_WRITE << 4;
-	msg[1] = address >> 8;
-	msg[2] = address & 0xff;
-	msg[3] = send_bytes - 1;
-	memcpy(&msg[4], send, send_bytes);
-	msg_bytes = send_bytes + 4;
-	for (retry = 0; retry < 7; retry++) {
-		ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes, &ack, 1);
-		if (ret < 0)
-			return ret;
-		ack >>= 4;
-		if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK)
-			return send_bytes;
-		else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
-			usleep_range(400, 500);
-		else
-			return -EIO;
-	}
+	switch (msg->request & ~DP_AUX_I2C_MOT) {
+	case DP_AUX_NATIVE_WRITE:
+	case DP_AUX_I2C_WRITE:
+		txsize = HEADER_SIZE + msg->size;
+		rxsize = 1;
 
-	DRM_ERROR("too many retries, giving up\n");
-	return -EIO;
-}
+		if (WARN_ON(txsize > 20))
+			return -E2BIG;
 
-/* Write a single byte to the aux channel in native mode */
-static int
-intel_dp_aux_native_write_1(struct intel_dp *intel_dp,
-			    uint16_t address, uint8_t byte)
-{
-	return intel_dp_aux_native_write(intel_dp, address, &byte, 1);
-}
+		memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
 
-/* read bytes from a native aux channel */
-static int
-intel_dp_aux_native_read(struct intel_dp *intel_dp,
-			 uint16_t address, uint8_t *recv, int recv_bytes)
-{
-	uint8_t msg[4];
-	int msg_bytes;
-	uint8_t reply[20];
-	int reply_bytes;
-	uint8_t ack;
-	int ret;
-	int retry;
+		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
+		if (ret > 0) {
+			msg->reply = rxbuf[0] >> 4;
 
-	if (WARN_ON(recv_bytes > 19))
-		return -E2BIG;
+			/* Return payload size. */
+			ret = msg->size;
+		}
+		break;
 
-	intel_dp_check_edp(intel_dp);
-	msg[0] = DP_AUX_NATIVE_READ << 4;
-	msg[1] = address >> 8;
-	msg[2] = address & 0xff;
-	msg[3] = recv_bytes - 1;
+	case DP_AUX_NATIVE_READ:
+	case DP_AUX_I2C_READ:
+		txsize = HEADER_SIZE;
+		rxsize = msg->size + 1;
 
-	msg_bytes = 4;
-	reply_bytes = recv_bytes + 1;
+		if (WARN_ON(rxsize > 20))
+			return -E2BIG;
 
-	for (retry = 0; retry < 7; retry++) {
-		ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes,
-				      reply, reply_bytes);
-		if (ret == 0)
-			return -EPROTO;
-		if (ret < 0)
-			return ret;
-		ack = reply[0] >> 4;
-		if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK) {
-			memcpy(recv, reply + 1, ret - 1);
-			return ret - 1;
+		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
+		if (ret > 0) {
+			msg->reply = rxbuf[0] >> 4;
+			/*
+			 * Assume happy day, and copy the data. The caller is
+			 * expected to check msg->reply before touching it.
+			 *
+			 * Return payload size.
+			 */
+			ret--;
+			memcpy(msg->buffer, rxbuf + 1, ret);
 		}
-		else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
-			usleep_range(400, 500);
-		else
-			return -EIO;
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
 	}
 
-	DRM_ERROR("too many retries, giving up\n");
-	return -EIO;
+	return ret;
+}
+
+static void
+intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+
+	intel_dp->aux.dev = dev->dev;
+	intel_dp->aux.transfer = intel_dp_aux_transfer;
 }
 
 static int
@@ -1472,8 +1452,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 		return;
 
 	if (mode != DRM_MODE_DPMS_ON) {
-		ret = intel_dp_aux_native_write_1(intel_dp, DP_SET_POWER,
-						  DP_SET_POWER_D3);
+		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
+					 DP_SET_POWER_D3);
 		if (ret != 1)
 			DRM_DEBUG_DRIVER("failed to write sink power state\n");
 	} else {
@@ -1482,9 +1462,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 		 * time to wake up.
 		 */
 		for (i = 0; i < 3; i++) {
-			ret = intel_dp_aux_native_write_1(intel_dp,
-							  DP_SET_POWER,
-							  DP_SET_POWER_D0);
+			ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
+						 DP_SET_POWER_D0);
 			if (ret == 1)
 				break;
 			msleep(1);
@@ -1708,13 +1687,11 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 
 	/* 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,
-					    DP_PSR_ENABLE &
-					    ~DP_PSR_MAIN_LINK_ACTIVE);
+		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
+				   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
 	else
-		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
-					    DP_PSR_ENABLE |
-					    DP_PSR_MAIN_LINK_ACTIVE);
+		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
+				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
 
 	/* Setup AUX registers */
 	I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
@@ -2026,26 +2003,25 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
 /*
  * Native read with retry for link status and receiver capability reads for
  * cases where the sink may still be asleep.
+ *
+ * Sinks are *supposed* to come up within 1ms from an off state, but we're also
+ * supposed to retry 3 times per the spec.
  */
-static bool
-intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
-			       uint8_t *recv, int recv_bytes)
+static ssize_t
+intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
+			void *buffer, size_t size)
 {
-	int ret, i;
+	ssize_t ret;
+	int i;
 
-	/*
-	 * Sinks are *supposed* to come up within 1ms from an off state,
-	 * but we're also supposed to retry 3 times per the spec.
-	 */
 	for (i = 0; i < 3; i++) {
-		ret = intel_dp_aux_native_read(intel_dp, address, recv,
-					       recv_bytes);
-		if (ret == recv_bytes)
-			return true;
+		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
+		if (ret == size)
+			return ret;
 		msleep(1);
 	}
 
-	return false;
+	return ret;
 }
 
 /*
@@ -2055,10 +2031,10 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
 static bool
 intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
-	return intel_dp_aux_native_read_retry(intel_dp,
-					      DP_LANE0_1_STATUS,
-					      link_status,
-					      DP_LINK_STATUS_SIZE);
+	return intel_dp_dpcd_read_wake(&intel_dp->aux,
+				       DP_LANE0_1_STATUS,
+				       link_status,
+				       DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
 }
 
 /*
@@ -2572,8 +2548,8 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 		len = intel_dp->lane_count + 1;
 	}
 
-	ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_PATTERN_SET,
-					buf, len);
+	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
+				buf, len);
 
 	return ret == len;
 }
@@ -2602,9 +2578,8 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
 	I915_WRITE(intel_dp->output_reg, *DP);
 	POSTING_READ(intel_dp->output_reg);
 
-	ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_LANE0_SET,
-					intel_dp->train_set,
-					intel_dp->lane_count);
+	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
+				intel_dp->train_set, intel_dp->lane_count);
 
 	return ret == intel_dp->lane_count;
 }
@@ -2660,11 +2635,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 	link_config[1] = intel_dp->lane_count;
 	if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
 		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
-	intel_dp_aux_native_write(intel_dp, DP_LINK_BW_SET, link_config, 2);
+	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
 
 	link_config[0] = 0;
 	link_config[1] = DP_SET_ANSI_8B10B;
-	intel_dp_aux_native_write(intel_dp, DP_DOWNSPREAD_CTRL, link_config, 2);
+	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
 
 	DP |= DP_PORT_EN;
 
@@ -2907,8 +2882,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 
 	char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
 
-	if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
-					   sizeof(intel_dp->dpcd)) == 0)
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
+				    sizeof(intel_dp->dpcd)) < 0)
 		return false; /* aux transfer failed */
 
 	hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd),
@@ -2921,9 +2896,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
-		intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
-					       intel_dp->psr_dpcd,
-					       sizeof(intel_dp->psr_dpcd));
+		intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
+					intel_dp->psr_dpcd,
+					sizeof(intel_dp->psr_dpcd));
 		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
 			dev_priv->psr.sink_support = true;
 			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
@@ -2945,9 +2920,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
 		return true; /* no per-port downstream info */
 
-	if (intel_dp_aux_native_read_retry(intel_dp, DP_DOWNSTREAM_PORT_0,
-					   intel_dp->downstream_ports,
-					   DP_MAX_DOWNSTREAM_PORTS) == 0)
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
+				    intel_dp->downstream_ports,
+				    DP_MAX_DOWNSTREAM_PORTS) < 0)
 		return false; /* downstream port status fetch failed */
 
 	return true;
@@ -2963,11 +2938,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 
 	edp_panel_vdd_on(intel_dp);
 
-	if (intel_dp_aux_native_read_retry(intel_dp, DP_SINK_OUI, buf, 3))
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 
-	if (intel_dp_aux_native_read_retry(intel_dp, DP_BRANCH_OUI, buf, 3))
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 
@@ -2982,46 +2957,40 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		to_intel_crtc(intel_dig_port->base.base.crtc);
 	u8 buf[1];
 
-	if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1))
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, buf) < 0);
 		return -EAGAIN;
 
 	if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
 		return -ENOTTY;
 
-	if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
-					 DP_TEST_SINK_START))
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
+			       DP_TEST_SINK_START) < 0)
 		return -EAGAIN;
 
 	/* Wait 2 vblanks to be sure we will have the correct CRC value */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-	if (!intel_dp_aux_native_read(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
 		return -EAGAIN;
 
-	intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
 	return 0;
 }
 
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
-	int ret;
-
-	ret = intel_dp_aux_native_read_retry(intel_dp,
-					     DP_DEVICE_SERVICE_IRQ_VECTOR,
-					     sink_irq_vector, 1);
-	if (!ret)
-		return false;
-
-	return true;
+	return intel_dp_dpcd_read_wake(&intel_dp->aux,
+				       DP_DEVICE_SERVICE_IRQ_VECTOR,
+				       sink_irq_vector, 1) == 1;
 }
 
 static void
 intel_dp_handle_test_request(struct intel_dp *intel_dp)
 {
 	/* NAK by default */
-	intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
 }
 
 /*
@@ -3060,9 +3029,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
 		/* Clear interrupt source */
-		intel_dp_aux_native_write_1(intel_dp,
-					    DP_DEVICE_SERVICE_IRQ_VECTOR,
-					    sink_irq_vector);
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+				   DP_DEVICE_SERVICE_IRQ_VECTOR,
+				   sink_irq_vector);
 
 		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
 			intel_dp_handle_test_request(intel_dp);
@@ -3097,9 +3066,11 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
 		uint8_t reg;
-		if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT,
-						    &reg, 1))
+
+		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
+					    &reg, 1) < 0)
 			return connector_status_unknown;
+
 		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
 					      : connector_status_disconnected;
 	}
@@ -3925,6 +3896,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
 	}
 
+	intel_dp_aux_init(intel_dp, intel_connector);
+
 	error = intel_dp_i2c_init(intel_dp, intel_connector, name);
 	WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
 	     error, port_name(port));
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2546cae0b4f0..561af4b013b1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -491,6 +491,7 @@ struct intel_dp {
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
 	struct i2c_adapter adapter;
 	struct i2c_algo_dp_aux_data algo;
+	struct drm_dp_aux aux;
 	uint8_t train_set[4];
 	int panel_power_up_delay;
 	int panel_power_down_delay;
-- 
1.7.9.5

Re: [PATCH 4/6] drm/i915/dp: use the new drm helpers for dp aux

[Rodrigo Vivi]

On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> Functionality remains largely the same as before.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |  257 +++++++++++++++++---------------------
>  drivers/gpu/drm/i915/intel_drv.h |    1 +
>  2 files changed, 116 insertions(+), 142 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 22134edc03dd..24cbf4bd36c4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -575,97 +575,77 @@ out:
>         return ret;
>  }
>
> -/* Write data to the aux channel in native mode */
> -static int
> -intel_dp_aux_native_write(struct intel_dp *intel_dp,
> -                         uint16_t address, uint8_t *send, int send_bytes)
> +#define HEADER_SIZE    4
> +static ssize_t
> +intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  {
> +       struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
> +       uint8_t txbuf[20], rxbuf[20];
> +       size_t txsize, rxsize;
>         int ret;
> -       uint8_t msg[20];
> -       int msg_bytes;
> -       uint8_t ack;
> -       int retry;
>
> -       if (WARN_ON(send_bytes > 16))
> -               return -E2BIG;
> +       txbuf[0] = msg->request << 4;
> +       txbuf[1] = msg->address >> 8;
> +       txbuf[2] = msg->address & 0xff;
> +       txbuf[3] = msg->size - 1;
>
> -       intel_dp_check_edp(intel_dp);
> -       msg[0] = DP_AUX_NATIVE_WRITE << 4;
> -       msg[1] = address >> 8;
> -       msg[2] = address & 0xff;
> -       msg[3] = send_bytes - 1;
> -       memcpy(&msg[4], send, send_bytes);
> -       msg_bytes = send_bytes + 4;
> -       for (retry = 0; retry < 7; retry++) {

we were retrying 7 times always, but now we are now retrying only 3
times or even none.
Couldn't it trigger some new bug or regression?

> -               ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes, &ack, 1);
> -               if (ret < 0)
> -                       return ret;
> -               ack >>= 4;
> -               if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK)
> -                       return send_bytes;
> -               else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
> -                       usleep_range(400, 500);

we are also not checking this ack x defer (here nor in native_write)
replies anymore.
Don't we need it anymore? why?

> -               else
> -                       return -EIO;
> -       }
> +       switch (msg->request & ~DP_AUX_I2C_MOT) {
> +       case DP_AUX_NATIVE_WRITE:
> +       case DP_AUX_I2C_WRITE:
> +               txsize = HEADER_SIZE + msg->size;
> +               rxsize = 1;
>
> -       DRM_ERROR("too many retries, giving up\n");
> -       return -EIO;
> -}
> +               if (WARN_ON(txsize > 20))
> +                       return -E2BIG;
>
> -/* Write a single byte to the aux channel in native mode */
> -static int
> -intel_dp_aux_native_write_1(struct intel_dp *intel_dp,
> -                           uint16_t address, uint8_t byte)
> -{
> -       return intel_dp_aux_native_write(intel_dp, address, &byte, 1);
> -}
> +               memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
>
> -/* read bytes from a native aux channel */
> -static int
> -intel_dp_aux_native_read(struct intel_dp *intel_dp,
> -                        uint16_t address, uint8_t *recv, int recv_bytes)
> -{
> -       uint8_t msg[4];
> -       int msg_bytes;
> -       uint8_t reply[20];
> -       int reply_bytes;
> -       uint8_t ack;
> -       int ret;
> -       int retry;
> +               ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
> +               if (ret > 0) {
> +                       msg->reply = rxbuf[0] >> 4;
>
> -       if (WARN_ON(recv_bytes > 19))
> -               return -E2BIG;
> +                       /* Return payload size. */
> +                       ret = msg->size;
> +               }
> +               break;
>
> -       intel_dp_check_edp(intel_dp);
> -       msg[0] = DP_AUX_NATIVE_READ << 4;
> -       msg[1] = address >> 8;
> -       msg[2] = address & 0xff;
> -       msg[3] = recv_bytes - 1;
> +       case DP_AUX_NATIVE_READ:
> +       case DP_AUX_I2C_READ:
> +               txsize = HEADER_SIZE;
> +               rxsize = msg->size + 1;
>
> -       msg_bytes = 4;
> -       reply_bytes = recv_bytes + 1;
> +               if (WARN_ON(rxsize > 20))
> +                       return -E2BIG;
>
> -       for (retry = 0; retry < 7; retry++) {
> -               ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes,
> -                                     reply, reply_bytes);
> -               if (ret == 0)
> -                       return -EPROTO;
> -               if (ret < 0)
> -                       return ret;
> -               ack = reply[0] >> 4;
> -               if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK) {
> -                       memcpy(recv, reply + 1, ret - 1);
> -                       return ret - 1;
> +               ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
> +               if (ret > 0) {
> +                       msg->reply = rxbuf[0] >> 4;
> +                       /*
> +                        * Assume happy day, and copy the data. The caller is
> +                        * expected to check msg->reply before touching it.
> +                        *
> +                        * Return payload size.
> +                        */
> +                       ret--;
> +                       memcpy(msg->buffer, rxbuf + 1, ret);
>                 }
> -               else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
> -                       usleep_range(400, 500);
> -               else
> -                       return -EIO;
> +               break;
> +
> +       default:
> +               ret = -EINVAL;
> +               break;
>         }
>
> -       DRM_ERROR("too many retries, giving up\n");
> -       return -EIO;
> +       return ret;
> +}
> +
> +static void
> +intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
> +{
> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +
> +       intel_dp->aux.dev = dev->dev;
> +       intel_dp->aux.transfer = intel_dp_aux_transfer;
>  }
>
>  static int
> @@ -1472,8 +1452,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>                 return;
>
>         if (mode != DRM_MODE_DPMS_ON) {
> -               ret = intel_dp_aux_native_write_1(intel_dp, DP_SET_POWER,
> -                                                 DP_SET_POWER_D3);
> +               ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> +                                        DP_SET_POWER_D3);
>                 if (ret != 1)
>                         DRM_DEBUG_DRIVER("failed to write sink power state\n");
>         } else {
> @@ -1482,9 +1462,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>                  * time to wake up.
>                  */
>                 for (i = 0; i < 3; i++) {
> -                       ret = intel_dp_aux_native_write_1(intel_dp,
> -                                                         DP_SET_POWER,
> -                                                         DP_SET_POWER_D0);
> +                       ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> +                                                DP_SET_POWER_D0);
>                         if (ret == 1)
>                                 break;
>                         msleep(1);
> @@ -1708,13 +1687,11 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>
>         /* 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,
> -                                           DP_PSR_ENABLE &
> -                                           ~DP_PSR_MAIN_LINK_ACTIVE);
> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> +                                  DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
>         else
> -               intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> -                                           DP_PSR_ENABLE |
> -                                           DP_PSR_MAIN_LINK_ACTIVE);
> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> +                                  DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
>
>         /* Setup AUX registers */
>         I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
> @@ -2026,26 +2003,25 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
>  /*
>   * Native read with retry for link status and receiver capability reads for
>   * cases where the sink may still be asleep.
> + *
> + * Sinks are *supposed* to come up within 1ms from an off state, but we're also
> + * supposed to retry 3 times per the spec.
>   */
> -static bool
> -intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
> -                              uint8_t *recv, int recv_bytes)
> +static ssize_t
> +intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> +                       void *buffer, size_t size)
>  {
> -       int ret, i;
> +       ssize_t ret;
> +       int i;
>
> -       /*
> -        * Sinks are *supposed* to come up within 1ms from an off state,
> -        * but we're also supposed to retry 3 times per the spec.
> -        */
>         for (i = 0; i < 3; i++) {
> -               ret = intel_dp_aux_native_read(intel_dp, address, recv,
> -                                              recv_bytes);
> -               if (ret == recv_bytes)
> -                       return true;
> +               ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> +               if (ret == size)
> +                       return ret;
>                 msleep(1);
>         }
>
> -       return false;
> +       return ret;
>  }
>
>  /*
> @@ -2055,10 +2031,10 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
>  static bool
>  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
>  {
> -       return intel_dp_aux_native_read_retry(intel_dp,
> -                                             DP_LANE0_1_STATUS,
> -                                             link_status,
> -                                             DP_LINK_STATUS_SIZE);
> +       return intel_dp_dpcd_read_wake(&intel_dp->aux,
> +                                      DP_LANE0_1_STATUS,
> +                                      link_status,
> +                                      DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
>  }
>
>  /*
> @@ -2572,8 +2548,8 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>                 len = intel_dp->lane_count + 1;
>         }
>
> -       ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_PATTERN_SET,
> -                                       buf, len);
> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
> +                               buf, len);
>
>         return ret == len;
>  }
> @@ -2602,9 +2578,8 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>         I915_WRITE(intel_dp->output_reg, *DP);
>         POSTING_READ(intel_dp->output_reg);
>
> -       ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_LANE0_SET,
> -                                       intel_dp->train_set,
> -                                       intel_dp->lane_count);
> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
> +                               intel_dp->train_set, intel_dp->lane_count);
>
>         return ret == intel_dp->lane_count;
>  }
> @@ -2660,11 +2635,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>         link_config[1] = intel_dp->lane_count;
>         if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>                 link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> -       intel_dp_aux_native_write(intel_dp, DP_LINK_BW_SET, link_config, 2);
> +       drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>
>         link_config[0] = 0;
>         link_config[1] = DP_SET_ANSI_8B10B;
> -       intel_dp_aux_native_write(intel_dp, DP_DOWNSPREAD_CTRL, link_config, 2);
> +       drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>
>         DP |= DP_PORT_EN;
>
> @@ -2907,8 +2882,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>
>         char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
>
> -       if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
> -                                          sizeof(intel_dp->dpcd)) == 0)
> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
> +                                   sizeof(intel_dp->dpcd)) < 0)
>                 return false; /* aux transfer failed */
>
>         hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd),
> @@ -2921,9 +2896,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>         /* Check if the panel supports PSR */
>         memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>         if (is_edp(intel_dp)) {
> -               intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> -                                              intel_dp->psr_dpcd,
> -                                              sizeof(intel_dp->psr_dpcd));
> +               intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
> +                                       intel_dp->psr_dpcd,
> +                                       sizeof(intel_dp->psr_dpcd));
>                 if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>                         dev_priv->psr.sink_support = true;
>                         DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> @@ -2945,9 +2920,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>         if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
>                 return true; /* no per-port downstream info */
>
> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_DOWNSTREAM_PORT_0,
> -                                          intel_dp->downstream_ports,
> -                                          DP_MAX_DOWNSTREAM_PORTS) == 0)
> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> +                                   intel_dp->downstream_ports,
> +                                   DP_MAX_DOWNSTREAM_PORTS) < 0)
>                 return false; /* downstream port status fetch failed */
>
>         return true;
> @@ -2963,11 +2938,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>
>         edp_panel_vdd_on(intel_dp);
>
> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_SINK_OUI, buf, 3))
> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
>                 DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>                               buf[0], buf[1], buf[2]);
>
> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_BRANCH_OUI, buf, 3))
> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
>                 DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>                               buf[0], buf[1], buf[2]);
>
> @@ -2982,46 +2957,40 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>                 to_intel_crtc(intel_dig_port->base.base.crtc);
>         u8 buf[1];
>
> -       if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1))
> +       if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, buf) < 0);
>                 return -EAGAIN;
>
>         if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
>                 return -ENOTTY;
>
> -       if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
> -                                        DP_TEST_SINK_START))
> +       if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> +                              DP_TEST_SINK_START) < 0)
>                 return -EAGAIN;
>
>         /* Wait 2 vblanks to be sure we will have the correct CRC value */
>         intel_wait_for_vblank(dev, intel_crtc->pipe);
>         intel_wait_for_vblank(dev, intel_crtc->pipe);
>
> -       if (!intel_dp_aux_native_read(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
> +       if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>                 return -EAGAIN;
>
> -       intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
> +       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
>         return 0;
>  }
>
>  static bool
>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> -       int ret;
> -
> -       ret = intel_dp_aux_native_read_retry(intel_dp,
> -                                            DP_DEVICE_SERVICE_IRQ_VECTOR,
> -                                            sink_irq_vector, 1);
> -       if (!ret)
> -               return false;
> -
> -       return true;
> +       return intel_dp_dpcd_read_wake(&intel_dp->aux,
> +                                      DP_DEVICE_SERVICE_IRQ_VECTOR,
> +                                      sink_irq_vector, 1) == 1;
>  }
>
>  static void
>  intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  {
>         /* NAK by default */
> -       intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
> +       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
>  }
>
>  /*
> @@ -3060,9 +3029,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>                 /* Clear interrupt source */
> -               intel_dp_aux_native_write_1(intel_dp,
> -                                           DP_DEVICE_SERVICE_IRQ_VECTOR,
> -                                           sink_irq_vector);
> +               drm_dp_dpcd_writeb(&intel_dp->aux,
> +                                  DP_DEVICE_SERVICE_IRQ_VECTOR,
> +                                  sink_irq_vector);
>
>                 if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
>                         intel_dp_handle_test_request(intel_dp);
> @@ -3097,9 +3066,11 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>             intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>                 uint8_t reg;
> -               if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT,
> -                                                   &reg, 1))
> +
> +               if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> +                                           &reg, 1) < 0)
>                         return connector_status_unknown;
> +
>                 return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>                                               : connector_status_disconnected;
>         }
> @@ -3925,6 +3896,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>                 intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
>         }
>
> +       intel_dp_aux_init(intel_dp, intel_connector);
> +
>         error = intel_dp_i2c_init(intel_dp, intel_connector, name);
>         WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
>              error, port_name(port));
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2546cae0b4f0..561af4b013b1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -491,6 +491,7 @@ struct intel_dp {
>         uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>         struct i2c_adapter adapter;
>         struct i2c_algo_dp_aux_data algo;
> +       struct drm_dp_aux aux;
>         uint8_t train_set[4];
>         int panel_power_up_delay;
>         int panel_power_down_delay;
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 4/6] drm/i915/dp: use the new drm helpers for dp aux

[Rodrigo Vivi]

On Mon, Mar 17, 2014 at 11:08 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>> Functionality remains largely the same as before.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c  |  257 +++++++++++++++++---------------------
>>  drivers/gpu/drm/i915/intel_drv.h |    1 +
>>  2 files changed, 116 insertions(+), 142 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 22134edc03dd..24cbf4bd36c4 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -575,97 +575,77 @@ out:
>>         return ret;
>>  }
>>
>> -/* Write data to the aux channel in native mode */
>> -static int
>> -intel_dp_aux_native_write(struct intel_dp *intel_dp,
>> -                         uint16_t address, uint8_t *send, int send_bytes)
>> +#define HEADER_SIZE    4
>> +static ssize_t
>> +intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>  {
>> +       struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
>> +       uint8_t txbuf[20], rxbuf[20];
>> +       size_t txsize, rxsize;
>>         int ret;
>> -       uint8_t msg[20];
>> -       int msg_bytes;
>> -       uint8_t ack;
>> -       int retry;
>>
>> -       if (WARN_ON(send_bytes > 16))
>> -               return -E2BIG;
>> +       txbuf[0] = msg->request << 4;
>> +       txbuf[1] = msg->address >> 8;
>> +       txbuf[2] = msg->address & 0xff;
>> +       txbuf[3] = msg->size - 1;
>>
>> -       intel_dp_check_edp(intel_dp);
>> -       msg[0] = DP_AUX_NATIVE_WRITE << 4;
>> -       msg[1] = address >> 8;
>> -       msg[2] = address & 0xff;
>> -       msg[3] = send_bytes - 1;
>> -       memcpy(&msg[4], send, send_bytes);
>> -       msg_bytes = send_bytes + 4;
>> -       for (retry = 0; retry < 7; retry++) {
>
> we were retrying 7 times always, but now we are now retrying only 3
> times or even none.
> Couldn't it trigger some new bug or regression?
>
>> -               ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes, &ack, 1);
>> -               if (ret < 0)
>> -                       return ret;
>> -               ack >>= 4;
>> -               if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK)
>> -                       return send_bytes;
>> -               else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
>> -                       usleep_range(400, 500);
>
> we are also not checking this ack x defer (here nor in native_write)
> replies anymore.
> Don't we need it anymore? why?
>
>> -               else
>> -                       return -EIO;
>> -       }
>> +       switch (msg->request & ~DP_AUX_I2C_MOT) {
>> +       case DP_AUX_NATIVE_WRITE:
>> +       case DP_AUX_I2C_WRITE:
>> +               txsize = HEADER_SIZE + msg->size;
>> +               rxsize = 1;
>>
>> -       DRM_ERROR("too many retries, giving up\n");
>> -       return -EIO;
>> -}
>> +               if (WARN_ON(txsize > 20))
>> +                       return -E2BIG;
>>
>> -/* Write a single byte to the aux channel in native mode */
>> -static int
>> -intel_dp_aux_native_write_1(struct intel_dp *intel_dp,
>> -                           uint16_t address, uint8_t byte)
>> -{
>> -       return intel_dp_aux_native_write(intel_dp, address, &byte, 1);
>> -}
>> +               memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
>>
>> -/* read bytes from a native aux channel */
>> -static int
>> -intel_dp_aux_native_read(struct intel_dp *intel_dp,
>> -                        uint16_t address, uint8_t *recv, int recv_bytes)
>> -{
>> -       uint8_t msg[4];
>> -       int msg_bytes;
>> -       uint8_t reply[20];
>> -       int reply_bytes;
>> -       uint8_t ack;
>> -       int ret;
>> -       int retry;
>> +               ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
>> +               if (ret > 0) {
>> +                       msg->reply = rxbuf[0] >> 4;
>>
>> -       if (WARN_ON(recv_bytes > 19))
>> -               return -E2BIG;
>> +                       /* Return payload size. */
>> +                       ret = msg->size;
>> +               }
>> +               break;
>>
>> -       intel_dp_check_edp(intel_dp);
>> -       msg[0] = DP_AUX_NATIVE_READ << 4;
>> -       msg[1] = address >> 8;
>> -       msg[2] = address & 0xff;
>> -       msg[3] = recv_bytes - 1;
>> +       case DP_AUX_NATIVE_READ:
>> +       case DP_AUX_I2C_READ:
>> +               txsize = HEADER_SIZE;
>> +               rxsize = msg->size + 1;
>>
>> -       msg_bytes = 4;
>> -       reply_bytes = recv_bytes + 1;
>> +               if (WARN_ON(rxsize > 20))
>> +                       return -E2BIG;
>>
>> -       for (retry = 0; retry < 7; retry++) {
>> -               ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes,
>> -                                     reply, reply_bytes);
>> -               if (ret == 0)
>> -                       return -EPROTO;
>> -               if (ret < 0)
>> -                       return ret;
>> -               ack = reply[0] >> 4;
>> -               if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK) {
>> -                       memcpy(recv, reply + 1, ret - 1);
>> -                       return ret - 1;
>> +               ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
>> +               if (ret > 0) {
>> +                       msg->reply = rxbuf[0] >> 4;
>> +                       /*
>> +                        * Assume happy day, and copy the data. The caller is
>> +                        * expected to check msg->reply before touching it.
>> +                        *
>> +                        * Return payload size.
>> +                        */
>> +                       ret--;
>> +                       memcpy(msg->buffer, rxbuf + 1, ret);
>>                 }
>> -               else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
>> -                       usleep_range(400, 500);
>> -               else
>> -                       return -EIO;
>> +               break;
>> +
>> +       default:
>> +               ret = -EINVAL;
>> +               break;
>>         }
>>
>> -       DRM_ERROR("too many retries, giving up\n");
>> -       return -EIO;
>> +       return ret;
>> +}
>> +
>> +static void
>> +intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
>> +{
>> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +
>> +       intel_dp->aux.dev = dev->dev;
>> +       intel_dp->aux.transfer = intel_dp_aux_transfer;
>>  }
>>
>>  static int
>> @@ -1472,8 +1452,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>>                 return;
>>
>>         if (mode != DRM_MODE_DPMS_ON) {
>> -               ret = intel_dp_aux_native_write_1(intel_dp, DP_SET_POWER,
>> -                                                 DP_SET_POWER_D3);
>> +               ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
>> +                                        DP_SET_POWER_D3);
>>                 if (ret != 1)
>>                         DRM_DEBUG_DRIVER("failed to write sink power state\n");
>>         } else {
>> @@ -1482,9 +1462,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>>                  * time to wake up.
>>                  */
>>                 for (i = 0; i < 3; i++) {
>> -                       ret = intel_dp_aux_native_write_1(intel_dp,
>> -                                                         DP_SET_POWER,
>> -                                                         DP_SET_POWER_D0);
>> +                       ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
>> +                                                DP_SET_POWER_D0);
>>                         if (ret == 1)
>>                                 break;
>>                         msleep(1);
>> @@ -1708,13 +1687,11 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>>
>>         /* 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,
>> -                                           DP_PSR_ENABLE &
>> -                                           ~DP_PSR_MAIN_LINK_ACTIVE);
>> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>> +                                  DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
>>         else
>> -               intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>> -                                           DP_PSR_ENABLE |
>> -                                           DP_PSR_MAIN_LINK_ACTIVE);
>> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>> +                                  DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
>>
>>         /* Setup AUX registers */
>>         I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
>> @@ -2026,26 +2003,25 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
>>  /*
>>   * Native read with retry for link status and receiver capability reads for
>>   * cases where the sink may still be asleep.
>> + *
>> + * Sinks are *supposed* to come up within 1ms from an off state, but we're also
>> + * supposed to retry 3 times per the spec.
>>   */
>> -static bool
>> -intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
>> -                              uint8_t *recv, int recv_bytes)
>> +static ssize_t
>> +intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
>> +                       void *buffer, size_t size)
>>  {
>> -       int ret, i;
>> +       ssize_t ret;
>> +       int i;
>>
>> -       /*
>> -        * Sinks are *supposed* to come up within 1ms from an off state,
>> -        * but we're also supposed to retry 3 times per the spec.
>> -        */
>>         for (i = 0; i < 3; i++) {
>> -               ret = intel_dp_aux_native_read(intel_dp, address, recv,
>> -                                              recv_bytes);
>> -               if (ret == recv_bytes)
>> -                       return true;
>> +               ret = drm_dp_dpcd_read(aux, offset, buffer, size);
>> +               if (ret == size)
>> +                       return ret;
>>                 msleep(1);
>>         }
>>
>> -       return false;
>> +       return ret;
>>  }
>>
>>  /*
>> @@ -2055,10 +2031,10 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
>>  static bool
>>  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
>>  {
>> -       return intel_dp_aux_native_read_retry(intel_dp,
>> -                                             DP_LANE0_1_STATUS,
>> -                                             link_status,
>> -                                             DP_LINK_STATUS_SIZE);
>> +       return intel_dp_dpcd_read_wake(&intel_dp->aux,
>> +                                      DP_LANE0_1_STATUS,
>> +                                      link_status,
>> +                                      DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
>>  }
>>
>>  /*
>> @@ -2572,8 +2548,8 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>>                 len = intel_dp->lane_count + 1;
>>         }
>>
>> -       ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_PATTERN_SET,
>> -                                       buf, len);
>> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
>> +                               buf, len);
>>
>>         return ret == len;
>>  }
>> @@ -2602,9 +2578,8 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>>         I915_WRITE(intel_dp->output_reg, *DP);
>>         POSTING_READ(intel_dp->output_reg);
>>
>> -       ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_LANE0_SET,
>> -                                       intel_dp->train_set,
>> -                                       intel_dp->lane_count);
>> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
>> +                               intel_dp->train_set, intel_dp->lane_count);
>>
>>         return ret == intel_dp->lane_count;
>>  }
>> @@ -2660,11 +2635,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>>         link_config[1] = intel_dp->lane_count;
>>         if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>>                 link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> -       intel_dp_aux_native_write(intel_dp, DP_LINK_BW_SET, link_config, 2);
>> +       drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>>
>>         link_config[0] = 0;
>>         link_config[1] = DP_SET_ANSI_8B10B;
>> -       intel_dp_aux_native_write(intel_dp, DP_DOWNSPREAD_CTRL, link_config, 2);
>> +       drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>>
>>         DP |= DP_PORT_EN;
>>
>> @@ -2907,8 +2882,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>
>>         char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
>>
>> -       if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
>> -                                          sizeof(intel_dp->dpcd)) == 0)
>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>> +                                   sizeof(intel_dp->dpcd)) < 0)
>>                 return false; /* aux transfer failed */
>>
>>         hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd),
>> @@ -2921,9 +2896,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>         /* Check if the panel supports PSR */
>>         memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>         if (is_edp(intel_dp)) {
>> -               intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>> -                                              intel_dp->psr_dpcd,
>> -                                              sizeof(intel_dp->psr_dpcd));
>> +               intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
>> +                                       intel_dp->psr_dpcd,
>> +                                       sizeof(intel_dp->psr_dpcd));
>>                 if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>>                         dev_priv->psr.sink_support = true;
>>                         DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>> @@ -2945,9 +2920,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>         if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
>>                 return true; /* no per-port downstream info */
>>
>> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_DOWNSTREAM_PORT_0,
>> -                                          intel_dp->downstream_ports,
>> -                                          DP_MAX_DOWNSTREAM_PORTS) == 0)
>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
>> +                                   intel_dp->downstream_ports,
>> +                                   DP_MAX_DOWNSTREAM_PORTS) < 0)
>>                 return false; /* downstream port status fetch failed */
>>
>>         return true;
>> @@ -2963,11 +2938,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>>
>>         edp_panel_vdd_on(intel_dp);
>>
>> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_SINK_OUI, buf, 3))
>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
>>                 DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>>                               buf[0], buf[1], buf[2]);
>>
>> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_BRANCH_OUI, buf, 3))
>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
>>                 DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>>                               buf[0], buf[1], buf[2]);
>>
>> @@ -2982,46 +2957,40 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>>                 to_intel_crtc(intel_dig_port->base.base.crtc);
>>         u8 buf[1];
>>
>> -       if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1))
>> +       if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, buf) < 0);

wrong ";" here..

>>                 return -EAGAIN;
>>
>>         if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
>>                 return -ENOTTY;
>>
>> -       if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
>> -                                        DP_TEST_SINK_START))
>> +       if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>> +                              DP_TEST_SINK_START) < 0)
>>                 return -EAGAIN;
>>
>>         /* Wait 2 vblanks to be sure we will have the correct CRC value */
>>         intel_wait_for_vblank(dev, intel_crtc->pipe);
>>         intel_wait_for_vblank(dev, intel_crtc->pipe);
>>
>> -       if (!intel_dp_aux_native_read(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
>> +       if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>>                 return -EAGAIN;
>>
>> -       intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
>> +       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
>>         return 0;
>>  }
>>
>>  static bool
>>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>  {
>> -       int ret;
>> -
>> -       ret = intel_dp_aux_native_read_retry(intel_dp,
>> -                                            DP_DEVICE_SERVICE_IRQ_VECTOR,
>> -                                            sink_irq_vector, 1);
>> -       if (!ret)
>> -               return false;
>> -
>> -       return true;
>> +       return intel_dp_dpcd_read_wake(&intel_dp->aux,
>> +                                      DP_DEVICE_SERVICE_IRQ_VECTOR,
>> +                                      sink_irq_vector, 1) == 1;
>>  }
>>
>>  static void
>>  intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>  {
>>         /* NAK by default */
>> -       intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
>> +       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
>>  }
>>
>>  /*
>> @@ -3060,9 +3029,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>>                 /* Clear interrupt source */
>> -               intel_dp_aux_native_write_1(intel_dp,
>> -                                           DP_DEVICE_SERVICE_IRQ_VECTOR,
>> -                                           sink_irq_vector);
>> +               drm_dp_dpcd_writeb(&intel_dp->aux,
>> +                                  DP_DEVICE_SERVICE_IRQ_VECTOR,
>> +                                  sink_irq_vector);
>>
>>                 if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
>>                         intel_dp_handle_test_request(intel_dp);
>> @@ -3097,9 +3066,11 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>             intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>>                 uint8_t reg;
>> -               if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT,
>> -                                                   &reg, 1))
>> +
>> +               if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> +                                           &reg, 1) < 0)
>>                         return connector_status_unknown;
>> +
>>                 return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>>                                               : connector_status_disconnected;
>>         }
>> @@ -3925,6 +3896,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>                 intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
>>         }
>>
>> +       intel_dp_aux_init(intel_dp, intel_connector);
>> +
>>         error = intel_dp_i2c_init(intel_dp, intel_connector, name);
>>         WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
>>              error, port_name(port));
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 2546cae0b4f0..561af4b013b1 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -491,6 +491,7 @@ struct intel_dp {
>>         uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>>         struct i2c_adapter adapter;
>>         struct i2c_algo_dp_aux_data algo;
>> +       struct drm_dp_aux aux;
>>         uint8_t train_set[4];
>>         int panel_power_up_delay;
>>         int panel_power_down_delay;
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 4/6] drm/i915/dp: use the new drm helpers for dp aux

[Jani Nikula]

On Mon, 17 Mar 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Mon, Mar 17, 2014 at 11:08 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>> On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>>> -       if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1))
>>> +       if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, buf) < 0);
>
> wrong ";" here..

Yikes, good catch. I've been rebasing this way too many times...

Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 4/6] drm/i915/dp: use the new drm helpers for dp aux

[Jani Nikula]

On Mon, 17 Mar 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>> Functionality remains largely the same as before.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c  |  257 +++++++++++++++++---------------------
>>  drivers/gpu/drm/i915/intel_drv.h |    1 +
>>  2 files changed, 116 insertions(+), 142 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 22134edc03dd..24cbf4bd36c4 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -575,97 +575,77 @@ out:
>>         return ret;
>>  }
>>
>> -/* Write data to the aux channel in native mode */
>> -static int
>> -intel_dp_aux_native_write(struct intel_dp *intel_dp,
>> -                         uint16_t address, uint8_t *send, int send_bytes)
>> +#define HEADER_SIZE    4
>> +static ssize_t
>> +intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>  {
>> +       struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
>> +       uint8_t txbuf[20], rxbuf[20];
>> +       size_t txsize, rxsize;
>>         int ret;
>> -       uint8_t msg[20];
>> -       int msg_bytes;
>> -       uint8_t ack;
>> -       int retry;
>>
>> -       if (WARN_ON(send_bytes > 16))
>> -               return -E2BIG;
>> +       txbuf[0] = msg->request << 4;
>> +       txbuf[1] = msg->address >> 8;
>> +       txbuf[2] = msg->address & 0xff;
>> +       txbuf[3] = msg->size - 1;
>>
>> -       intel_dp_check_edp(intel_dp);
>> -       msg[0] = DP_AUX_NATIVE_WRITE << 4;
>> -       msg[1] = address >> 8;
>> -       msg[2] = address & 0xff;
>> -       msg[3] = send_bytes - 1;
>> -       memcpy(&msg[4], send, send_bytes);
>> -       msg_bytes = send_bytes + 4;
>> -       for (retry = 0; retry < 7; retry++) {
>
> we were retrying 7 times always, but now we are now retrying only 3
> times or even none.

I'm confused, what are you referring to? drm_dp_dpcd_access() in
drm_dp_helper.c has the retry loop now.

I think you're mixing this with the intel_dp_dpcd_read_wake() wrapper,
but that ends up in drm_dp_dpcd_access() as well.

> Couldn't it trigger some new bug or regression?
>
>> -               ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes, &ack, 1);
>> -               if (ret < 0)
>> -                       return ret;
>> -               ack >>= 4;
>> -               if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK)
>> -                       return send_bytes;
>> -               else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
>> -                       usleep_range(400, 500);
>
> we are also not checking this ack x defer (here nor in native_write)
> replies anymore.

Ditto, we store ack >>= 4 into msg->reply which gets checked in
drm_dp_dpcd_access().

BR,
Jani.

> Don't we need it anymore? why?
>
>> -               else
>> -                       return -EIO;
>> -       }
>> +       switch (msg->request & ~DP_AUX_I2C_MOT) {
>> +       case DP_AUX_NATIVE_WRITE:
>> +       case DP_AUX_I2C_WRITE:
>> +               txsize = HEADER_SIZE + msg->size;
>> +               rxsize = 1;
>>
>> -       DRM_ERROR("too many retries, giving up\n");
>> -       return -EIO;
>> -}
>> +               if (WARN_ON(txsize > 20))
>> +                       return -E2BIG;
>>
>> -/* Write a single byte to the aux channel in native mode */
>> -static int
>> -intel_dp_aux_native_write_1(struct intel_dp *intel_dp,
>> -                           uint16_t address, uint8_t byte)
>> -{
>> -       return intel_dp_aux_native_write(intel_dp, address, &byte, 1);
>> -}
>> +               memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
>>
>> -/* read bytes from a native aux channel */
>> -static int
>> -intel_dp_aux_native_read(struct intel_dp *intel_dp,
>> -                        uint16_t address, uint8_t *recv, int recv_bytes)
>> -{
>> -       uint8_t msg[4];
>> -       int msg_bytes;
>> -       uint8_t reply[20];
>> -       int reply_bytes;
>> -       uint8_t ack;
>> -       int ret;
>> -       int retry;
>> +               ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
>> +               if (ret > 0) {
>> +                       msg->reply = rxbuf[0] >> 4;
>>
>> -       if (WARN_ON(recv_bytes > 19))
>> -               return -E2BIG;
>> +                       /* Return payload size. */
>> +                       ret = msg->size;
>> +               }
>> +               break;
>>
>> -       intel_dp_check_edp(intel_dp);
>> -       msg[0] = DP_AUX_NATIVE_READ << 4;
>> -       msg[1] = address >> 8;
>> -       msg[2] = address & 0xff;
>> -       msg[3] = recv_bytes - 1;
>> +       case DP_AUX_NATIVE_READ:
>> +       case DP_AUX_I2C_READ:
>> +               txsize = HEADER_SIZE;
>> +               rxsize = msg->size + 1;
>>
>> -       msg_bytes = 4;
>> -       reply_bytes = recv_bytes + 1;
>> +               if (WARN_ON(rxsize > 20))
>> +                       return -E2BIG;
>>
>> -       for (retry = 0; retry < 7; retry++) {
>> -               ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes,
>> -                                     reply, reply_bytes);
>> -               if (ret == 0)
>> -                       return -EPROTO;
>> -               if (ret < 0)
>> -                       return ret;
>> -               ack = reply[0] >> 4;
>> -               if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK) {
>> -                       memcpy(recv, reply + 1, ret - 1);
>> -                       return ret - 1;
>> +               ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
>> +               if (ret > 0) {
>> +                       msg->reply = rxbuf[0] >> 4;
>> +                       /*
>> +                        * Assume happy day, and copy the data. The caller is
>> +                        * expected to check msg->reply before touching it.
>> +                        *
>> +                        * Return payload size.
>> +                        */
>> +                       ret--;
>> +                       memcpy(msg->buffer, rxbuf + 1, ret);
>>                 }
>> -               else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
>> -                       usleep_range(400, 500);
>> -               else
>> -                       return -EIO;
>> +               break;
>> +
>> +       default:
>> +               ret = -EINVAL;
>> +               break;
>>         }
>>
>> -       DRM_ERROR("too many retries, giving up\n");
>> -       return -EIO;
>> +       return ret;
>> +}
>> +
>> +static void
>> +intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
>> +{
>> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +
>> +       intel_dp->aux.dev = dev->dev;
>> +       intel_dp->aux.transfer = intel_dp_aux_transfer;
>>  }
>>
>>  static int
>> @@ -1472,8 +1452,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>>                 return;
>>
>>         if (mode != DRM_MODE_DPMS_ON) {
>> -               ret = intel_dp_aux_native_write_1(intel_dp, DP_SET_POWER,
>> -                                                 DP_SET_POWER_D3);
>> +               ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
>> +                                        DP_SET_POWER_D3);
>>                 if (ret != 1)
>>                         DRM_DEBUG_DRIVER("failed to write sink power state\n");
>>         } else {
>> @@ -1482,9 +1462,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>>                  * time to wake up.
>>                  */
>>                 for (i = 0; i < 3; i++) {
>> -                       ret = intel_dp_aux_native_write_1(intel_dp,
>> -                                                         DP_SET_POWER,
>> -                                                         DP_SET_POWER_D0);
>> +                       ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
>> +                                                DP_SET_POWER_D0);
>>                         if (ret == 1)
>>                                 break;
>>                         msleep(1);
>> @@ -1708,13 +1687,11 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>>
>>         /* 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,
>> -                                           DP_PSR_ENABLE &
>> -                                           ~DP_PSR_MAIN_LINK_ACTIVE);
>> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>> +                                  DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
>>         else
>> -               intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>> -                                           DP_PSR_ENABLE |
>> -                                           DP_PSR_MAIN_LINK_ACTIVE);
>> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>> +                                  DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
>>
>>         /* Setup AUX registers */
>>         I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
>> @@ -2026,26 +2003,25 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
>>  /*
>>   * Native read with retry for link status and receiver capability reads for
>>   * cases where the sink may still be asleep.
>> + *
>> + * Sinks are *supposed* to come up within 1ms from an off state, but we're also
>> + * supposed to retry 3 times per the spec.
>>   */
>> -static bool
>> -intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
>> -                              uint8_t *recv, int recv_bytes)
>> +static ssize_t
>> +intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
>> +                       void *buffer, size_t size)
>>  {
>> -       int ret, i;
>> +       ssize_t ret;
>> +       int i;
>>
>> -       /*
>> -        * Sinks are *supposed* to come up within 1ms from an off state,
>> -        * but we're also supposed to retry 3 times per the spec.
>> -        */
>>         for (i = 0; i < 3; i++) {
>> -               ret = intel_dp_aux_native_read(intel_dp, address, recv,
>> -                                              recv_bytes);
>> -               if (ret == recv_bytes)
>> -                       return true;
>> +               ret = drm_dp_dpcd_read(aux, offset, buffer, size);
>> +               if (ret == size)
>> +                       return ret;
>>                 msleep(1);
>>         }
>>
>> -       return false;
>> +       return ret;
>>  }
>>
>>  /*
>> @@ -2055,10 +2031,10 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
>>  static bool
>>  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
>>  {
>> -       return intel_dp_aux_native_read_retry(intel_dp,
>> -                                             DP_LANE0_1_STATUS,
>> -                                             link_status,
>> -                                             DP_LINK_STATUS_SIZE);
>> +       return intel_dp_dpcd_read_wake(&intel_dp->aux,
>> +                                      DP_LANE0_1_STATUS,
>> +                                      link_status,
>> +                                      DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
>>  }
>>
>>  /*
>> @@ -2572,8 +2548,8 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>>                 len = intel_dp->lane_count + 1;
>>         }
>>
>> -       ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_PATTERN_SET,
>> -                                       buf, len);
>> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
>> +                               buf, len);
>>
>>         return ret == len;
>>  }
>> @@ -2602,9 +2578,8 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>>         I915_WRITE(intel_dp->output_reg, *DP);
>>         POSTING_READ(intel_dp->output_reg);
>>
>> -       ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_LANE0_SET,
>> -                                       intel_dp->train_set,
>> -                                       intel_dp->lane_count);
>> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
>> +                               intel_dp->train_set, intel_dp->lane_count);
>>
>>         return ret == intel_dp->lane_count;
>>  }
>> @@ -2660,11 +2635,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>>         link_config[1] = intel_dp->lane_count;
>>         if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>>                 link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> -       intel_dp_aux_native_write(intel_dp, DP_LINK_BW_SET, link_config, 2);
>> +       drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>>
>>         link_config[0] = 0;
>>         link_config[1] = DP_SET_ANSI_8B10B;
>> -       intel_dp_aux_native_write(intel_dp, DP_DOWNSPREAD_CTRL, link_config, 2);
>> +       drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>>
>>         DP |= DP_PORT_EN;
>>
>> @@ -2907,8 +2882,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>
>>         char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
>>
>> -       if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
>> -                                          sizeof(intel_dp->dpcd)) == 0)
>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>> +                                   sizeof(intel_dp->dpcd)) < 0)
>>                 return false; /* aux transfer failed */
>>
>>         hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd),
>> @@ -2921,9 +2896,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>         /* Check if the panel supports PSR */
>>         memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>         if (is_edp(intel_dp)) {
>> -               intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>> -                                              intel_dp->psr_dpcd,
>> -                                              sizeof(intel_dp->psr_dpcd));
>> +               intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
>> +                                       intel_dp->psr_dpcd,
>> +                                       sizeof(intel_dp->psr_dpcd));
>>                 if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>>                         dev_priv->psr.sink_support = true;
>>                         DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>> @@ -2945,9 +2920,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>         if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
>>                 return true; /* no per-port downstream info */
>>
>> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_DOWNSTREAM_PORT_0,
>> -                                          intel_dp->downstream_ports,
>> -                                          DP_MAX_DOWNSTREAM_PORTS) == 0)
>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
>> +                                   intel_dp->downstream_ports,
>> +                                   DP_MAX_DOWNSTREAM_PORTS) < 0)
>>                 return false; /* downstream port status fetch failed */
>>
>>         return true;
>> @@ -2963,11 +2938,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>>
>>         edp_panel_vdd_on(intel_dp);
>>
>> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_SINK_OUI, buf, 3))
>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
>>                 DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>>                               buf[0], buf[1], buf[2]);
>>
>> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_BRANCH_OUI, buf, 3))
>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
>>                 DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>>                               buf[0], buf[1], buf[2]);
>>
>> @@ -2982,46 +2957,40 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>>                 to_intel_crtc(intel_dig_port->base.base.crtc);
>>         u8 buf[1];
>>
>> -       if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1))
>> +       if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, buf) < 0);
>>                 return -EAGAIN;
>>
>>         if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
>>                 return -ENOTTY;
>>
>> -       if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
>> -                                        DP_TEST_SINK_START))
>> +       if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>> +                              DP_TEST_SINK_START) < 0)
>>                 return -EAGAIN;
>>
>>         /* Wait 2 vblanks to be sure we will have the correct CRC value */
>>         intel_wait_for_vblank(dev, intel_crtc->pipe);
>>         intel_wait_for_vblank(dev, intel_crtc->pipe);
>>
>> -       if (!intel_dp_aux_native_read(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
>> +       if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>>                 return -EAGAIN;
>>
>> -       intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
>> +       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
>>         return 0;
>>  }
>>
>>  static bool
>>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>  {
>> -       int ret;
>> -
>> -       ret = intel_dp_aux_native_read_retry(intel_dp,
>> -                                            DP_DEVICE_SERVICE_IRQ_VECTOR,
>> -                                            sink_irq_vector, 1);
>> -       if (!ret)
>> -               return false;
>> -
>> -       return true;
>> +       return intel_dp_dpcd_read_wake(&intel_dp->aux,
>> +                                      DP_DEVICE_SERVICE_IRQ_VECTOR,
>> +                                      sink_irq_vector, 1) == 1;
>>  }
>>
>>  static void
>>  intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>  {
>>         /* NAK by default */
>> -       intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
>> +       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
>>  }
>>
>>  /*
>> @@ -3060,9 +3029,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>>                 /* Clear interrupt source */
>> -               intel_dp_aux_native_write_1(intel_dp,
>> -                                           DP_DEVICE_SERVICE_IRQ_VECTOR,
>> -                                           sink_irq_vector);
>> +               drm_dp_dpcd_writeb(&intel_dp->aux,
>> +                                  DP_DEVICE_SERVICE_IRQ_VECTOR,
>> +                                  sink_irq_vector);
>>
>>                 if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
>>                         intel_dp_handle_test_request(intel_dp);
>> @@ -3097,9 +3066,11 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>             intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>>                 uint8_t reg;
>> -               if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT,
>> -                                                   &reg, 1))
>> +
>> +               if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> +                                           &reg, 1) < 0)
>>                         return connector_status_unknown;
>> +
>>                 return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>>                                               : connector_status_disconnected;
>>         }
>> @@ -3925,6 +3896,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>                 intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
>>         }
>>
>> +       intel_dp_aux_init(intel_dp, intel_connector);
>> +
>>         error = intel_dp_i2c_init(intel_dp, intel_connector, name);
>>         WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
>>              error, port_name(port));
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 2546cae0b4f0..561af4b013b1 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -491,6 +491,7 @@ struct intel_dp {
>>         uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>>         struct i2c_adapter adapter;
>>         struct i2c_algo_dp_aux_data algo;
>> +       struct drm_dp_aux aux;
>>         uint8_t train_set[4];
>>         int panel_power_up_delay;
>>         int panel_power_down_delay;
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 4/6] drm/i915/dp: use the new drm helpers for dp aux

[Rodrigo Vivi]

Thanks for explanations Jani and Daniel.

Feel free to use Reviewe-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Tue, Mar 18, 2014 at 7:54 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> On Mon, 17 Mar 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>> On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>>> Functionality remains largely the same as before.
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_dp.c  |  257 +++++++++++++++++---------------------
>>>  drivers/gpu/drm/i915/intel_drv.h |    1 +
>>>  2 files changed, 116 insertions(+), 142 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 22134edc03dd..24cbf4bd36c4 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -575,97 +575,77 @@ out:
>>>         return ret;
>>>  }
>>>
>>> -/* Write data to the aux channel in native mode */
>>> -static int
>>> -intel_dp_aux_native_write(struct intel_dp *intel_dp,
>>> -                         uint16_t address, uint8_t *send, int send_bytes)
>>> +#define HEADER_SIZE    4
>>> +static ssize_t
>>> +intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>>  {
>>> +       struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
>>> +       uint8_t txbuf[20], rxbuf[20];
>>> +       size_t txsize, rxsize;
>>>         int ret;
>>> -       uint8_t msg[20];
>>> -       int msg_bytes;
>>> -       uint8_t ack;
>>> -       int retry;
>>>
>>> -       if (WARN_ON(send_bytes > 16))
>>> -               return -E2BIG;
>>> +       txbuf[0] = msg->request << 4;
>>> +       txbuf[1] = msg->address >> 8;
>>> +       txbuf[2] = msg->address & 0xff;
>>> +       txbuf[3] = msg->size - 1;
>>>
>>> -       intel_dp_check_edp(intel_dp);
>>> -       msg[0] = DP_AUX_NATIVE_WRITE << 4;
>>> -       msg[1] = address >> 8;
>>> -       msg[2] = address & 0xff;
>>> -       msg[3] = send_bytes - 1;
>>> -       memcpy(&msg[4], send, send_bytes);
>>> -       msg_bytes = send_bytes + 4;
>>> -       for (retry = 0; retry < 7; retry++) {
>>
>> we were retrying 7 times always, but now we are now retrying only 3
>> times or even none.
>
> I'm confused, what are you referring to? drm_dp_dpcd_access() in
> drm_dp_helper.c has the retry loop now.
>
> I think you're mixing this with the intel_dp_dpcd_read_wake() wrapper,
> but that ends up in drm_dp_dpcd_access() as well.
>
>> Couldn't it trigger some new bug or regression?
>>
>>> -               ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes, &ack, 1);
>>> -               if (ret < 0)
>>> -                       return ret;
>>> -               ack >>= 4;
>>> -               if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK)
>>> -                       return send_bytes;
>>> -               else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
>>> -                       usleep_range(400, 500);
>>
>> we are also not checking this ack x defer (here nor in native_write)
>> replies anymore.
>
> Ditto, we store ack >>= 4 into msg->reply which gets checked in
> drm_dp_dpcd_access().
>
> BR,
> Jani.
>
>> Don't we need it anymore? why?
>>
>>> -               else
>>> -                       return -EIO;
>>> -       }
>>> +       switch (msg->request & ~DP_AUX_I2C_MOT) {
>>> +       case DP_AUX_NATIVE_WRITE:
>>> +       case DP_AUX_I2C_WRITE:
>>> +               txsize = HEADER_SIZE + msg->size;
>>> +               rxsize = 1;
>>>
>>> -       DRM_ERROR("too many retries, giving up\n");
>>> -       return -EIO;
>>> -}
>>> +               if (WARN_ON(txsize > 20))
>>> +                       return -E2BIG;
>>>
>>> -/* Write a single byte to the aux channel in native mode */
>>> -static int
>>> -intel_dp_aux_native_write_1(struct intel_dp *intel_dp,
>>> -                           uint16_t address, uint8_t byte)
>>> -{
>>> -       return intel_dp_aux_native_write(intel_dp, address, &byte, 1);
>>> -}
>>> +               memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
>>>
>>> -/* read bytes from a native aux channel */
>>> -static int
>>> -intel_dp_aux_native_read(struct intel_dp *intel_dp,
>>> -                        uint16_t address, uint8_t *recv, int recv_bytes)
>>> -{
>>> -       uint8_t msg[4];
>>> -       int msg_bytes;
>>> -       uint8_t reply[20];
>>> -       int reply_bytes;
>>> -       uint8_t ack;
>>> -       int ret;
>>> -       int retry;
>>> +               ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
>>> +               if (ret > 0) {
>>> +                       msg->reply = rxbuf[0] >> 4;
>>>
>>> -       if (WARN_ON(recv_bytes > 19))
>>> -               return -E2BIG;
>>> +                       /* Return payload size. */
>>> +                       ret = msg->size;
>>> +               }
>>> +               break;
>>>
>>> -       intel_dp_check_edp(intel_dp);
>>> -       msg[0] = DP_AUX_NATIVE_READ << 4;
>>> -       msg[1] = address >> 8;
>>> -       msg[2] = address & 0xff;
>>> -       msg[3] = recv_bytes - 1;
>>> +       case DP_AUX_NATIVE_READ:
>>> +       case DP_AUX_I2C_READ:
>>> +               txsize = HEADER_SIZE;
>>> +               rxsize = msg->size + 1;
>>>
>>> -       msg_bytes = 4;
>>> -       reply_bytes = recv_bytes + 1;
>>> +               if (WARN_ON(rxsize > 20))
>>> +                       return -E2BIG;
>>>
>>> -       for (retry = 0; retry < 7; retry++) {
>>> -               ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes,
>>> -                                     reply, reply_bytes);
>>> -               if (ret == 0)
>>> -                       return -EPROTO;
>>> -               if (ret < 0)
>>> -                       return ret;
>>> -               ack = reply[0] >> 4;
>>> -               if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK) {
>>> -                       memcpy(recv, reply + 1, ret - 1);
>>> -                       return ret - 1;
>>> +               ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
>>> +               if (ret > 0) {
>>> +                       msg->reply = rxbuf[0] >> 4;
>>> +                       /*
>>> +                        * Assume happy day, and copy the data. The caller is
>>> +                        * expected to check msg->reply before touching it.
>>> +                        *
>>> +                        * Return payload size.
>>> +                        */
>>> +                       ret--;
>>> +                       memcpy(msg->buffer, rxbuf + 1, ret);
>>>                 }
>>> -               else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
>>> -                       usleep_range(400, 500);
>>> -               else
>>> -                       return -EIO;
>>> +               break;
>>> +
>>> +       default:
>>> +               ret = -EINVAL;
>>> +               break;
>>>         }
>>>
>>> -       DRM_ERROR("too many retries, giving up\n");
>>> -       return -EIO;
>>> +       return ret;
>>> +}
>>> +
>>> +static void
>>> +intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
>>> +{
>>> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>> +
>>> +       intel_dp->aux.dev = dev->dev;
>>> +       intel_dp->aux.transfer = intel_dp_aux_transfer;
>>>  }
>>>
>>>  static int
>>> @@ -1472,8 +1452,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>>>                 return;
>>>
>>>         if (mode != DRM_MODE_DPMS_ON) {
>>> -               ret = intel_dp_aux_native_write_1(intel_dp, DP_SET_POWER,
>>> -                                                 DP_SET_POWER_D3);
>>> +               ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
>>> +                                        DP_SET_POWER_D3);
>>>                 if (ret != 1)
>>>                         DRM_DEBUG_DRIVER("failed to write sink power state\n");
>>>         } else {
>>> @@ -1482,9 +1462,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>>>                  * time to wake up.
>>>                  */
>>>                 for (i = 0; i < 3; i++) {
>>> -                       ret = intel_dp_aux_native_write_1(intel_dp,
>>> -                                                         DP_SET_POWER,
>>> -                                                         DP_SET_POWER_D0);
>>> +                       ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
>>> +                                                DP_SET_POWER_D0);
>>>                         if (ret == 1)
>>>                                 break;
>>>                         msleep(1);
>>> @@ -1708,13 +1687,11 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>>>
>>>         /* 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,
>>> -                                           DP_PSR_ENABLE &
>>> -                                           ~DP_PSR_MAIN_LINK_ACTIVE);
>>> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>>> +                                  DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
>>>         else
>>> -               intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>>> -                                           DP_PSR_ENABLE |
>>> -                                           DP_PSR_MAIN_LINK_ACTIVE);
>>> +               drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>>> +                                  DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
>>>
>>>         /* Setup AUX registers */
>>>         I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
>>> @@ -2026,26 +2003,25 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
>>>  /*
>>>   * Native read with retry for link status and receiver capability reads for
>>>   * cases where the sink may still be asleep.
>>> + *
>>> + * Sinks are *supposed* to come up within 1ms from an off state, but we're also
>>> + * supposed to retry 3 times per the spec.
>>>   */
>>> -static bool
>>> -intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
>>> -                              uint8_t *recv, int recv_bytes)
>>> +static ssize_t
>>> +intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
>>> +                       void *buffer, size_t size)
>>>  {
>>> -       int ret, i;
>>> +       ssize_t ret;
>>> +       int i;
>>>
>>> -       /*
>>> -        * Sinks are *supposed* to come up within 1ms from an off state,
>>> -        * but we're also supposed to retry 3 times per the spec.
>>> -        */
>>>         for (i = 0; i < 3; i++) {
>>> -               ret = intel_dp_aux_native_read(intel_dp, address, recv,
>>> -                                              recv_bytes);
>>> -               if (ret == recv_bytes)
>>> -                       return true;
>>> +               ret = drm_dp_dpcd_read(aux, offset, buffer, size);
>>> +               if (ret == size)
>>> +                       return ret;
>>>                 msleep(1);
>>>         }
>>>
>>> -       return false;
>>> +       return ret;
>>>  }
>>>
>>>  /*
>>> @@ -2055,10 +2031,10 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
>>>  static bool
>>>  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
>>>  {
>>> -       return intel_dp_aux_native_read_retry(intel_dp,
>>> -                                             DP_LANE0_1_STATUS,
>>> -                                             link_status,
>>> -                                             DP_LINK_STATUS_SIZE);
>>> +       return intel_dp_dpcd_read_wake(&intel_dp->aux,
>>> +                                      DP_LANE0_1_STATUS,
>>> +                                      link_status,
>>> +                                      DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
>>>  }
>>>
>>>  /*
>>> @@ -2572,8 +2548,8 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>>>                 len = intel_dp->lane_count + 1;
>>>         }
>>>
>>> -       ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_PATTERN_SET,
>>> -                                       buf, len);
>>> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
>>> +                               buf, len);
>>>
>>>         return ret == len;
>>>  }
>>> @@ -2602,9 +2578,8 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>>>         I915_WRITE(intel_dp->output_reg, *DP);
>>>         POSTING_READ(intel_dp->output_reg);
>>>
>>> -       ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_LANE0_SET,
>>> -                                       intel_dp->train_set,
>>> -                                       intel_dp->lane_count);
>>> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
>>> +                               intel_dp->train_set, intel_dp->lane_count);
>>>
>>>         return ret == intel_dp->lane_count;
>>>  }
>>> @@ -2660,11 +2635,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>>>         link_config[1] = intel_dp->lane_count;
>>>         if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>>>                 link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>>> -       intel_dp_aux_native_write(intel_dp, DP_LINK_BW_SET, link_config, 2);
>>> +       drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>>>
>>>         link_config[0] = 0;
>>>         link_config[1] = DP_SET_ANSI_8B10B;
>>> -       intel_dp_aux_native_write(intel_dp, DP_DOWNSPREAD_CTRL, link_config, 2);
>>> +       drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>>>
>>>         DP |= DP_PORT_EN;
>>>
>>> @@ -2907,8 +2882,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>
>>>         char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
>>>
>>> -       if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
>>> -                                          sizeof(intel_dp->dpcd)) == 0)
>>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>>> +                                   sizeof(intel_dp->dpcd)) < 0)
>>>                 return false; /* aux transfer failed */
>>>
>>>         hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd),
>>> @@ -2921,9 +2896,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>         /* Check if the panel supports PSR */
>>>         memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>>         if (is_edp(intel_dp)) {
>>> -               intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>>> -                                              intel_dp->psr_dpcd,
>>> -                                              sizeof(intel_dp->psr_dpcd));
>>> +               intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
>>> +                                       intel_dp->psr_dpcd,
>>> +                                       sizeof(intel_dp->psr_dpcd));
>>>                 if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>>>                         dev_priv->psr.sink_support = true;
>>>                         DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>>> @@ -2945,9 +2920,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>         if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
>>>                 return true; /* no per-port downstream info */
>>>
>>> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_DOWNSTREAM_PORT_0,
>>> -                                          intel_dp->downstream_ports,
>>> -                                          DP_MAX_DOWNSTREAM_PORTS) == 0)
>>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
>>> +                                   intel_dp->downstream_ports,
>>> +                                   DP_MAX_DOWNSTREAM_PORTS) < 0)
>>>                 return false; /* downstream port status fetch failed */
>>>
>>>         return true;
>>> @@ -2963,11 +2938,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>>>
>>>         edp_panel_vdd_on(intel_dp);
>>>
>>> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_SINK_OUI, buf, 3))
>>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
>>>                 DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>>>                               buf[0], buf[1], buf[2]);
>>>
>>> -       if (intel_dp_aux_native_read_retry(intel_dp, DP_BRANCH_OUI, buf, 3))
>>> +       if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
>>>                 DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>>>                               buf[0], buf[1], buf[2]);
>>>
>>> @@ -2982,46 +2957,40 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>>>                 to_intel_crtc(intel_dig_port->base.base.crtc);
>>>         u8 buf[1];
>>>
>>> -       if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1))
>>> +       if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, buf) < 0);
>>>                 return -EAGAIN;
>>>
>>>         if (!(buf[0] & DP_TEST_CRC_SUPPORTED))
>>>                 return -ENOTTY;
>>>
>>> -       if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK,
>>> -                                        DP_TEST_SINK_START))
>>> +       if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>>> +                              DP_TEST_SINK_START) < 0)
>>>                 return -EAGAIN;
>>>
>>>         /* Wait 2 vblanks to be sure we will have the correct CRC value */
>>>         intel_wait_for_vblank(dev, intel_crtc->pipe);
>>>         intel_wait_for_vblank(dev, intel_crtc->pipe);
>>>
>>> -       if (!intel_dp_aux_native_read(intel_dp, DP_TEST_CRC_R_CR, crc, 6))
>>> +       if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>>>                 return -EAGAIN;
>>>
>>> -       intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0);
>>> +       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
>>>         return 0;
>>>  }
>>>
>>>  static bool
>>>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>>  {
>>> -       int ret;
>>> -
>>> -       ret = intel_dp_aux_native_read_retry(intel_dp,
>>> -                                            DP_DEVICE_SERVICE_IRQ_VECTOR,
>>> -                                            sink_irq_vector, 1);
>>> -       if (!ret)
>>> -               return false;
>>> -
>>> -       return true;
>>> +       return intel_dp_dpcd_read_wake(&intel_dp->aux,
>>> +                                      DP_DEVICE_SERVICE_IRQ_VECTOR,
>>> +                                      sink_irq_vector, 1) == 1;
>>>  }
>>>
>>>  static void
>>>  intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>>  {
>>>         /* NAK by default */
>>> -       intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
>>> +       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
>>>  }
>>>
>>>  /*
>>> @@ -3060,9 +3029,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>>>                 /* Clear interrupt source */
>>> -               intel_dp_aux_native_write_1(intel_dp,
>>> -                                           DP_DEVICE_SERVICE_IRQ_VECTOR,
>>> -                                           sink_irq_vector);
>>> +               drm_dp_dpcd_writeb(&intel_dp->aux,
>>> +                                  DP_DEVICE_SERVICE_IRQ_VECTOR,
>>> +                                  sink_irq_vector);
>>>
>>>                 if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
>>>                         intel_dp_handle_test_request(intel_dp);
>>> @@ -3097,9 +3066,11 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>             intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>>>                 uint8_t reg;
>>> -               if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT,
>>> -                                                   &reg, 1))
>>> +
>>> +               if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>>> +                                           &reg, 1) < 0)
>>>                         return connector_status_unknown;
>>> +
>>>                 return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>>>                                               : connector_status_disconnected;
>>>         }
>>> @@ -3925,6 +3896,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>>                 intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
>>>         }
>>>
>>> +       intel_dp_aux_init(intel_dp, intel_connector);
>>> +
>>>         error = intel_dp_i2c_init(intel_dp, intel_connector, name);
>>>         WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
>>>              error, port_name(port));
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 2546cae0b4f0..561af4b013b1 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -491,6 +491,7 @@ struct intel_dp {
>>>         uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>>>         struct i2c_adapter adapter;
>>>         struct i2c_algo_dp_aux_data algo;
>>> +       struct drm_dp_aux aux;
>>>         uint8_t train_set[4];
>>>         int panel_power_up_delay;
>>>         int panel_power_down_delay;
>>> --
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Rodrigo Vivi
>> Blog: http://blog.vivi.eng.br
>
> --
> Jani Nikula, Intel Open Source Technology Center



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 4/6] drm/i915/dp: use the new drm helpers for dp aux

[Daniel Vetter]

On Tue, Mar 18, 2014 at 3:02 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> Thanks for explanations Jani and Daniel.
>
> Feel free to use Reviewe-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Thanks to patches & review, all pulled into a topic branch, will
probably merge into dinq in a few days.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 5/6] drm/i915/dp: move dp aux ch register init to aux init

[Jani Nikula]

Do a slight rearrangement of the switch to prep for follow-up.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   42 ++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 24cbf4bd36c4..71a76d00d634 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -643,6 +643,28 @@ static void
 intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	enum port port = intel_dig_port->port;
+
+	switch (port) {
+	case PORT_A:
+		intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
+		break;
+	case PORT_B:
+		intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL;
+		break;
+	case PORT_C:
+		intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL;
+		break;
+	case PORT_D:
+		intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL;
+		break;
+	default:
+		BUG();
+	}
+
+	if (!HAS_DDI(dev))
+		intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
 
 	intel_dp->aux.dev = dev->dev;
 	intel_dp->aux.transfer = intel_dp_aux_transfer;
@@ -3849,26 +3871,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		intel_connector->get_hw_state = intel_connector_get_hw_state;
 	intel_connector->unregister = intel_dp_connector_unregister;
 
-	intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
-	if (HAS_DDI(dev)) {
-		switch (intel_dig_port->port) {
-		case PORT_A:
-			intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
-			break;
-		case PORT_B:
-			intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL;
-			break;
-		case PORT_C:
-			intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL;
-			break;
-		case PORT_D:
-			intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL;
-			break;
-		default:
-			BUG();
-		}
-	}
-
 	/* Set up the DDC bus. */
 	switch (port) {
 	case PORT_A:
-- 
1.7.9.5

Re: [PATCH 5/6] drm/i915/dp: move dp aux ch register init to aux init

[Rodrigo Vivi]

I would prefer an else instead of unecessary comparisons and
assignment that will be override..

But anyway it works, so feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> Do a slight rearrangement of the switch to prep for follow-up.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   42 ++++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 24cbf4bd36c4..71a76d00d634 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -643,6 +643,28 @@ static void
>  intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
>  {
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +       struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +       enum port port = intel_dig_port->port;
> +
> +       switch (port) {
> +       case PORT_A:
> +               intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
> +               break;
> +       case PORT_B:
> +               intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL;
> +               break;
> +       case PORT_C:
> +               intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL;
> +               break;
> +       case PORT_D:
> +               intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL;
> +               break;
> +       default:
> +               BUG();
> +       }
> +
> +       if (!HAS_DDI(dev))
> +               intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
>
>         intel_dp->aux.dev = dev->dev;
>         intel_dp->aux.transfer = intel_dp_aux_transfer;
> @@ -3849,26 +3871,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>                 intel_connector->get_hw_state = intel_connector_get_hw_state;
>         intel_connector->unregister = intel_dp_connector_unregister;
>
> -       intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
> -       if (HAS_DDI(dev)) {
> -               switch (intel_dig_port->port) {
> -               case PORT_A:
> -                       intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
> -                       break;
> -               case PORT_B:
> -                       intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL;
> -                       break;
> -               case PORT_C:
> -                       intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL;
> -                       break;
> -               case PORT_D:
> -                       intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL;
> -                       break;
> -               default:
> -                       BUG();
> -               }
> -       }
> -
>         /* Set up the DDC bus. */
>         switch (port) {
>         case PORT_A:
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 5/6] drm/i915/dp: move dp aux ch register init to aux init

[Jani Nikula]

On Mon, 17 Mar 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> I would prefer an else instead of unecessary comparisons and
> assignment that will be override..

In general I agree, but see next patch. ;)

BR,
Jani.


>
> But anyway it works, so feel free to use:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
> On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>> Do a slight rearrangement of the switch to prep for follow-up.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c |   42 ++++++++++++++++++++-------------------
>>  1 file changed, 22 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 24cbf4bd36c4..71a76d00d634 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -643,6 +643,28 @@ static void
>>  intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
>>  {
>>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +       struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> +       enum port port = intel_dig_port->port;
>> +
>> +       switch (port) {
>> +       case PORT_A:
>> +               intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
>> +               break;
>> +       case PORT_B:
>> +               intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL;
>> +               break;
>> +       case PORT_C:
>> +               intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL;
>> +               break;
>> +       case PORT_D:
>> +               intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL;
>> +               break;
>> +       default:
>> +               BUG();
>> +       }
>> +
>> +       if (!HAS_DDI(dev))
>> +               intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
>>
>>         intel_dp->aux.dev = dev->dev;
>>         intel_dp->aux.transfer = intel_dp_aux_transfer;
>> @@ -3849,26 +3871,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>                 intel_connector->get_hw_state = intel_connector_get_hw_state;
>>         intel_connector->unregister = intel_dp_connector_unregister;
>>
>> -       intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
>> -       if (HAS_DDI(dev)) {
>> -               switch (intel_dig_port->port) {
>> -               case PORT_A:
>> -                       intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
>> -                       break;
>> -               case PORT_B:
>> -                       intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL;
>> -                       break;
>> -               case PORT_C:
>> -                       intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL;
>> -                       break;
>> -               case PORT_D:
>> -                       intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL;
>> -                       break;
>> -               default:
>> -                       BUG();
>> -               }
>> -       }
>> -
>>         /* Set up the DDC bus. */
>>         switch (port) {
>>         case PORT_A:
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 6/6] drm/i915/dp: use the new drm helpers for dp i2c-over-aux

[Jani Nikula]

The functionality remains largerly the same. The main difference is that
i2c-over-aux defer timeouts are increased to be safe for all use cases
instead of depending on DP device type and properties.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |  197 ++++++--------------------------------
 drivers/gpu/drm/i915/intel_drv.h |    2 -
 2 files changed, 30 insertions(+), 169 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 71a76d00d634..208fdf075140 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -645,19 +645,25 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	enum port port = intel_dig_port->port;
+	const char *name = NULL;
+	int ret;
 
 	switch (port) {
 	case PORT_A:
 		intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
+		name = "DPDDC-A";
 		break;
 	case PORT_B:
 		intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL;
+		name = "DPDDC-B";
 		break;
 	case PORT_C:
 		intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL;
+		name = "DPDDC-C";
 		break;
 	case PORT_D:
 		intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL;
+		name = "DPDDC-D";
 		break;
 	default:
 		BUG();
@@ -666,128 +672,27 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
 	if (!HAS_DDI(dev))
 		intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
 
+	intel_dp->aux.name = name;
 	intel_dp->aux.dev = dev->dev;
 	intel_dp->aux.transfer = intel_dp_aux_transfer;
-}
 
-static int
-intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
-		    uint8_t write_byte, uint8_t *read_byte)
-{
-	struct i2c_algo_dp_aux_data *algo_data = adapter->algo_data;
-	struct intel_dp *intel_dp = container_of(adapter,
-						struct intel_dp,
-						adapter);
-	uint16_t address = algo_data->address;
-	uint8_t msg[5];
-	uint8_t reply[2];
-	unsigned retry;
-	int msg_bytes;
-	int reply_bytes;
-	int ret;
-
-	/* Set up the command byte */
-	if (mode & MODE_I2C_READ)
-		msg[0] = DP_AUX_I2C_READ << 4;
-	else
-		msg[0] = DP_AUX_I2C_WRITE << 4;
+	DRM_DEBUG_KMS("registering %s bus for %s\n", name,
+		      connector->base.kdev->kobj.name);
 
-	if (!(mode & MODE_I2C_STOP))
-		msg[0] |= DP_AUX_I2C_MOT << 4;
-
-	msg[1] = address >> 8;
-	msg[2] = address;
-
-	switch (mode) {
-	case MODE_I2C_WRITE:
-		msg[3] = 0;
-		msg[4] = write_byte;
-		msg_bytes = 5;
-		reply_bytes = 1;
-		break;
-	case MODE_I2C_READ:
-		msg[3] = 0;
-		msg_bytes = 4;
-		reply_bytes = 2;
-		break;
-	default:
-		msg_bytes = 3;
-		reply_bytes = 1;
-		break;
+	ret = drm_dp_aux_register_i2c_bus(&intel_dp->aux);
+	if (ret < 0) {
+		DRM_ERROR("drm_dp_aux_register_i2c_bus() for %s failed (%d)\n",
+			  name, ret);
+		return;
 	}
 
-	/*
-	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device is
-	 * required to retry at least seven times upon receiving AUX_DEFER
-	 * before giving up the AUX transaction.
-	 */
-	for (retry = 0; retry < 7; retry++) {
-		ret = intel_dp_aux_ch(intel_dp,
-				      msg, msg_bytes,
-				      reply, reply_bytes);
-		if (ret < 0) {
-			DRM_DEBUG_KMS("aux_ch failed %d\n", ret);
-			goto out;
-		}
-
-		switch ((reply[0] >> 4) & DP_AUX_NATIVE_REPLY_MASK) {
-		case DP_AUX_NATIVE_REPLY_ACK:
-			/* I2C-over-AUX Reply field is only valid
-			 * when paired with AUX ACK.
-			 */
-			break;
-		case DP_AUX_NATIVE_REPLY_NACK:
-			DRM_DEBUG_KMS("aux_ch native nack\n");
-			ret = -EREMOTEIO;
-			goto out;
-		case DP_AUX_NATIVE_REPLY_DEFER:
-			/*
-			 * For now, just give more slack to branch devices. We
-			 * could check the DPCD for I2C bit rate capabilities,
-			 * and if available, adjust the interval. We could also
-			 * be more careful with DP-to-Legacy adapters where a
-			 * long legacy cable may force very low I2C bit rates.
-			 */
-			if (intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
-			    DP_DWN_STRM_PORT_PRESENT)
-				usleep_range(500, 600);
-			else
-				usleep_range(300, 400);
-			continue;
-		default:
-			DRM_ERROR("aux_ch invalid native reply 0x%02x\n",
-				  reply[0]);
-			ret = -EREMOTEIO;
-			goto out;
-		}
-
-		switch ((reply[0] >> 4) & DP_AUX_I2C_REPLY_MASK) {
-		case DP_AUX_I2C_REPLY_ACK:
-			if (mode == MODE_I2C_READ) {
-				*read_byte = reply[1];
-			}
-			ret = reply_bytes - 1;
-			goto out;
-		case DP_AUX_I2C_REPLY_NACK:
-			DRM_DEBUG_KMS("aux_i2c nack\n");
-			ret = -EREMOTEIO;
-			goto out;
-		case DP_AUX_I2C_REPLY_DEFER:
-			DRM_DEBUG_KMS("aux_i2c defer\n");
-			udelay(100);
-			break;
-		default:
-			DRM_ERROR("aux_i2c invalid reply 0x%02x\n", reply[0]);
-			ret = -EREMOTEIO;
-			goto out;
-		}
+	ret = sysfs_create_link(&connector->base.kdev->kobj,
+				&intel_dp->aux.ddc.dev.kobj,
+				intel_dp->aux.ddc.dev.kobj.name);
+	if (ret < 0) {
+		DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, ret);
+		drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
 	}
-
-	DRM_ERROR("too many retries, giving up\n");
-	ret = -EREMOTEIO;
-
-out:
-	return ret;
 }
 
 static void
@@ -796,43 +701,10 @@ intel_dp_connector_unregister(struct intel_connector *intel_connector)
 	struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base);
 
 	sysfs_remove_link(&intel_connector->base.kdev->kobj,
-			  intel_dp->adapter.dev.kobj.name);
+			  intel_dp->aux.ddc.dev.kobj.name);
 	intel_connector_unregister(intel_connector);
 }
 
-static int
-intel_dp_i2c_init(struct intel_dp *intel_dp,
-		  struct intel_connector *intel_connector, const char *name)
-{
-	int	ret;
-
-	DRM_DEBUG_KMS("i2c_init %s\n", name);
-	intel_dp->algo.running = false;
-	intel_dp->algo.address = 0;
-	intel_dp->algo.aux_ch = intel_dp_i2c_aux_ch;
-
-	memset(&intel_dp->adapter, '\0', sizeof(intel_dp->adapter));
-	intel_dp->adapter.owner = THIS_MODULE;
-	intel_dp->adapter.class = I2C_CLASS_DDC;
-	strncpy(intel_dp->adapter.name, name, sizeof(intel_dp->adapter.name) - 1);
-	intel_dp->adapter.name[sizeof(intel_dp->adapter.name) - 1] = '\0';
-	intel_dp->adapter.algo_data = &intel_dp->algo;
-	intel_dp->adapter.dev.parent = intel_connector->base.dev->dev;
-
-	ret = i2c_dp_aux_add_bus(&intel_dp->adapter);
-	if (ret < 0)
-		return ret;
-
-	ret = sysfs_create_link(&intel_connector->base.kdev->kobj,
-				&intel_dp->adapter.dev.kobj,
-				intel_dp->adapter.dev.kobj.name);
-
-	if (ret < 0)
-		i2c_del_adapter(&intel_dp->adapter);
-
-	return ret;
-}
-
 static void
 intel_dp_set_clock(struct intel_encoder *encoder,
 		   struct intel_crtc_config *pipe_config, int link_bw)
@@ -3098,7 +2970,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	}
 
 	/* If no HPD, poke DDC gently */
-	if (drm_probe_ddc(&intel_dp->adapter))
+	if (drm_probe_ddc(&intel_dp->aux.ddc))
 		return connector_status_connected;
 
 	/* Well we tried, say unknown for unreliable port types */
@@ -3266,7 +3138,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	if (intel_dp->force_audio != HDMI_AUDIO_AUTO) {
 		intel_dp->has_audio = (intel_dp->force_audio == HDMI_AUDIO_ON);
 	} else {
-		edid = intel_dp_get_edid(connector, &intel_dp->adapter);
+		edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
 		if (edid) {
 			intel_dp->has_audio = drm_detect_monitor_audio(edid);
 			kfree(edid);
@@ -3302,7 +3174,7 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	ret = intel_dp_get_edid_modes(connector, &intel_dp->adapter);
+	ret = intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc);
 	intel_display_power_put(dev_priv, power_domain);
 	if (ret)
 		return ret;
@@ -3335,7 +3207,7 @@ intel_dp_detect_audio(struct drm_connector *connector)
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	edid = intel_dp_get_edid(connector, &intel_dp->adapter);
+	edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
 	if (edid) {
 		has_audio = drm_detect_monitor_audio(edid);
 		kfree(edid);
@@ -3457,7 +3329,7 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 
-	i2c_del_adapter(&intel_dp->adapter);
+	drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
@@ -3769,7 +3641,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	/* We now know it's not a ghost, init power sequence regs. */
 	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
 
-	edid = drm_get_edid(connector, &intel_dp->adapter);
+	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
 	if (edid) {
 		if (drm_add_edid_modes(connector, edid)) {
 			drm_mode_connector_update_edid_property(connector,
@@ -3817,8 +3689,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum port port = intel_dig_port->port;
 	struct edp_power_seq power_seq = { 0 };
-	const char *name = NULL;
-	int type, error;
+	int type;
 
 	/* intel_dp vfuncs */
 	if (IS_VALLEYVIEW(dev))
@@ -3871,23 +3742,19 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		intel_connector->get_hw_state = intel_connector_get_hw_state;
 	intel_connector->unregister = intel_dp_connector_unregister;
 
-	/* Set up the DDC bus. */
+	/* Set up the hotplug pin. */
 	switch (port) {
 	case PORT_A:
 		intel_encoder->hpd_pin = HPD_PORT_A;
-		name = "DPDDC-A";
 		break;
 	case PORT_B:
 		intel_encoder->hpd_pin = HPD_PORT_B;
-		name = "DPDDC-B";
 		break;
 	case PORT_C:
 		intel_encoder->hpd_pin = HPD_PORT_C;
-		name = "DPDDC-C";
 		break;
 	case PORT_D:
 		intel_encoder->hpd_pin = HPD_PORT_D;
-		name = "DPDDC-D";
 		break;
 	default:
 		BUG();
@@ -3900,14 +3767,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 
 	intel_dp_aux_init(intel_dp, intel_connector);
 
-	error = intel_dp_i2c_init(intel_dp, intel_connector, name);
-	WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
-	     error, port_name(port));
-
 	intel_dp->psr_setup_done = false;
 
 	if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
-		i2c_del_adapter(&intel_dp->adapter);
+		drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
 		if (is_edp(intel_dp)) {
 			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
 			mutex_lock(&dev->mode_config.mutex);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 561af4b013b1..604d57e5e4c8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -489,8 +489,6 @@ struct intel_dp {
 	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
 	uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
-	struct i2c_adapter adapter;
-	struct i2c_algo_dp_aux_data algo;
 	struct drm_dp_aux aux;
 	uint8_t train_set[4];
 	int panel_power_up_delay;
-- 
1.7.9.5

Re: [PATCH 6/6] drm/i915/dp: use the new drm helpers for dp i2c-over-aux

[Rodrigo Vivi]

On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> The functionality remains largerly the same. The main difference is that
> i2c-over-aux defer timeouts are increased to be safe for all use cases
> instead of depending on DP device type and properties.

I was going to ask about different timeouts but you had already answered here ;)

>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |  197 ++++++--------------------------------
>  drivers/gpu/drm/i915/intel_drv.h |    2 -
>  2 files changed, 30 insertions(+), 169 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 71a76d00d634..208fdf075140 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -645,19 +645,25 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>         struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>         enum port port = intel_dig_port->port;
> +       const char *name = NULL;
> +       int ret;
>
>         switch (port) {
>         case PORT_A:
>                 intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
> +               name = "DPDDC-A";
>                 break;
>         case PORT_B:
>                 intel_dp->aux_ch_ctl_reg = PCH_DPB_AUX_CH_CTL;
> +               name = "DPDDC-B";
>                 break;
>         case PORT_C:
>                 intel_dp->aux_ch_ctl_reg = PCH_DPC_AUX_CH_CTL;
> +               name = "DPDDC-C";
>                 break;
>         case PORT_D:
>                 intel_dp->aux_ch_ctl_reg = PCH_DPD_AUX_CH_CTL;
> +               name = "DPDDC-D";
>                 break;
>         default:
>                 BUG();
> @@ -666,128 +672,27 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
>         if (!HAS_DDI(dev))
>                 intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
>
> +       intel_dp->aux.name = name;
>         intel_dp->aux.dev = dev->dev;
>         intel_dp->aux.transfer = intel_dp_aux_transfer;
> -}
>
> -static int
> -intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
> -                   uint8_t write_byte, uint8_t *read_byte)
> -{
> -       struct i2c_algo_dp_aux_data *algo_data = adapter->algo_data;
> -       struct intel_dp *intel_dp = container_of(adapter,
> -                                               struct intel_dp,
> -                                               adapter);
> -       uint16_t address = algo_data->address;
> -       uint8_t msg[5];
> -       uint8_t reply[2];
> -       unsigned retry;
> -       int msg_bytes;
> -       int reply_bytes;
> -       int ret;
> -
> -       /* Set up the command byte */
> -       if (mode & MODE_I2C_READ)
> -               msg[0] = DP_AUX_I2C_READ << 4;
> -       else
> -               msg[0] = DP_AUX_I2C_WRITE << 4;
> +       DRM_DEBUG_KMS("registering %s bus for %s\n", name,
> +                     connector->base.kdev->kobj.name);
>
> -       if (!(mode & MODE_I2C_STOP))
> -               msg[0] |= DP_AUX_I2C_MOT << 4;
> -
> -       msg[1] = address >> 8;
> -       msg[2] = address;
> -
> -       switch (mode) {
> -       case MODE_I2C_WRITE:
> -               msg[3] = 0;
> -               msg[4] = write_byte;
> -               msg_bytes = 5;
> -               reply_bytes = 1;
> -               break;
> -       case MODE_I2C_READ:
> -               msg[3] = 0;
> -               msg_bytes = 4;
> -               reply_bytes = 2;
> -               break;
> -       default:
> -               msg_bytes = 3;
> -               reply_bytes = 1;
> -               break;
> +       ret = drm_dp_aux_register_i2c_bus(&intel_dp->aux);
> +       if (ret < 0) {
> +               DRM_ERROR("drm_dp_aux_register_i2c_bus() for %s failed (%d)\n",
> +                         name, ret);
> +               return;
>         }
>
> -       /*
> -        * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device is
> -        * required to retry at least seven times upon receiving AUX_DEFER
> -        * before giving up the AUX transaction.
> -        */
> -       for (retry = 0; retry < 7; retry++) {
> -               ret = intel_dp_aux_ch(intel_dp,
> -                                     msg, msg_bytes,
> -                                     reply, reply_bytes);
> -               if (ret < 0) {
> -                       DRM_DEBUG_KMS("aux_ch failed %d\n", ret);
> -                       goto out;
> -               }
> -
> -               switch ((reply[0] >> 4) & DP_AUX_NATIVE_REPLY_MASK) {
> -               case DP_AUX_NATIVE_REPLY_ACK:
> -                       /* I2C-over-AUX Reply field is only valid
> -                        * when paired with AUX ACK.
> -                        */
> -                       break;
> -               case DP_AUX_NATIVE_REPLY_NACK:
> -                       DRM_DEBUG_KMS("aux_ch native nack\n");
> -                       ret = -EREMOTEIO;
> -                       goto out;
> -               case DP_AUX_NATIVE_REPLY_DEFER:
> -                       /*
> -                        * For now, just give more slack to branch devices. We
> -                        * could check the DPCD for I2C bit rate capabilities,
> -                        * and if available, adjust the interval. We could also
> -                        * be more careful with DP-to-Legacy adapters where a
> -                        * long legacy cable may force very low I2C bit rates.
> -                        */
> -                       if (intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> -                           DP_DWN_STRM_PORT_PRESENT)
> -                               usleep_range(500, 600);
> -                       else
> -                               usleep_range(300, 400);
> -                       continue;
> -               default:
> -                       DRM_ERROR("aux_ch invalid native reply 0x%02x\n",
> -                                 reply[0]);
> -                       ret = -EREMOTEIO;
> -                       goto out;
> -               }
> -
> -               switch ((reply[0] >> 4) & DP_AUX_I2C_REPLY_MASK) {
> -               case DP_AUX_I2C_REPLY_ACK:
> -                       if (mode == MODE_I2C_READ) {
> -                               *read_byte = reply[1];
> -                       }
> -                       ret = reply_bytes - 1;
> -                       goto out;
> -               case DP_AUX_I2C_REPLY_NACK:
> -                       DRM_DEBUG_KMS("aux_i2c nack\n");
> -                       ret = -EREMOTEIO;
> -                       goto out;
> -               case DP_AUX_I2C_REPLY_DEFER:
> -                       DRM_DEBUG_KMS("aux_i2c defer\n");
> -                       udelay(100);
> -                       break;
> -               default:
> -                       DRM_ERROR("aux_i2c invalid reply 0x%02x\n", reply[0]);
> -                       ret = -EREMOTEIO;
> -                       goto out;
> -               }
> +       ret = sysfs_create_link(&connector->base.kdev->kobj,
> +                               &intel_dp->aux.ddc.dev.kobj,
> +                               intel_dp->aux.ddc.dev.kobj.name);
> +       if (ret < 0) {
> +               DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, ret);
> +               drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
>         }
> -
> -       DRM_ERROR("too many retries, giving up\n");
> -       ret = -EREMOTEIO;
> -
> -out:
> -       return ret;
>  }
>
>  static void
> @@ -796,43 +701,10 @@ intel_dp_connector_unregister(struct intel_connector *intel_connector)
>         struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base);
>
>         sysfs_remove_link(&intel_connector->base.kdev->kobj,
> -                         intel_dp->adapter.dev.kobj.name);
> +                         intel_dp->aux.ddc.dev.kobj.name);
>         intel_connector_unregister(intel_connector);
>  }
>
> -static int
> -intel_dp_i2c_init(struct intel_dp *intel_dp,
> -                 struct intel_connector *intel_connector, const char *name)
> -{
> -       int     ret;
> -
> -       DRM_DEBUG_KMS("i2c_init %s\n", name);
> -       intel_dp->algo.running = false;
> -       intel_dp->algo.address = 0;
> -       intel_dp->algo.aux_ch = intel_dp_i2c_aux_ch;
> -
> -       memset(&intel_dp->adapter, '\0', sizeof(intel_dp->adapter));
> -       intel_dp->adapter.owner = THIS_MODULE;
> -       intel_dp->adapter.class = I2C_CLASS_DDC;
> -       strncpy(intel_dp->adapter.name, name, sizeof(intel_dp->adapter.name) - 1);
> -       intel_dp->adapter.name[sizeof(intel_dp->adapter.name) - 1] = '\0';
> -       intel_dp->adapter.algo_data = &intel_dp->algo;
> -       intel_dp->adapter.dev.parent = intel_connector->base.dev->dev;
> -
> -       ret = i2c_dp_aux_add_bus(&intel_dp->adapter);
> -       if (ret < 0)
> -               return ret;
> -
> -       ret = sysfs_create_link(&intel_connector->base.kdev->kobj,
> -                               &intel_dp->adapter.dev.kobj,
> -                               intel_dp->adapter.dev.kobj.name);
> -
> -       if (ret < 0)
> -               i2c_del_adapter(&intel_dp->adapter);
> -
> -       return ret;
> -}
> -
>  static void
>  intel_dp_set_clock(struct intel_encoder *encoder,
>                    struct intel_crtc_config *pipe_config, int link_bw)
> @@ -3098,7 +2970,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>         }
>
>         /* If no HPD, poke DDC gently */
> -       if (drm_probe_ddc(&intel_dp->adapter))
> +       if (drm_probe_ddc(&intel_dp->aux.ddc))
>                 return connector_status_connected;
>
>         /* Well we tried, say unknown for unreliable port types */
> @@ -3266,7 +3138,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>         if (intel_dp->force_audio != HDMI_AUDIO_AUTO) {
>                 intel_dp->has_audio = (intel_dp->force_audio == HDMI_AUDIO_ON);
>         } else {
> -               edid = intel_dp_get_edid(connector, &intel_dp->adapter);
> +               edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
>                 if (edid) {
>                         intel_dp->has_audio = drm_detect_monitor_audio(edid);
>                         kfree(edid);
> @@ -3302,7 +3174,7 @@ static int intel_dp_get_modes(struct drm_connector *connector)
>         power_domain = intel_display_port_power_domain(intel_encoder);
>         intel_display_power_get(dev_priv, power_domain);
>
> -       ret = intel_dp_get_edid_modes(connector, &intel_dp->adapter);
> +       ret = intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc);
>         intel_display_power_put(dev_priv, power_domain);
>         if (ret)
>                 return ret;
> @@ -3335,7 +3207,7 @@ intel_dp_detect_audio(struct drm_connector *connector)
>         power_domain = intel_display_port_power_domain(intel_encoder);
>         intel_display_power_get(dev_priv, power_domain);
>
> -       edid = intel_dp_get_edid(connector, &intel_dp->adapter);
> +       edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
>         if (edid) {
>                 has_audio = drm_detect_monitor_audio(edid);
>                 kfree(edid);
> @@ -3457,7 +3329,7 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>         struct intel_dp *intel_dp = &intel_dig_port->dp;
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>
> -       i2c_del_adapter(&intel_dp->adapter);
> +       drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
>         drm_encoder_cleanup(encoder);
>         if (is_edp(intel_dp)) {
>                 cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> @@ -3769,7 +3641,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>         /* We now know it's not a ghost, init power sequence regs. */
>         intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
>
> -       edid = drm_get_edid(connector, &intel_dp->adapter);
> +       edid = drm_get_edid(connector, &intel_dp->aux.ddc);
>         if (edid) {
>                 if (drm_add_edid_modes(connector, edid)) {
>                         drm_mode_connector_update_edid_property(connector,
> @@ -3817,8 +3689,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         enum port port = intel_dig_port->port;
>         struct edp_power_seq power_seq = { 0 };
> -       const char *name = NULL;
> -       int type, error;
> +       int type;
>
>         /* intel_dp vfuncs */
>         if (IS_VALLEYVIEW(dev))
> @@ -3871,23 +3742,19 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>                 intel_connector->get_hw_state = intel_connector_get_hw_state;
>         intel_connector->unregister = intel_dp_connector_unregister;
>
> -       /* Set up the DDC bus. */
> +       /* Set up the hotplug pin. */
>         switch (port) {
>         case PORT_A:
>                 intel_encoder->hpd_pin = HPD_PORT_A;
> -               name = "DPDDC-A";
>                 break;
>         case PORT_B:
>                 intel_encoder->hpd_pin = HPD_PORT_B;
> -               name = "DPDDC-B";
>                 break;
>         case PORT_C:
>                 intel_encoder->hpd_pin = HPD_PORT_C;
> -               name = "DPDDC-C";
>                 break;
>         case PORT_D:
>                 intel_encoder->hpd_pin = HPD_PORT_D;
> -               name = "DPDDC-D";
>                 break;
>         default:
>                 BUG();
> @@ -3900,14 +3767,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>
>         intel_dp_aux_init(intel_dp, intel_connector);
>
> -       error = intel_dp_i2c_init(intel_dp, intel_connector, name);
> -       WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
> -            error, port_name(port));
> -
>         intel_dp->psr_setup_done = false;
>
>         if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
> -               i2c_del_adapter(&intel_dp->adapter);
> +               drm_dp_aux_unregister_i2c_bus(&intel_dp->aux);
>                 if (is_edp(intel_dp)) {
>                         cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
>                         mutex_lock(&dev->mode_config.mutex);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 561af4b013b1..604d57e5e4c8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -489,8 +489,6 @@ struct intel_dp {
>         uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
>         uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
>         uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
> -       struct i2c_adapter adapter;
> -       struct i2c_algo_dp_aux_data algo;

is this algo really not needed/used anymore? If this is true patch
seems correct so feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

>         struct drm_dp_aux aux;
>         uint8_t train_set[4];
>         int panel_power_up_delay;
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br