[PATCH 2/7] drm/i915: Try to stop sink crc calculation on error.

[PATCH 2/7] drm/i915: Try to stop sink crc calculation on error.

[Rodrigo Vivi]

Right now if we face any kind of error sink crc calculation
stays enabled.

So, let's give a shot and try to stop it anyway if it got enabled.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f1b9f93..70a4a37 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3994,7 +3994,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
 		ret = -EIO;
-		goto out;
+		goto stop;
 	}
 
 	test_crc_count = buf & DP_TEST_COUNT_MASK;
@@ -4003,7 +4003,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		if (drm_dp_dpcd_readb(&intel_dp->aux,
 				      DP_TEST_SINK_MISC, &buf) < 0) {
 			ret = -EIO;
-			goto out;
+			goto stop;
 		}
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 	} while (--attempts && (buf & DP_TEST_COUNT_MASK) == test_crc_count);
@@ -4011,14 +4011,15 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	if (attempts == 0) {
 		DRM_DEBUG_KMS("Panel is unable to calculate CRC after 6 vblanks\n");
 		ret = -ETIMEDOUT;
-		goto out;
+		goto stop;
 	}
 
 	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
 		ret = -EIO;
-		goto out;
+		goto stop;
 	}
 
+stop:
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
 		ret = -EIO;
 		goto out;
-- 
2.1.0

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

[PATCH 3/7] drm/i915: Don't return error on sink crc stop.

[Rodrigo Vivi]

If we got to the point where we are trying to stop sink CRC
the main output of this function was already gotten properly,
so don't return the error and let userspace use the crc data.

Let's replace the errnos returns with some log messages.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 70a4a37..44f8a32 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4021,12 +4021,12 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 
 stop:
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
-		ret = -EIO;
+		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
 		goto out;
 	}
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
 			       buf & ~DP_TEST_SINK_START) < 0) {
-		ret = -EIO;
+		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
 		goto out;
 	}
 out:
-- 
2.1.0

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

Re: [PATCH 3/7] drm/i915: Don't return error on sink crc stop.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:46PM -0700, Rodrigo Vivi wrote:
> If we got to the point where we are trying to stop sink CRC
> the main output of this function was already gotten properly,
> so don't return the error and let userspace use the crc data.
> 
> Let's replace the errnos returns with some log messages.

So, up to this commit, there's no way to know that an error has ocurred
and the next CRC calculation can go wrong.

I know that in a follow up patch this is fixed by trying to stop the
calculation at the beginning too, but just pointing out that this one
specifically has this problem.

Not sure if this is a problem though, since the patches are submitted
together. If not, then

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


> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 70a4a37..44f8a32 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4021,12 +4021,12 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  
>  stop:
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> -		ret = -EIO;
> +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
>  		goto out;
>  	}
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>  			       buf & ~DP_TEST_SINK_START) < 0) {
> -		ret = -EIO;
> +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
>  		goto out;
>  	}
>  out:
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/7] drm/i915: Don't return error on sink crc stop.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 2710 bytes --]

On Fri, Jul 24, 2015 at 11:25 AM Rafael Antognolli <
rafael.antognolli@intel.com> wrote:

> On Thu, Jul 23, 2015 at 04:35:46PM -0700, Rodrigo Vivi wrote:
> > If we got to the point where we are trying to stop sink CRC
> > the main output of this function was already gotten properly,
> > so don't return the error and let userspace use the crc data.
> >
> > Let's replace the errnos returns with some log messages.
>
> So, up to this commit, there's no way to know that an error has ocurred
> and the next CRC calculation can go wrong.
>
> I know that in a follow up patch this is fixed by trying to stop the
> calculation at the beginning too, but just pointing out that this one
> specifically has this problem.
>

Actually it isn't a problem if crc calculation continue enabled. Mainly
with the mask s/0x7/0xf fix.
But it is good to disable and force counter reset anyway. (the following
patch you mentioned)

This patch here is just to highlight that errors during read has more
priority than errors on stopping crc.

Another way would be to create a stop_ret and
if (!ret and stop_ret)
     ret = stop_ret;

But I decided this cleaned version would be better.


>
> Not sure if this is a problem though, since the patches are submitted
> together. If not, then
>
> Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>
>

Thanks!


>
>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> > index 70a4a37..44f8a32 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4021,12 +4021,12 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp,
> u8 *crc)
> >
> >  stop:
> >       if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > -             ret = -EIO;
> > +             DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> >               goto out;
> >       }
> >       if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> >                              buf & ~DP_TEST_SINK_START) < 0) {
> > -             ret = -EIO;
> > +             DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> >               goto out;
> >       }
> >  out:
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Rafael
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 4280 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 4/7] drm/i915: Split sink_crc function in start, stop and read.

[Rodrigo Vivi]

No functional change. Just a preparation patch to make clear
what operation we are performing.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 44f8a32..10cbc98 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3958,40 +3958,64 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
 	return intel_dp->is_mst;
 }
 
-int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
+static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 {
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
-	struct intel_crtc *intel_crtc =
-		to_intel_crtc(intel_dig_port->base.base.crtc);
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
 	u8 buf;
-	int test_crc_count;
-	int attempts = 6;
-	int ret = 0;
-
-	hsw_disable_ips(intel_crtc);
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
-		ret = -EIO;
-		goto out;
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
+		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
+		return;
 	}
 
-	if (!(buf & DP_TEST_CRC_SUPPORTED)) {
-		ret = -ENOTTY;
-		goto out;
-	}
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
+			       buf & ~DP_TEST_SINK_START) < 0)
+		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
-		ret = -EIO;
-		goto out;
-	}
+	hsw_enable_ips(intel_crtc);
+}
+
+static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
+	u8 buf;
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
+		return -EIO;
+
+	if (!(buf & DP_TEST_CRC_SUPPORTED))
+		return -ENOTTY;
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
+		return -EIO;
+
+	hsw_disable_ips(intel_crtc);
 
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-				buf | DP_TEST_SINK_START) < 0) {
-		ret = -EIO;
-		goto out;
+			       buf | DP_TEST_SINK_START) < 0) {
+		hsw_enable_ips(intel_crtc);
+		return -EIO;
 	}
 
+	return 0;
+}
+
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
+	u8 buf;
+	int test_crc_count;
+	int attempts = 6;
+	int ret;
+
+	ret = intel_dp_sink_crc_start(intel_dp);
+	if (ret)
+		return ret;
+
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
 		ret = -EIO;
 		goto stop;
@@ -4014,23 +4038,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		goto stop;
 	}
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
 		ret = -EIO;
-		goto stop;
-	}
-
 stop:
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
-		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
-		goto out;
-	}
-	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-			       buf & ~DP_TEST_SINK_START) < 0) {
-		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
-		goto out;
-	}
-out:
-	hsw_enable_ips(intel_crtc);
+	intel_dp_sink_crc_stop(intel_dp);
 	return ret;
 }
 
-- 
2.1.0

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

Re: [PATCH 4/7] drm/i915: Split sink_crc function in start, stop and read.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:47PM -0700, Rodrigo Vivi wrote:
> No functional change. Just a preparation patch to make clear
> what operation we are performing.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Good. The place where you call hsw_disable_ips() changes, but as you
explained earlier, this is required.

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 89 +++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 44f8a32..10cbc98 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3958,40 +3958,64 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
>  	return intel_dp->is_mst;
>  }
>  
> -int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> +static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  {
> -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct drm_device *dev = intel_dig_port->base.base.dev;
> -	struct intel_crtc *intel_crtc =
> -		to_intel_crtc(intel_dig_port->base.base.crtc);
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>  	u8 buf;
> -	int test_crc_count;
> -	int attempts = 6;
> -	int ret = 0;
> -
> -	hsw_disable_ips(intel_crtc);
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
> -		ret = -EIO;
> -		goto out;
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> +		return;
>  	}
>  
> -	if (!(buf & DP_TEST_CRC_SUPPORTED)) {
> -		ret = -ENOTTY;
> -		goto out;
> -	}
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> +			       buf & ~DP_TEST_SINK_START) < 0)
> +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> -		ret = -EIO;
> -		goto out;
> -	}
> +	hsw_enable_ips(intel_crtc);
> +}
> +
> +static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> +	u8 buf;
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
> +		return -EIO;
> +
> +	if (!(buf & DP_TEST_CRC_SUPPORTED))
> +		return -ENOTTY;
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
> +		return -EIO;
> +
> +	hsw_disable_ips(intel_crtc);
>  
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> -				buf | DP_TEST_SINK_START) < 0) {
> -		ret = -EIO;
> -		goto out;
> +			       buf | DP_TEST_SINK_START) < 0) {
> +		hsw_enable_ips(intel_crtc);
> +		return -EIO;
>  	}
>  
> +	return 0;
> +}
> +
> +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = dig_port->base.base.dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> +	u8 buf;
> +	int test_crc_count;
> +	int attempts = 6;
> +	int ret;
> +
> +	ret = intel_dp_sink_crc_start(intel_dp);
> +	if (ret)
> +		return ret;
> +
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
>  		ret = -EIO;
>  		goto stop;
> @@ -4014,23 +4038,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  		goto stop;
>  	}
>  
> -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>  		ret = -EIO;
> -		goto stop;
> -	}
> -
>  stop:
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> -		goto out;
> -	}
> -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> -			       buf & ~DP_TEST_SINK_START) < 0) {
> -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> -		goto out;
> -	}
> -out:
> -	hsw_enable_ips(intel_crtc);
> +	intel_dp_sink_crc_stop(intel_dp);
>  	return ret;
>  }
>  
> -- 
> 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

Re: [PATCH 4/7] drm/i915: Split sink_crc function in start, stop and read.

[Daniel Vetter]

On Tue, Jul 28, 2015 at 01:10:38PM -0700, Rafael Antognolli wrote:
> On Thu, Jul 23, 2015 at 04:35:47PM -0700, Rodrigo Vivi wrote:
> > No functional change. Just a preparation patch to make clear
> > what operation we are performing.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Good. The place where you call hsw_disable_ips() changes, but as you
> explained earlier, this is required.

That explanation should be in the commit message. Merged the two earlier
patches in the series meanwhile.
-Daniel

