[7/7] drm/i915: Dont -ETIMEDOUT on identical new and previous (count, crc).
diff mbox

Message ID 1437694550-5667-6-git-send-email-rodrigo.vivi@intel.com
State New
Headers show

Commit Message

Rodrigo Vivi July 23, 2015, 11:35 p.m. UTC
By Vesa DP 1.2 spec TEST_CRC_COUNT is a "4 bit wrap counter which
increments each time the TEST_CRC_x_x are updated."

However if we are trying to verify the screen hasn't changed we get
same (count, crc) pair twice. Without this patch we would return
-ETIMEOUT in this case.

So, if in 6 vblanks the pair (count, crc) hasn't changed we
return it anyway instead of returning error and let test case decide
if it was right or not.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Comments

Rafael Antognolli July 28, 2015, 8:25 p.m. UTC | #1
On Thu, Jul 23, 2015 at 04:35:50PM -0700, Rodrigo Vivi wrote:
> By Vesa DP 1.2 spec TEST_CRC_COUNT is a "4 bit wrap counter which
> increments each time the TEST_CRC_x_x are updated."
> 
> However if we are trying to verify the screen hasn't changed we get
> same (count, crc) pair twice. Without this patch we would return
> -ETIMEOUT in this case.
> 
> So, if in 6 vblanks the pair (count, crc) hasn't changed we
> return it anyway instead of returning error and let test case decide
> if it was right or not.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Looks good.

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c7372a1..e99ec7a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4028,6 +4028,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	u8 buf;
>  	int count, ret;
>  	int attempts = 6;
> +	bool old_equal_new;
>  
>  	ret = intel_dp_sink_crc_start(intel_dp);
>  	if (ret)
> @@ -4042,6 +4043,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  			goto stop;
>  		}
>  		count = buf & DP_TEST_COUNT_MASK;
> +
>  		/*
>  		 * Count might be reset during the loop. In this case
>  		 * last known count needs to be reset as well.
> @@ -4053,17 +4055,24 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  			ret = -EIO;
>  			goto stop;
>  		}
> -	} while (--attempts && (count == 0 || (count == intel_dp->sink_crc.last_count &&
> -					       !memcmp(intel_dp->sink_crc.last_crc, crc,
> -						       6 * sizeof(u8)))));
> +
> +		old_equal_new = (count == intel_dp->sink_crc.last_count &&
> +				 !memcmp(intel_dp->sink_crc.last_crc, crc,
> +					 6 * sizeof(u8)));
> +
> +	} while (--attempts && (count == 0 || old_equal_new));
>  
>  	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
>  	memcpy(intel_dp->sink_crc.last_crc, crc, 6 * sizeof(u8));
>  
>  	if (attempts == 0) {
> -		DRM_DEBUG_KMS("Panel is unable to calculate CRC after 6 vblanks\n");
> -		ret = -ETIMEDOUT;
> -		goto stop;
> +		if (old_equal_new) {
> +			DRM_DEBUG_KMS("Unreliable Sink CRC counter: Current returned CRC is identical to the previous one\n");

Isn't this line a little too long?

> +		} else {
> +			DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
> +			ret = -ETIMEDOUT;
> +			goto stop;
> +		}
>  	}
>  
>  stop:
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Rodrigo Vivi July 28, 2015, 10:05 p.m. UTC | #2
On Tue, 2015-07-28 at 13:25 -0700, Rafael Antognolli wrote:
> On Thu, Jul 23, 2015 at 04:35:50PM -0700, Rodrigo Vivi wrote:

> > By Vesa DP 1.2 spec TEST_CRC_COUNT is a "4 bit wrap counter which

> > increments each time the TEST_CRC_x_x are updated."

> > 

> > However if we are trying to verify the screen hasn't changed we get

> > same (count, crc) pair twice. Without this patch we would return

> > -ETIMEOUT in this case.

> > 

> > So, if in 6 vblanks the pair (count, crc) hasn't changed we

> > return it anyway instead of returning error and let test case decide

> > if it was right or not.

> > 

> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 

> Looks good.

> 

> > ---

> >  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++------

> >  1 file changed, 15 insertions(+), 6 deletions(-)

> > 

> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c

> > index c7372a1..e99ec7a 100644

> > --- a/drivers/gpu/drm/i915/intel_dp.c

> > +++ b/drivers/gpu/drm/i915/intel_dp.c

> > @@ -4028,6 +4028,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)

> >  	u8 buf;

> >  	int count, ret;

> >  	int attempts = 6;

> > +	bool old_equal_new;

> >  

> >  	ret = intel_dp_sink_crc_start(intel_dp);

> >  	if (ret)

> > @@ -4042,6 +4043,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)

> >  			goto stop;

> >  		}

> >  		count = buf & DP_TEST_COUNT_MASK;

> > +

> >  		/*

> >  		 * Count might be reset during the loop. In this case

> >  		 * last known count needs to be reset as well.

> > @@ -4053,17 +4055,24 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)

> >  			ret = -EIO;

> >  			goto stop;

> >  		}

> > -	} while (--attempts && (count == 0 || (count == intel_dp->sink_crc.last_count &&

> > -					       !memcmp(intel_dp->sink_crc.last_crc, crc,

> > -						       6 * sizeof(u8)))));

> > +

> > +		old_equal_new = (count == intel_dp->sink_crc.last_count &&

> > +				 !memcmp(intel_dp->sink_crc.last_crc, crc,

> > +					 6 * sizeof(u8)));

> > +

> > +	} while (--attempts && (count == 0 || old_equal_new));

> >  

> >  	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;

> >  	memcpy(intel_dp->sink_crc.last_crc, crc, 6 * sizeof(u8));

> >  

> >  	if (attempts == 0) {

> > -		DRM_DEBUG_KMS("Panel is unable to calculate CRC after 6 vblanks\n");

> > -		ret = -ETIMEDOUT;

> > -		goto stop;

> > +		if (old_equal_new) {

> > +			DRM_DEBUG_KMS("Unreliable Sink CRC counter: Current returned CRC is identical to the previous one\n");

> 

> Isn't this line a little too long?


I agree, but I had no idea how to make it shorter. I believe this long
debug message is the only case where we can go over 80 characters in
i915. but if it isn't true and/or have a suggestion how to make it
shorter please let me know that I can change.

> 

> > +		} else {

> > +			DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");

> > +			ret = -ETIMEDOUT;

> > +			goto stop;

> > +		}

> >  	}

> >  

> >  stop:

> > -- 

> > 2.1.0

> > 

> > _______________________________________________

> > Intel-gfx mailing list

> > Intel-gfx@lists.freedesktop.org

> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter July 29, 2015, 8:26 a.m. UTC | #3
On Tue, Jul 28, 2015 at 10:05:21PM +0000, Vivi, Rodrigo wrote:
> On Tue, 2015-07-28 at 13:25 -0700, Rafael Antognolli wrote:
> > On Thu, Jul 23, 2015 at 04:35:50PM -0700, Rodrigo Vivi wrote:
> > > By Vesa DP 1.2 spec TEST_CRC_COUNT is a "4 bit wrap counter which
> > > increments each time the TEST_CRC_x_x are updated."
> > > 
> > > However if we are trying to verify the screen hasn't changed we get
> > > same (count, crc) pair twice. Without this patch we would return
> > > -ETIMEOUT in this case.
> > > 
> > > So, if in 6 vblanks the pair (count, crc) hasn't changed we
> > > return it anyway instead of returning error and let test case decide
> > > if it was right or not.
> > > 
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > Looks good.
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++------
> > >  1 file changed, 15 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index c7372a1..e99ec7a 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4028,6 +4028,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > >  	u8 buf;
> > >  	int count, ret;
> > >  	int attempts = 6;
> > > +	bool old_equal_new;
> > >  
> > >  	ret = intel_dp_sink_crc_start(intel_dp);
> > >  	if (ret)
> > > @@ -4042,6 +4043,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > >  			goto stop;
> > >  		}
> > >  		count = buf & DP_TEST_COUNT_MASK;
> > > +
> > >  		/*
> > >  		 * Count might be reset during the loop. In this case
> > >  		 * last known count needs to be reset as well.
> > > @@ -4053,17 +4055,24 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > >  			ret = -EIO;
> > >  			goto stop;
> > >  		}
> > > -	} while (--attempts && (count == 0 || (count == intel_dp->sink_crc.last_count &&
> > > -					       !memcmp(intel_dp->sink_crc.last_crc, crc,
> > > -						       6 * sizeof(u8)))));
> > > +
> > > +		old_equal_new = (count == intel_dp->sink_crc.last_count &&
> > > +				 !memcmp(intel_dp->sink_crc.last_crc, crc,
> > > +					 6 * sizeof(u8)));
> > > +
> > > +	} while (--attempts && (count == 0 || old_equal_new));
> > >  
> > >  	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
> > >  	memcpy(intel_dp->sink_crc.last_crc, crc, 6 * sizeof(u8));
> > >  
> > >  	if (attempts == 0) {
> > > -		DRM_DEBUG_KMS("Panel is unable to calculate CRC after 6 vblanks\n");
> > > -		ret = -ETIMEDOUT;
> > > -		goto stop;
> > > +		if (old_equal_new) {
> > > +			DRM_DEBUG_KMS("Unreliable Sink CRC counter: Current returned CRC is identical to the previous one\n");
> > 
> > Isn't this line a little too long?
> 
> I agree, but I had no idea how to make it shorter. I believe this long
> debug message is the only case where we can go over 80 characters in
> i915. but if it isn't true and/or have a suggestion how to make it
> shorter please let me know that I can change.

dmesg output is explicitly an exception since breaking lines makes it much
harder to grep for a line you spot in dmesg. Ofc 500 lines would be a bit
too much, we're breaking those. But this one here is totally fine.

Remember, checkpatch is just suggestions mostly, not law.
-Daniel

> 
> > 
> > > +		} else {
> > > +			DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
> > > +			ret = -ETIMEDOUT;
> > > +			goto stop;
> > > +		}
> > >  	}
> > >  
> > >  stop:
> > > -- 
> > > 2.1.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Rafael Antognolli July 29, 2015, 4:05 p.m. UTC | #4
On Wed, Jul 29, 2015 at 10:26:53AM +0200, Daniel Vetter wrote:
> On Tue, Jul 28, 2015 at 10:05:21PM +0000, Vivi, Rodrigo wrote:
> > On Tue, 2015-07-28 at 13:25 -0700, Rafael Antognolli wrote:
> > > On Thu, Jul 23, 2015 at 04:35:50PM -0700, Rodrigo Vivi wrote:
> > > > By Vesa DP 1.2 spec TEST_CRC_COUNT is a "4 bit wrap counter which
> > > > increments each time the TEST_CRC_x_x are updated."
> > > > 
> > > > However if we are trying to verify the screen hasn't changed we get
> > > > same (count, crc) pair twice. Without this patch we would return
> > > > -ETIMEOUT in this case.
> > > > 
> > > > So, if in 6 vblanks the pair (count, crc) hasn't changed we
> > > > return it anyway instead of returning error and let test case decide
> > > > if it was right or not.
> > > > 
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > Looks good.
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++++------
> > > >  1 file changed, 15 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index c7372a1..e99ec7a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4028,6 +4028,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > > >  	u8 buf;
> > > >  	int count, ret;
> > > >  	int attempts = 6;
> > > > +	bool old_equal_new;
> > > >  
> > > >  	ret = intel_dp_sink_crc_start(intel_dp);
> > > >  	if (ret)
> > > > @@ -4042,6 +4043,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > > >  			goto stop;
> > > >  		}
> > > >  		count = buf & DP_TEST_COUNT_MASK;
> > > > +
> > > >  		/*
> > > >  		 * Count might be reset during the loop. In this case
> > > >  		 * last known count needs to be reset as well.
> > > > @@ -4053,17 +4055,24 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > > >  			ret = -EIO;
> > > >  			goto stop;
> > > >  		}
> > > > -	} while (--attempts && (count == 0 || (count == intel_dp->sink_crc.last_count &&
> > > > -					       !memcmp(intel_dp->sink_crc.last_crc, crc,
> > > > -						       6 * sizeof(u8)))));
> > > > +
> > > > +		old_equal_new = (count == intel_dp->sink_crc.last_count &&
> > > > +				 !memcmp(intel_dp->sink_crc.last_crc, crc,
> > > > +					 6 * sizeof(u8)));
> > > > +
> > > > +	} while (--attempts && (count == 0 || old_equal_new));
> > > >  
> > > >  	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
> > > >  	memcpy(intel_dp->sink_crc.last_crc, crc, 6 * sizeof(u8));
> > > >  
> > > >  	if (attempts == 0) {
> > > > -		DRM_DEBUG_KMS("Panel is unable to calculate CRC after 6 vblanks\n");
> > > > -		ret = -ETIMEDOUT;
> > > > -		goto stop;
> > > > +		if (old_equal_new) {
> > > > +			DRM_DEBUG_KMS("Unreliable Sink CRC counter: Current returned CRC is identical to the previous one\n");
> > > 
> > > Isn't this line a little too long?
> > 
> > I agree, but I had no idea how to make it shorter. I believe this long
> > debug message is the only case where we can go over 80 characters in
> > i915. but if it isn't true and/or have a suggestion how to make it
> > shorter please let me know that I can change.
> 
> dmesg output is explicitly an exception since breaking lines makes it much
> harder to grep for a line you spot in dmesg. Ofc 500 lines would be a bit
> too much, we're breaking those. But this one here is totally fine.

Nice, I never thought about being able to grep, but makes total sense.

> Remember, checkpatch is just suggestions mostly, not law.

I wasn't aware of it, but good to know that it exists. I'll check it out.

Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>

> > 
> > > 
> > > > +		} else {
> > > > +			DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
> > > > +			ret = -ETIMEDOUT;
> > > > +			goto stop;
> > > > +		}
> > > >  	}
> > > >  
> > > >  stop:
> > > > -- 
> > > > 2.1.0
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c7372a1..e99ec7a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4028,6 +4028,7 @@  int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	u8 buf;
 	int count, ret;
 	int attempts = 6;
+	bool old_equal_new;
 
 	ret = intel_dp_sink_crc_start(intel_dp);
 	if (ret)
@@ -4042,6 +4043,7 @@  int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 			goto stop;
 		}
 		count = buf & DP_TEST_COUNT_MASK;
+
 		/*
 		 * Count might be reset during the loop. In this case
 		 * last known count needs to be reset as well.
@@ -4053,17 +4055,24 @@  int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 			ret = -EIO;
 			goto stop;
 		}
-	} while (--attempts && (count == 0 || (count == intel_dp->sink_crc.last_count &&
-					       !memcmp(intel_dp->sink_crc.last_crc, crc,
-						       6 * sizeof(u8)))));
+
+		old_equal_new = (count == intel_dp->sink_crc.last_count &&
+				 !memcmp(intel_dp->sink_crc.last_crc, crc,
+					 6 * sizeof(u8)));
+
+	} while (--attempts && (count == 0 || old_equal_new));
 
 	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
 	memcpy(intel_dp->sink_crc.last_crc, crc, 6 * sizeof(u8));
 
 	if (attempts == 0) {
-		DRM_DEBUG_KMS("Panel is unable to calculate CRC after 6 vblanks\n");
-		ret = -ETIMEDOUT;
-		goto stop;
+		if (old_equal_new) {
+			DRM_DEBUG_KMS("Unreliable Sink CRC counter: Current returned CRC is identical to the previous one\n");
+		} else {
+			DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
+			ret = -ETIMEDOUT;
+			goto stop;
+		}
 	}
 
 stop: