| Message ID | 1520361468-32087-1-git-send-email-matthew.s.atwood@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Matt, On Tue, Mar 06, 2018 at 10:37:48AM -0800, matthew.s.atwood@intel.com wrote: > From: Matt Atwood <matthew.s.atwood@intel.com> > > DP_TRAINING_AUX_RD_INTERVAL with DP 1.3 spec changed bit scheme from 8 > bits to 7 bits in DPCD 0x000e. The 8th bit describes a new feature, for > panels that use this new feature, this would cause a wait interval for > clock recovery of at least 512 ms, much higher then spec maximum of 16 ms. > This behavior is described in table 2-158 of DP 1.4 spec address 0000Eh. > To avoid breaking panels that are not spec compliant we now warn on > invalid values. > > V2: commit title/message, masking all 7 bits, warn on out of spec values. > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> Tested-by: Benson Leung <bleung@chromium.org> Tested this patch on a DP 1.3 panel which sets the EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT bit in DPCD 0000Eh. It has a value of 0x80 in that field, indicating the extended caps, and 400us for the Main-Link Channel Equalization phase. Confirmed that link training passes normally where prior to this it would fail after the driver waits too long. Thanks for the fix!
On Tue, Mar 06, 2018 at 10:37:48AM -0800, matthew.s.atwood@intel.com wrote: > From: Matt Atwood <matthew.s.atwood@intel.com> > > DP_TRAINING_AUX_RD_INTERVAL with DP 1.3 spec changed bit scheme from 8 > bits to 7 bits in DPCD 0x000e. The 8th bit describes a new feature, for > panels that use this new feature, this would cause a wait interval for > clock recovery of at least 512 ms, much higher then spec maximum of 16 ms. > This behavior is described in table 2-158 of DP 1.4 spec address 0000Eh. > To avoid breaking panels that are not spec compliant we now warn on > invalid values. > > V2: commit title/message, masking all 7 bits, warn on out of spec values. this approach is even better imho. > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/drm_dp_helper.c | 18 ++++++++++++++---- > include/drm/drm_dp_helper.h | 1 + > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index adf79be..a718ccc 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -119,18 +119,28 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI > EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); > > void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > + > + if (rd_interval > 4) > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > + > + if (rd_interval == 0) > udelay(100); > else > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > + mdelay(rd_interval * 4); > } > EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay); > > void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > + > + if (rd_interval > 4) > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > + > + if (rd_interval == 0) > udelay(400); > else > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > + mdelay(rd_interval * 4); > } > EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index da58a42..f80acf1 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -118,6 +118,7 @@ > # define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) /* edp v1.2 or higher */ > > #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ > +# define DP_TRAINING_AUX_RD_MASK 0x7F /* 1.3 */ > > #define DP_ADAPTER_CAP 0x00f /* 1.2 */ > # define DP_FORCE_LOAD_SENSE_CAP (1 << 0) > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2018-03-06 at 15:24 -0800, Rodrigo Vivi wrote: > On Tue, Mar 06, 2018 at 10:37:48AM -0800, matthew.s.atwood@intel.com wrote: > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > DP_TRAINING_AUX_RD_INTERVAL with DP 1.3 spec changed bit scheme from 8 > > bits to 7 bits in DPCD 0x000e. The 8th bit describes a new feature, for > > panels that use this new feature, this would cause a wait interval for > > clock recovery of at least 512 ms, much higher then spec maximum of 16 ms. > > This behavior is described in table 2-158 of DP 1.4 spec address 0000Eh. > > To avoid breaking panels See comment below: > that are not spec compliant we now warn on > > invalid values. > > > > V2: commit title/message, masking all 7 bits, warn on out of spec values. > > this approach is even better imho. > > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > drivers/gpu/drm/drm_dp_helper.c | 18 ++++++++++++++---- > > include/drm/drm_dp_helper.h | 1 + > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > index adf79be..a718ccc 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -119,18 +119,28 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI > > EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); > > > > void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > + > > + if (rd_interval > 4) > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); Some default for panels without a valid value? rd_interval = 4; "AUX read interval out of range, using max %d ms" > > + > > + if (rd_interval == 0) > > udelay(100); > > else > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > + mdelay(rd_interval * 4); > > } > > EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay); > > > > void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > + > > + if (rd_interval > 4) > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > + > > + if (rd_interval == 0) > > udelay(400); > > else > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > + mdelay(rd_interval * 4); > > } > > EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > index da58a42..f80acf1 100644 > > --- a/include/drm/drm_dp_helper.h > > +++ b/include/drm/drm_dp_helper.h > > @@ -118,6 +118,7 @@ > > # define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) /* edp v1.2 or higher */ > > > > #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ > > +# define DP_TRAINING_AUX_RD_MASK 0x7F /* 1.3 */ > > > > #define DP_ADAPTER_CAP 0x00f /* 1.2 */ > > # define DP_FORCE_LOAD_SENSE_CAP (1 << 0) > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2018-03-06 at 16:48 -0800, Dhinakaran Pandiyan wrote: > > > On Tue, 2018-03-06 at 15:24 -0800, Rodrigo Vivi wrote: > > On Tue, Mar 06, 2018 at 10:37:48AM -0800, matthew.s.atwood@intel.com wrote: > > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > DP_TRAINING_AUX_RD_INTERVAL with DP 1.3 spec changed bit scheme from 8 > > > bits to 7 bits in DPCD 0x000e. The 8th bit describes a new feature, for > > > panels that use this new feature, this would cause a wait interval for > > > clock recovery of at least 512 ms, much higher then spec maximum of 16 ms. > > > This behavior is described in table 2-158 of DP 1.4 spec address 0000Eh. > > > To avoid breaking panels > > See comment below: > > > that are not spec compliant we now warn on > > > invalid values. > > > > > > V2: commit title/message, masking all 7 bits, warn on out of spec values. > > > > this approach is even better imho. > > > > > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > --- > > > drivers/gpu/drm/drm_dp_helper.c | 18 ++++++++++++++---- > > > include/drm/drm_dp_helper.h | 1 + > > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > index adf79be..a718ccc 100644 > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > @@ -119,18 +119,28 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI > > > EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); > > > > > > void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > + > > > + if (rd_interval > 4) > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > Some default for panels without a valid value? > rd_interval = 4; > "AUX read interval out of range, using max %d ms" > > > > > + > > > + if (rd_interval == 0) > > > udelay(100); > > > else > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > + mdelay(rd_interval * 4); Btw, DP 1.4 spec also says this is 100 us for *all* values in the register. Updating that for DP 1.4 panels should speed up link training. > > > } > > > EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay); > > > > > > void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > + > > > + if (rd_interval > 4) > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > > + > > > + if (rd_interval == 0) > > > udelay(400); > > > else > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > + mdelay(rd_interval * 4); > > > } > > > EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); > > > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > index da58a42..f80acf1 100644 > > > --- a/include/drm/drm_dp_helper.h > > > +++ b/include/drm/drm_dp_helper.h > > > @@ -118,6 +118,7 @@ > > > # define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) /* edp v1.2 or higher */ > > > > > > #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ > > > +# define DP_TRAINING_AUX_RD_MASK 0x7F /* 1.3 */ > > > > > > #define DP_ADAPTER_CAP 0x00f /* 1.2 */ > > > # define DP_FORCE_LOAD_SENSE_CAP (1 << 0) > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Mar 07, 2018 at 12:24:46AM +0000, Pandiyan, Dhinakaran wrote: > > > > On Tue, 2018-03-06 at 15:24 -0800, Rodrigo Vivi wrote: > > On Tue, Mar 06, 2018 at 10:37:48AM -0800, matthew.s.atwood@intel.com wrote: > > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > DP_TRAINING_AUX_RD_INTERVAL with DP 1.3 spec changed bit scheme from 8 > > > bits to 7 bits in DPCD 0x000e. The 8th bit describes a new feature, for > > > panels that use this new feature, this would cause a wait interval for > > > clock recovery of at least 512 ms, much higher then spec maximum of 16 ms. > > > This behavior is described in table 2-158 of DP 1.4 spec address 0000Eh. > > > To avoid breaking panels > > See comment below: > > > that are not spec compliant we now warn on > > > invalid values. > > > > > > V2: commit title/message, masking all 7 bits, warn on out of spec values. > > > > this approach is even better imho. > > > > > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > --- > > > drivers/gpu/drm/drm_dp_helper.c | 18 ++++++++++++++---- > > > include/drm/drm_dp_helper.h | 1 + > > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > index adf79be..a718ccc 100644 > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > @@ -119,18 +119,28 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI > > > EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); > > > > > > void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > + > > > + if (rd_interval > 4) > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > Some default for panels without a valid value? > rd_interval = 4; > "AUX read interval out of range, using max %d ms" > The problem with setting the upper bound to 4 is that there are panels that do not follow the spec and expect a longer than 16 ms delay. So if we set the upper bound to 4 in those cases the panels might not work. So we decided to go with this approach where we tell the users that panel is requesting out of range AUX value but then set it to the value * 4 in the else part. Manasi > > > > + > > > + if (rd_interval == 0) > > > udelay(100); > > > else > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > + mdelay(rd_interval * 4); > > > } > > > EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay); > > > > > > void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > + > > > + if (rd_interval > 4) > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > > + > > > + if (rd_interval == 0) > > > udelay(400); > > > else > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > + mdelay(rd_interval * 4); > > > } > > > EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); > > > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > index da58a42..f80acf1 100644 > > > --- a/include/drm/drm_dp_helper.h > > > +++ b/include/drm/drm_dp_helper.h > > > @@ -118,6 +118,7 @@ > > > # define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) /* edp v1.2 or higher */ > > > > > > #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ > > > +# define DP_TRAINING_AUX_RD_MASK 0x7F /* 1.3 */ > > > > > > #define DP_ADAPTER_CAP 0x00f /* 1.2 */ > > > # define DP_FORCE_LOAD_SENSE_CAP (1 << 0) > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2018-03-06 at 17:36 -0800, Manasi Navare wrote: > On Wed, Mar 07, 2018 at 12:24:46AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > On Tue, 2018-03-06 at 15:24 -0800, Rodrigo Vivi wrote: > > > On Tue, Mar 06, 2018 at 10:37:48AM -0800, matthew.s.atwood@intel.com wrote: > > > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > > > DP_TRAINING_AUX_RD_INTERVAL with DP 1.3 spec changed bit scheme from 8 > > > > bits to 7 bits in DPCD 0x000e. The 8th bit describes a new feature, for > > > > panels that use this new feature, this would cause a wait interval for > > > > clock recovery of at least 512 ms, much higher then spec maximum of 16 ms. > > > > This behavior is described in table 2-158 of DP 1.4 spec address 0000Eh. > > > > To avoid breaking panels > > > > See comment below: > > > > > that are not spec compliant we now warn on > > > > invalid values. > > > > > > > > V2: commit title/message, masking all 7 bits, warn on out of spec values. > > > > > > this approach is even better imho. > > > > > > > > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > --- > > > > drivers/gpu/drm/drm_dp_helper.c | 18 ++++++++++++++---- > > > > include/drm/drm_dp_helper.h | 1 + > > > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > > index adf79be..a718ccc 100644 > > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > > @@ -119,18 +119,28 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI > > > > EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); > > > > > > > > void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > > + > > > > + if (rd_interval > 4) > > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > > > Some default for panels without a valid value? > > rd_interval = 4; > > "AUX read interval out of range, using max %d ms" > > > > The problem with setting the upper bound to 4 is that there are panels > that do not follow the spec and expect a longer than 16 ms delay. So > if we set the upper bound to 4 in those cases the panels might not work. > > So we decided to go with this approach where we tell the users that panel is requesting > out of range AUX value but then set it to the value * 4 in the else part. > Thanks for the clarification. My concern is if the DPCD is advertizing an out of spec value, it might as well be advertizing a delay that the panel doesn't need. And I thought panel quirks were supposed to be used for working around things like this. To be clear, this is not a big enough concern to block this fix. Like I said in the other email, this patch refers to DP 1.4, shouldn't the clock recovery delay be updated too (in a separate patch)? > Manasi > > > > > > > + > > > > + if (rd_interval == 0) > > > > udelay(100); > > > > else > > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > > + mdelay(rd_interval * 4); > > > > } > > > > EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay); > > > > > > > > void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > > + > > > > + if (rd_interval > 4) > > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > > > + > > > > + if (rd_interval == 0) > > > > udelay(400); > > > > else > > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > > + mdelay(rd_interval * 4); > > > > } > > > > EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); > > > > > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > > index da58a42..f80acf1 100644 > > > > --- a/include/drm/drm_dp_helper.h > > > > +++ b/include/drm/drm_dp_helper.h > > > > @@ -118,6 +118,7 @@ > > > > # define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) /* edp v1.2 or higher */ > > > > > > > > #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ > > > > +# define DP_TRAINING_AUX_RD_MASK 0x7F /* 1.3 */ > > > > > > > > #define DP_ADAPTER_CAP 0x00f /* 1.2 */ > > > > # define DP_FORCE_LOAD_SENSE_CAP (1 << 0) > > > > -- > > > > 2.7.4 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Mar 07, 2018 at 02:13:21AM +0000, Pandiyan, Dhinakaran wrote: > > > > On Tue, 2018-03-06 at 17:36 -0800, Manasi Navare wrote: > > On Wed, Mar 07, 2018 at 12:24:46AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > On Tue, 2018-03-06 at 15:24 -0800, Rodrigo Vivi wrote: > > > > On Tue, Mar 06, 2018 at 10:37:48AM -0800, matthew.s.atwood@intel.com wrote: > > > > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > > > > > DP_TRAINING_AUX_RD_INTERVAL with DP 1.3 spec changed bit scheme from 8 > > > > > bits to 7 bits in DPCD 0x000e. The 8th bit describes a new feature, for > > > > > panels that use this new feature, this would cause a wait interval for > > > > > clock recovery of at least 512 ms, much higher then spec maximum of 16 ms. > > > > > This behavior is described in table 2-158 of DP 1.4 spec address 0000Eh. > > > > > To avoid breaking panels > > > > > > See comment below: > > > > > > > that are not spec compliant we now warn on > > > > > invalid values. > > > > > > > > > > V2: commit title/message, masking all 7 bits, warn on out of spec values. > > > > > > > > this approach is even better imho. > > > > > > > > > > > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > > > --- > > > > > drivers/gpu/drm/drm_dp_helper.c | 18 ++++++++++++++---- > > > > > include/drm/drm_dp_helper.h | 1 + > > > > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > > > index adf79be..a718ccc 100644 > > > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > > > @@ -119,18 +119,28 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI > > > > > EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); > > > > > > > > > > void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > > > + > > > > > + if (rd_interval > 4) > > > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > > > > > Some default for panels without a valid value? > > > rd_interval = 4; > > > "AUX read interval out of range, using max %d ms" > > > > > > > The problem with setting the upper bound to 4 is that there are panels > > that do not follow the spec and expect a longer than 16 ms delay. So > > if we set the upper bound to 4 in those cases the panels might not work. > > > > So we decided to go with this approach where we tell the users that panel is requesting > > out of range AUX value but then set it to the value * 4 in the else part. > > > > Thanks for the clarification. My concern is if the DPCD is advertizing > an out of spec value, it might as well be advertizing a delay that the > panel doesn't need. And I thought panel quirks were supposed to be used > for working around things like this. To be clear, this is not a big > enough concern to block this fix. > > Like I said in the other email, this patch refers to DP 1.4, shouldn't > the clock recovery delay be updated too (in a separate patch)? We clearly need more work here. I can see here on DP-v1.2a_d11: 00h = 100us for the Main Link Clock Recovery phase 400us for the Main Link Channel Equalization phase and for FAUX training. 01h = 4ms all. 02h = 8ms all. 03h = 12ms all. 04h = 16ms all. So probably the initial mask on this patch should be marked with /* XXX 1.2? */ because it clearly got introduced in some 1.2 minor release. But even for DP 1.2 it doesn't seem we are doing it right on the 0 case. It seems that we are using 100us for both channel eq and clock recovery, right? or am I missing something? Then DP 1.3 keeps same config. But DP 1.4 change all values. clock recovery is always 100us and channel eq is depending on this bit * 4 and 400us when bit is zeroed. But limited to 4. So we probably need 3 patches here: 1. - This one to protect against bad panels masking it and mentioning DP 1.2, nor 1.3 or 1.4. Also limiting rd_interval to 4 as DK suggested. Panels cannot expect all drivers are using this value * 4 blindly since it is not on spec. 2. - Fix channel eq for 0 case since 1.2. It should be 400us. 3. - For DP version >= 1.4 always use 100us for clock req or follow this register for channel eq. Thoughts? > > > > Manasi > > > > > > > > > > + > > > > > + if (rd_interval == 0) > > > > > udelay(100); > > > > > else > > > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > > > + mdelay(rd_interval * 4); > > > > > } > > > > > EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay); > > > > > > > > > > void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > > > + > > > > > + if (rd_interval > 4) > > > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > > > > + > > > > > + if (rd_interval == 0) > > > > > udelay(400); > > > > > else > > > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > > > + mdelay(rd_interval * 4); > > > > > } > > > > > EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); > > > > > > > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > > > index da58a42..f80acf1 100644 > > > > > --- a/include/drm/drm_dp_helper.h > > > > > +++ b/include/drm/drm_dp_helper.h > > > > > @@ -118,6 +118,7 @@ > > > > > # define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) /* edp v1.2 or higher */ > > > > > > > > > > #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ > > > > > +# define DP_TRAINING_AUX_RD_MASK 0x7F /* 1.3 */ > > > > > > > > > > #define DP_ADAPTER_CAP 0x00f /* 1.2 */ > > > > > # define DP_FORCE_LOAD_SENSE_CAP (1 << 0) > > > > > -- > > > > > 2.7.4 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Mar 07, 2018 at 02:06:08PM -0800, Rodrigo Vivi wrote: > On Wed, Mar 07, 2018 at 02:13:21AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > On Tue, 2018-03-06 at 17:36 -0800, Manasi Navare wrote: > > > On Wed, Mar 07, 2018 at 12:24:46AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > > > > > > > > > On Tue, 2018-03-06 at 15:24 -0800, Rodrigo Vivi wrote: > > > > > On Tue, Mar 06, 2018 at 10:37:48AM -0800, matthew.s.atwood@intel.com wrote: > > > > > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > > > > > > > DP_TRAINING_AUX_RD_INTERVAL with DP 1.3 spec changed bit scheme from 8 > > > > > > bits to 7 bits in DPCD 0x000e. The 8th bit describes a new feature, for > > > > > > panels that use this new feature, this would cause a wait interval for > > > > > > clock recovery of at least 512 ms, much higher then spec maximum of 16 ms. > > > > > > This behavior is described in table 2-158 of DP 1.4 spec address 0000Eh. > > > > > > To avoid breaking panels > > > > > > > > See comment below: > > > > > > > > > that are not spec compliant we now warn on > > > > > > invalid values. > > > > > > > > > > > > V2: commit title/message, masking all 7 bits, warn on out of spec values. > > > > > > > > > > this approach is even better imho. > > > > > > > > > > > > > > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > > > > > --- > > > > > > drivers/gpu/drm/drm_dp_helper.c | 18 ++++++++++++++---- > > > > > > include/drm/drm_dp_helper.h | 1 + > > > > > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > > > > index adf79be..a718ccc 100644 > > > > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > > > > @@ -119,18 +119,28 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI > > > > > > EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); > > > > > > > > > > > > void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > > > > + > > > > > > + if (rd_interval > 4) > > > > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > > > > > > > Some default for panels without a valid value? > > > > rd_interval = 4; > > > > "AUX read interval out of range, using max %d ms" > > > > > > > > > > The problem with setting the upper bound to 4 is that there are panels > > > that do not follow the spec and expect a longer than 16 ms delay. So > > > if we set the upper bound to 4 in those cases the panels might not work. > > > > > > So we decided to go with this approach where we tell the users that panel is requesting > > > out of range AUX value but then set it to the value * 4 in the else part. > > > > > > > Thanks for the clarification. My concern is if the DPCD is advertizing > > an out of spec value, it might as well be advertizing a delay that the > > panel doesn't need. And I thought panel quirks were supposed to be used > > for working around things like this. To be clear, this is not a big > > enough concern to block this fix. > > > > Like I said in the other email, this patch refers to DP 1.4, shouldn't > > the clock recovery delay be updated too (in a separate patch)? > > We clearly need more work here. > > I can see here on DP-v1.2a_d11: > > 00h = 100us for the Main Link Clock Recovery phase 400us for the Main Link Channel > Equalization phase and for FAUX training. > 01h = 4ms all. > 02h = 8ms all. > 03h = 12ms all. > 04h = 16ms all. > > So probably the initial mask on this patch should be marked with /* XXX 1.2? */ > because it clearly got introduced in some 1.2 minor release. > > But even for DP 1.2 it doesn't seem we are doing it right on the 0 case. > It seems that we are using 100us for both channel eq and clock recovery, right? > or am I missing something? > > Then DP 1.3 keeps same config. > > But DP 1.4 change all values. > > clock recovery is always 100us and channel eq is depending on this bit * 4 and 400us when bit is zeroed. > > But limited to 4. > > So we probably need 3 patches here: > 1. - This one to protect against bad panels masking it and mentioning DP 1.2, > nor 1.3 or 1.4. Also limiting rd_interval to 4 as DK suggested. Panels cannot > expect all drivers are using this value * 4 blindly since it is not on spec. So if some panels still expect a greater delay, those will fail link training. But yes if we want them to be DP compliant, just follow the spec, limit it to the max value of 4 with a warning. > 2. - Fix channel eq for 0 case since 1.2. It should be 400us. Channel eq is 400 us for DP 1.2, 1.3 and 1.4 and then *4 for all other values. We are doing that correctly here. So no change there. > 3. - For DP version >= 1.4 always use 100us for clock req or follow this register for > channel eq. > yes this needs to be fixed for DP REV >= 1.4 Manasi > Thoughts? > > > > > > > > Manasi > > > > > > > > > > > > > + > > > > > > + if (rd_interval == 0) > > > > > > udelay(100); > > > > > > else > > > > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > > > > + mdelay(rd_interval * 4); > > > > > > } > > > > > > EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay); > > > > > > > > > > > > void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > > > > > > - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > > > > > > + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; > > > > > > + > > > > > > + if (rd_interval > 4) > > > > > > + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); > > > > > > + > > > > > > + if (rd_interval == 0) > > > > > > udelay(400); > > > > > > else > > > > > > - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); > > > > > > + mdelay(rd_interval * 4); > > > > > > } > > > > > > EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); > > > > > > > > > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > > > > index da58a42..f80acf1 100644 > > > > > > --- a/include/drm/drm_dp_helper.h > > > > > > +++ b/include/drm/drm_dp_helper.h > > > > > > @@ -118,6 +118,7 @@ > > > > > > # define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) /* edp v1.2 or higher */ > > > > > > > > > > > > #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ > > > > > > +# define DP_TRAINING_AUX_RD_MASK 0x7F /* 1.3 */ > > > > > > > > > > > > #define DP_ADAPTER_CAP 0x00f /* 1.2 */ > > > > > > # define DP_FORCE_LOAD_SENSE_CAP (1 << 0) > > > > > > -- > > > > > > 2.7.4 > > > > > > > > > > > > _______________________________________________ > > > > > > Intel-gfx mailing list > > > > > > Intel-gfx@lists.freedesktop.org > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index adf79be..a718ccc 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -119,18 +119,28 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; + + if (rd_interval > 4) + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); + + if (rd_interval == 0) udelay(100); else - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); + mdelay(rd_interval * 4); } EXPORT_SYMBOL(drm_dp_link_train_clock_recovery_delay); void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { - if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) + int rd_interval = dpcd[DP_TRAINING_AUX_RD_INTERVAL] & DP_TRAINING_AUX_RD_MASK; + + if (rd_interval > 4) + DRM_DEBUG_KMS("AUX interval %d, out of range (max 4)", rd_interval); + + if (rd_interval == 0) udelay(400); else - mdelay(dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4); + mdelay(rd_interval * 4); } EXPORT_SYMBOL(drm_dp_link_train_channel_eq_delay); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index da58a42..f80acf1 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -118,6 +118,7 @@ # define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3) /* edp v1.2 or higher */ #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ +# define DP_TRAINING_AUX_RD_MASK 0x7F /* 1.3 */ #define DP_ADAPTER_CAP 0x00f /* 1.2 */ # define DP_FORCE_LOAD_SENSE_CAP (1 << 0)