[v5,3/4] drm/dp_helper: Perform throw-away read before actual read in drm_dp_dpcd_read()

Submitted by cpaul@redhat.com on April 13, 2016, 2:58 p.m.

Details

Message ID 1460559513-32280-4-git-send-email-cpaul@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

cpaul@redhat.com April 13, 2016, 2:58 p.m.
This is part of a patch series to migrate all of the workarounds for
commonly seen behavior from bad sinks in intel_dp_dpcd_read_wake() to drm's
DP helper.

Some sinks will just return garbage for the first aux tranaction they
receive when coming out of sleep mode, so we need to perform an additional
read before the actual read to workaround this.

			    Changes since v5
- If the throwaway read in drm_dp_dpcd_read() fails, return the error
  from that instead of continuing. This follows the same logic we do in
  drm_dp_dpcd_access() (e.g. the error from the first transaction may
  differ from the errors that proceeding attempts might return).

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 540c3e4..aa80702 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -248,6 +248,18 @@  unlock:
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 			 void *buffer, size_t size)
 {
+	int ret;
+
+	/*
+	 * Sometimes we just get the same incorrect byte repeated over the
+	 * entire buffer. Doing one throw away read initially seems to "solve"
+	 * it.
+	 */
+	ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, DP_DPCD_REV, buffer,
+				 1);
+	if (ret != 1)
+		return ret;
+
 	return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
 				  size);
 }

Comments

On Wed, Apr 13, 2016 at 10:58:32AM -0400, Lyude wrote:
> This is part of a patch series to migrate all of the workarounds for
> commonly seen behavior from bad sinks in intel_dp_dpcd_read_wake() to drm's
> DP helper.
> 
> Some sinks will just return garbage for the first aux tranaction they
> receive when coming out of sleep mode, so we need to perform an additional
> read before the actual read to workaround this.
> 
> 			    Changes since v5
> - If the throwaway read in drm_dp_dpcd_read() fails, return the error
>   from that instead of continuing. This follows the same logic we do in
>   drm_dp_dpcd_access() (e.g. the error from the first transaction may
>   differ from the errors that proceeding attempts might return).
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 540c3e4..aa80702 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -248,6 +248,18 @@ unlock:
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>  			 void *buffer, size_t size)
>  {
> +	int ret;
> +
> +	/*
> +	 * Sometimes we just get the same incorrect byte repeated over the
> +	 * entire buffer. Doing one throw away read initially seems to "solve"
> +	 * it.
> +	 */

We might want to improve the comment a bit, now that I analyzed the
problem in more detail. So perhaps something like:

/*
 * HP ZR24w corrupts the first DPCD access after entering power save
 * mode. Eg. on a read, the entire buffer will be filled with the same
 * byte. Do a throw away read to avoid corrupting anything we care
 * about. Afterwards things will work correctly until the monitor
 * gets woken up and subsequently re-enters power save mode.
 *
 * The user pressing any button on the monitor is enough to wake it
 * up, so there is no particularly good place to do the workaround.
 * We just have to do it before any DPCD access and hope that the
 * monitor doesn't power down exactly after the throw away read.
 */

I'm thinking we should probably also do it for writes, since those
seem to be affected as well.

> +	ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, DP_DPCD_REV, buffer,
> +				 1);
> +	if (ret != 1)
> +		return ret;
> +
>  	return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
>  				  size);
>  }
> -- 
> 2.5.5
On Fri, Apr 15, 2016 at 10:25:35AM -0400, Lyude wrote:
> This is part of a patch series to migrate all of the workarounds for
> commonly seen behavior from bad sinks in intel_dp_dpcd_read_wake() to drm's
> DP helper.
> 
> Some sinks will just return garbage for the first aux tranaction they
> receive when coming out of sleep mode, so we need to perform an additional
> read before the actual read to workaround this.
> 
> 			    Changes since v5
> - If the throwaway read in drm_dp_dpcd_read() fails, return the error
>   from that instead of continuing. This follows the same logic we do in
>   drm_dp_dpcd_access() (e.g. the error from the first transaction may
>   differ from the errors that proceeding attempts might return).
> 
> Signed-off-by: Lyude <cpaul@redhat.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_dp_helper.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 540c3e4..eeaf5a7 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -248,6 +248,25 @@ unlock:
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>  			 void *buffer, size_t size)
>  {
> +	int ret;
> +
> +	/*
> +	 * HP ZR24w corrupts the first DPCD access after entering power save
> +	 * mode. Eg. on a read, the entire buffer will be filled with the same
> +	 * byte. Do a throw away read to avoid corrupting anything we care
> +	 * about. Afterwards things will work correctly until the monitor
> +	 * gets woken up and subsequently re-enters power save mode.
> +	 *
> +	 * The user pressing any button on the monitor is enough to wake it
> +	 * up, so there is no particularly good place to do the workaround.
> +	 * We just have to do it before any DPCD access and hope that the
> +	 * monitor doesn't power down exactly after the throw away read.
> +	 */
> +	ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, DP_DPCD_REV, buffer,
> +				 1);
> +	if (ret != 1)
> +		return ret;
> +
>  	return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
>  				  size);
>  }
> -- 
> 2.5.5