Hi Rodrigo The previous email (quote below) is tested with that patch applied. >> >> > lane_align = dp_link_status(link_status, >> >> > DP_LANE_ALIGN_STATUS_UPDATED); >> >> > if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0) >> >> > return false; >> >> >> >> I always got lane_align = 128 (DP_LINK_STATUS_UPDATED) >> >> So drm_dp_channel_eq_ok() always return false. Thanks On Tue, Oct 17, 2017 at 4:39 PM, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote: > > Puthikorn, > > could you please test the patch Manasi had pointed out [1] to see if that > solves your issue instead of your patch? > > If that one solves it you put a Tested-by on that one to help that getting merged. > > If that is not the case so we need to hunt down PSR2 bugs that are killing the link status. > > [1]: https://patchwork.freedesktop.org/patch/178035/ > > Thanks, > Rodrigo. > > On Tue, Oct 17, 2017 at 11:31:46PM +0000, Puthikorn Voravootivat wrote: >> + Vathsala for PSR related code. >> >> On Tue, Oct 17, 2017 at 4:29 PM, Manasi Navare >> <manasi.d.navare@xxxxxxxxx> wrote: >> > On Tue, Oct 17, 2017 at 03:46:00PM -0700, Puthikorn Voravootivat wrote: >> >> I tried https://patchwork.freedesktop.org/series/30670/ but it doesn't work. >> >> >> >> > When you see the display blinking, do you know for sure that drm_dp_channel_eq_ok() returns false? >> >> Yes. Repro is running "modetest" while PSR is on (easier to repro with >> >> PSR2 but PSR1 is fine too) >> >> >> >> > lane_align = dp_link_status(link_status, >> >> > DP_LANE_ALIGN_STATUS_UPDATED); >> >> > if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0) >> >> > return false; >> >> >> >> I always got lane_align = 128 (DP_LINK_STATUS_UPDATED) >> >> So drm_dp_channel_eq_ok() always return false. >> > >> > Hmm, it looks like there is a bug somewhere else that is causing the >> > link status to get updated and bit LINK_STATUS_UPDATED from LANE_ALIGN_STATUS_UPDATED register >> > is not being read so not being cleared causing the link to be retrained again. >> > >> > So I still feel that this patch is a workaround for another bug in the PSR code. >> > But I will let others comment on this. >> > >> > Regards >> > Manasi >> > >> >> >> >> If I disable PSR, drm_dp_channel_eq_ok() will return true and no >> >> blinking occurs. >> >> >> >> On Tue, Oct 17, 2017 at 2:21 PM, Manasi Navare >> >> <manasi.d.navare@xxxxxxxxx> wrote: >> >> > On Tue, Oct 17, 2017 at 02:01:56PM -0700, Puthikorn Voravootivat wrote: >> >> >> intel_dp_long_pulse() is always checking link status because >> >> >> there has been known issues of link loss triggerring long pulse. >> >> >> >> >> >> However this is not needed for eDP display since we won't have >> >> >> link loss for internal display. Also there are reports that >> >> >> screens are flickering during link status check. (repro by >> >> >> running modetest command repeatedly) >> >> >> >> >> >> Signed-off-by: Puthikorn Voravootivat <puthik@xxxxxxxxxxxx> >> >> >> --- >> >> >> 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 4b65cf137f79..75a77ef257e2 100644 >> >> >> --- a/drivers/gpu/drm/i915/intel_dp.c >> >> >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> >> >> @@ -4763,7 +4763,8 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) >> >> >> */ >> >> >> status = connector_status_disconnected; >> >> >> goto out; >> >> >> - } else { >> >> >> + } else if (status != connector_status_connected || >> >> >> + intel_encoder->type != INTEL_OUTPUT_EDP) { >> >> >> /* >> >> >> * If display is now connected check links status, >> >> >> * there has been known issues of link loss triggerring >> >> >> @@ -4775,6 +4776,10 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) >> >> >> * going back up soon after. And once that happens we must >> >> >> * retrain the link to get a picture. That's in case no >> >> >> * userspace component reacted to intermittent HPD dip. >> >> >> + * >> >> >> + * Skip checking links status for connected eDP display. >> >> >> + * There are known issues of display blinking during checking >> >> >> + * link status and we don't have link loss for internal display. >> >> >> */ >> >> > >> >> > Inside intel_dp_check_link_status(), it actually checks if link status is good by >> >> > checking both clock recovery and Channel EQ bits in DPCD, only then it will retrain. >> >> > So in case of eDP panel, if there is no link loss then it should always return link >> >> > status as good and not retrain. >> >> > So IMHO I dont think we need to explicitly avoid this for eDP. >> >> > >> >> > When you see the display blinking, do you know for sure that drm_dp_channel_eq_ok() returns false? >> >> > Also look at this patch: https://patchwork.freedesktop.org/series/30670/ >> >> > This makes sure none of the eDP training parameters change unless link is bad. So with this patch, if the >> >> > link is good then it should never retrain even in intel_dp_check_link_status() for eDP. >> >> > >> >> > Manasi >> >> >> intel_dp_check_link_status(intel_dp); >> >> >> } >> >> >> -- >> >> >> 2.15.0.rc0.271.g36b669edcc-goog >> >> >> >> >> >> _______________________________________________ >> >> >> 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
