| Message ID | 1412869090-48010-3-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-10-09 12:38 GMT-03:00 Todd Previte <tprevite@gmail.com>: > These counters are used for Displayort complinace testing to detect error conditions > when executing certain compliance tests. Currently these are used in the EDID tests > to determine if the video mode needs to be set to the preferred mode or the failsafe > mode. > > Cc: dri-devel@lists.freedesktop.org I see that this patch and a few others in your series still have unaddressed/unanswered review comments, given on the first time you sent the patches. Please take a look at them. > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/drm_dp_helper.c | 2 ++ > include/drm/drm_dp_helper.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 08e33b8..8353051 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) > > case DP_AUX_I2C_REPLY_NACK: > DRM_DEBUG_KMS("I2C nack\n"); > + aux->i2c_nack_count++; > return -EREMOTEIO; > > case DP_AUX_I2C_REPLY_DEFER: > DRM_DEBUG_KMS("I2C defer\n"); > + aux->i2c_defer_count++; > usleep_range(400, 500); > continue; > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 8edeed0..45f3ee8 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -551,6 +551,7 @@ struct drm_dp_aux { > struct mutex hw_mutex; > ssize_t (*transfer)(struct drm_dp_aux *aux, > struct drm_dp_aux_msg *msg); > + uint8_t i2c_nack_count, i2c_defer_count; > }; > > ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset, > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
To address previous feedback, I'll quote below and answer. >It would be nice if you could cite on the commit message the name of >the specification and the name of the test(s) that use it. Done. For reference, in the Displayport Link CTS spec, tests 4.2.2.4 and 4.2.2.5 are the ones that use these, specifically. >Does it really need to be uint8_t? I see on patch 7 that you don't >really write this value to a place that only accepts uint8_t-sized >arguments, so I fear that if we get 256 NACKs or DEFERs we may end up >doing the wrong thing. There are no compliance tests that are concerned with the number of native DEFERs or NACKs received, hence why they are not addressed here. There's no requirement for the size of this value and I selected uint8_t as the smallest reasonable size for it. It's only used to count the number of NACKs and DEFERs during compliance testing and it gets reset to 0 at the beginning of the test block where it's used in a later patch. It's unlikely that a case would occur during this compliance test where exactly 256 NACKs or DEFERs occur, but your point is well-taken. I'll make them uint32s and be done with it. I don't think 6 extra bytes is going to run the kernel out of memory. :) >Also, why don't we need to count the native NACKs and DEFERs? There are no compliance tests that are concerned with the number of native DEFERs or NACKs received. New patch will be here shortly. -T On 10/21/2014 10:10 AM, Paulo Zanoni wrote: > 2014-10-09 12:38 GMT-03:00 Todd Previte <tprevite@gmail.com>: >> These counters are used for Displayort complinace testing to detect error conditions >> when executing certain compliance tests. Currently these are used in the EDID tests >> to determine if the video mode needs to be set to the preferred mode or the failsafe >> mode. >> >> Cc: dri-devel@lists.freedesktop.org > I see that this patch and a few others in your series still have > unaddressed/unanswered review comments, given on the first time you > sent the patches. Please take a look at them. > >> Signed-off-by: Todd Previte <tprevite@gmail.com> >> --- >> drivers/gpu/drm/drm_dp_helper.c | 2 ++ >> include/drm/drm_dp_helper.h | 1 + >> 2 files changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c >> index 08e33b8..8353051 100644 >> --- a/drivers/gpu/drm/drm_dp_helper.c >> +++ b/drivers/gpu/drm/drm_dp_helper.c >> @@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) >> >> case DP_AUX_I2C_REPLY_NACK: >> DRM_DEBUG_KMS("I2C nack\n"); >> + aux->i2c_nack_count++; >> return -EREMOTEIO; >> >> case DP_AUX_I2C_REPLY_DEFER: >> DRM_DEBUG_KMS("I2C defer\n"); >> + aux->i2c_defer_count++; >> usleep_range(400, 500); >> continue; >> >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h >> index 8edeed0..45f3ee8 100644 >> --- a/include/drm/drm_dp_helper.h >> +++ b/include/drm/drm_dp_helper.h >> @@ -551,6 +551,7 @@ struct drm_dp_aux { >> struct mutex hw_mutex; >> ssize_t (*transfer)(struct drm_dp_aux *aux, >> struct drm_dp_aux_msg *msg); >> + uint8_t i2c_nack_count, i2c_defer_count; >> }; >> >> ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset, >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Tue, Nov 04, 2014 at 03:12:27PM -0700, Todd Previte wrote: > >Does it really need to be uint8_t? I see on patch 7 that you don't > >really write this value to a place that only accepts uint8_t-sized > >arguments, so I fear that if we get 256 NACKs or DEFERs we may end up > >doing the wrong thing. > > There are no compliance tests that are concerned with the number of native > DEFERs or NACKs received, hence why they are not addressed here. > There's no requirement for the size of this value and I selected uint8_t as > the smallest reasonable size for it. It's only used to count the number of > NACKs and DEFERs during compliance testing and it gets reset to 0 at the > beginning of the test block where it's used in a later patch. It's unlikely > that a case would occur during this compliance test where exactly 256 NACKs > or DEFERs occur, but your point is well-taken. I'll make them uint32s and be > done with it. I don't think 6 extra bytes is going to run the kernel out of > memory. :) Du to struct alignement rules a lone uint8_t will actually occupy as much as a plain unsigned. Also uint8_t in drivers has a bit the air of a carefully picked size, usually in relation to some hw limit (e.g. register size). Imo just go with unsigned, it's clearer in intent and actually doesn't use more space. -Daniel
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 08e33b8..8353051 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) case DP_AUX_I2C_REPLY_NACK: DRM_DEBUG_KMS("I2C nack\n"); + aux->i2c_nack_count++; return -EREMOTEIO; case DP_AUX_I2C_REPLY_DEFER: DRM_DEBUG_KMS("I2C defer\n"); + aux->i2c_defer_count++; usleep_range(400, 500); continue; diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 8edeed0..45f3ee8 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -551,6 +551,7 @@ struct drm_dp_aux { struct mutex hw_mutex; ssize_t (*transfer)(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); + uint8_t i2c_nack_count, i2c_defer_count; }; ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
These counters are used for Displayort complinace testing to detect error conditions when executing certain compliance tests. Currently these are used in the EDID tests to determine if the video mode needs to be set to the preferred mode or the failsafe mode. Cc: dri-devel@lists.freedesktop.org Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/drm_dp_helper.c | 2 ++ include/drm/drm_dp_helper.h | 1 + 2 files changed, 3 insertions(+)