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

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

[Puthikorn Voravootivat]

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@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.
 		 */
 		intel_dp_check_link_status(intel_dp);
 	}
-- 
2.15.0.rc0.271.g36b669edcc-goog

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

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@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@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

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

[Puthikorn Voravootivat]

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.

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@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@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@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

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

[Manasi Navare]

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@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@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@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

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

[Puthikorn Voravootivat]

+ Vathsala for PSR related code.

On Tue, Oct 17, 2017 at 4:29 PM, Manasi Navare
<manasi.d.navare@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@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@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@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

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

[Rodrigo Vivi]

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@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@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@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@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

Re: [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@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@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@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@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@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

✓ Fi.CI.BAT: success for drm/i915: Don't recheck link status for eDP display.

[Patchwork]

== Series Details ==

Series: drm/i915: Don't recheck link status for eDP display.
URL   : https://patchwork.freedesktop.org/series/32164/
State : success

== Summary ==

Series 32164v1 drm/i915: Don't recheck link status for eDP display.
https://patchwork.freedesktop.org/api/1.0/series/32164/revisions/1/mbox/

Test chamelium:
        Subgroup dp-edid-read:
                incomplete -> SKIP       (fi-hsw-4770r)
        Subgroup dp-crc-fast:
                dmesg-fail -> PASS       (fi-kbl-7500u) fdo#102514

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:439s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:455s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:370s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:522s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:263s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:496s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:495s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:490s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:470s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:556s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:419s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:250s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:584s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:423s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:431s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:459s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:489s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:570s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:580s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:538s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:650s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:499s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:453s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:560s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:416s
fi-skl-6260u failed to connect after reboot

17972293741c44cf70b30bb740bc3d4f2333403c drm-tip: 2017y-10m-17d-18h-04m-07s UTC integration manifest
463a0b758573 drm/i915: Don't recheck link status for eDP display.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6083/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Don't recheck link status for eDP display.

[Patchwork]

== Series Details ==

Series: drm/i915: Don't recheck link status for eDP display.
URL   : https://patchwork.freedesktop.org/series/32164/
State : success

== Summary ==

Test kms_flip:
        Subgroup flip-vs-rmfb:
                pass       -> DMESG-WARN (shard-hsw) fdo#102614 +1
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912
Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252

fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2551 pass:1439 dwarn:2   dfail:0   fail:9   skip:1101 time:9323s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6083/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx