| Message ID | 1428372676-4784-1-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-04-06 23:11 GMT-03:00 Todd Previte <tprevite@gmail.com>: > For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source > device must attempt at least 7 times to read the EDID when it receives an > I2C defer. The normal DRM code makes only 7 retries, regardless of whether > or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails > since there are native defers interspersed with the I2C defers which > results in less than 7 EDID read attempts. > > The solution is to decrement the retry counter when an I2C DEFER is returned > such that another read attempt will be made. This situation should normally > only occur in compliance testing, however, as a worse case real-world > scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers) > for a single transaction to complete. The net result is a slightly slower > response to an EDID read that shouldn't significantly impact overall > performance. > > V2: > - Added a check on the number of I2C Defers to limit the number > of times that the retries variable will be decremented. This > is to address review feedback regarding possible infinite loops > from misbehaving sink devices. > > Signed-off-by: Todd Previte <tprevite@gmail.com> > Cc: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/drm_dp_helper.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 79968e3..23025cf 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) > case DP_AUX_I2C_REPLY_DEFER: > DRM_DEBUG_KMS("I2C defer\n"); > aux->i2c_defer_count++; > + /* DP Compliance Test 4.2.2.5 Requirement: > + * Must have at least 7 retries for I2C defers on the > + * transaction to pass this test > + */ > + if (aux->i2c_defer_count < 8) I don't think this is the way to go. During normal (non-compliance-testing) operation we never zero i2c_defer_count, so we can't expect this to work, since we may start drm_dp_i2c_do_msg with a i2c_defer_count value different than zero. Also, during i915.ko DP compliance we only zero i2c_defer_count at the very beginning of each test, not at every aux transaction, and I really think we need a solution that is not specific to compliance testing. > + retry--; > usleep_range(400, 500); > continue; > > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Apr 07, 2015 at 11:29:43AM -0300, Paulo Zanoni wrote: > 2015-04-06 23:11 GMT-03:00 Todd Previte <tprevite@gmail.com>: > > For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source > > device must attempt at least 7 times to read the EDID when it receives an > > I2C defer. The normal DRM code makes only 7 retries, regardless of whether > > or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails > > since there are native defers interspersed with the I2C defers which > > results in less than 7 EDID read attempts. > > > > The solution is to decrement the retry counter when an I2C DEFER is returned > > such that another read attempt will be made. This situation should normally > > only occur in compliance testing, however, as a worse case real-world > > scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers) > > for a single transaction to complete. The net result is a slightly slower > > response to an EDID read that shouldn't significantly impact overall > > performance. > > > > V2: > > - Added a check on the number of I2C Defers to limit the number > > of times that the retries variable will be decremented. This > > is to address review feedback regarding possible infinite loops > > from misbehaving sink devices. > > > > Signed-off-by: Todd Previte <tprevite@gmail.com> > > Cc: dri-devel@lists.freedesktop.org > > --- > > drivers/gpu/drm/drm_dp_helper.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > index 79968e3..23025cf 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) > > case DP_AUX_I2C_REPLY_DEFER: > > DRM_DEBUG_KMS("I2C defer\n"); > > aux->i2c_defer_count++; > > + /* DP Compliance Test 4.2.2.5 Requirement: > > + * Must have at least 7 retries for I2C defers on the > > + * transaction to pass this test > > + */ > > + if (aux->i2c_defer_count < 8) > > I don't think this is the way to go. During normal > (non-compliance-testing) operation we never zero i2c_defer_count, so > we can't expect this to work, since we may start drm_dp_i2c_do_msg > with a i2c_defer_count value different than zero. Also, during i915.ko > DP compliance we only zero i2c_defer_count at the very beginning of > each test, not at every aux transaction, and I really think we need a > solution that is not specific to compliance testing. What I was suggesting earlier (or trying to at least) would be simply something like this: int defer_native = 0, defer_i2c = 0; while (defer_native < 7 && defer_i2c < 7) { ... case DP_AUX_NATIVE_REPLY_NACK: ... defer_native++; continue; } ... case DP_AUX_I2C_REPLY_DEFER: ... defer_i2c++; continue; } ... } > > > > + retry--; > > usleep_range(400, 500); > > continue; > > > > -- > > 1.9.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Paulo Zanoni > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 4/7/15 7:29 AM, Paulo Zanoni wrote: > 2015-04-06 23:11 GMT-03:00 Todd Previte <tprevite@gmail.com>: >> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source >> device must attempt at least 7 times to read the EDID when it receives an >> I2C defer. The normal DRM code makes only 7 retries, regardless of whether >> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails >> since there are native defers interspersed with the I2C defers which >> results in less than 7 EDID read attempts. >> >> The solution is to decrement the retry counter when an I2C DEFER is returned >> such that another read attempt will be made. This situation should normally >> only occur in compliance testing, however, as a worse case real-world >> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers) >> for a single transaction to complete. The net result is a slightly slower >> response to an EDID read that shouldn't significantly impact overall >> performance. >> >> V2: >> - Added a check on the number of I2C Defers to limit the number >> of times that the retries variable will be decremented. This >> is to address review feedback regarding possible infinite loops >> from misbehaving sink devices. >> >> Signed-off-by: Todd Previte <tprevite@gmail.com> >> Cc: dri-devel@lists.freedesktop.org >> --- >> drivers/gpu/drm/drm_dp_helper.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c >> index 79968e3..23025cf 100644 >> --- a/drivers/gpu/drm/drm_dp_helper.c >> +++ b/drivers/gpu/drm/drm_dp_helper.c >> @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) >> case DP_AUX_I2C_REPLY_DEFER: >> DRM_DEBUG_KMS("I2C defer\n"); >> aux->i2c_defer_count++; >> + /* DP Compliance Test 4.2.2.5 Requirement: >> + * Must have at least 7 retries for I2C defers on the >> + * transaction to pass this test >> + */ >> + if (aux->i2c_defer_count < 8) > I don't think this is the way to go. During normal > (non-compliance-testing) operation we never zero i2c_defer_count, so > we can't expect this to work, since we may start drm_dp_i2c_do_msg > with a i2c_defer_count value different than zero. Also, during i915.ko > DP compliance we only zero i2c_defer_count at the very beginning of > each test, not at every aux transaction, and I really think we need a > solution that is not specific to compliance testing. To capture the discussion from IRC: The primary issue previously was the potential for an infinite loop when decrementing the retry count. That is clearly addressed by this code, by only decrementing the loop while the defer count is below 8. In actuality, that needs to be 7, so a followup patch with that change will be posted shortly. There are other solutions (Ville has one in his reply), but this one works correctly, is minimally invasive and doesn't require changes to the loop structure. The defer counter isn't used anywhere outside of Displayport compliance testing. In fact, the counters in the aux struct didn't even exist until I put them in there in a previous patch to support this exact test case. So there really isn't a non-DP compliance test specific solution to be had here. It's also unlikely that this has any significant effect on normal operations. In the event that a device misbehaves and begins issuing loads of defers, this code would only delay it by at most 7 retries before the counter exceeded its value. Since this value is not used outside of compliance testing, a reset per-transaction is unnecessary. Additionally, that would break the EDID compliance testing code as it would have no way of detecting that the read had failed due to continuous defers. As long as the counter is reset for each test request (which it is), this code will function properly for both real world and compliance operations. As indicated in the comments, this code exists to ensure compliance with test 4.2.2.5 from the Displayport Link CTS Core 1.2 rev1.1. This test verifies that the source device is capable of responding to a failure to read the EDID when the sink device continuously defers the transaction. It ensures that at least 7 I2C defers are received before calling it quits, versus simply retrying the transaction 7 times regardless of the failure mode. Since I'm going to post an updated patch anyways, I will likely roll the defer count increment into the if-statement to remove one line of code. That also requires moving the increment to a pre- versus post- condition, as follows: ... if (++aux->i2c_defer_count < 7) ... ... > >> + retry--; >> usleep_range(400, 500); >> continue; >> >> -- >> 1.9.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > >
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 79968e3..23025cf 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) case DP_AUX_I2C_REPLY_DEFER: DRM_DEBUG_KMS("I2C defer\n"); aux->i2c_defer_count++; + /* DP Compliance Test 4.2.2.5 Requirement: + * Must have at least 7 retries for I2C defers on the + * transaction to pass this test + */ + if (aux->i2c_defer_count < 8) + retry--; usleep_range(400, 500); continue;
For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source device must attempt at least 7 times to read the EDID when it receives an I2C defer. The normal DRM code makes only 7 retries, regardless of whether or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails since there are native defers interspersed with the I2C defers which results in less than 7 EDID read attempts. The solution is to decrement the retry counter when an I2C DEFER is returned such that another read attempt will be made. This situation should normally only occur in compliance testing, however, as a worse case real-world scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers) for a single transaction to complete. The net result is a slightly slower response to an EDID read that shouldn't significantly impact overall performance. V2: - Added a check on the number of I2C Defers to limit the number of times that the retries variable will be decremented. This is to address review feedback regarding possible infinite loops from misbehaving sink devices. Signed-off-by: Todd Previte <tprevite@gmail.com> Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/drm_dp_helper.c | 6 ++++++ 1 file changed, 6 insertions(+)