* [PATCH 1/4] drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER
2013-09-20 13:42 [PATCH 0/4] drm/i915/dp: aux and dpcd fixes Jani Nikula
@ 2013-09-20 13:42 ` Jani Nikula
2013-09-20 20:38 ` Todd Previte
2013-09-20 13:42 ` [PATCH 2/4] drm/i915/dp: increase i2c-over-aux retry interval " Jani Nikula
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2013-09-20 13:42 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
Per DP1.2 spec.
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9770160..6626514 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -654,7 +654,12 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
break;
}
- for (retry = 0; retry < 5; retry++) {
+ /*
+ * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device is
+ * required to retry at least seven times upon receiving AUX_DEFER
+ * before giving up the AUX transaction.
+ */
+ for (retry = 0; retry < 7; retry++) {
ret = intel_dp_aux_ch(intel_dp,
msg, msg_bytes,
reply, reply_bytes);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/4] drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER
2013-09-20 13:42 ` [PATCH 1/4] drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER Jani Nikula
@ 2013-09-20 20:38 ` Todd Previte
2013-09-24 6:39 ` Jani Nikula
2013-09-27 11:51 ` Jani Nikula
0 siblings, 2 replies; 18+ messages in thread
From: Todd Previte @ 2013-09-20 20:38 UTC (permalink / raw)
To: intel-gfx
On 09/20/2013 06:42 AM, Jani Nikula wrote:
> Per DP1.2 spec.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9770160..6626514 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -654,7 +654,12 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
> break;
> }
>
> - for (retry = 0; retry < 5; retry++) {
> + /*
> + * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device is
> + * required to retry at least seven times upon receiving AUX_DEFER
> + * before giving up the AUX transaction.
> + */
> + for (retry = 0; retry < 7; retry++) {
> ret = intel_dp_aux_ch(intel_dp,
> msg, msg_bytes,
> reply, reply_bytes);
>
Hey Jani,
Although it's not explicitly stated in the specification (I went through
both the 1.1a and 1.2a specifications and couldn't find it), I think the
the retry count of 7 applies to all AUX transactions, both native and
I2C. That's alluded to in the description of the link training sequence
(pg 382, 2nd paragraph from the bottom) where the sink may defer up to
seven times before the source can abort training.
With that in mind, it might be a good idea to use a defined constant for
the retry count for both native and I2C AUX transactions. That would
keep it consistent throughout the code and be easier to update moving
forward should the specification change.
-T
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/4] drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER
2013-09-20 20:38 ` Todd Previte
@ 2013-09-24 6:39 ` Jani Nikula
2013-09-27 11:51 ` Jani Nikula
1 sibling, 0 replies; 18+ messages in thread
From: Jani Nikula @ 2013-09-24 6:39 UTC (permalink / raw)
To: Todd Previte, intel-gfx
Hi Todd -
On Fri, 20 Sep 2013, Todd Previte <tprevite@gmail.com> wrote:
> On 09/20/2013 06:42 AM, Jani Nikula wrote:
>> Per DP1.2 spec.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 9770160..6626514 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -654,7 +654,12 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
>> break;
>> }
>>
>> - for (retry = 0; retry < 5; retry++) {
>> + /*
>> + * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device is
>> + * required to retry at least seven times upon receiving AUX_DEFER
>> + * before giving up the AUX transaction.
>> + */
>> + for (retry = 0; retry < 7; retry++) {
>> ret = intel_dp_aux_ch(intel_dp,
>> msg, msg_bytes,
>> reply, reply_bytes);
>>
> Hey Jani,
>
> Although it's not explicitly stated in the specification (I went through
> both the 1.1a and 1.2a specifications and couldn't find it), I think the
> the retry count of 7 applies to all AUX transactions, both native and
> I2C. That's alluded to in the description of the link training sequence
> (pg 382, 2nd paragraph from the bottom) where the sink may defer up to
> seven times before the source can abort training.
>
> With that in mind, it might be a good idea to use a defined constant for
> the retry count for both native and I2C AUX transactions. That would
> keep it consistent throughout the code and be easier to update moving
> forward should the specification change.
I agree in principle; defines good, magic values bad.
However we don't currently have a retry max for AUX DEFER in native
transactions. Maybe we should. But I'm wary of adding 7 there, because
we don't have a max now, and the spec is vague on this. It just might
break stuff.
Also, for i2c over AUX the retry count of 7 is the minimum by the
spec. And it goes on to say that depending on the moon position and some
other variables, like i2c bit rate, it is recommended the source adjusts
the retry count and interval.
So the retry count currently isn't the same in our code, and I don't
think it's reasonable to force them to be the same either.
BR,
Jani.
>
>
> -T
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/4] drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER
2013-09-20 20:38 ` Todd Previte
2013-09-24 6:39 ` Jani Nikula
@ 2013-09-27 11:51 ` Jani Nikula
2013-09-27 19:07 ` Todd Previte
1 sibling, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2013-09-27 11:51 UTC (permalink / raw)
To: Todd Previte, intel-gfx
On Fri, 20 Sep 2013, Todd Previte <tprevite@gmail.com> wrote:
> On 09/20/2013 06:42 AM, Jani Nikula wrote:
>> Per DP1.2 spec.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 9770160..6626514 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -654,7 +654,12 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
>> break;
>> }
>>
>> - for (retry = 0; retry < 5; retry++) {
>> + /*
>> + * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device is
>> + * required to retry at least seven times upon receiving AUX_DEFER
>> + * before giving up the AUX transaction.
>> + */
>> + for (retry = 0; retry < 7; retry++) {
>> ret = intel_dp_aux_ch(intel_dp,
>> msg, msg_bytes,
>> reply, reply_bytes);
>>
> Hey Jani,
>
> Although it's not explicitly stated in the specification (I went through
> both the 1.1a and 1.2a specifications and couldn't find it), I think the
> the retry count of 7 applies to all AUX transactions, both native and
> I2C. That's alluded to in the description of the link training sequence
> (pg 382, 2nd paragraph from the bottom) where the sink may defer up to
> seven times before the source can abort training.
>
> With that in mind, it might be a good idea to use a defined constant for
> the retry count for both native and I2C AUX transactions. That would
> keep it consistent throughout the code and be easier to update moving
> forward should the specification change.
Todd, are you willing to give your reviewed-by on this one patch per our
irc discussion? We can do further cleanups later.
I've sent a new version of 3/4 with the #defines separately.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/4] drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER
2013-09-27 11:51 ` Jani Nikula
@ 2013-09-27 19:07 ` Todd Previte
2013-09-27 19:59 ` Daniel Vetter
0 siblings, 1 reply; 18+ messages in thread
From: Todd Previte @ 2013-09-27 19:07 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 2308 bytes --]
Yep. Good to go.
Reviewed-by: Todd Previte <tprevite@gmail.com>
On Fri, Sep 27, 2013 at 4:51 AM, Jani Nikula <jani.nikula@linux.intel.com>wrote:
> On Fri, 20 Sep 2013, Todd Previte <tprevite@gmail.com> wrote:
> > On 09/20/2013 06:42 AM, Jani Nikula wrote:
> >> Per DP1.2 spec.
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> >> index 9770160..6626514 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -654,7 +654,12 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter,
> int mode,
> >> break;
> >> }
> >>
> >> - for (retry = 0; retry < 5; retry++) {
> >> + /*
> >> + * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source
> device is
> >> + * required to retry at least seven times upon receiving AUX_DEFER
> >> + * before giving up the AUX transaction.
> >> + */
> >> + for (retry = 0; retry < 7; retry++) {
> >> ret = intel_dp_aux_ch(intel_dp,
> >> msg, msg_bytes,
> >> reply, reply_bytes);
> >>
> > Hey Jani,
> >
> > Although it's not explicitly stated in the specification (I went through
> > both the 1.1a and 1.2a specifications and couldn't find it), I think the
> > the retry count of 7 applies to all AUX transactions, both native and
> > I2C. That's alluded to in the description of the link training sequence
> > (pg 382, 2nd paragraph from the bottom) where the sink may defer up to
> > seven times before the source can abort training.
> >
> > With that in mind, it might be a good idea to use a defined constant for
> > the retry count for both native and I2C AUX transactions. That would
> > keep it consistent throughout the code and be easier to update moving
> > forward should the specification change.
>
> Todd, are you willing to give your reviewed-by on this one patch per our
> irc discussion? We can do further cleanups later.
>
> I've sent a new version of 3/4 with the #defines separately.
>
>
> BR,
> Jani.
>
>
>
> --
> Jani Nikula, Intel Open Source Technology Center
>
[-- Attachment #1.2: Type: text/html, Size: 3274 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/4] drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER
2013-09-27 19:07 ` Todd Previte
@ 2013-09-27 19:59 ` Daniel Vetter
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2013-09-27 19:59 UTC (permalink / raw)
To: Todd Previte; +Cc: intel-gfx
On Fri, Sep 27, 2013 at 12:07:29PM -0700, Todd Previte wrote:
> Yep. Good to go.
>
> Reviewed-by: Todd Previte <tprevite@gmail.com>
Queued for -next, thanks for the patch.
-Daniel
>
>
> On Fri, Sep 27, 2013 at 4:51 AM, Jani Nikula <jani.nikula@linux.intel.com>wrote:
>
> > On Fri, 20 Sep 2013, Todd Previte <tprevite@gmail.com> wrote:
> > > On 09/20/2013 06:42 AM, Jani Nikula wrote:
> > >> Per DP1.2 spec.
> > >>
> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > >> ---
> > >> drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
> > >> 1 file changed, 6 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > >> index 9770160..6626514 100644
> > >> --- a/drivers/gpu/drm/i915/intel_dp.c
> > >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> > >> @@ -654,7 +654,12 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter,
> > int mode,
> > >> break;
> > >> }
> > >>
> > >> - for (retry = 0; retry < 5; retry++) {
> > >> + /*
> > >> + * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source
> > device is
> > >> + * required to retry at least seven times upon receiving AUX_DEFER
> > >> + * before giving up the AUX transaction.
> > >> + */
> > >> + for (retry = 0; retry < 7; retry++) {
> > >> ret = intel_dp_aux_ch(intel_dp,
> > >> msg, msg_bytes,
> > >> reply, reply_bytes);
> > >>
> > > Hey Jani,
> > >
> > > Although it's not explicitly stated in the specification (I went through
> > > both the 1.1a and 1.2a specifications and couldn't find it), I think the
> > > the retry count of 7 applies to all AUX transactions, both native and
> > > I2C. That's alluded to in the description of the link training sequence
> > > (pg 382, 2nd paragraph from the bottom) where the sink may defer up to
> > > seven times before the source can abort training.
> > >
> > > With that in mind, it might be a good idea to use a defined constant for
> > > the retry count for both native and I2C AUX transactions. That would
> > > keep it consistent throughout the code and be easier to update moving
> > > forward should the specification change.
> >
> > Todd, are you willing to give your reviewed-by on this one patch per our
> > irc discussion? We can do further cleanups later.
> >
> > I've sent a new version of 3/4 with the #defines separately.
> >
> >
> > BR,
> > Jani.
> >
> >
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/4] drm/i915/dp: increase i2c-over-aux retry interval on AUX DEFER
2013-09-20 13:42 [PATCH 0/4] drm/i915/dp: aux and dpcd fixes Jani Nikula
2013-09-20 13:42 ` [PATCH 1/4] drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER Jani Nikula
@ 2013-09-20 13:42 ` Jani Nikula
2013-09-20 20:56 ` Todd Previte
2013-09-20 13:42 ` [PATCH 3/4] drm/i915/dp: downstream port capabilities are not present in DPCD 1.0 Jani Nikula
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2013-09-20 13:42 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
There is no clear cut rules or specs for the retry interval, as there
are many factors that affect overall response time. Increase the
interval, and even more so on branch devices which may have limited i2c
bit rates.
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6626514..3afbea9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -678,7 +678,18 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
DRM_DEBUG_KMS("aux_ch native nack\n");
return -EREMOTEIO;
case AUX_NATIVE_REPLY_DEFER:
- udelay(100);
+ /*
+ * For now, just give more slack to branch devices. We
+ * could check the DPCD for I2C bit rate capabilities,
+ * and if available, adjust the interval. We could also
+ * be more careful with DP-to-Legacy adapters where a
+ * long legacy cable may force very low I2C bit rates.
+ */
+ if (intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
+ DP_DWN_STRM_PORT_PRESENT)
+ usleep_range(500, 600);
+ else
+ usleep_range(300, 400);
continue;
default:
DRM_ERROR("aux_ch invalid native reply 0x%02x\n",
--
1.7.9.5
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 2/4] drm/i915/dp: increase i2c-over-aux retry interval on AUX DEFER
2013-09-20 13:42 ` [PATCH 2/4] drm/i915/dp: increase i2c-over-aux retry interval " Jani Nikula
@ 2013-09-20 20:56 ` Todd Previte
2013-09-20 21:53 ` Daniel Vetter
0 siblings, 1 reply; 18+ messages in thread
From: Todd Previte @ 2013-09-20 20:56 UTC (permalink / raw)
To: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 1539 bytes --]
On 09/20/2013 06:42 AM, Jani Nikula wrote:
> There is no clear cut rules or specs for the retry interval, as there
> are many factors that affect overall response time. Increase the
> interval, and even more so on branch devices which may have limited i2c
> bit rates.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6626514..3afbea9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -678,7 +678,18 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
> DRM_DEBUG_KMS("aux_ch native nack\n");
> return -EREMOTEIO;
> case AUX_NATIVE_REPLY_DEFER:
> - udelay(100);
> + /*
> + * For now, just give more slack to branch devices. We
> + * could check the DPCD for I2C bit rate capabilities,
> + * and if available, adjust the interval. We could also
> + * be more careful with DP-to-Legacy adapters where a
> + * long legacy cable may force very low I2C bit rates.
> + */
> + if (intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> + DP_DWN_STRM_PORT_PRESENT)
> + usleep_range(500, 600);
> + else
> + usleep_range(300, 400);
> continue;
> default:
> DRM_ERROR("aux_ch invalid native reply 0x%02x\n",
Those look like reasonable values to me.
[Reviewed-by]: Todd Previte <tprevite@gmail.com
<mailto:tprevite@gmail.com>>
[-- Attachment #1.2: Type: text/html, Size: 1988 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/4] drm/i915/dp: increase i2c-over-aux retry interval on AUX DEFER
2013-09-20 20:56 ` Todd Previte
@ 2013-09-20 21:53 ` Daniel Vetter
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2013-09-20 21:53 UTC (permalink / raw)
To: Todd Previte; +Cc: intel-gfx
On Fri, Sep 20, 2013 at 01:56:20PM -0700, Todd Previte wrote:
> On 09/20/2013 06:42 AM, Jani Nikula wrote:
> >There is no clear cut rules or specs for the retry interval, as there
> >are many factors that affect overall response time. Increase the
> >interval, and even more so on branch devices which may have limited i2c
> >bit rates.
> >
> >Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >---
> > drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >index 6626514..3afbea9 100644
> >--- a/drivers/gpu/drm/i915/intel_dp.c
> >+++ b/drivers/gpu/drm/i915/intel_dp.c
> >@@ -678,7 +678,18 @@ intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
> > DRM_DEBUG_KMS("aux_ch native nack\n");
> > return -EREMOTEIO;
> > case AUX_NATIVE_REPLY_DEFER:
> >- udelay(100);
> >+ /*
> >+ * For now, just give more slack to branch devices. We
> >+ * could check the DPCD for I2C bit rate capabilities,
> >+ * and if available, adjust the interval. We could also
> >+ * be more careful with DP-to-Legacy adapters where a
> >+ * long legacy cable may force very low I2C bit rates.
> >+ */
> >+ if (intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> >+ DP_DWN_STRM_PORT_PRESENT)
> >+ usleep_range(500, 600);
> >+ else
> >+ usleep_range(300, 400);
> > continue;
> > default:
> > DRM_ERROR("aux_ch invalid native reply 0x%02x\n",
>
> Those look like reasonable values to me.
>
> [Reviewed-by]: Todd Previte <tprevite@gmail.com
> <mailto:tprevite@gmail.com>>
Your mailer sends out html+plaintext multipart mails and in the plaintext
section your r-b tags look rather funny. Can you please fix that?
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/4] drm/i915/dp: downstream port capabilities are not present in DPCD 1.0
2013-09-20 13:42 [PATCH 0/4] drm/i915/dp: aux and dpcd fixes Jani Nikula
2013-09-20 13:42 ` [PATCH 1/4] drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER Jani Nikula
2013-09-20 13:42 ` [PATCH 2/4] drm/i915/dp: increase i2c-over-aux retry interval " Jani Nikula
@ 2013-09-20 13:42 ` Jani Nikula
2013-09-20 13:42 ` [PATCH 4/4] drm/i915/dp: read DPCD PSR capability only on eDP Jani Nikula
2013-09-23 5:59 ` [PATCH 0/4] drm/i915/dp: aux and dpcd fixes Jani Nikula
4 siblings, 0 replies; 18+ messages in thread
From: Jani Nikula @ 2013-09-20 13:42 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
We haven't read the downstream port caps for DPCD 1.0, so don't use
them.
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3afbea9..dd780bf 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2815,7 +2815,6 @@ static enum drm_connector_status
intel_dp_detect_dpcd(struct intel_dp *intel_dp)
{
uint8_t *dpcd = intel_dp->dpcd;
- bool hpd;
uint8_t type;
if (!intel_dp_get_dpcd(intel_dp))
@@ -2826,8 +2825,8 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
return connector_status_connected;
/* If we're HPD-aware, SINK_COUNT changes dynamically */
- hpd = !!(intel_dp->downstream_ports[0] & DP_DS_PORT_HPD);
- if (hpd) {
+ if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
+ intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
uint8_t reg;
if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT,
®, 1))
@@ -2841,9 +2840,17 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
return connector_status_connected;
/* Well we tried, say unknown for unreliable port types */
- type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
- if (type == DP_DS_PORT_TYPE_VGA || type == DP_DS_PORT_TYPE_NON_EDID)
- return connector_status_unknown;
+ if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
+ type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
+ if (type == DP_DS_PORT_TYPE_VGA ||
+ type == DP_DS_PORT_TYPE_NON_EDID)
+ return connector_status_unknown;
+ } else {
+ type = intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
+ DP_DWN_STRM_PORT_TYPE_MASK;
+ if (type == (1 << 1) || type == (3 << 1))
+ return connector_status_unknown;
+ }
/* Anything else is out of spec, warn and ignore */
DRM_DEBUG_KMS("Broken DP branch device, ignoring\n");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 4/4] drm/i915/dp: read DPCD PSR capability only on eDP
2013-09-20 13:42 [PATCH 0/4] drm/i915/dp: aux and dpcd fixes Jani Nikula
` (2 preceding siblings ...)
2013-09-20 13:42 ` [PATCH 3/4] drm/i915/dp: downstream port capabilities are not present in DPCD 1.0 Jani Nikula
@ 2013-09-20 13:42 ` Jani Nikula
2013-09-20 20:50 ` Todd Previte
2013-09-23 5:59 ` [PATCH 0/4] drm/i915/dp: aux and dpcd fixes Jani Nikula
4 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2013-09-20 13:42 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
Reduce AUX transactions for non-eDP.
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dd780bf..f2e16a1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2691,11 +2691,14 @@ 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));
- intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
- intel_dp->psr_dpcd,
- sizeof(intel_dp->psr_dpcd));
- if (is_edp_psr(intel_dp))
- DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
+ if (is_edp(intel_dp)) {
+ intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
+ intel_dp->psr_dpcd,
+ sizeof(intel_dp->psr_dpcd));
+ if (is_edp_psr(intel_dp))
+ DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
+ }
+
if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
DP_DWN_STRM_PORT_PRESENT))
return true; /* native DP sink */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] drm/i915/dp: read DPCD PSR capability only on eDP
2013-09-20 13:42 ` [PATCH 4/4] drm/i915/dp: read DPCD PSR capability only on eDP Jani Nikula
@ 2013-09-20 20:50 ` Todd Previte
2013-09-20 20:57 ` Paulo Zanoni
0 siblings, 1 reply; 18+ messages in thread
From: Todd Previte @ 2013-09-20 20:50 UTC (permalink / raw)
To: intel-gfx
On 09/20/2013 06:42 AM, Jani Nikula wrote:
> Reduce AUX transactions for non-eDP.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index dd780bf..f2e16a1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2691,11 +2691,14 @@ 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));
> - intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> - intel_dp->psr_dpcd,
> - sizeof(intel_dp->psr_dpcd));
> - if (is_edp_psr(intel_dp))
> - DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> + if (is_edp(intel_dp)) {
> + intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> + intel_dp->psr_dpcd,
> + sizeof(intel_dp->psr_dpcd));
> + if (is_edp_psr(intel_dp))
> + DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> + }
> +
> if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> DP_DWN_STRM_PORT_PRESENT))
> return true; /* native DP sink */
Couldn't you just use is_edp_psr() at the top and not need the second
is_edp() inside the if() statement? As in:
+ if (is_edp_psr(intel_dp)) {
+ intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
+ intel_dp->psr_dpcd,
+ sizeof(intel_dp->psr_dpcd));
+ DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
+ }
-T
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] drm/i915/dp: read DPCD PSR capability only on eDP
2013-09-20 20:50 ` Todd Previte
@ 2013-09-20 20:57 ` Paulo Zanoni
2013-09-20 22:31 ` Todd Previte
0 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2013-09-20 20:57 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development
2013/9/20 Todd Previte <tprevite@gmail.com>:
> On 09/20/2013 06:42 AM, Jani Nikula wrote:
>>
>> Reduce AUX transactions for non-eDP.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index dd780bf..f2e16a1 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2691,11 +2691,14 @@ 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));
>> - intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>> - intel_dp->psr_dpcd,
>> - sizeof(intel_dp->psr_dpcd));
>> - if (is_edp_psr(intel_dp))
>> - DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>> + if (is_edp(intel_dp)) {
>> + intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>> + intel_dp->psr_dpcd,
>> +
>> sizeof(intel_dp->psr_dpcd));
>> + if (is_edp_psr(intel_dp))
>> + DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>> + }
>> +
>> if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>> DP_DWN_STRM_PORT_PRESENT))
>> return true; /* native DP sink */
>
>
> Couldn't you just use is_edp_psr() at the top and not need the second
> is_edp() inside the if() statement? As in:
No, because is_edp_psr() only makes sense after we do the DPCD read
that checks the PSR bit.
>
> + if (is_edp_psr(intel_dp)) {
>
> + intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> + intel_dp->psr_dpcd,
> + sizeof(intel_dp->psr_dpcd));
> + DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> + }
>
>
> -T
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] drm/i915/dp: read DPCD PSR capability only on eDP
2013-09-20 20:57 ` Paulo Zanoni
@ 2013-09-20 22:31 ` Todd Previte
2013-09-23 7:29 ` Daniel Vetter
0 siblings, 1 reply; 18+ messages in thread
From: Todd Previte @ 2013-09-20 22:31 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development
On 09/20/2013 01:57 PM, Paulo Zanoni wrote:
> 2013/9/20 Todd Previte <tprevite@gmail.com>:
>> On 09/20/2013 06:42 AM, Jani Nikula wrote:
>>> Reduce AUX transactions for non-eDP.
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++-----
>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index dd780bf..f2e16a1 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -2691,11 +2691,14 @@ 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));
>>> - intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>>> - intel_dp->psr_dpcd,
>>> - sizeof(intel_dp->psr_dpcd));
>>> - if (is_edp_psr(intel_dp))
>>> - DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>>> + if (is_edp(intel_dp)) {
>>> + intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>>> + intel_dp->psr_dpcd,
>>> +
>>> sizeof(intel_dp->psr_dpcd));
>>> + if (is_edp_psr(intel_dp))
>>> + DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>>> + }
>>> +
>>> if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>>> DP_DWN_STRM_PORT_PRESENT))
>>> return true; /* native DP sink */
>>
>> Couldn't you just use is_edp_psr() at the top and not need the second
>> is_edp() inside the if() statement? As in:
> No, because is_edp_psr() only makes sense after we do the DPCD read
> that checks the PSR bit.
Yep, I see that now. And thus...
[Reviewed-by]: Todd Previte <tprevite@gmail.com>
>
>> + if (is_edp_psr(intel_dp)) {
>>
>> + intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
>> + intel_dp->psr_dpcd,
>> + sizeof(intel_dp->psr_dpcd));
>> + DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
>> + }
>>
>>
>> -T
>>
>>
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] drm/i915/dp: read DPCD PSR capability only on eDP
2013-09-20 22:31 ` Todd Previte
@ 2013-09-23 7:29 ` Daniel Vetter
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2013-09-23 7:29 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development
On Fri, Sep 20, 2013 at 03:31:42PM -0700, Todd Previte wrote:
> On 09/20/2013 01:57 PM, Paulo Zanoni wrote:
> >2013/9/20 Todd Previte <tprevite@gmail.com>:
> >>On 09/20/2013 06:42 AM, Jani Nikula wrote:
> >>>Reduce AUX transactions for non-eDP.
> >>>
> >>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>>---
> >>> drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++-----
> >>> 1 file changed, 8 insertions(+), 5 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >>>b/drivers/gpu/drm/i915/intel_dp.c
> >>>index dd780bf..f2e16a1 100644
> >>>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>@@ -2691,11 +2691,14 @@ 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));
> >>>- intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> >>>- intel_dp->psr_dpcd,
> >>>- sizeof(intel_dp->psr_dpcd));
> >>>- if (is_edp_psr(intel_dp))
> >>>- DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> >>>+ if (is_edp(intel_dp)) {
> >>>+ intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> >>>+ intel_dp->psr_dpcd,
> >>>+
> >>>sizeof(intel_dp->psr_dpcd));
> >>>+ if (is_edp_psr(intel_dp))
> >>>+ DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> >>>+ }
> >>>+
> >>> if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> >>> DP_DWN_STRM_PORT_PRESENT))
> >>> return true; /* native DP sink */
> >>
> >>Couldn't you just use is_edp_psr() at the top and not need the second
> >>is_edp() inside the if() statement? As in:
> >No, because is_edp_psr() only makes sense after we do the DPCD read
> >that checks the PSR bit.
>
> Yep, I see that now. And thus...
>
> [Reviewed-by]: Todd Previte <tprevite@gmail.com>
Queued for -next, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] drm/i915/dp: aux and dpcd fixes
2013-09-20 13:42 [PATCH 0/4] drm/i915/dp: aux and dpcd fixes Jani Nikula
` (3 preceding siblings ...)
2013-09-20 13:42 ` [PATCH 4/4] drm/i915/dp: read DPCD PSR capability only on eDP Jani Nikula
@ 2013-09-23 5:59 ` Jani Nikula
2013-09-23 7:30 ` Daniel Vetter
4 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2013-09-23 5:59 UTC (permalink / raw)
To: intel-gfx
On Fri, 20 Sep 2013, Jani Nikula <jani.nikula@intel.com> wrote:
> I haven't received confirmation from various bug reporters whether these
> actually fix any issues, but here goes anyway.
These are on the series, but I bet it's patch 2/4 that does the trick.
Reference: https://bugs.freedesktop.org/show_bug.cgi?id=60263
Tested-by: Nicolas Suzor <nic@suzor.com>
> We may want to revisit patch 2/4 in a future patch, adding some
> heuristic to increase delay with each retry, and maybe taking into
> account i2c bit rate capabilities if reported in dpcd.
>
> BR,
> Jani.
>
>
> Jani Nikula (4):
> drm/i915/dp: retry i2c-over-aux seven times on AUX DEFER
> drm/i915/dp: increase i2c-over-aux retry interval on AUX DEFER
> drm/i915/dp: downstream port capabilities are not present in DPCD 1.0
> drm/i915/dp: read DPCD PSR capability only on eDP
>
> drivers/gpu/drm/i915/intel_dp.c | 52 +++++++++++++++++++++++++++++----------
> 1 file changed, 39 insertions(+), 13 deletions(-)
>
> --
> 1.7.9.5
>
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 0/4] drm/i915/dp: aux and dpcd fixes
2013-09-23 5:59 ` [PATCH 0/4] drm/i915/dp: aux and dpcd fixes Jani Nikula
@ 2013-09-23 7:30 ` Daniel Vetter
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2013-09-23 7:30 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Mon, Sep 23, 2013 at 08:59:43AM +0300, Jani Nikula wrote:
> On Fri, 20 Sep 2013, Jani Nikula <jani.nikula@intel.com> wrote:
> > I haven't received confirmation from various bug reporters whether these
> > actually fix any issues, but here goes anyway.
>
> These are on the series, but I bet it's patch 2/4 that does the trick.
>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=60263
> Tested-by: Nicolas Suzor <nic@suzor.com>
I've picked up patch 2 from this series for -fixes, 4 for dinq (the other
two lack r-bs).
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 18+ messages in thread