> 
> Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 89 +++++++++++++++++++++++------------------
> >  1 file changed, 50 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 44f8a32..10cbc98 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3958,40 +3958,64 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
> >  	return intel_dp->is_mst;
> >  }
> >  
> > -int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > +static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >  {
> > -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > -	struct drm_device *dev = intel_dig_port->base.base.dev;
> > -	struct intel_crtc *intel_crtc =
> > -		to_intel_crtc(intel_dig_port->base.base.crtc);
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> >  	u8 buf;
> > -	int test_crc_count;
> > -	int attempts = 6;
> > -	int ret = 0;
> > -
> > -	hsw_disable_ips(intel_crtc);
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
> > -		ret = -EIO;
> > -		goto out;
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > +		return;
> >  	}
> >  
> > -	if (!(buf & DP_TEST_CRC_SUPPORTED)) {
> > -		ret = -ENOTTY;
> > -		goto out;
> > -	}
> > +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > +			       buf & ~DP_TEST_SINK_START) < 0)
> > +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > -		ret = -EIO;
> > -		goto out;
> > -	}
> > +	hsw_enable_ips(intel_crtc);
> > +}
> > +
> > +static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> > +	u8 buf;
> > +
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
> > +		return -EIO;
> > +
> > +	if (!(buf & DP_TEST_CRC_SUPPORTED))
> > +		return -ENOTTY;
> > +
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
> > +		return -EIO;
> > +
> > +	hsw_disable_ips(intel_crtc);
> >  
> >  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > -				buf | DP_TEST_SINK_START) < 0) {
> > -		ret = -EIO;
> > -		goto out;
> > +			       buf | DP_TEST_SINK_START) < 0) {
> > +		hsw_enable_ips(intel_crtc);
> > +		return -EIO;
> >  	}
> >  
> > +	return 0;
> > +}
> > +
> > +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > +{
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_device *dev = dig_port->base.base.dev;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> > +	u8 buf;
> > +	int test_crc_count;
> > +	int attempts = 6;
> > +	int ret;
> > +
> > +	ret = intel_dp_sink_crc_start(intel_dp);
> > +	if (ret)
> > +		return ret;
> > +
> >  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
> >  		ret = -EIO;
> >  		goto stop;
> > @@ -4014,23 +4038,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> >  		goto stop;
> >  	}
> >  
> > -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
> >  		ret = -EIO;
> > -		goto stop;
> > -	}
> > -
> >  stop:
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > -		goto out;
> > -	}
> > -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > -			       buf & ~DP_TEST_SINK_START) < 0) {
> > -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > -		goto out;
> > -	}
> > -out:
> > -	hsw_enable_ips(intel_crtc);
> > +	intel_dp_sink_crc_stop(intel_dp);
> >  	return ret;
> >  }
> >  
> > -- 
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Split sink_crc function in start, stop and read.

[Rodrigo Vivi]

This is just a preparation patch to make clear what operation we
are performing. There is no functional change on the sink crc
logic.

hsw_disable_ips has been moved a bit further in the start function
to avoid disabling ips when sink crc is not going to be started.
and to avoid goto on this function.

v2: explain why hsw_disable_ips() call place has changed.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 89 +++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 44f8a32..10cbc98 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3958,40 +3958,64 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
 	return intel_dp->is_mst;
 }
 
-int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
+static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 {
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = intel_dig_port->base.base.dev;
-	struct intel_crtc *intel_crtc =
-		to_intel_crtc(intel_dig_port->base.base.crtc);
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
 	u8 buf;
-	int test_crc_count;
-	int attempts = 6;
-	int ret = 0;
-
-	hsw_disable_ips(intel_crtc);
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
-		ret = -EIO;
-		goto out;
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
+		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
+		return;
 	}
 
-	if (!(buf & DP_TEST_CRC_SUPPORTED)) {
-		ret = -ENOTTY;
-		goto out;
-	}
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
+			       buf & ~DP_TEST_SINK_START) < 0)
+		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
-		ret = -EIO;
-		goto out;
-	}
+	hsw_enable_ips(intel_crtc);
+}
+
+static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
+	u8 buf;
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
+		return -EIO;
+
+	if (!(buf & DP_TEST_CRC_SUPPORTED))
+		return -ENOTTY;
+
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
+		return -EIO;
+
+	hsw_disable_ips(intel_crtc);
 
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-				buf | DP_TEST_SINK_START) < 0) {
-		ret = -EIO;
-		goto out;
+			       buf | DP_TEST_SINK_START) < 0) {
+		hsw_enable_ips(intel_crtc);
+		return -EIO;
 	}
 
+	return 0;
+}
+
+int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
+	u8 buf;
+	int test_crc_count;
+	int attempts = 6;
+	int ret;
+
+	ret = intel_dp_sink_crc_start(intel_dp);
+	if (ret)
+		return ret;
+
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
 		ret = -EIO;
 		goto stop;
@@ -4014,23 +4038,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		goto stop;
 	}
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
 		ret = -EIO;
-		goto stop;
-	}
-
 stop:
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
-		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
-		goto out;
-	}
-	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-			       buf & ~DP_TEST_SINK_START) < 0) {
-		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
-		goto out;
-	}
-out:
-	hsw_enable_ips(intel_crtc);
+	intel_dp_sink_crc_stop(intel_dp);
 	return ret;
 }
 
-- 
2.1.0

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

Re: [PATCH] drm/i915: Split sink_crc function in start, stop and read.

[Daniel Vetter]

