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@xxxxxxxxx wrote: > > > > > From: Matt Atwood <matthew.s.atwood@xxxxxxxxx> > > > > > > > > > > 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@xxxxxxxxx> > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > > > > > > > --- > > > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel