[Intel-gfx] [PATCH] drm/i915: Don't recheck link status for eDP display.

[Puthikorn Voravootivat]

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 at intel.com> 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 at intel.com> 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 at intel.com> 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 at chromium.org>
>> >> >> ---
>> >> >>  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 at lists.freedesktop.org
>> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx