[PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[Rodrigo Vivi]

From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>

Let's use a native read with retry as suggested per spec to
fix Sink CRC on SKL when PSR is enabled.

With PSR enabled panel is probably taking more time to wake
and dpcd read is faling.

Cc: Sonika Jindal <sonika.jindal@intel.com>
Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d32ce48..34f5e33 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 	u8 buf;
 	int ret = 0;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK,
+				    &buf, 1) < 0) {
 		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
 		ret = -EIO;
 		goto out;
@@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 			return ret;
 	}
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK_MISC,
+				    &buf, 1) < 0)
 		return -EIO;
 
 	if (!(buf & DP_TEST_CRC_SUPPORTED))
@@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 
 	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, &buf, 1) < 0)
 		return -EIO;
 
 	hsw_disable_ips(intel_crtc);
@@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	do {
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-		if (drm_dp_dpcd_readb(&intel_dp->aux,
-				      DP_TEST_SINK_MISC, &buf) < 0) {
+		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
+					    DP_TEST_SINK_MISC, &buf, 1) < 0) {
 			ret = -EIO;
 			goto stop;
 		}
@@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		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) {
+		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_CRC_R_CR,
+					    crc, 6) < 0) {
 			ret = -EIO;
 			goto stop;
 		}
-- 
2.1.0

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

[PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[Rodrigo Vivi]

Let's use a native read with retry as suggested per spec to
fix Sink CRC on SKL when PSR is enabled.

With PSR enabled panel is probably taking more time to wake
and dpcd read is faling.

v2: Fix my email domain on commit message. Thanks Rafael.

Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Sonika Jindal <sonika.jindal@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d32ce48..34f5e33 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 	u8 buf;
 	int ret = 0;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK,
+				    &buf, 1) < 0) {
 		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
 		ret = -EIO;
 		goto out;
@@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 			return ret;
 	}
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK_MISC,
+				    &buf, 1) < 0)
 		return -EIO;
 
 	if (!(buf & DP_TEST_CRC_SUPPORTED))
@@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 
 	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, &buf, 1) < 0)
 		return -EIO;
 
 	hsw_disable_ips(intel_crtc);
@@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	do {
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-		if (drm_dp_dpcd_readb(&intel_dp->aux,
-				      DP_TEST_SINK_MISC, &buf) < 0) {
+		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
+					    DP_TEST_SINK_MISC, &buf, 1) < 0) {
 			ret = -EIO;
 			goto stop;
 		}
@@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		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) {
+		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_CRC_R_CR,
+					    crc, 6) < 0) {
 			ret = -EIO;
 			goto stop;
 		}
-- 
2.4.3

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

Re: [PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[Jindal, Sonika]

So, sink crc during psr works now?
Great, I will give a try with this patch..

Regards,
Sonika

-----Original Message-----
From: Vivi, Rodrigo 
Sent: Friday, August 21, 2015 4:53 AM
To: intel-gfx@lists.freedesktop.org
Cc: Vivi, Rodrigo; Antognolli, Rafael; Jindal, Sonika
Subject: [PATCH] drm/i915: Use dpcd read wake for sink crc calls.

Let's use a native read with retry as suggested per spec to fix Sink CRC on SKL when PSR is enabled.

With PSR enabled panel is probably taking more time to wake and dpcd read is faling.

v2: Fix my email domain on commit message. Thanks Rafael.

Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Sonika Jindal <sonika.jindal@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index d32ce48..34f5e33 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
 	u8 buf;
 	int ret = 0;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK,
+				    &buf, 1) < 0) {
 		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
 		ret = -EIO;
 		goto out;
@@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 			return ret;
 	}
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK_MISC,
+				    &buf, 1) < 0)
 		return -EIO;
 
 	if (!(buf & DP_TEST_CRC_SUPPORTED))
@@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
 
 	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, &buf, 1) < 
+0)
 		return -EIO;
 
 	hsw_disable_ips(intel_crtc);
@@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	do {
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-		if (drm_dp_dpcd_readb(&intel_dp->aux,
-				      DP_TEST_SINK_MISC, &buf) < 0) {
+		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
+					    DP_TEST_SINK_MISC, &buf, 1) < 0) {
 			ret = -EIO;
 			goto stop;
 		}
@@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 		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) {
+		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_CRC_R_CR,
+					    crc, 6) < 0) {
 			ret = -EIO;
 			goto stop;
 		}
--
2.4.3

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

Re: [PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[Zanoni, Paulo R]

Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu:
> Let's use a native read with retry as suggested per spec to
> fix Sink CRC on SKL when PSR is enabled.
> 
> With PSR enabled panel is probably taking more time to wake
> and dpcd read is faling.

Does this commit actually fix any known problem with Sink CRC? Or is it
just a try? It would be nice to have this clarified in the commit
message.

Anyway, it looks correct, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> v2: Fix my email domain on commit message. Thanks Rafael.
> 
> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> b/drivers/gpu/drm/i915/intel_dp.c
> index d32ce48..34f5e33 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct 
> intel_dp *intel_dp)
>  	u8 buf;
>  	int ret = 0;
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 
> 0) {
> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK,
> +				    &buf, 1) < 0) {
>  		DRM_DEBUG_KMS("Sink CRC couldn't be stopped 
> properly\n");
>  		ret = -EIO;
>  		goto out;
> @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct 
> intel_dp *intel_dp)
>  			return ret;
>  	}
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, 
> &buf) < 0)
> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> DP_TEST_SINK_MISC,
> +				    &buf, 1) < 0)
>  		return -EIO;
>  
>  	if (!(buf & DP_TEST_CRC_SUPPORTED))
> @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct 
> intel_dp *intel_dp)
>  
>  	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 
> 0)
> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, 
> &buf, 1) < 0)
>  		return -EIO;
>  
>  	hsw_disable_ips(intel_crtc);
> @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp 
> *intel_dp, u8 *crc)
>  	do {
>  		intel_wait_for_vblank(dev, intel_crtc->pipe);
>  
> -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> -				      DP_TEST_SINK_MISC, &buf) < 0) 
> {
> +		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> +					    DP_TEST_SINK_MISC, &buf, 
> 1) < 0) {
>  			ret = -EIO;
>  			goto stop;
>  		}
> @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp 
> *intel_dp, u8 *crc)
>  		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) {
> +		if (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> DP_TEST_CRC_R_CR,
> +					    crc, 6) < 0) {
>  			ret = -EIO;
>  			goto stop;
>  		}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[Vivi, Rodrigo]

On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu:
> > Let's use a native read with retry as suggested per spec to
> > fix Sink CRC on SKL when PSR is enabled.
> > 
> > With PSR enabled panel is probably taking more time to wake
> > and dpcd read is faling.
> 
> Does this commit actually fix any known problem with Sink CRC? Or is 
> it
> just a try? It would be nice to have this clarified in the commit
> message.

It was just a try but that made sink crc working on my SKL when PSR is
enabled. nothing much to add... 

> 
> Anyway, it looks correct, so:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > 
> > v2: Fix my email domain on commit message. Thanks Rafael.
> > 
> > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > Cc: Sonika Jindal <sonika.jindal@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index d32ce48..34f5e33 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct 
> > intel_dp *intel_dp)
> >  	u8 buf;
> >  	int ret = 0;
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) 
> > < 
> > 0) {
> > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK,
> > +				    &buf, 1) < 0) {
> >  		DRM_DEBUG_KMS("Sink CRC couldn't be stopped 
> > properly\n");
> >  		ret = -EIO;
> >  		goto out;
> > @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct 
> > intel_dp *intel_dp)
> >  			return ret;
> >  	}
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, 
> > &buf) < 0)
> > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> > DP_TEST_SINK_MISC,
> > +				    &buf, 1) < 0)
> >  		return -EIO;
> >  
> >  	if (!(buf & DP_TEST_CRC_SUPPORTED))
> > @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct 
> > intel_dp *intel_dp)
> >  
> >  	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) 
> > < 
> > 0)
> > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, 
> > &buf, 1) < 0)
> >  		return -EIO;
> >  
> >  	hsw_disable_ips(intel_crtc);
> > @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp 
> > *intel_dp, u8 *crc)
> >  	do {
> >  		intel_wait_for_vblank(dev, intel_crtc->pipe);
> >  
> > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > -				      DP_TEST_SINK_MISC, &buf) < 
> > 0) 
> > {
> > +		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > +					    DP_TEST_SINK_MISC, 
> > &buf, 
> > 1) < 0) {
> >  			ret = -EIO;
> >  			goto stop;
> >  		}
> > @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp 
> > *intel_dp, u8 *crc)
> >  		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) {
> > +		if (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> > DP_TEST_CRC_R_CR,
> > +					    crc, 6) < 0) {
> >  			ret = -EIO;
> >  			goto stop;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[Thulasimani, Sivakumar]

On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote:
> On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote:
>> Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu:
>>> Let's use a native read with retry as suggested per spec to
>>> fix Sink CRC on SKL when PSR is enabled.
>>>
>>> With PSR enabled panel is probably taking more time to wake
>>> and dpcd read is faling.
>> Does this commit actually fix any known problem with Sink CRC? Or is
>> it
>> just a try? It would be nice to have this clarified in the commit
>> message.
> It was just a try but that made sink crc working on my SKL when PSR is
> enabled. nothing much to add...
SKL has new register AUX_MUTEX which should be used when accessing dpcd
on edp. just searched the nightly code and could not find it. it might 
be the reason
for random dpcd failures reported in the other thread.

regards,
Sivakumar
>> Anyway, it looks correct, so:
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>>> v2: Fix my email domain on commit message. Thanks Rafael.
>>>
>>> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
>>> Cc: Sonika Jindal <sonika.jindal@intel.com>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------
>>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index d32ce48..34f5e33 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct
>>> intel_dp *intel_dp)
>>>   	u8 buf;
>>>   	int ret = 0;
>>>   
>>> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf)
>>> <
>>> 0) {
>>> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK,
>>> +				    &buf, 1) < 0) {
>>>   		DRM_DEBUG_KMS("Sink CRC couldn't be stopped
>>> properly\n");
>>>   		ret = -EIO;
>>>   		goto out;
>>> @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct
>>> intel_dp *intel_dp)
>>>   			return ret;
>>>   	}
>>>   
>>> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC,
>>> &buf) < 0)
>>> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux,
>>> DP_TEST_SINK_MISC,
>>> +				    &buf, 1) < 0)
>>>   		return -EIO;
>>>   
>>>   	if (!(buf & DP_TEST_CRC_SUPPORTED))
>>> @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct
>>> intel_dp *intel_dp)
>>>   
>>>   	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
>>>   
>>> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf)
>>> <
>>> 0)
>>> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK,
>>> &buf, 1) < 0)
>>>   		return -EIO;
>>>   
>>>   	hsw_disable_ips(intel_crtc);
>>> @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp
>>> *intel_dp, u8 *crc)
>>>   	do {
>>>   		intel_wait_for_vblank(dev, intel_crtc->pipe);
>>>   
>>> -		if (drm_dp_dpcd_readb(&intel_dp->aux,
>>> -				      DP_TEST_SINK_MISC, &buf) <
>>> 0)
>>> {
>>> +		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
>>> +					    DP_TEST_SINK_MISC,
>>> &buf,
>>> 1) < 0) {
>>>   			ret = -EIO;
>>>   			goto stop;
>>>   		}
>>> @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp
>>> *intel_dp, u8 *crc)
>>>   		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) {
>>> +		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
>>> DP_TEST_CRC_R_CR,
>>> +					    crc, 6) < 0) {
>>>   			ret = -EIO;
>>>   			goto stop;
> _______________________________________________
> 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] drm/i915: Use dpcd read wake for sink crc calls.

[Vivi, Rodrigo]

On Thu, 2015-10-22 at 00:01 +0530, Thulasimani, Sivakumar wrote:
> 
> On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote:
> > On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote:
> > > Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu:
> > > > Let's use a native read with retry as suggested per spec to
> > > > fix Sink CRC on SKL when PSR is enabled.
> > > > 
> > > > With PSR enabled panel is probably taking more time to wake
> > > > and dpcd read is faling.
> > > Does this commit actually fix any known problem with Sink CRC? Or 
> > > is
> > > it
> > > just a try? It would be nice to have this clarified in the commit
> > > message.
> > It was just a try but that made sink crc working on my SKL when PSR 
> > is
> > enabled. nothing much to add...
> SKL has new register AUX_MUTEX which should be used when accessing 
> dpcd
> on edp. just searched the nightly code and could not find it. it 
> might 
> be the reason
> for random dpcd failures reported in the other thread.

Oh, this is interesting... I didn't know that.
That would explain that forbidden value on patch 3/4...

> 
> regards,
> Sivakumar
> > > Anyway, it looks correct, so:
> > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > > v2: Fix my email domain on commit message. Thanks Rafael.
> > > > 
> > > > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > > > Cc: Sonika Jindal <sonika.jindal@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------
> > > >   1 file changed, 9 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index d32ce48..34f5e33 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct
> > > > intel_dp *intel_dp)
> > > >   	u8 buf;
> > > >   	int ret = 0;
> > > >   
> > > > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, 
> > > > &buf)
> > > > <
> > > > 0) {
> > > > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> > > > DP_TEST_SINK,
> > > > +				    &buf, 1) < 0) {
> > > >   		DRM_DEBUG_KMS("Sink CRC couldn't be stopped
> > > > properly\n");
> > > >   		ret = -EIO;
> > > >   		goto out;
> > > > @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct
> > > > intel_dp *intel_dp)
> > > >   			return ret;
> > > >   	}
> > > >   
> > > > -	if (drm_dp_dpcd_readb(&intel_dp->aux, 
> > > > DP_TEST_SINK_MISC,
> > > > &buf) < 0)
> > > > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > > > DP_TEST_SINK_MISC,
> > > > +				    &buf, 1) < 0)
> > > >   		return -EIO;
> > > >   
> > > >   	if (!(buf & DP_TEST_CRC_SUPPORTED))
> > > > @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct
> > > > intel_dp *intel_dp)
> > > >   
> > > >   	intel_dp->sink_crc.last_count = buf & 
> > > > DP_TEST_COUNT_MASK;
> > > >   
> > > > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, 
> > > > &buf)
> > > > <
> > > > 0)
> > > > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> > > > DP_TEST_SINK,
> > > > &buf, 1) < 0)
> > > >   		return -EIO;
> > > >   
> > > >   	hsw_disable_ips(intel_crtc);
> > > > @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp
> > > > *intel_dp, u8 *crc)
> > > >   	do {
> > > >   		intel_wait_for_vblank(dev, intel_crtc->pipe);
> > > >   
> > > > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > > -				      DP_TEST_SINK_MISC, &buf) 
> > > > <
> > > > 0)
> > > > {
> > > > +		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > > > +					    DP_TEST_SINK_MISC,
> > > > &buf,
> > > > 1) < 0) {
> > > >   			ret = -EIO;
> > > >   			goto stop;
> > > >   		}
> > > > @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp
> > > > *intel_dp, u8 *crc)
> > > >   		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) {
> > > > +		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > > > DP_TEST_CRC_R_CR,
> > > > +					    crc, 6) < 0) {
> > > >   			ret = -EIO;
> > > >   			goto stop;
> > _______________________________________________
> > 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] drm/i915: Use dpcd read wake for sink crc calls.

[Damien Lespiau]

On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote:
> >On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote:
> >>Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu:
> >>>Let's use a native read with retry as suggested per spec to
> >>>fix Sink CRC on SKL when PSR is enabled.
> >>>
> >>>With PSR enabled panel is probably taking more time to wake
> >>>and dpcd read is faling.
> >>Does this commit actually fix any known problem with Sink CRC? Or is
> >>it
> >>just a try? It would be nice to have this clarified in the commit
> >>message.
> >It was just a try but that made sink crc working on my SKL when PSR is
> >enabled. nothing much to add...
> SKL has new register AUX_MUTEX which should be used when accessing dpcd
> on edp. just searched the nightly code and could not find it. it might be
> the reason
> for random dpcd failures reported in the other thread.

We had patches for that back in December 2013 :)

The feedback from Art was:

    The non-software aux users are PSR/SRD and GTC.
    Better leave out the mutex for now. Hardware is going to try do the
    arbitration itself. I expect you will then need to increase any software
    timeout you may have.

Do you know if anything has changed since then?

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

Re: [PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[Thulasimani, Sivakumar]

On 10/22/2015 1:44 AM, Damien Lespiau wrote:
> On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, Sivakumar wrote:
>>
>> On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote:
>>> On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote:
>>>> Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu:
>>>>> Let's use a native read with retry as suggested per spec to
>>>>> fix Sink CRC on SKL when PSR is enabled.
>>>>>
>>>>> With PSR enabled panel is probably taking more time to wake
>>>>> and dpcd read is faling.
>>>> Does this commit actually fix any known problem with Sink CRC? Or is
>>>> it
>>>> just a try? It would be nice to have this clarified in the commit
>>>> message.
>>> It was just a try but that made sink crc working on my SKL when PSR is
>>> enabled. nothing much to add...
>> SKL has new register AUX_MUTEX which should be used when accessing dpcd
>> on edp. just searched the nightly code and could not find it. it might be
>> the reason
>> for random dpcd failures reported in the other thread.
> We had patches for that back in December 2013 :)
>
> The feedback from Art was:
>
>      The non-software aux users are PSR/SRD and GTC.
>      Better leave out the mutex for now. Hardware is going to try do the
>      arbitration itself. I expect you will then need to increase any software
>      timeout you may have.
>
> Do you know if anything has changed since then?
>
Not sure, it is in the bspec sequence to use AUX hence forwarded. Art 
might be the
right person to contact :). it might be due to some minor DPCD access 
issues we
observed in BDW when PSR was enabled.

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

Re: [PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[Rodrigo Vivi]

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

Ok, so after trying it we saw that we really cannot trust on aux mutex.At
least not on all SKL/KBL
It worked in a KBL but failed on a SKL that I have here...

So without aux mutex option we still need to get sink_crc more reliable and
I see only 2 quick ways here:
- This read wake
- Return -EBUSY to force the drm retries on message size = 0.

Daniel, what do you believe?

Please let me know witch way and if necessary I rebase the patch and
re-send.

Thanks,
Rodrigo.




On Wed, Oct 21, 2015 at 8:27 PM Thulasimani, Sivakumar <
sivakumar.thulasimani@intel.com> wrote:

>
>
> On 10/22/2015 1:44 AM, Damien Lespiau wrote:
> > On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, Sivakumar wrote:
> >>
> >> On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote:
> >>> On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote:
> >>>> Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu:
> >>>>> Let's use a native read with retry as suggested per spec to
> >>>>> fix Sink CRC on SKL when PSR is enabled.
> >>>>>
> >>>>> With PSR enabled panel is probably taking more time to wake
> >>>>> and dpcd read is faling.
> >>>> Does this commit actually fix any known problem with Sink CRC? Or is
> >>>> it
> >>>> just a try? It would be nice to have this clarified in the commit
> >>>> message.
> >>> It was just a try but that made sink crc working on my SKL when PSR is
> >>> enabled. nothing much to add...
> >> SKL has new register AUX_MUTEX which should be used when accessing dpcd
> >> on edp. just searched the nightly code and could not find it. it might
> be
> >> the reason
> >> for random dpcd failures reported in the other thread.
> > We had patches for that back in December 2013 :)
> >
> > The feedback from Art was:
> >
> >      The non-software aux users are PSR/SRD and GTC.
> >      Better leave out the mutex for now. Hardware is going to try do the
> >      arbitration itself. I expect you will then need to increase any
> software
> >      timeout you may have.
> >
> > Do you know if anything has changed since then?
> >
> Not sure, it is in the bspec sequence to use AUX hence forwarded. Art
> might be the
> right person to contact :). it might be due to some minor DPCD access
> issues we
> observed in BDW when PSR was enabled.
>
> regards,
> Sivakumar
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 3445 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

Re: [PATCH] drm/i915: Use dpcd read wake for sink crc calls.

[Daniel Vetter]

On Mon, Nov 16, 2015 at 04:05:42PM +0000, Rodrigo Vivi wrote:
> Ok, so after trying it we saw that we really cannot trust on aux mutex.At
> least not on all SKL/KBL
> It worked in a KBL but failed on a SKL that I have here...
> 
> So without aux mutex option we still need to get sink_crc more reliable and
> I see only 2 quick ways here:
> - This read wake
> - Return -EBUSY to force the drm retries on message size = 0.
> 
> Daniel, what do you believe?

It's still a mess. My opinion is still that we should move the hacks from
read_wake into a more suitable place:
a) either into drm_dp_dpcd_read in drm_dp_helper.c
b) or into intel_dp_aux_transfer in intel_dp.c

Option a) is the right one if this is a generic sink issue (and it seems
to be the case, at least for edp panels). Option b) if it's an issue with
our hw. Either way I think intel_dp_dpcd_read_wake should die.

On a personal gut level I'd go with option a).

Cheers, Daniel

> 
> Please let me know witch way and if necessary I rebase the patch and
> re-send.
> 
> Thanks,
> Rodrigo.
> 
> 
> 
> 
> On Wed, Oct 21, 2015 at 8:27 PM Thulasimani, Sivakumar <
> sivakumar.thulasimani@intel.com> wrote:
> 
> >
> >
> > On 10/22/2015 1:44 AM, Damien Lespiau wrote:
> > > On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, Sivakumar wrote:
> > >>
> > >> On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote:
> > >>> On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote:
> > >>>> Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu:
> > >>>>> Let's use a native read with retry as suggested per spec to
> > >>>>> fix Sink CRC on SKL when PSR is enabled.
> > >>>>>
> > >>>>> With PSR enabled panel is probably taking more time to wake
> > >>>>> and dpcd read is faling.
> > >>>> Does this commit actually fix any known problem with Sink CRC? Or is
> > >>>> it
> > >>>> just a try? It would be nice to have this clarified in the commit
> > >>>> message.
> > >>> It was just a try but that made sink crc working on my SKL when PSR is
> > >>> enabled. nothing much to add...
> > >> SKL has new register AUX_MUTEX which should be used when accessing dpcd
> > >> on edp. just searched the nightly code and could not find it. it might
> > be
> > >> the reason
> > >> for random dpcd failures reported in the other thread.
> > > We had patches for that back in December 2013 :)
> > >
> > > The feedback from Art was:
> > >
> > >      The non-software aux users are PSR/SRD and GTC.
> > >      Better leave out the mutex for now. Hardware is going to try do the
> > >      arbitration itself. I expect you will then need to increase any
> > software
> > >      timeout you may have.
> > >
> > > Do you know if anything has changed since then?
> > >
> > Not sure, it is in the bspec sequence to use AUX hence forwarded. Art
> > might be the
> > right person to contact :). it might be due to some minor DPCD access
> > issues we
> > observed in BDW when PSR was enabled.
> >
> > regards,
> > Sivakumar
> > _______________________________________________
> > 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] drm/i915: Use dpcd read wake for sink crc calls.

[Vivi, Rodrigo]

On Tue, 2015-11-17 at 15:08 +0100, Daniel Vetter wrote:
> On Mon, Nov 16, 2015 at 04:05:42PM +0000, Rodrigo Vivi wrote:
> > Ok, so after trying it we saw that we really cannot trust on aux 
> > mutex.At
> > least not on all SKL/KBL
> > It worked in a KBL but failed on a SKL that I have here...
> > 
> > So without aux mutex option we still need to get sink_crc more 
> > reliable and
> > I see only 2 quick ways here:
> > - This read wake
> > - Return -EBUSY to force the drm retries on message size = 0.
> > 
> > Daniel, what do you believe?
> 
> It's still a mess. My opinion is still that we should move the hacks 
> from
> read_wake into a more suitable place:
> a) either into drm_dp_dpcd_read in drm_dp_helper.c

Well, but drm_dp_helper already does many retries already (32 times)
but only on EBUSY. My idea is that we should consider that and return
EBUSY instead of adding another retry case at drm.


> b) or into intel_dp_aux_transfer in intel_dp.c

Oh, I thought you had nacked this option.

> 
> Option a) is the right one if this is a generic sink issue (and it 
> seems
> to be the case, at least for edp panels). Option b) if it's an issue 
> with
> our hw. Either way I think intel_dp_dpcd_read_wake should die.

Well, Jani and Ville kind of nacked this while we don't understand why
this was ever introduced at first place.
Although I believe with the proper EBUSY returns in place and 32 times
retry I believe we could safely remove this as I tried on that series.

> 
> On a personal gut level I'd go with option a).

So, EBUSY is ok or should we create another case on drm level?

> 
> Cheers, Daniel

Thanks,
Rodrigo.

> 
> > 
> > Please let me know witch way and if necessary I rebase the patch 
> > and
> > re-send.
> > 
> > Thanks,
> > Rodrigo.
> > 
> > 
> > 
> > 
> > On Wed, Oct 21, 2015 at 8:27 PM Thulasimani, Sivakumar <
> > sivakumar.thulasimani@intel.com> wrote:
> > 
> > > 
> > > 
> > > On 10/22/2015 1:44 AM, Damien Lespiau wrote:
> > > > On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, 
> > > > Sivakumar wrote:
> > > > > 
> > > > > On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote:
> > > > > > On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote:
> > > > > > > Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu:
> > > > > > > > Let's use a native read with retry as suggested per 
> > > > > > > > spec to
> > > > > > > > fix Sink CRC on SKL when PSR is enabled.
> > > > > > > > 
> > > > > > > > With PSR enabled panel is probably taking more time to 
> > > > > > > > wake
> > > > > > > > and dpcd read is faling.
> > > > > > > Does this commit actually fix any known problem with Sink 
> > > > > > > CRC? Or is
> > > > > > > it
> > > > > > > just a try? It would be nice to have this clarified in 
> > > > > > > the commit
> > > > > > > message.
> > > > > > It was just a try but that made sink crc working on my SKL 
> > > > > > when PSR is
> > > > > > enabled. nothing much to add...
> > > > > SKL has new register AUX_MUTEX which should be used when 
> > > > > accessing dpcd
> > > > > on edp. just searched the nightly code and could not find it. 
> > > > > it might
> > > be
> > > > > the reason
> > > > > for random dpcd failures reported in the other thread.
> > > > We had patches for that back in December 2013 :)
> > > > 
> > > > The feedback from Art was:
> > > > 
> > > >      The non-software aux users are PSR/SRD and GTC.
> > > >      Better leave out the mutex for now. Hardware is going to 
> > > > try do the
> > > >      arbitration itself. I expect you will then need to 
> > > > increase any
> > > software
> > > >      timeout you may have.
> > > > 
> > > > Do you know if anything has changed since then?
> > > > 
> > > Not sure, it is in the bspec sequence to use AUX hence forwarded. 
> > > Art
> > > might be the
> > > right person to contact :). it might be due to some minor DPCD 
> > > access
> > > issues we
> > > observed in BDW when PSR was enabled.
> > > 
> > > regards,
> > > Sivakumar
> > > _______________________________________________
> > > 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] drm/i915: Use dpcd read wake for sink crc calls.

[Daniel Vetter]

On Wed, Nov 18, 2015 at 06:31:05PM +0000, Vivi, Rodrigo wrote:
> On Tue, 2015-11-17 at 15:08 +0100, Daniel Vetter wrote:
> > On Mon, Nov 16, 2015 at 04:05:42PM +0000, Rodrigo Vivi wrote:
> > > Ok, so after trying it we saw that we really cannot trust on aux 
> > > mutex.At
> > > least not on all SKL/KBL
> > > It worked in a KBL but failed on a SKL that I have here...
> > > 
> > > So without aux mutex option we still need to get sink_crc more 
> > > reliable and
> > > I see only 2 quick ways here:
> > > - This read wake
> > > - Return -EBUSY to force the drm retries on message size = 0.
> > > 
> > > Daniel, what do you believe?
> > 
> > It's still a mess. My opinion is still that we should move the hacks 
> > from
> > read_wake into a more suitable place:
> > a) either into drm_dp_dpcd_read in drm_dp_helper.c
> 
> Well, but drm_dp_helper already does many retries already (32 times)
> but only on EBUSY. My idea is that we should consider that and return
> EBUSY instead of adding another retry case at drm.
> 
> 
> > b) or into intel_dp_aux_transfer in intel_dp.c
> 
> Oh, I thought you had nacked this option.
> 
> > 
> > Option a) is the right one if this is a generic sink issue (and it 
> > seems
> > to be the case, at least for edp panels). Option b) if it's an issue 
> > with
> > our hw. Either way I think intel_dp_dpcd_read_wake should die.
> 
> Well, Jani and Ville kind of nacked this while we don't understand why
> this was ever introduced at first place.
> Although I believe with the proper EBUSY returns in place and 32 times
> retry I believe we could safely remove this as I tried on that series.
> 
> > 
> > On a personal gut level I'd go with option a).
> 
> So, EBUSY is ok or should we create another case on drm level?

Well, what I'd really like is to get rid of our read_wake code, since
pretty obviously it's a hack we seem to need everywhere. If the EBUSY
trick will allow us to do that I'm all for it.
-Daniel
-- 
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: Use dpcd read wake for sink crc calls.

[Daniel Vetter]

On Thu, Aug 20, 2015 at 04:12:00PM -0700, Rodrigo Vivi wrote:
> From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
> 
> Let's use a native read with retry as suggested per spec to
> fix Sink CRC on SKL when PSR is enabled.
> 
> With PSR enabled panel is probably taking more time to wake
> and dpcd read is faling.
> 
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>

Seems like we should just move the trickery we do in our own version into
the dp helpers in the core if this is needed all over the place?

At least in i915 we use it everywhere and it doesn't seem actively harmful
really ... Maybe the only exception would be the i2c-over-dp_aux code.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d32ce48..34f5e33 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
>  	u8 buf;
>  	int ret = 0;
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK,
> +				    &buf, 1) < 0) {
>  		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
>  		ret = -EIO;
>  		goto out;
> @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  			return ret;
>  	}
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK_MISC,
> +				    &buf, 1) < 0)
>  		return -EIO;
>  
>  	if (!(buf & DP_TEST_CRC_SUPPORTED))
> @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
>  
>  	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, &buf, 1) < 0)
>  		return -EIO;
>  
>  	hsw_disable_ips(intel_crtc);
> @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	do {
>  		intel_wait_for_vblank(dev, intel_crtc->pipe);
>  
> -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> -				      DP_TEST_SINK_MISC, &buf) < 0) {
> +		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> +					    DP_TEST_SINK_MISC, &buf, 1) < 0) {
>  			ret = -EIO;
>  			goto stop;
>  		}
> @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  		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) {
> +		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_CRC_R_CR,
> +					    crc, 6) < 0) {
>  			ret = -EIO;
>  			goto stop;
>  		}
> -- 
> 2.1.0
> 
> _______________________________________________
> 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] drm/i915: Use dpcd read wake for sink crc calls.

[Vivi, Rodrigo]

On Wed, 2015-08-26 at 11:06 +0200, Daniel Vetter wrote:
> On Thu, Aug 20, 2015 at 04:12:00PM -0700, Rodrigo Vivi wrote:
> > From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
> > 
> > Let's use a native read with retry as suggested per spec to
> > fix Sink CRC on SKL when PSR is enabled.
> > 
> > With PSR enabled panel is probably taking more time to wake
> > and dpcd read is faling.
> > 
> > Cc: Sonika Jindal <sonika.jindal@intel.com>
> > Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
> 
> Seems like we should just move the trickery we do in our own version 
> into
> the dp helpers in the core if this is needed all over the place?

I've wondered this, but I thought there was a good reason to let this
trick separated.

> At least in i915 we use it everywhere and it doesn't seem actively 
> harmful
> really ... Maybe the only exception would be the i2c-over-dp_aux 
> code.

Why this would be the exception? Maybe this was the good reason?


> -Daniel
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index d32ce48..34f5e33 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct 
> > intel_dp *intel_dp)
> >  	u8 buf;
> >  	int ret = 0;
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) 
> > < 0) {
> > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK,
> > +				    &buf, 1) < 0) {
> >  		DRM_DEBUG_KMS("Sink CRC couldn't be stopped 
> > properly\n");
> >  		ret = -EIO;
> >  		goto out;
> > @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct 
> > intel_dp *intel_dp)
> >  			return ret;
> >  	}
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, 
> > &buf) < 0)
> > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> > DP_TEST_SINK_MISC,
> > +				    &buf, 1) < 0)
> >  		return -EIO;
> >  
> >  	if (!(buf & DP_TEST_CRC_SUPPORTED))
> > @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct 
> > intel_dp *intel_dp)
> >  
> >  	intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK;
> >  
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) 
> > < 0)
> > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, 
> > &buf, 1) < 0)
> >  		return -EIO;
> >  
> >  	hsw_disable_ips(intel_crtc);
> > @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp 
> > *intel_dp, u8 *crc)
> >  	do {
> >  		intel_wait_for_vblank(dev, intel_crtc->pipe);
> >  
> > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > -				      DP_TEST_SINK_MISC, &buf) < 
> > 0) {
> > +		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > +					    DP_TEST_SINK_MISC, 
> > &buf, 1) < 0) {
> >  			ret = -EIO;
> >  			goto stop;
> >  		}
> > @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp 
> > *intel_dp, u8 *crc)
> >  		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) {
> > +		if (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> > DP_TEST_CRC_R_CR,
> > +					    crc, 6) < 0) {
> >  			ret = -EIO;
> >  			goto 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] drm/i915: Use dpcd read wake for sink crc calls.

[Daniel Vetter]

On Wed, Aug 26, 2015 at 6:41 PM, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote:
> On Wed, 2015-08-26 at 11:06 +0200, Daniel Vetter wrote:
>> On Thu, Aug 20, 2015 at 04:12:00PM -0700, Rodrigo Vivi wrote:
>> > From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
>> >
>> > Let's use a native read with retry as suggested per spec to
>> > fix Sink CRC on SKL when PSR is enabled.
>> >
>> > With PSR enabled panel is probably taking more time to wake
>> > and dpcd read is faling.
>> >
>> > Cc: Sonika Jindal <sonika.jindal@intel.com>
>> > Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
>>
>> Seems like we should just move the trickery we do in our own version
>> into
>> the dp helpers in the core if this is needed all over the place?
>
> I've wondered this, but I thought there was a good reason to let this
> trick separated.

I think in general you can assume that if i915 dp sink handling is
special it's because we have more testing on various broken hw out
there.

>> At least in i915 we use it everywhere and it doesn't seem actively
>> harmful
>> really ... Maybe the only exception would be the i2c-over-dp_aux
>> code.
>
> Why this would be the exception? Maybe this was the good reason?

I'd be fairly easy to keep an internal __drm_dp_aux_read (need it
anyway to implement this trick) and use that in i2c. At least that's
what I'd do without any evidence that we need to make this wake dance
also for i2c transactions. i2c uses a special dp-aux mode on the wire,
so makes some sense if it's different. See also the recent work from
Ville to tune the i2c dp-aux timeouts and retries, it really seems to
be a world of its own a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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: Use dpcd read wake for sink crc calls.

[Jani Nikula]

On Thu, 27 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 26, 2015 at 6:41 PM, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote:
>> On Wed, 2015-08-26 at 11:06 +0200, Daniel Vetter wrote:
>>> On Thu, Aug 20, 2015 at 04:12:00PM -0700, Rodrigo Vivi wrote:
>>> > From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
>>> >
>>> > Let's use a native read with retry as suggested per spec to
>>> > fix Sink CRC on SKL when PSR is enabled.
>>> >
>>> > With PSR enabled panel is probably taking more time to wake
>>> > and dpcd read is faling.
>>> >
>>> > Cc: Sonika Jindal <sonika.jindal@intel.com>
>>> > Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
>>>
>>> Seems like we should just move the trickery we do in our own version
>>> into
>>> the dp helpers in the core if this is needed all over the place?
>>
>> I've wondered this, but I thought there was a good reason to let this
>> trick separated.
>
> I think in general you can assume that if i915 dp sink handling is
> special it's because we have more testing on various broken hw out
> there.

In truth our inconsistent use of wake vs. non-wake can be mostly
attributed to the fact that we're clueless about sink sleep states, and
we've just added more wakes here and there to paper over it.

BR,
Jani.

>
>>> At least in i915 we use it everywhere and it doesn't seem actively
>>> harmful
>>> really ... Maybe the only exception would be the i2c-over-dp_aux
>>> code.
>>
>> Why this would be the exception? Maybe this was the good reason?
>
> I'd be fairly easy to keep an internal __drm_dp_aux_read (need it
> anyway to implement this trick) and use that in i2c. At least that's
> what I'd do without any evidence that we need to make this wake dance
> also for i2c transactions. i2c uses a special dp-aux mode on the wire,
> so makes some sense if it's different. See also the recent work from
> Ville to tune the i2c dp-aux timeouts and retries, it really seems to
> be a world of its own a bit.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Retry on every aux read.

[Rodrigo Vivi]

We have an inconsistency on our code on using intel_dp_dpcd_read_wake with
retries and when using drm_dp_dpcd_read helper without retry.

Since the retries help in many cases let's be consistent and be on
the safe side retrying on every aux read, including i2c ones.

So we can kill the intel_dp_dpcd_read function and keep code consistent.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 09bdd94..46f3c2d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -944,7 +944,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
 	uint8_t txbuf[20], rxbuf[20];
 	size_t txsize, rxsize;
-	int ret;
+	int ret, i;
 
 	txbuf[0] = (msg->request << 4) |
 		((msg->address >> 16) & 0xf);
@@ -986,17 +986,27 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		if (WARN_ON(rxsize > 20))
 			return -E2BIG;
 
-		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
-		if (ret > 0) {
-			msg->reply = rxbuf[0] >> 4;
-			/*
-			 * Assume happy day, and copy the data. The caller is
-			 * expected to check msg->reply before touching it.
-			 *
-			 * Return payload size.
-			 */
-			ret--;
-			memcpy(msg->buffer, rxbuf + 1, ret);
+		/*
+		 * Read with retry for link status and receiver capability reads
+		 * for cases where the sink may still be asleep.
+		 * Sinks are *supposed* to come up within 1ms from an off state,
+		 * but we're also supposed to retry 3 times per the spec.
+		 */
+		for (i = 0, ret = 0; ret <= 0 && i < 3; i++) {
+			ret = intel_dp_aux_ch(intel_dp, txbuf, txsize,
+					      rxbuf, rxsize);
+			if (ret > 0) {
+				msg->reply = rxbuf[0] >> 4;
+				/*
+				 * Assume happy day, and copy the data. The caller is
+				 * expected to check msg->reply before touching it.
+				 *
+				 * Return payload size.
+				 */
+				ret--;
+				memcpy(msg->buffer, rxbuf + 1, ret);
+			} else
+				msleep(1);
 		}
 		break;
 
@@ -3012,47 +3022,16 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
 }
 
 /*
- * Native read with retry for link status and receiver capability reads for
- * cases where the sink may still be asleep.
- *
- * Sinks are *supposed* to come up within 1ms from an off state, but we're also
- * supposed to retry 3 times per the spec.
- */
-static ssize_t
-intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
-			void *buffer, size_t size)
-{
-	ssize_t ret;
-	int i;
-
-	/*
-	 * Sometime we just get the same incorrect byte repeated
-	 * over the entire buffer. Doing just one throw away read
-	 * initially seems to "solve" it.
-	 */
-	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
-
-	for (i = 0; i < 3; i++) {
-		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
-		if (ret == size)
-			return ret;
-		msleep(1);
-	}
-
-	return ret;
-}
-
-/*
  * Fetch AUX CH registers 0x202 - 0x207 which contain
  * link status information
  */
 static bool
 intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
-	return intel_dp_dpcd_read_wake(&intel_dp->aux,
-				       DP_LANE0_1_STATUS,
-				       link_status,
-				       DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
+	return drm_dp_dpcd_read(&intel_dp->aux,
+				DP_LANE0_1_STATUS,
+				link_status,
+				DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
 }
 
 /* These are source-specific values. */
@@ -3979,8 +3958,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint8_t rev;
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
-				    sizeof(intel_dp->dpcd)) < 0)
+	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
+			     sizeof(intel_dp->dpcd)) < 0)
 		return false; /* aux transfer failed */
 
 	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd);
@@ -3991,9 +3970,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
-		intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
-					intel_dp->psr_dpcd,
-					sizeof(intel_dp->psr_dpcd));
+		drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT,
+				 intel_dp->psr_dpcd,
+				 sizeof(intel_dp->psr_dpcd));
 		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
 			dev_priv->psr.sink_support = true;
 			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
@@ -4004,9 +3983,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 			uint8_t frame_sync_cap;
 
 			dev_priv->psr.sink_support = true;
-			intel_dp_dpcd_read_wake(&intel_dp->aux,
-					DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
-					&frame_sync_cap, 1);
+			drm_dp_dpcd_readb(&intel_dp->aux,
+					  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
+					  &frame_sync_cap);
 			dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
 			/* PSR2 needs frame sync as well */
 			dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
@@ -4022,15 +4001,15 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	/* Intermediate frequency support */
 	if (is_edp(intel_dp) &&
 	    (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &	DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
-	    (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
+	    (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DPCD_REV, &rev) == 1) &&
 	    (rev >= 0x03)) { /* eDp v1.4 or higher */
 		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
 		int i;
 
-		intel_dp_dpcd_read_wake(&intel_dp->aux,
-				DP_SUPPORTED_LINK_RATES,
-				sink_rates,
-				sizeof(sink_rates));
+		drm_dp_dpcd_read(&intel_dp->aux,
+				 DP_SUPPORTED_LINK_RATES,
+				 sink_rates,
+				 sizeof(sink_rates));
 
 		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
 			int val = le16_to_cpu(sink_rates[i]);
@@ -4053,9 +4032,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
 		return true; /* no per-port downstream info */
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
-				    intel_dp->downstream_ports,
-				    DP_MAX_DOWNSTREAM_PORTS) < 0)
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
+			     intel_dp->downstream_ports,
+			     DP_MAX_DOWNSTREAM_PORTS) < 0)
 		return false; /* downstream port status fetch failed */
 
 	return true;
@@ -4069,11 +4048,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
 		return;
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 }
@@ -4089,7 +4068,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
 		return false;
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, buf)) {
 		if (buf[0] & DP_MST_CAP) {
 			DRM_DEBUG_KMS("Sink is MST capable\n");
 			intel_dp->is_mst = true;
@@ -4228,9 +4207,9 @@ stop:
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
-	return intel_dp_dpcd_read_wake(&intel_dp->aux,
-				       DP_DEVICE_SERVICE_IRQ_VECTOR,
-				       sink_irq_vector, 1) == 1;
+	return drm_dp_dpcd_readb(&intel_dp->aux,
+				 DP_DEVICE_SERVICE_IRQ_VECTOR,
+				 sink_irq_vector) == 1;
 }
 
 static bool
@@ -4238,9 +4217,9 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
 	int ret;
 
-	ret = intel_dp_dpcd_read_wake(&intel_dp->aux,
-					     DP_SINK_COUNT_ESI,
-					     sink_irq_vector, 14);
+	ret = drm_dp_dpcd_read(&intel_dp->aux,
+			       DP_SINK_COUNT_ESI,
+			       sink_irq_vector, 14);
 	if (ret != 14)
 		return false;
 
@@ -4496,8 +4475,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
 		uint8_t reg;
 
-		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &reg, 1) < 0)
+		if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &reg) < 0)
 			return connector_status_unknown;
 
 		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
-- 
2.4.3

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

Re: [PATCH] drm/i915: Retry on every aux read.

[Jani Nikula]

On Tue, 20 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> We have an inconsistency on our code on using intel_dp_dpcd_read_wake with
> retries and when using drm_dp_dpcd_read helper without retry.

We're supposed to do the retries when the sink may be in a power down
state. We're not very good at tracking that, and we've cargo culted the
retry variants all over the place without thinking. This is why we're
inconsistent.

> Since the retries help in many cases let's be consistent and be on
> the safe side retrying on every aux read, including i2c ones.

Please let's not add superfluous retries at different levels of the
stack just to be safe. It's like a variant of the C programmer's
disease.

If the retries really help [citation needed] we need to figure out what
we're doing wrong and where, and preferrably fix this at the DP helper
level for all drivers, if possible.

Other comments in-line.

