| Message ID | f1fa07e0fe19934456b6ca685624887378244c90.1379684248.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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 >
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
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);
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(-)