diff mbox series

[2/5] drm/i915/dp: Restrict link retrain workaround to external monitors

Message ID 20180918072009.4836-2-dhinakaran.pandiyan@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/5] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse() | expand

Commit Message

Dhinakaran Pandiyan Sept. 18, 2018, 7:20 a.m. UTC 
commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check,
unconditionally during long pulse"")' applies a work around for monitors
that don't signal link loss. Limit this only for external displays as
eDP features like PSR when active will have the link turned off and the
driver ends up retraining the link seeeing that link is not synchronized.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Jan-Marek Glogowski <glogow@fbihome.de>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Lyude Paul Sept. 18, 2018, 3:33 p.m. UTC | #1 
Did the patch this references get pushed? I saw it fly by and I thought we had
decided not to pull it in

On Tue, 2018-09-18 at 00:20 -0700, Dhinakaran Pandiyan wrote:
> commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check,
> unconditionally during long pulse"")' applies a work around for monitors
> that don't signal link loss. Limit this only for external displays as
> eDP features like PSR when active will have the link turned off and the
> driver ends up retraining the link seeeing that link is not synchronized.
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Jan-Marek Glogowski <glogow@fbihome.de>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check,
> unconditionally during long pulse"")
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cac1c7c6cbfd..8bf9afa5683c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5072,7 +5072,9 @@ intel_dp_long_pulse(struct intel_connector *connector,
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else {
> +	}
> +
> +	if (!intel_dp_is_edp(intel_dp)) {
>  		/*
>  		 * Some monitors do not signal loss of link synchronization
>  		 * with an IRQ_HPD, so force a link status check.
Rodrigo Vivi Sept. 18, 2018, 3:50 p.m. UTC | #2 
On Tue, Sep 18, 2018 at 11:33:24AM -0400, Lyude Paul wrote:
> Did the patch this references get pushed? I saw it fly by and I thought we had
> decided not to pull it in

ops, I created the confusion. Sorry.

What I decided that week was to not pull to drm-intel-fixes until Ville
or Jani acking. But it was already part of dinq, so moving to drm-next
and I didn't removed or reverted it.

So... But ville acked on irc and I pulled to -fixes on the following week
as well.

So yes, patch is there ;)

> 
> On Tue, 2018-09-18 at 00:20 -0700, Dhinakaran Pandiyan wrote:
> > commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check,
> > unconditionally during long pulse"")' applies a work around for monitors
> > that don't signal link loss. Limit this only for external displays as
> > eDP features like PSR when active will have the link turned off and the
> > driver ends up retraining the link seeeing that link is not synchronized.
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Jan-Marek Glogowski <glogow@fbihome.de>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check,
> > unconditionally during long pulse"")
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index cac1c7c6cbfd..8bf9afa5683c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5072,7 +5072,9 @@ intel_dp_long_pulse(struct intel_connector *connector,
> >  		 */
> >  		status = connector_status_disconnected;
> >  		goto out;
> > -	} else {
> > +	}
> > +
> > +	if (!intel_dp_is_edp(intel_dp)) {
> >  		/*
> >  		 * Some monitors do not signal loss of link synchronization
> >  		 * with an IRQ_HPD, so force a link status check.
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dhinakaran Pandiyan Sept. 19, 2018, 5:58 p.m. UTC | #3 
On Tue, 2018-09-18 at 08:50 -0700, Rodrigo Vivi wrote:
> On Tue, Sep 18, 2018 at 11:33:24AM -0400, Lyude Paul wrote:
> > Did the patch this references get pushed? I saw it fly by and I
> > thought we had
> > decided not to pull it in
> 
> ops, I created the confusion. Sorry.
> 
> What I decided that week was to not pull to drm-intel-fixes until
> Ville
> or Jani acking. But it was already part of dinq, so moving to drm-
> next
> and I didn't removed or reverted it.

I would have preferred to limit this work around to the specific
monitor it was needed for. The next best thing is to at least limit it
to external monitors? The code adds a bunch of extra dpcd reads to
check link status and acquires locks whenever user spaces wants a
->detect() and most sinks do not need this.

Let me know what your thoughts are.

-DK


> 
> So... But ville acked on irc and I pulled to -fixes on the following
> week
> as well.
> 
> So yes, patch is there ;)
> 
> > 
> > On Tue, 2018-09-18 at 00:20 -0700, Dhinakaran Pandiyan wrote:
> > > commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality
> > > check,
> > > unconditionally during long pulse"")' applies a work around for
> > > monitors
> > > that don't signal link loss. Limit this only for external
> > > displays as
> > > eDP features like PSR when active will have the link turned off
> > > and the
> > > driver ends up retraining the link seeeing that link is not
> > > synchronized.
> > > 
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Jan-Marek Glogowski <glogow@fbihome.de>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link
> > > quality check,
> > > unconditionally during long pulse"")
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > >
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index cac1c7c6cbfd..8bf9afa5683c 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5072,7 +5072,9 @@ intel_dp_long_pulse(struct intel_connector
> > > *connector,
> > >  		 */
> > >  		status = connector_status_disconnected;
> > >  		goto out;
> > > -	} else {
> > > +	}
> > > +
> > > +	if (!intel_dp_is_edp(intel_dp)) {
> > >  		/*
> > >  		 * Some monitors do not signal loss of link
> > > synchronization
> > >  		 * with an IRQ_HPD, so force a link status
> > > check.
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lyude Paul Sept. 19, 2018, 6:28 p.m. UTC | #4 
SGTM

On Wed, 2018-09-19 at 17:58 +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-09-18 at 08:50 -0700, Rodrigo Vivi wrote:
> > On Tue, Sep 18, 2018 at 11:33:24AM -0400, Lyude Paul wrote:
> > > Did the patch this references get pushed? I saw it fly by and I
> > > thought we had
> > > decided not to pull it in
> > 
> > ops, I created the confusion. Sorry.
> > 
> > What I decided that week was to not pull to drm-intel-fixes until
> > Ville
> > or Jani acking. But it was already part of dinq, so moving to drm-
> > next
> > and I didn't removed or reverted it.
> 
> I would have preferred to limit this work around to the specific
> monitor it was needed for. The next best thing is to at least limit it
> to external monitors? The code adds a bunch of extra dpcd reads to
> check link status and acquires locks whenever user spaces wants a
> ->detect() and most sinks do not need this.
> 
> Let me know what your thoughts are.
> 
> -DK
> 
> 
> > 
> > So... But ville acked on irc and I pulled to -fixes on the following
> > week
> > as well.
> > 
> > So yes, patch is there ;)
> > 
> > > 
> > > On Tue, 2018-09-18 at 00:20 -0700, Dhinakaran Pandiyan wrote:
> > > > commit '3cf71bc9904d ("drm/i915: Re-apply "Perform link quality
> > > > check,
> > > > unconditionally during long pulse"")' applies a work around for
> > > > monitors
> > > > that don't signal link loss. Limit this only for external
> > > > displays as
> > > > eDP features like PSR when active will have the link turned off
> > > > and the
> > > > driver ends up retraining the link seeeing that link is not
> > > > synchronized.
> > > > 
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Cc: Jan-Marek Glogowski <glogow@fbihome.de>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link
> > > > quality check,
> > > > unconditionally during long pulse"")
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > > 
> > > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index cac1c7c6cbfd..8bf9afa5683c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -5072,7 +5072,9 @@ intel_dp_long_pulse(struct intel_connector
> > > > *connector,
> > > >  		 */
> > > >  		status = connector_status_disconnected;
> > > >  		goto out;
> > > > -	} else {
> > > > +	}
> > > > +
> > > > +	if (!intel_dp_is_edp(intel_dp)) {
> > > >  		/*
> > > >  		 * Some monitors do not signal loss of link
> > > > synchronization
> > > >  		 * with an IRQ_HPD, so force a link status
> > > > check.
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cac1c7c6cbfd..8bf9afa5683c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5072,7 +5072,9 @@  intel_dp_long_pulse(struct intel_connector *connector,
 		 */
 		status = connector_status_disconnected;
 		goto out;
-	} else {
+	}
+
+	if (!intel_dp_is_edp(intel_dp)) {
 		/*
 		 * Some monitors do not signal loss of link synchronization
 		 * with an IRQ_HPD, so force a link status check.