>
> So we can kill the intel_dp_dpcd_read function and keep code consistent.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 126 +++++++++++++++++-----------------------
>  1 file changed, 52 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 09bdd94..46f3c2d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -944,7 +944,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  	struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
>  	uint8_t txbuf[20], rxbuf[20];
>  	size_t txsize, rxsize;
> -	int ret;
> +	int ret, i;
>  
>  	txbuf[0] = (msg->request << 4) |
>  		((msg->address >> 16) & 0xf);
> @@ -986,17 +986,27 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  		if (WARN_ON(rxsize > 20))
>  			return -E2BIG;
>  
> -		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
> -		if (ret > 0) {
> -			msg->reply = rxbuf[0] >> 4;
> -			/*
> -			 * Assume happy day, and copy the data. The caller is
> -			 * expected to check msg->reply before touching it.
> -			 *
> -			 * Return payload size.
> -			 */
> -			ret--;
> -			memcpy(msg->buffer, rxbuf + 1, ret);
> +		/*
> +		 * Read with retry for link status and receiver capability reads
> +		 * for cases where the sink may still be asleep.
> +		 * Sinks are *supposed* to come up within 1ms from an off state,
> +		 * but we're also supposed to retry 3 times per the spec.
> +		 */

The comment would need an update.

> +		for (i = 0, ret = 0; ret <= 0 && i < 3; i++) {
> +			ret = intel_dp_aux_ch(intel_dp, txbuf, txsize,
> +					      rxbuf, rxsize);
> +			if (ret > 0) {
> +				msg->reply = rxbuf[0] >> 4;
> +				/*
> +				 * Assume happy day, and copy the data. The caller is
> +				 * expected to check msg->reply before touching it.
> +				 *
> +				 * Return payload size.
> +				 */
> +				ret--;
> +				memcpy(msg->buffer, rxbuf + 1, ret);
> +			} else
> +				msleep(1);
>  		}
>  		break;
>  
> @@ -3012,47 +3022,16 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
>  }
>  
>  /*
> - * Native read with retry for link status and receiver capability reads for
> - * cases where the sink may still be asleep.
> - *
> - * Sinks are *supposed* to come up within 1ms from an off state, but we're also
> - * supposed to retry 3 times per the spec.
> - */
> -static ssize_t
> -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> -			void *buffer, size_t size)
> -{
> -	ssize_t ret;
> -	int i;
> -
> -	/*
> -	 * Sometime we just get the same incorrect byte repeated
> -	 * over the entire buffer. Doing just one throw away read
> -	 * initially seems to "solve" it.
> -	 */
> -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);

See

commit f6a1906674005377b64ee5431c1418077c1b2425
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Thu Oct 16 20:46:09 2014 +0300

    drm/i915: Do a dummy DPCD read before the actual read



> -
> -	for (i = 0; i < 3; i++) {
> -		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> -		if (ret == size)
> -			return ret;
> -		msleep(1);
> -	}
> -
> -	return ret;
> -}
> -
> -/*
>   * Fetch AUX CH registers 0x202 - 0x207 which contain
>   * link status information
>   */
>  static bool
>  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
>  {
> -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> -				       DP_LANE0_1_STATUS,
> -				       link_status,
> -				       DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> +	return drm_dp_dpcd_read(&intel_dp->aux,
> +				DP_LANE0_1_STATUS,
> +				link_status,
> +				DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
>  }
>  
>  /* These are source-specific values. */
> @@ -3979,8 +3958,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint8_t rev;
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
> -				    sizeof(intel_dp->dpcd)) < 0)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
> +			     sizeof(intel_dp->dpcd)) < 0)
>  		return false; /* aux transfer failed */
>  
>  	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd);
> @@ -3991,9 +3970,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	/* Check if the panel supports PSR */
>  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>  	if (is_edp(intel_dp)) {
> -		intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
> -					intel_dp->psr_dpcd,
> -					sizeof(intel_dp->psr_dpcd));
> +		drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT,
> +				 intel_dp->psr_dpcd,
> +				 sizeof(intel_dp->psr_dpcd));
>  		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>  			dev_priv->psr.sink_support = true;
>  			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> @@ -4004,9 +3983,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  			uint8_t frame_sync_cap;
>  
>  			dev_priv->psr.sink_support = true;
> -			intel_dp_dpcd_read_wake(&intel_dp->aux,
> -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> -					&frame_sync_cap, 1);
> +			drm_dp_dpcd_readb(&intel_dp->aux,
> +					  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> +					  &frame_sync_cap);
>  			dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
>  			/* PSR2 needs frame sync as well */
>  			dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> @@ -4022,15 +4001,15 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	/* Intermediate frequency support */
>  	if (is_edp(intel_dp) &&
>  	    (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &	DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
> -	    (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> +	    (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DPCD_REV, &rev) == 1) &&
>  	    (rev >= 0x03)) { /* eDp v1.4 or higher */
>  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
>  		int i;
>  
> -		intel_dp_dpcd_read_wake(&intel_dp->aux,
> -				DP_SUPPORTED_LINK_RATES,
> -				sink_rates,
> -				sizeof(sink_rates));
> +		drm_dp_dpcd_read(&intel_dp->aux,
> +				 DP_SUPPORTED_LINK_RATES,
> +				 sink_rates,
> +				 sizeof(sink_rates));
>  
>  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
>  			int val = le16_to_cpu(sink_rates[i]);
> @@ -4053,9 +4032,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
>  		return true; /* no per-port downstream info */
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> -				    intel_dp->downstream_ports,
> -				    DP_MAX_DOWNSTREAM_PORTS) < 0)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> +			     intel_dp->downstream_ports,
> +			     DP_MAX_DOWNSTREAM_PORTS) < 0)
>  		return false; /* downstream port status fetch failed */
>  
>  	return true;
> @@ -4069,11 +4048,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>  	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
>  		return;
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
>  		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>  			      buf[0], buf[1], buf[2]);
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
>  		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>  			      buf[0], buf[1], buf[2]);
>  }
> @@ -4089,7 +4068,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
>  		return false;
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, buf)) {
>  		if (buf[0] & DP_MST_CAP) {
>  			DRM_DEBUG_KMS("Sink is MST capable\n");
>  			intel_dp->is_mst = true;
> @@ -4228,9 +4207,9 @@ stop:
>  static bool
>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> -				       DP_DEVICE_SERVICE_IRQ_VECTOR,
> -				       sink_irq_vector, 1) == 1;
> +	return drm_dp_dpcd_readb(&intel_dp->aux,
> +				 DP_DEVICE_SERVICE_IRQ_VECTOR,
> +				 sink_irq_vector) == 1;
>  }
>  
>  static bool
> @@ -4238,9 +4217,9 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
>  	int ret;
>  
> -	ret = intel_dp_dpcd_read_wake(&intel_dp->aux,
> -					     DP_SINK_COUNT_ESI,
> -					     sink_irq_vector, 14);
> +	ret = drm_dp_dpcd_read(&intel_dp->aux,
> +			       DP_SINK_COUNT_ESI,
> +			       sink_irq_vector, 14);
>  	if (ret != 14)
>  		return false;
>  
> @@ -4496,8 +4475,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>  		uint8_t reg;
>  
> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> -					    &reg, 1) < 0)
> +		if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &reg) < 0)
>  			return connector_status_unknown;
>  
>  		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
> -- 
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Retry on every aux read.

[Daniel Vetter]

On Tue, Oct 20, 2015 at 10:02:21AM +0300, Jani Nikula wrote:
> On Tue, 20 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > We have an inconsistency on our code on using intel_dp_dpcd_read_wake with
> > retries and when using drm_dp_dpcd_read helper without retry.
> 
> We're supposed to do the retries when the sink may be in a power down
> state. We're not very good at tracking that, and we've cargo culted the
> retry variants all over the place without thinking. This is why we're
> inconsistent.
> 
> > Since the retries help in many cases let's be consistent and be on
> > the safe side retrying on every aux read, including i2c ones.
> 
> Please let's not add superfluous retries at different levels of the
> stack just to be safe. It's like a variant of the C programmer's
> disease.
> 
> If the retries really help [citation needed] we need to figure out what
> we're doing wrong and where, and preferrably fix this at the DP helper
> level for all drivers, if possible.

Yeah, same comment as I've done last time around. If we need this, we need
to do this in core dp helpers.
-Daniel
-- 
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: Retry on every aux read.

[Vivi, Rodrigo]

On Tue, 2015-10-20 at 09:39 +0200, Daniel Vetter wrote:
> On Tue, Oct 20, 2015 at 10:02:21AM +0300, Jani Nikula wrote:
> > On Tue, 20 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > We have an inconsistency on our code on using 
> > > intel_dp_dpcd_read_wake with
> > > retries and when using drm_dp_dpcd_read helper without retry.
> > 
> > We're supposed to do the retries when the sink may be in a power 
> > down
> > state. We're not very good at tracking that, and we've cargo culted 
> > the
> > retry variants all over the place without thinking. This is why 
> > we're
> > inconsistent.
> > 
> > > Since the retries help in many cases let's be consistent and be 
> > > on
> > > the safe side retrying on every aux read, including i2c ones.
> > 
> > Please let's not add superfluous retries at different levels of the
> > stack just to be safe. It's like a variant of the C programmer's
> > disease.

I'm not the one who is adding, but just being consistent on our side. 

> > 
> > If the retries really help [citation needed] we need to figure out 
> > what
> > we're doing wrong and where, and preferrably fix this at the DP 
> > helper
> > level for all drivers, if possible.

Yes, I'm sorry, I forgot to copy the citation from the first patch in
this thread that mentions this fix sink crc for Skylake... and also for
some Broadwells what made me to remind about this patch....

> 
> Yeah, same comment as I've done last time around. If we need this, we 
> need
> to do this in core dp helpers.

If it is "superfluous" and "disease" to make the consistence in our
driver I can't imagine the words that I'm going to hear if I try to
touch other drivers...

To see if we should actually do this in our level or on the drm level I
was trying to understand where does:
" Sinks are *supposed* to come up within 1ms from an off state, but
we're also- * supposed to retry 3 times per the spec."

But I couldn't find anything in Vesa spec that tells that neither on
BSPec... at BSpec for some platforms I found the 3 times retry for aux
ctl programming what we do already...

So, yeah, I'm not convinced that we should through this to another
level... So it is either this or the first one that fix the sink crc
case individually and let sink crc reliable...

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

Re: [PATCH] drm/i915: Retry on every aux read.

[Vivi, Rodrigo]

On Tue, 2015-10-20 at 08:38 -0700, Rodrigo Vivi wrote:
> On Tue, 2015-10-20 at 09:39 +0200, Daniel Vetter wrote:
> > On Tue, Oct 20, 2015 at 10:02:21AM +0300, Jani Nikula wrote:
> > > On Tue, 20 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > > We have an inconsistency on our code on using 
> > > > intel_dp_dpcd_read_wake with
> > > > retries and when using drm_dp_dpcd_read helper without retry.
> > > 
> > > We're supposed to do the retries when the sink may be in a power 
> > > down
> > > state. We're not very good at tracking that, and we've cargo 
> > > culted 
> > > the
> > > retry variants all over the place without thinking. This is why 
> > > we're
> > > inconsistent.
> > > 
> > > > Since the retries help in many cases let's be consistent and be 
> > > > on
> > > > the safe side retrying on every aux read, including i2c ones.
> > > 
> > > Please let's not add superfluous retries at different levels of 
> > > the
> > > stack just to be safe. It's like a variant of the C programmer's
> > > disease.
> 
> I'm not the one who is adding, but just being consistent on our side. 
> 
> > > 
> > > If the retries really help [citation needed] we need to figure 
> > > out 
> > > what
> > > we're doing wrong and where, and preferrably fix this at the DP 
> > > helper
> > > level for all drivers, if possible.
> 
> Yes, I'm sorry, I forgot to copy the citation from the first patch in
> this thread that mentions this fix sink crc for Skylake... and also 
> for
> some Broadwells what made me to remind about this patch....
> 
> > 
> > Yeah, same comment as I've done last time around. If we need this, 
> > we 
> > need
> > to do this in core dp helpers.
> 
> If it is "superfluous" and "disease" to make the consistence in our
> driver I can't imagine the words that I'm going to hear if I try to
> touch other drivers...
> 
> To see if we should actually do this in our level or on the drm level 
> I
> was trying to understand where does:
> " Sinks are *supposed* to come up within 1ms from an off state, but
> we're also- * supposed to retry 3 times per the spec."
> 
> But I couldn't find anything in Vesa spec that tells that neither on
> BSPec... at BSpec for some platforms I found the 3 times retry for 
> aux
> ctl programming what we do already...

Besides, I noticed on drm level we have 32 times retry already but when
aux return -EBUSY... 

So I wonder if it would be acceptable to retry for all errors not only
for -EBUSY or if we should find out when to return EBUSY propperly on
our aux transfer...

> 
> So, yeah, I'm not convinced that we should through this to another
> level... So it is either this or the first one that fix the sink crc
> case individually and let sink crc reliable...
> 
> > -Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Retry on every aux read.

[Daniel Vetter]

On Tue, Oct 20, 2015 at 05:45:29PM +0000, Vivi, Rodrigo wrote:
> On Tue, 2015-10-20 at 08:38 -0700, Rodrigo Vivi wrote:
> > On Tue, 2015-10-20 at 09:39 +0200, Daniel Vetter wrote:
> > > On Tue, Oct 20, 2015 at 10:02:21AM +0300, Jani Nikula wrote:
> > > > On Tue, 20 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > > > We have an inconsistency on our code on using 
> > > > > intel_dp_dpcd_read_wake with
> > > > > retries and when using drm_dp_dpcd_read helper without retry.
> > > > 
> > > > We're supposed to do the retries when the sink may be in a power 
> > > > down
> > > > state. We're not very good at tracking that, and we've cargo 
> > > > culted 
> > > > the
> > > > retry variants all over the place without thinking. This is why 
> > > > we're
> > > > inconsistent.
> > > > 
> > > > > Since the retries help in many cases let's be consistent and be 
> > > > > on
> > > > > the safe side retrying on every aux read, including i2c ones.
> > > > 
> > > > Please let's not add superfluous retries at different levels of 
> > > > the
> > > > stack just to be safe. It's like a variant of the C programmer's
> > > > disease.
> > 
> > I'm not the one who is adding, but just being consistent on our side. 
> > 
> > > > 
> > > > If the retries really help [citation needed] we need to figure 
> > > > out 
> > > > what
> > > > we're doing wrong and where, and preferrably fix this at the DP 
> > > > helper
> > > > level for all drivers, if possible.
> > 
> > Yes, I'm sorry, I forgot to copy the citation from the first patch in
> > this thread that mentions this fix sink crc for Skylake... and also 
> > for
> > some Broadwells what made me to remind about this patch....
> > 
> > > 
> > > Yeah, same comment as I've done last time around. If we need this, 
> > > we 
> > > need
> > > to do this in core dp helpers.
> > 
> > If it is "superfluous" and "disease" to make the consistence in our
> > driver I can't imagine the words that I'm going to hear if I try to
> > touch other drivers...
> > 
> > To see if we should actually do this in our level or on the drm level 
> > I
> > was trying to understand where does:
> > " Sinks are *supposed* to come up within 1ms from an off state, but
> > we're also- * supposed to retry 3 times per the spec."
> > 
> > But I couldn't find anything in Vesa spec that tells that neither on
> > BSPec... at BSpec for some platforms I found the 3 times retry for 
> > aux
> > ctl programming what we do already...
> 
> Besides, I noticed on drm level we have 32 times retry already but when
> aux return -EBUSY... 
> 
> So I wonder if it would be acceptable to retry for all errors not only
> for -EBUSY or if we should find out when to return EBUSY propperly on
> our aux transfer...

I thought we only return -EBUSY when the dp aux hw controlled died, otoh
maybe it should be -EIO in that case. It's a bit unclear, which is why I
think trying to clarify this would be useful.
-Daniel
-- 
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