On Thu, Jul 30, 2015 at 04:26:39PM -0700, Rodrigo Vivi wrote:
> This is just a preparation patch to make clear what operation we
> are performing. There is no functional change on the sink crc
> logic.
> 
> hsw_disable_ips has been moved a bit further in the start function
> to avoid disabling ips when sink crc is not going to be started.
> and to avoid goto on this function.
> 
> v2: explain why hsw_disable_ips() call place has changed.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks for the updated commit message. Queued for -next, thanks for the
patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 89 +++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 44f8a32..10cbc98 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3958,40 +3958,64 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
>  	return intel_dp->is_mst;
>  }
>  
> -int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> +static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  {
> -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct drm_device *dev = intel_dig_port->base.base.dev;
> -	struct intel_crtc *intel_crtc =
> -		to_intel_crtc(intel_dig_port->base.base.crtc);
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>  	u8 buf;
> -	int test_crc_count;
> -	int attempts = 6;
> -	int ret = 0;
> -
> -	hsw_disable_ips(intel_crtc);
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
> -		ret = -EIO;
> -		goto out;
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> +		return;
>  	}
>  
> -	if (!(buf & DP_TEST_CRC_SUPPORTED)) {
> -		ret = -ENOTTY;
> -		goto out;
> -	}
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> +			       buf & ~DP_TEST_SINK_START) < 0)
> +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> -		ret = -EIO;
> -		goto out;
> -	}
> +	hsw_enable_ips(intel_crtc);
> +}
> +
> +static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> +	u8 buf;
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
> +		return -EIO;
> +
> +	if (!(buf & DP_TEST_CRC_SUPPORTED))
> +		return -ENOTTY;
> +
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
> +		return -EIO;
> +
> +	hsw_disable_ips(intel_crtc);
>  
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> -				buf | DP_TEST_SINK_START) < 0) {
> -		ret = -EIO;
> -		goto out;
> +			       buf | DP_TEST_SINK_START) < 0) {
> +		hsw_enable_ips(intel_crtc);
> +		return -EIO;
>  	}
>  
> +	return 0;
> +}
> +
> +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = dig_port->base.base.dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> +	u8 buf;
> +	int test_crc_count;
> +	int attempts = 6;
> +	int ret;
> +
> +	ret = intel_dp_sink_crc_start(intel_dp);
> +	if (ret)
> +		return ret;
> +
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
>  		ret = -EIO;
>  		goto stop;
> @@ -4014,23 +4038,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  		goto stop;
>  	}
>  
> -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>  		ret = -EIO;
> -		goto stop;
> -	}
> -
>  stop:
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> -		goto out;
> -	}
> -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> -			       buf & ~DP_TEST_SINK_START) < 0) {
> -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> -		goto out;
> -	}
> -out:
> -	hsw_enable_ips(intel_crtc);
> +	intel_dp_sink_crc_stop(intel_dp);
>  	return ret;
>  }
>  
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Split sink_crc function in start, stop and read.

[Vivi, Rodrigo]

On Wed, 2015-08-05 at 10:07 +0200, Daniel Vetter wrote:
> On Thu, Jul 30, 2015 at 04:26:39PM -0700, Rodrigo Vivi wrote:
> > This is just a preparation patch to make clear what operation we
> > are performing. There is no functional change on the sink crc
> > logic.
> > 
> > hsw_disable_ips has been moved a bit further in the start function
> > to avoid disabling ips when sink crc is not going to be started.
> > and to avoid goto on this function.
> > 
> > v2: explain why hsw_disable_ips() call place has changed.
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Thanks for the updated commit message. Queued for -next, thanks for the
> patch.

Thanks!

Do you intend to merge the other 3 remaining patches?
Or am I missing something there?

Thanks,
Rodrigo.

> -Daniel
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 89 +++++++++++++++++++++++------------------
> >  1 file changed, 50 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 44f8a32..10cbc98 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3958,40 +3958,64 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
> >  	return intel_dp->is_mst;
> >  }
> >  
> > -int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > +static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >  {
> > -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > -	struct drm_device *dev = intel_dig_port->base.base.dev;
> > -	struct intel_crtc *intel_crtc =
> > -		to_intel_crtc(intel_dig_port->base.base.crtc);
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> >  	u8 buf;
> > -	int test_crc_count;
> > -	int attempts = 6;
> > -	int ret = 0;
> > -
> > -	hsw_disable_ips(intel_crtc);
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
> > -		ret = -EIO;
> > -		goto out;
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > +		return;
> >  	}
> >  
> > -	if (!(buf & DP_TEST_CRC_SUPPORTED)) {
> > -		ret = -ENOTTY;
> > -		goto out;
> > -	}
> > +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > +			       buf & ~DP_TEST_SINK_START) < 0)
> > +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > -		ret = -EIO;
> > -		goto out;
> > -	}
> > +	hsw_enable_ips(intel_crtc);
> > +}
> > +
> > +static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> > +	u8 buf;
> > +
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
> > +		return -EIO;
> > +
> > +	if (!(buf & DP_TEST_CRC_SUPPORTED))
> > +		return -ENOTTY;
> > +
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
> > +		return -EIO;
> > +
> > +	hsw_disable_ips(intel_crtc);
> >  
> >  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > -				buf | DP_TEST_SINK_START) < 0) {
> > -		ret = -EIO;
> > -		goto out;
> > +			       buf | DP_TEST_SINK_START) < 0) {
> > +		hsw_enable_ips(intel_crtc);
> > +		return -EIO;
> >  	}
> >  
> > +	return 0;
> > +}
> > +
> > +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > +{
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_device *dev = dig_port->base.base.dev;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> > +	u8 buf;
> > +	int test_crc_count;
> > +	int attempts = 6;
> > +	int ret;
> > +
> > +	ret = intel_dp_sink_crc_start(intel_dp);
> > +	if (ret)
> > +		return ret;
> > +
> >  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
> >  		ret = -EIO;
> >  		goto stop;
> > @@ -4014,23 +4038,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> >  		goto stop;
> >  	}
> >  
> > -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
> >  		ret = -EIO;
> > -		goto stop;
> > -	}
> > -
> >  stop:
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > -		goto out;
> > -	}
> > -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > -			       buf & ~DP_TEST_SINK_START) < 0) {
> > -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > -		goto out;
> > -	}
> > -out:
> > -	hsw_enable_ips(intel_crtc);
> > +	intel_dp_sink_crc_stop(intel_dp);
> >  	return ret;
> >  }
> >  
> > -- 
> > 2.1.0
> > 
> 

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

Re: [PATCH] drm/i915: Split sink_crc function in start, stop and read.

[Daniel Vetter]

On Wed, Aug 05, 2015 at 08:30:01PM +0000, Vivi, Rodrigo wrote:
> On Wed, 2015-08-05 at 10:07 +0200, Daniel Vetter wrote:
> > On Thu, Jul 30, 2015 at 04:26:39PM -0700, Rodrigo Vivi wrote:
> > > This is just a preparation patch to make clear what operation we
> > > are performing. There is no functional change on the sink crc
> > > logic.
> > > 
> > > hsw_disable_ips has been moved a bit further in the start function
> > > to avoid disabling ips when sink crc is not going to be started.
> > > and to avoid goto on this function.
> > > 
> > > v2: explain why hsw_disable_ips() call place has changed.
> > > 
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > Thanks for the updated commit message. Queued for -next, thanks for the
> > patch.
> 
> Thanks!
> 
> Do you intend to merge the other 3 remaining patches?
> Or am I missing something there?

Nah just oversight, merged 3 more patches. Do I have them all now?
-Daniel

> 
> Thanks,
> Rodrigo.
> 
> > -Daniel
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 89 +++++++++++++++++++++++------------------
> > >  1 file changed, 50 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 44f8a32..10cbc98 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3958,40 +3958,64 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
> > >  	return intel_dp->is_mst;
> > >  }
> > >  
> > > -int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > > +static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> > >  {
> > > -	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > -	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > -	struct intel_crtc *intel_crtc =
> > > -		to_intel_crtc(intel_dig_port->base.base.crtc);
> > > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> > >  	u8 buf;
> > > -	int test_crc_count;
> > > -	int attempts = 6;
> > > -	int ret = 0;
> > > -
> > > -	hsw_disable_ips(intel_crtc);
> > >  
> > > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
> > > -		ret = -EIO;
> > > -		goto out;
> > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > > +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > > +		return;
> > >  	}
> > >  
> > > -	if (!(buf & DP_TEST_CRC_SUPPORTED)) {
> > > -		ret = -ENOTTY;
> > > -		goto out;
> > > -	}
> > > +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > > +			       buf & ~DP_TEST_SINK_START) < 0)
> > > +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > >  
> > > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > > -		ret = -EIO;
> > > -		goto out;
> > > -	}
> > > +	hsw_enable_ips(intel_crtc);
> > > +}
> > > +
> > > +static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> > > +	u8 buf;
> > > +
> > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
> > > +		return -EIO;
> > > +
> > > +	if (!(buf & DP_TEST_CRC_SUPPORTED))
> > > +		return -ENOTTY;
> > > +
> > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
> > > +		return -EIO;
> > > +
> > > +	hsw_disable_ips(intel_crtc);
> > >  
> > >  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > > -				buf | DP_TEST_SINK_START) < 0) {
> > > -		ret = -EIO;
> > > -		goto out;
> > > +			       buf | DP_TEST_SINK_START) < 0) {
> > > +		hsw_enable_ips(intel_crtc);
> > > +		return -EIO;
> > >  	}
> > >  
> > > +	return 0;
> > > +}
> > > +
> > > +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > > +{
> > > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_device *dev = dig_port->base.base.dev;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> > > +	u8 buf;
> > > +	int test_crc_count;
> > > +	int attempts = 6;
> > > +	int ret;
> > > +
> > > +	ret = intel_dp_sink_crc_start(intel_dp);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
> > >  		ret = -EIO;
> > >  		goto stop;
> > > @@ -4014,23 +4038,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > >  		goto stop;
> > >  	}
> > >  
> > > -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> > > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
> > >  		ret = -EIO;
> > > -		goto stop;
> > > -	}
> > > -
> > >  stop:
> > > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > > -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > > -		goto out;
> > > -	}
> > > -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > > -			       buf & ~DP_TEST_SINK_START) < 0) {
> > > -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > > -		goto out;
> > > -	}
> > > -out:
> > > -	hsw_enable_ips(intel_crtc);
> > > +	intel_dp_sink_crc_stop(intel_dp);
> > >  	return ret;
> > >  }
> > >  
> > > -- 
> > > 2.1.0
> > > 
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Split sink_crc function in start, stop and read.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 6310 bytes --]

On Thu, Aug 6, 2015 at 5:20 AM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Aug 05, 2015 at 08:30:01PM +0000, Vivi, Rodrigo wrote:
> > On Wed, 2015-08-05 at 10:07 +0200, Daniel Vetter wrote:
> > > On Thu, Jul 30, 2015 at 04:26:39PM -0700, Rodrigo Vivi wrote:
> > > > This is just a preparation patch to make clear what operation we
> > > > are performing. There is no functional change on the sink crc
> > > > logic.
> > > >
> > > > hsw_disable_ips has been moved a bit further in the start function
> > > > to avoid disabling ips when sink crc is not going to be started.
> > > > and to avoid goto on this function.
> > > >
> > > > v2: explain why hsw_disable_ips() call place has changed.
> > > >
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >
> > > Thanks for the updated commit message. Queued for -next, thanks for the
> > > patch.
> >
> > Thanks!
> >
> > Do you intend to merge the other 3 remaining patches?
> > Or am I missing something there?
>
> Nah just oversight, merged 3 more patches. Do I have them all now?
>

yes, thanks!


> -Daniel
>
> >
> > Thanks,
> > Rodrigo.
> >
> > > -Daniel
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 89
> +++++++++++++++++++++++------------------
> > > >  1 file changed, 50 insertions(+), 39 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 44f8a32..10cbc98 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3958,40 +3958,64 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
> > > >   return intel_dp->is_mst;
> > > >  }
> > > >
> > > > -int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > > > +static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> > > >  {
> > > > - struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> > > > - struct drm_device *dev = intel_dig_port->base.base.dev;
> > > > - struct intel_crtc *intel_crtc =
> > > > -         to_intel_crtc(intel_dig_port->base.base.crtc);
> > > > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > > + struct intel_crtc *intel_crtc =
> to_intel_crtc(dig_port->base.base.crtc);
> > > >   u8 buf;
> > > > - int test_crc_count;
> > > > - int attempts = 6;
> > > > - int ret = 0;
> > > > -
> > > > - hsw_disable_ips(intel_crtc);
> > > >
> > > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) <
> 0) {
> > > > -         ret = -EIO;
> > > > -         goto out;
> > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > > > +         DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > > > +         return;
> > > >   }
> > > >
> > > > - if (!(buf & DP_TEST_CRC_SUPPORTED)) {
> > > > -         ret = -ENOTTY;
> > > > -         goto out;
> > > > - }
> > > > + if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > > > +                        buf & ~DP_TEST_SINK_START) < 0)
> > > > +         DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > > >
> > > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > > > -         ret = -EIO;
> > > > -         goto out;
> > > > - }
> > > > + hsw_enable_ips(intel_crtc);
> > > > +}
> > > > +
> > > > +static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> > > > +{
> > > > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > > + struct intel_crtc *intel_crtc =
> to_intel_crtc(dig_port->base.base.crtc);
> > > > + u8 buf;
> > > > +
> > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
> > > > +         return -EIO;
> > > > +
> > > > + if (!(buf & DP_TEST_CRC_SUPPORTED))
> > > > +         return -ENOTTY;
> > > > +
> > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
> > > > +         return -EIO;
> > > > +
> > > > + hsw_disable_ips(intel_crtc);
> > > >
> > > >   if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > > > -                         buf | DP_TEST_SINK_START) < 0) {
> > > > -         ret = -EIO;
> > > > -         goto out;
> > > > +                        buf | DP_TEST_SINK_START) < 0) {
> > > > +         hsw_enable_ips(intel_crtc);
> > > > +         return -EIO;
> > > >   }
> > > >
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > > > +{
> > > > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > > + struct drm_device *dev = dig_port->base.base.dev;
> > > > + struct intel_crtc *intel_crtc =
> to_intel_crtc(dig_port->base.base.crtc);
> > > > + u8 buf;
> > > > + int test_crc_count;
> > > > + int attempts = 6;
> > > > + int ret;
> > > > +
> > > > + ret = intel_dp_sink_crc_start(intel_dp);
> > > > + if (ret)
> > > > +         return ret;
> > > > +
> > > >   if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) <
> 0) {
> > > >           ret = -EIO;
> > > >           goto stop;
> > > > @@ -4014,23 +4038,10 @@ int intel_dp_sink_crc(struct intel_dp
> *intel_dp, u8 *crc)
> > > >           goto stop;
> > > >   }
> > > >
> > > > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) <
> 0) {
> > > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
> > > >           ret = -EIO;
> > > > -         goto stop;
> > > > - }
> > > > -
> > > >  stop:
> > > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > > > -         DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > > > -         goto out;
> > > > - }
> > > > - if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > > > -                        buf & ~DP_TEST_SINK_START) < 0) {
> > > > -         DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > > > -         goto out;
> > > > - }
> > > > -out:
> > > > - hsw_enable_ips(intel_crtc);
> > > > + intel_dp_sink_crc_stop(intel_dp);
> > > >   return ret;
> > > >  }
> > > >
> > > > --
> > > > 2.1.0
> > > >
> > >
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 9409 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 5/7] drm/i915: Force sink crc stop before start.

[Rodrigo Vivi]

By Vesa DP spec, test counter at DP_TEST_SINK_MISC just reset to 0
when unsetting DP_TEST_SINK_START, so let's force this stop here.

But let's minimize the aux transactions and just do it when we know
it hasn't been properly stoped.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 10cbc98..3ba031d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3958,22 +3958,30 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
 	return intel_dp->is_mst;
 }
 
-static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
+static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
 	u8 buf;
+	int ret = 0;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
 		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
-		return;
+		ret = -EIO;
+		goto out;
 	}
 
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-			       buf & ~DP_TEST_SINK_START) < 0)
+			       buf & ~DP_TEST_SINK_START) < 0) {
 		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
+		ret = -EIO;
+		goto out;
+	}
 
+	intel_dp->sink_crc_started = false;
+ out:
 	hsw_enable_ips(intel_crtc);
+	return ret;
 }
 
 static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
@@ -3981,6 +3989,13 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
 	u8 buf;
+	int ret;
+
+	if (intel_dp->sink_crc_started) {
+		ret = intel_dp_sink_crc_stop(intel_dp);
+		if (ret)
+			return ret;
+	}
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
 		return -EIO;
@@ -3999,6 +4014,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 		return -EIO;
 	}
 
+	intel_dp->sink_crc_started = true;
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 47cef0e..cc74400 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -714,6 +714,7 @@ struct intel_dp {
 	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
 	uint8_t num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
+	bool sink_crc_started;
 	struct drm_dp_aux aux;
 	uint8_t train_set[4];
 	int panel_power_up_delay;
-- 
2.1.0

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

Re: [PATCH 5/7] drm/i915: Force sink crc stop before start.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:48PM -0700, Rodrigo Vivi wrote:
> By Vesa DP spec, test counter at DP_TEST_SINK_MISC just reset to 0
> when unsetting DP_TEST_SINK_START, so let's force this stop here.
> 
> But let's minimize the aux transactions and just do it when we know
> it hasn't been properly stoped.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 22 +++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 10cbc98..3ba031d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3958,22 +3958,30 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
>  	return intel_dp->is_mst;
>  }
>  
> -static void intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> +static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>  	u8 buf;
> +	int ret = 0;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
>  		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> -		return;
> +		ret = -EIO;
> +		goto out;
>  	}
>  
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> -			       buf & ~DP_TEST_SINK_START) < 0)
> +			       buf & ~DP_TEST_SINK_START) < 0) {
>  		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> +		ret = -EIO;
> +		goto out;
> +	}
>  
> +	intel_dp->sink_crc_started = false;
> + out:
>  	hsw_enable_ips(intel_crtc);
> +	return ret;
>  }
>  
>  static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
> @@ -3981,6 +3989,13 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>  	u8 buf;
> +	int ret;
> +
> +	if (intel_dp->sink_crc_started) {
> +		ret = intel_dp_sink_crc_stop(intel_dp);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
>  		return -EIO;
> @@ -3999,6 +4014,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  		return -EIO;
>  	}
>  
> +	intel_dp->sink_crc_started = true;
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 47cef0e..cc74400 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -714,6 +714,7 @@ struct intel_dp {
>  	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
>  	uint8_t num_sink_rates;
>  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> +	bool sink_crc_started;
>  	struct drm_dp_aux aux;
>  	uint8_t train_set[4];
>  	int panel_power_up_delay;
> -- 
> 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

[PATCH 6/7] drm/i915: Save latest known sink CRC to compensate delayed counter reset.

[Rodrigo Vivi]

By Vesa DP 1.2 Spec TEST_CRC_COUNT should be
"reset to 0 when TEST_SINK bit 0 = 0."

However for some strange reason when PSR is enabled in
certain platforms this is not true. At least not immediatelly.

So we face cases like this:

first get_sink_crc operation:
	     count: 0, crc: 000000000000
	     count: 1, crc: c101c101c101
returned expected crc: c101c101c101

secont get_sink_crc operation:
	     count: 1, crc: c101c101c101
	     count: 0, crc: 000000000000
	     count: 1, crc: 0000c1010000
should return expected crc: 0000c1010000

But also the reset to 0 should be faster resulting into:

get_sink_crc operation:
	     count: 1, crc: c101c101c101
	     count: 1, crc: 0000c1010000
should return expected crc: 0000c1010000

So in order to know that the second one is valid one
we need to compare the pair (count, crc) with latest (count, crc).

If the pair changed you have your valid CRC.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 42 +++++++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h |  8 +++++++-
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3ba031d..c7372a1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3978,7 +3978,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 		goto out;
 	}
 
-	intel_dp->sink_crc_started = false;
+	intel_dp->sink_crc.started = false;
  out:
 	hsw_enable_ips(intel_crtc);
 	return ret;
@@ -3991,7 +3991,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	u8 buf;
 	int ret;
 
-	if (intel_dp->sink_crc_started) {
+	if (intel_dp->sink_crc.started) {
 		ret = intel_dp_sink_crc_stop(intel_dp);
 		if (ret)
 			return ret;
@@ -4003,6 +4003,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 	if (!(buf & DP_TEST_CRC_SUPPORTED))
 		return -ENOTTY;
 
+	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
+
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
 		return -EIO;
 
@@ -4014,7 +4016,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 		return -EIO;
 	}
 
-	intel_dp->sink_crc_started = true;
+	intel_dp->sink_crc.started = true;
 	return 0;
 }
 
@@ -4024,29 +4026,39 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
 	u8 buf;
-	int test_crc_count;
+	int count, ret;
 	int attempts = 6;
-	int ret;
 
 	ret = intel_dp_sink_crc_start(intel_dp);
 	if (ret)
 		return ret;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
-		ret = -EIO;
-		goto stop;
-	}
-
-	test_crc_count = buf & DP_TEST_COUNT_MASK;
-
 	do {
+		intel_wait_for_vblank(dev, intel_crtc->pipe);
+
 		if (drm_dp_dpcd_readb(&intel_dp->aux,
 				      DP_TEST_SINK_MISC, &buf) < 0) {
 			ret = -EIO;
 			goto stop;
 		}
-		intel_wait_for_vblank(dev, intel_crtc->pipe);
-	} while (--attempts && (buf & DP_TEST_COUNT_MASK) == test_crc_count);
+		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.
+		 */
+		if (count == 0)
+			intel_dp->sink_crc.last_count = 0;
+
+		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
+			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)))));
+
+	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");
@@ -4054,8 +4066,6 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		goto stop;
 	}
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
-		ret = -EIO;
 stop:
 	intel_dp_sink_crc_stop(intel_dp);
 	return ret;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cc74400..c072820 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -697,6 +697,12 @@ enum link_m_n_set {
 	M2_N2
 };
 
+struct sink_crc {
+	bool started;
+	u8 last_crc[6];
+	int last_count;
+};
+
 struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
@@ -714,7 +720,7 @@ struct intel_dp {
 	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
 	uint8_t num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
-	bool sink_crc_started;
+	struct sink_crc sink_crc;
 	struct drm_dp_aux aux;
 	uint8_t train_set[4];
 	int panel_power_up_delay;
-- 
2.1.0

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

Re: [PATCH 6/7] drm/i915: Save latest known sink CRC to compensate delayed counter reset.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:49PM -0700, Rodrigo Vivi wrote:
> By Vesa DP 1.2 Spec TEST_CRC_COUNT should be
> "reset to 0 when TEST_SINK bit 0 = 0."
> 
> However for some strange reason when PSR is enabled in
> certain platforms this is not true. At least not immediatelly.
> 
> So we face cases like this:
> 
> first get_sink_crc operation:
> 	     count: 0, crc: 000000000000
> 	     count: 1, crc: c101c101c101
> returned expected crc: c101c101c101
> 
> secont get_sink_crc operation:
> 	     count: 1, crc: c101c101c101
> 	     count: 0, crc: 000000000000
> 	     count: 1, crc: 0000c1010000
> should return expected crc: 0000c1010000
> 
> But also the reset to 0 should be faster resulting into:
> 
> get_sink_crc operation:
> 	     count: 1, crc: c101c101c101
> 	     count: 1, crc: 0000c1010000
> should return expected crc: 0000c1010000
> 
> So in order to know that the second one is valid one
> we need to compare the pair (count, crc) with latest (count, crc).
> 
> If the pair changed you have your valid CRC.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

We discussed this before, unfortunately I don't see any other way to do
this, since there is no way to know that the crc count really restarted,
became 0 and then 1 again. So I think this workaround is needed.

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 42 +++++++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h |  8 +++++++-
>  2 files changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3ba031d..c7372a1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3978,7 +3978,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  		goto out;
>  	}
>  
> -	intel_dp->sink_crc_started = false;
> +	intel_dp->sink_crc.started = false;
>   out:
>  	hsw_enable_ips(intel_crtc);
>  	return ret;
> @@ -3991,7 +3991,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  	u8 buf;
>  	int ret;
>  
> -	if (intel_dp->sink_crc_started) {
> +	if (intel_dp->sink_crc.started) {
>  		ret = intel_dp_sink_crc_stop(intel_dp);
>  		if (ret)
>  			return ret;
> @@ -4003,6 +4003,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  	if (!(buf & DP_TEST_CRC_SUPPORTED))
>  		return -ENOTTY;
>  
> +	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
> +
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
>  		return -EIO;
>  
> @@ -4014,7 +4016,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  		return -EIO;
>  	}
>  
> -	intel_dp->sink_crc_started = true;
> +	intel_dp->sink_crc.started = true;
>  	return 0;
>  }
>  
> @@ -4024,29 +4026,39 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>  	u8 buf;
> -	int test_crc_count;
> +	int count, ret;
>  	int attempts = 6;
> -	int ret;
>  
>  	ret = intel_dp_sink_crc_start(intel_dp);
>  	if (ret)
>  		return ret;
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
> -		ret = -EIO;
> -		goto stop;
> -	}
> -
> -	test_crc_count = buf & DP_TEST_COUNT_MASK;
> -
>  	do {
> +		intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
>  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				      DP_TEST_SINK_MISC, &buf) < 0) {
>  			ret = -EIO;
>  			goto stop;
>  		}
> -		intel_wait_for_vblank(dev, intel_crtc->pipe);
> -	} while (--attempts && (buf & DP_TEST_COUNT_MASK) == test_crc_count);
> +		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.
> +		 */
> +		if (count == 0)
> +			intel_dp->sink_crc.last_count = 0;
> +
> +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> +			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)))));
> +
> +	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");
> @@ -4054,8 +4066,6 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  		goto stop;
>  	}
>  
> -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
> -		ret = -EIO;
>  stop:
>  	intel_dp_sink_crc_stop(intel_dp);
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cc74400..c072820 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -697,6 +697,12 @@ enum link_m_n_set {
>  	M2_N2
>  };
>  
> +struct sink_crc {
> +	bool started;
> +	u8 last_crc[6];
> +	int last_count;
> +};
> +
>  struct intel_dp {
>  	uint32_t output_reg;
>  	uint32_t aux_ch_ctl_reg;
> @@ -714,7 +720,7 @@ struct intel_dp {
>  	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
>  	uint8_t num_sink_rates;
>  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> -	bool sink_crc_started;
> +	struct sink_crc sink_crc;
>  	struct drm_dp_aux aux;
>  	uint8_t train_set[4];
>  	int panel_power_up_delay;
> -- 
> 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

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

[Rodrigo Vivi]

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(-)

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:
-- 
2.1.0

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

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

[Rafael Antognolli]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Vivi, Rodrigo]

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

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

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

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rafael Antognolli]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Try to stop sink crc calculation on error.

[Rafael Antognolli]

On Thu, Jul 23, 2015 at 04:35:45PM -0700, Rodrigo Vivi wrote:
> Right now if we face any kind of error sink crc calculation
> stays enabled.
> 
> So, let's give a shot and try to stop it anyway if it got enabled.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f1b9f93..70a4a37 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3994,7 +3994,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
>  		ret = -EIO;
> -		goto out;
> +		goto stop;
>  	}
>  
>  	test_crc_count = buf & DP_TEST_COUNT_MASK;
> @@ -4003,7 +4003,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				      DP_TEST_SINK_MISC, &buf) < 0) {
>  			ret = -EIO;
> -			goto out;
> +			goto stop;
>  		}
>  		intel_wait_for_vblank(dev, intel_crtc->pipe);
>  	} while (--attempts && (buf & DP_TEST_COUNT_MASK) == test_crc_count);
> @@ -4011,14 +4011,15 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	if (attempts == 0) {
>  		DRM_DEBUG_KMS("Panel is unable to calculate CRC after 6 vblanks\n");
>  		ret = -ETIMEDOUT;
> -		goto out;
> +		goto stop;
>  	}
>  
>  	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
>  		ret = -EIO;
> -		goto out;
> +		goto stop;
>  	}
>  
> +stop:
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
>  		ret = -EIO;
>  		goto out;

Nice cleanup on exit.

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


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