[PATCH] drm/i915: Do not reset detect_done flag in intel_dp_detect

[PATCH] drm/i915: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

The detect_done flag was introduced in the commit
7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c in order to avoid
multiple detects on hotplug where intel_dp_long_pulse()
was called from HPD handler as well as in intel_dp_detect.
So this detect_done flag was required to make sure intel_dp_detect()
did not call long pulse handler again if it was already been called
from HPD handler. However commit 1015811609c0328b5ed670d07748591b837e74eb
differs the long hpd handling entirely until the hotplug work runs to
avoid the double long hpd handling the "detect_done" flag is trying
to prevent.

So now we do not need to reset the detect_done flag to false in
intel_dp_detect. It will be reset in the intel_dp_hpd_pulse so
that intel_dp_detect does a full detect. However if the .detect
gets called during mode enumeration then we do not need to do a
full detect. This patch avoids the WARNS_ONS during connected boot
case when it calls intel_dp_check_link_status() due to multiple
detects and also avoids DP compliance failures. It avoids doing
a full detect every single time on .detect().

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index db75bb9..9c9277e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4470,8 +4470,6 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
 	if (!intel_dp->detect_done)
 		status = intel_dp_long_pulse(intel_dp->attached_connector);
 
-	intel_dp->detect_done = false;
-
 	return status;
 }
 
-- 
1.9.1

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

✓ Fi.CI.BAT: success for drm/i915: Do not reset detect_done flag in intel_dp_detect

[Patchwork]

== Series Details ==

Series: drm/i915: Do not reset detect_done flag in intel_dp_detect
URL   : https://patchwork.freedesktop.org/series/16456/
State : success

== Summary ==

Series 16456v1 drm/i915: Do not reset detect_done flag in intel_dp_detect
https://patchwork.freedesktop.org/api/1.0/series/16456/revisions/1/mbox/


fi-bdw-5557u     total:247  pass:218  dwarn:0   dfail:0   fail:0   skip:29 
fi-bsw-n3050     total:247  pass:194  dwarn:0   dfail:0   fail:0   skip:53 
fi-bxt-t5700     total:247  pass:205  dwarn:0   dfail:0   fail:0   skip:42 
fi-byt-j1900     total:247  pass:205  dwarn:0   dfail:0   fail:0   skip:42 
fi-byt-n2820     total:247  pass:201  dwarn:0   dfail:0   fail:0   skip:46 
fi-hsw-4770      total:247  pass:213  dwarn:0   dfail:0   fail:0   skip:34 
fi-hsw-4770r     total:247  pass:213  dwarn:0   dfail:0   fail:0   skip:34 
fi-ilk-650       total:247  pass:180  dwarn:0   dfail:0   fail:0   skip:67 
fi-ivb-3520m     total:247  pass:212  dwarn:0   dfail:0   fail:0   skip:35 
fi-ivb-3770      total:247  pass:211  dwarn:0   dfail:0   fail:0   skip:36 
fi-kbl-7500u     total:247  pass:211  dwarn:0   dfail:0   fail:0   skip:36 
fi-skl-6260u     total:247  pass:219  dwarn:0   dfail:0   fail:0   skip:28 
fi-skl-6700hq    total:247  pass:213  dwarn:0   dfail:0   fail:0   skip:34 
fi-skl-6700k     total:247  pass:209  dwarn:3   dfail:0   fail:0   skip:35 
fi-skl-6770hq    total:247  pass:219  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2520m     total:247  pass:201  dwarn:0   dfail:0   fail:0   skip:46 
fi-snb-2600      total:247  pass:200  dwarn:0   dfail:0   fail:0   skip:47 

08aa3ac5c18159c9105252903713cb21c27e9442 drm-tip: 2016y-12m-06d-18h-32m-52s UTC integration manifest
9e30950 drm/i915: Do not reset detect_done flag in intel_dp_detect

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3210/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not reset detect_done flag in intel_dp_detect

[Jani Nikula]

On Wed, 07 Dec 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> The detect_done flag was introduced in the commit
> 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c in order to avoid

The preferred format to cite commits is:

7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")

or

commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
Date:   Wed Mar 30 18:05:23 2016 +0530

    drm/i915: Cleaning up intel_dp_hpd_pulse


> multiple detects on hotplug where intel_dp_long_pulse()
> was called from HPD handler as well as in intel_dp_detect.
> So this detect_done flag was required to make sure intel_dp_detect()
> did not call long pulse handler again if it was already been called
> from HPD handler. However commit 1015811609c0328b5ed670d07748591b837e74eb

1015811609c0 ("drm/i915: Move long hpd handling into the hotplug work")

> differs the long hpd handling entirely until the hotplug work runs to
> avoid the double long hpd handling the "detect_done" flag is trying
> to prevent.
>
> So now we do not need to reset the detect_done flag to false in
> intel_dp_detect. It will be reset in the intel_dp_hpd_pulse so
> that intel_dp_detect does a full detect. However if the .detect
> gets called during mode enumeration then we do not need to do a
> full detect. This patch avoids the WARNS_ONS during connected boot

Please include such a backtrace in the commit message; it makes matching
bugs and fixes so much easier.

IIUC the warnings were introduced by 1015811609c0 ("drm/i915: Move long
hpd handling into the hotplug work"), and that's cc: stable, so this one
should be too, along with

Fixes: 1015811609c0 ("drm/i915: Move long hpd handling into the hotplug work")
Cc: stable@vger.kernel.org

Of course, someone(tm) will still need to make sure this is the right
fix...


BR,
Jani.


> case when it calls intel_dp_check_link_status() due to multiple
> detects and also avoids DP compliance failures. It avoids doing
> a full detect every single time on .detect().
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index db75bb9..9c9277e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4470,8 +4470,6 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>  	if (!intel_dp->detect_done)
>  		status = intel_dp_long_pulse(intel_dp->attached_connector);
>  
> -	intel_dp->detect_done = false;
> -
>  	return status;
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not reset detect_done flag in intel_dp_detect

[Ville Syrjälä]

On Tue, Dec 06, 2016 at 04:43:51PM -0800, Manasi Navare wrote:
> The detect_done flag was introduced in the commit
> 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c in order to avoid
> multiple detects on hotplug where intel_dp_long_pulse()
> was called from HPD handler as well as in intel_dp_detect.
> So this detect_done flag was required to make sure intel_dp_detect()
> did not call long pulse handler again if it was already been called
> from HPD handler. However commit 1015811609c0328b5ed670d07748591b837e74eb
> differs the long hpd handling entirely until the hotplug work runs to
> avoid the double long hpd handling the "detect_done" flag is trying
> to prevent.

That sentence doesn't parse here. Anyways, the flag indeed is now a nop
and your patch is pretty much the same what I did here:
https://patchwork.freedesktop.org/patch/101476/

> 
> So now we do not need to reset the detect_done flag to false in
> intel_dp_detect. It will be reset in the intel_dp_hpd_pulse so
> that intel_dp_detect does a full detect. However if the .detect
> gets called during mode enumeration then we do not need to do a
> full detect. This patch avoids the WARNS_ONS during connected boot
> case when it calls intel_dp_check_link_status() due to multiple
> detects

How exactly does it do that? Also we shouldn't sweep that under the rug
anyway. Instead someone should actually fix the problem that causes the
WARN.

> and also avoids DP compliance failures. It avoids doing
> a full detect every single time on .detect().
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index db75bb9..9c9277e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4470,8 +4470,6 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>  	if (!intel_dp->detect_done)
>  		status = intel_dp_long_pulse(intel_dp->attached_connector);
>  
> -	intel_dp->detect_done = false;
> -
>  	return status;
>  }
>  
> -- 
> 1.9.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

On Wed, Dec 07, 2016 at 04:05:56PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 06, 2016 at 04:43:51PM -0800, Manasi Navare wrote:
> > The detect_done flag was introduced in the commit
> > 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c in order to avoid
> > multiple detects on hotplug where intel_dp_long_pulse()
> > was called from HPD handler as well as in intel_dp_detect.
> > So this detect_done flag was required to make sure intel_dp_detect()
> > did not call long pulse handler again if it was already been called
> > from HPD handler. However commit 1015811609c0328b5ed670d07748591b837e74eb
> > differs the long hpd handling entirely until the hotplug work runs to
> > avoid the double long hpd handling the "detect_done" flag is trying
> > to prevent.
> 
> That sentence doesn't parse here. Anyways, the flag indeed is now a nop
> and your patch is pretty much the same what I did here:
> https://patchwork.freedesktop.org/patch/101476/
>

I looked at this patch and yes it is the same thing except that it keeps
the name of the flag unchanged plus previous patch is too old and wont
apply cleanly unless you rebase it. I will add the reseting of this flag in the
resume case. 
Thoughts?

> > 
> > So now we do not need to reset the detect_done flag to false in
> > intel_dp_detect. It will be reset in the intel_dp_hpd_pulse so
> > that intel_dp_detect does a full detect. However if the .detect
> > gets called during mode enumeration then we do not need to do a
> > full detect. This patch avoids the WARNS_ONS during connected boot
> > case when it calls intel_dp_check_link_status() due to multiple
> > detects
> 
> How exactly does it do that? Also we shouldn't sweep that under the rug
> anyway. Instead someone should actually fix the problem that causes the
> WARN.
>

Actually an additional call to intel_dp_long_pulse from intel_dp_detect()
because we are not resetting the detect_done flag, makes the function
call intel_dp_check_link_status() in the case when intel_dp->lane_count is 
0 and throws the WARN about that. So by resetting the flag we avoid this
unwanted call to intel_dp_long_pulse and that gets rid of the warning.

Manasi
 
> > and also avoids DP compliance failures. It avoids doing
> > a full detect every single time on .detect().
> > 
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index db75bb9..9c9277e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4470,8 +4470,6 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> >  	if (!intel_dp->detect_done)
> >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> >  
> > -	intel_dp->detect_done = false;
> > -
> >  	return status;
> >  }
> >  
> > -- 
> > 1.9.1
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

Ville,
You mentioned that this detect_done should be set to false also
in the resume case so in i915_display_resume where it calls
intel_hpd_init that eventually schedules the hpd_poll_init_work and
calls drm_helper_hpd_irq_event() which then calls .detect().
So in this case if I have to reset this detect_done in the
hpd_poll_init_work, how do I make sure I do it only if the
connector is DP?

Manasi 

On Wed, Dec 07, 2016 at 04:05:56PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 06, 2016 at 04:43:51PM -0800, Manasi Navare wrote:
> > The detect_done flag was introduced in the commit
> > 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c in order to avoid
> > multiple detects on hotplug where intel_dp_long_pulse()
> > was called from HPD handler as well as in intel_dp_detect.
> > So this detect_done flag was required to make sure intel_dp_detect()
> > did not call long pulse handler again if it was already been called
> > from HPD handler. However commit 1015811609c0328b5ed670d07748591b837e74eb
> > differs the long hpd handling entirely until the hotplug work runs to
> > avoid the double long hpd handling the "detect_done" flag is trying
> > to prevent.
> 
> That sentence doesn't parse here. Anyways, the flag indeed is now a nop
> and your patch is pretty much the same what I did here:
> https://patchwork.freedesktop.org/patch/101476/
> 
> > 
> > So now we do not need to reset the detect_done flag to false in
> > intel_dp_detect. It will be reset in the intel_dp_hpd_pulse so
> > that intel_dp_detect does a full detect. However if the .detect
> > gets called during mode enumeration then we do not need to do a
> > full detect. This patch avoids the WARNS_ONS during connected boot
> > case when it calls intel_dp_check_link_status() due to multiple
> > detects
> 
> How exactly does it do that? Also we shouldn't sweep that under the rug
> anyway. Instead someone should actually fix the problem that causes the
> WARN.
> 
> > and also avoids DP compliance failures. It avoids doing
> > a full detect every single time on .detect().
> > 
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index db75bb9..9c9277e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4470,8 +4470,6 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> >  	if (!intel_dp->detect_done)
> >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> >  
> > -	intel_dp->detect_done = false;
> > -
> >  	return status;
> >  }
> >  
> > -- 
> > 1.9.1
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not reset detect_done flag in intel_dp_detect

[Ville Syrjälä]

On Wed, Dec 07, 2016 at 07:51:21PM -0800, Manasi Navare wrote:
> Ville,
> You mentioned that this detect_done should be set to false also
> in the resume case so in i915_display_resume where it calls
> intel_hpd_init that eventually schedules the hpd_poll_init_work and
> calls drm_helper_hpd_irq_event() which then calls .detect().
> So in this case if I have to reset this detect_done in the
> hpd_poll_init_work, how do I make sure I do it only if the
> connector is DP?

We have the .reset hook for these sort of things.

> 
> Manasi 
> 
> On Wed, Dec 07, 2016 at 04:05:56PM +0200, Ville Syrjälä wrote:
> > On Tue, Dec 06, 2016 at 04:43:51PM -0800, Manasi Navare wrote:
> > > The detect_done flag was introduced in the commit
> > > 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c in order to avoid
> > > multiple detects on hotplug where intel_dp_long_pulse()
> > > was called from HPD handler as well as in intel_dp_detect.
> > > So this detect_done flag was required to make sure intel_dp_detect()
> > > did not call long pulse handler again if it was already been called
> > > from HPD handler. However commit 1015811609c0328b5ed670d07748591b837e74eb
> > > differs the long hpd handling entirely until the hotplug work runs to
> > > avoid the double long hpd handling the "detect_done" flag is trying
> > > to prevent.
> > 
> > That sentence doesn't parse here. Anyways, the flag indeed is now a nop
> > and your patch is pretty much the same what I did here:
> > https://patchwork.freedesktop.org/patch/101476/
> > 
> > > 
> > > So now we do not need to reset the detect_done flag to false in
> > > intel_dp_detect. It will be reset in the intel_dp_hpd_pulse so
> > > that intel_dp_detect does a full detect. However if the .detect
> > > gets called during mode enumeration then we do not need to do a
> > > full detect. This patch avoids the WARNS_ONS during connected boot
> > > case when it calls intel_dp_check_link_status() due to multiple
> > > detects
> > 
> > How exactly does it do that? Also we shouldn't sweep that under the rug
> > anyway. Instead someone should actually fix the problem that causes the
> > WARN.
> > 
> > > and also avoids DP compliance failures. It avoids doing
> > > a full detect every single time on .detect().
> > > 
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index db75bb9..9c9277e 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4470,8 +4470,6 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> > >  	if (!intel_dp->detect_done)
> > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > >  
> > > -	intel_dp->detect_done = false;
> > > -
> > >  	return status;
> > >  }
> > >  
> > > -- 
> > > 1.9.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

On Thu, Dec 08, 2016 at 05:58:44PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 07, 2016 at 07:51:21PM -0800, Manasi Navare wrote:
> > Ville,
> > You mentioned that this detect_done should be set to false also
> > in the resume case so in i915_display_resume where it calls
> > intel_hpd_init that eventually schedules the hpd_poll_init_work and
> > calls drm_helper_hpd_irq_event() which then calls .detect().
> > So in this case if I have to reset this detect_done in the
> > hpd_poll_init_work, how do I make sure I do it only if the
> > connector is DP?
> 
> We have the .reset hook for these sort of things.
>

How can I use .reset hook to set the intel_dp->detect_done flag
to false only for DP connector before
sending drm_hpd_irq_event()?

Manasi 
> > 
> > Manasi 
> > 
> > On Wed, Dec 07, 2016 at 04:05:56PM +0200, Ville Syrjälä wrote:
> > > On Tue, Dec 06, 2016 at 04:43:51PM -0800, Manasi Navare wrote:
> > > > The detect_done flag was introduced in the commit
> > > > 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c in order to avoid
> > > > multiple detects on hotplug where intel_dp_long_pulse()
> > > > was called from HPD handler as well as in intel_dp_detect.
> > > > So this detect_done flag was required to make sure intel_dp_detect()
> > > > did not call long pulse handler again if it was already been called
> > > > from HPD handler. However commit 1015811609c0328b5ed670d07748591b837e74eb
> > > > differs the long hpd handling entirely until the hotplug work runs to
> > > > avoid the double long hpd handling the "detect_done" flag is trying
> > > > to prevent.
> > > 
> > > That sentence doesn't parse here. Anyways, the flag indeed is now a nop
> > > and your patch is pretty much the same what I did here:
> > > https://patchwork.freedesktop.org/patch/101476/
> > > 
> > > > 
> > > > So now we do not need to reset the detect_done flag to false in
> > > > intel_dp_detect. It will be reset in the intel_dp_hpd_pulse so
> > > > that intel_dp_detect does a full detect. However if the .detect
> > > > gets called during mode enumeration then we do not need to do a
> > > > full detect. This patch avoids the WARNS_ONS during connected boot
> > > > case when it calls intel_dp_check_link_status() due to multiple
> > > > detects
> > > 
> > > How exactly does it do that? Also we shouldn't sweep that under the rug
> > > anyway. Instead someone should actually fix the problem that causes the
> > > WARN.
> > > 
> > > > and also avoids DP compliance failures. It avoids doing
> > > > a full detect every single time on .detect().
> > > > 
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 2 --
> > > >  1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index db75bb9..9c9277e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4470,8 +4470,6 @@ static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> > > >  	if (!intel_dp->detect_done)
> > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > >  
> > > > -	intel_dp->detect_done = false;
> > > > -
> > > >  	return status;
> > > >  }
> > > >  
> > > > -- 
> > > > 1.9.1
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Dhinakaran Pandiyan]

From: "Navare, Manasi D" <manasi.d.navare@intel.com>

The detect_done flag was introduced in the 'commit 7d23e3c37bb3
("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
detects on hotplug where intel_dp_long_pulse() was called from HPD handler
as well as intel_dp_detect(). Later, 'commit 1015811609c0
("drm/i915: Move long hpd handling into the hotplug work")' deferred long
hpd handling to hotplug work to avoid handling it twice. But, resetting the
flag after long hpd handling leads to the code being executed again during
mode enumeration.

So, do not reset the detect_done flag to false in intel_dp_detect. The flag
is reset in intel_dp_hpd_pulse to allow a full detect and set when the
hotplug work does a full DPCD detect. However if .detect() gets called
during enumeration we return the cached status.

v2:
Allow full detect after encoder reset. (Ville)
Set the detect_done even connector if disconnected (DK)
Commit message changes.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 45ebc96..5689cf3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4473,7 +4473,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_set_edid(intel_dp);
 	if (is_edp(intel_dp) || intel_connector->detect_edid)
 		status = connector_status_connected;
-	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -4508,10 +4507,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		      connector->base.id, connector->name);
 
 	/* If full detect is not performed yet, do a full detect */
-	if (!intel_dp->detect_done)
+	if (!intel_dp->detect_done) {
+		intel_dp->detect_done = true;
 		status = intel_dp_long_pulse(intel_dp->attached_connector);
-
-	intel_dp->detect_done = false;
+	}
 
 	return status;
 }
@@ -4803,6 +4802,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
 	if (lspcon->active)
 		lspcon_resume(lspcon);
 
+	intel_dp->detect_done = false;
+
 	if (to_intel_encoder(encoder)->type != INTEL_OUTPUT_EDP)
 		return;
 
-- 
2.7.4

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

Re: [PATCH v2] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Jani Nikula]

On Tue, 20 Dec 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> From: "Navare, Manasi D" <manasi.d.navare@intel.com>
>
> The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> as well as intel_dp_detect(). Later, 'commit 1015811609c0
> ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> hpd handling to hotplug work to avoid handling it twice. But, resetting the
> flag after long hpd handling leads to the code being executed again during
> mode enumeration.
>
> So, do not reset the detect_done flag to false in intel_dp_detect. The flag
> is reset in intel_dp_hpd_pulse to allow a full detect and set when the
> hotplug work does a full DPCD detect. However if .detect() gets called
> during enumeration we return the cached status.
>
> v2:
> Allow full detect after encoder reset. (Ville)
> Set the detect_done even connector if disconnected (DK)
> Commit message changes.
>

Fixes: ?

> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 45ebc96..5689cf3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4473,7 +4473,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  	intel_dp_set_edid(intel_dp);
>  	if (is_edp(intel_dp) || intel_connector->detect_edid)
>  		status = connector_status_connected;
> -	intel_dp->detect_done = true;
>  
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> @@ -4508,10 +4507,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  		      connector->base.id, connector->name);
>  
>  	/* If full detect is not performed yet, do a full detect */
> -	if (!intel_dp->detect_done)
> +	if (!intel_dp->detect_done) {
> +		intel_dp->detect_done = true;
>  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> -
> -	intel_dp->detect_done = false;
> +	}
>  
>  	return status;
>  }
> @@ -4803,6 +4802,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
>  	if (lspcon->active)
>  		lspcon_resume(lspcon);
>  
> +	intel_dp->detect_done = false;
> +
>  	if (to_intel_encoder(encoder)->type != INTEL_OUTPUT_EDP)
>  		return;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Pandiyan, Dhinakaran]

On Tue, 2016-12-20 at 10:08 +0200, Jani Nikula wrote:
> On Tue, 20 Dec 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> >
> > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > flag after long hpd handling leads to the code being executed again during
> > mode enumeration.
> >
> > So, do not reset the detect_done flag to false in intel_dp_detect. The flag
> > is reset in intel_dp_hpd_pulse to allow a full detect and set when the
> > hotplug work does a full DPCD detect. However if .detect() gets called
> > during enumeration we return the cached status.
> >
> > v2:
> > Allow full detect after encoder reset. (Ville)
> > Set the detect_done even connector if disconnected (DK)
> > Commit message changes.
> >
> 
> Fixes: ?
> 

There have been too many changes here, I don't know which commit to
really blame. Most likely 
Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")


-DK

> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 45ebc96..5689cf3 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4473,7 +4473,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  	intel_dp_set_edid(intel_dp);
> >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> >  		status = connector_status_connected;
> > -	intel_dp->detect_done = true;
> >  
> >  	/* Try to read the source of the interrupt */
> >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > @@ -4508,10 +4507,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> >  		      connector->base.id, connector->name);
> >  
> >  	/* If full detect is not performed yet, do a full detect */
> > -	if (!intel_dp->detect_done)
> > +	if (!intel_dp->detect_done) {
> > +		intel_dp->detect_done = true;
> >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > -
> > -	intel_dp->detect_done = false;
> > +	}
> >  
> >  	return status;
> >  }
> > @@ -4803,6 +4802,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> >  	if (lspcon->active)
> >  		lspcon_resume(lspcon);
> >  
> > +	intel_dp->detect_done = false;
> > +
> >  	if (to_intel_encoder(encoder)->type != INTEL_OUTPUT_EDP)
> >  		return;
> 

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

[PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Dhinakaran Pandiyan]

From: "Navare, Manasi D" <manasi.d.navare@intel.com>

The detect_done flag was introduced in the 'commit 7d23e3c37bb3
("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
detects on hotplug where intel_dp_long_pulse() was called from HPD handler
as well as intel_dp_detect(). Later, 'commit 1015811609c0
("drm/i915: Move long hpd handling into the hotplug work")' deferred long
hpd handling to hotplug work to avoid handling it twice. But, resetting the
flag after long hpd handling leads to the code being executed again during
mode enumeration.

So, do not reset the detect_done flag to False in intel_dp_detect(). The
flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
the hotplug work does a full DPCD detect. However if ->detect() gets called
during mode enumeration after a DPCD detect, return the cached connector
status.

Resetting the flag in the encoder's reset callback should take care of
hotplug between suspend/resume.

v2:
Allow full detect after encoder reset. (Ville)
Set the detect_done flag for connector disconnected case too. (DK)
Commit message changes.

Cc: stable@vger.kernel.org
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 45ebc96..5689cf3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4473,7 +4473,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_set_edid(intel_dp);
 	if (is_edp(intel_dp) || intel_connector->detect_edid)
 		status = connector_status_connected;
-	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -4508,10 +4507,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		      connector->base.id, connector->name);
 
 	/* If full detect is not performed yet, do a full detect */
-	if (!intel_dp->detect_done)
+	if (!intel_dp->detect_done) {
+		intel_dp->detect_done = true;
 		status = intel_dp_long_pulse(intel_dp->attached_connector);
-
-	intel_dp->detect_done = false;
+	}
 
 	return status;
 }
@@ -4803,6 +4802,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
 	if (lspcon->active)
 		lspcon_resume(lspcon);
 
+	intel_dp->detect_done = false;
+
 	if (to_intel_encoder(encoder)->type != INTEL_OUTPUT_EDP)
 		return;
 
-- 
2.7.4

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

On Thu, Jan 19, 2017 at 09:59:54PM +0200, Ville Syrj�l� wrote:
> On Thu, Jan 19, 2017 at 11:38:56AM -0800, Manasi Navare wrote:
> > On Thu, Jan 19, 2017 at 09:26:36PM +0200, Ville Syrj�l� wrote:
> > > On Thu, Jan 19, 2017 at 11:01:01AM -0800, Manasi Navare wrote:
> > > > On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrj�l� wrote:
> > > > > On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > > > > > I have verified this patch with the latest drm-tip and it is also
> > > > > > absolutely necessary to ensure the link gets properly retrained
> > > > > > after link-status is BAD and after anew modeset is triggered by
> > > > > > userspace. Without this patch, since intel_dp_long_pulse gets called
> > > > > > it resets the link failure values and link retraining does not happen
> > > > > > at lower link rates.
> > > > > 
> > > > > Why are you resetting the failure values in this function? You should
> > > > > only do that when a long pulse is actually detected IMO (and yes, 
> > > > > calling the function intel_dp_long_pulse() name is pretty much wrong
> > > > > now).
> > > > >
> > > > 
> > > > Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
> > > > and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
> > > > get called on hotplug and that we need reread the dpcd registers. So setting these to max values
> > > > mean that we have lost the information about the link rate and lane count at which link training failed.
> > > > 
> > > > If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
> > > > then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
> > > > lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
> > > > So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()
> > > 
> > > No. What you're asking for is a change in behaviour (to call
> > > intel_dp_long_pulse() from ->detect() only if it was just preceded by an
> > > actual long pulse). That was never how this code worked. The point of
> > > the flag was to avoid calling it twice when processing the HPD since it
> > > was directly called from intel_dp_hpd_pulse() and then again from
> > > ->detect(). Since we no longer call it directly from
> > > intel_dp_hpd_pulse() the flag is in fact useless.
> > >
> > 
> > No I think there is some confusion. I am not asking the change in behaviour. I dont
> > want to call intel_dp_long_pulse from ->detect() only if it was just preceeded by an actual long pulse.
> 
> That is precisely what you're asking since intel_dp_hpd_pulse() would
> be the only place left that would reset the flag.
> 
> > I want to avoid calling it twice and which exactly was the point of adding that flag.
> 
> The commit that added the flag did exactly what I stated above. It
> didn't avoid any calls via mode enumeration, it only avoid the double
> call during actual hpd handling.
> 
> Which is also how the code behaved before someone tried to move the
> long hpd handling into the hpd pulse handler, and it's how the code
> behaves now that I moved the long hpd handling back into ->detect().
> 
> > The long pulse should
> > be called only when the full detect is not done, we shouldnt be doing full detect twice. In intel_dp_hpd_pulse()
> > we do set the detect_done flag to false and it does a full detect through ->detect(). But then
> > for mode enumeration, we do not need to do a full detect so that ->detect() call should actually not
> > call intel_dp_long_pulse because that is exactly the behavour we were trying to avoid (calling it twice)
> > 
> > Manasi 
> > > While the change you ask for may be desirable, history has shown
> > > that the DP code is very fragile, so I don't think we should make
> > > the link state property depend on something that may need to be
> > > reverted if a regression crops up. And so I think you should just
> > > change your new code to work with the existing scheme. We can push
> > > the detect_done optimization in afterwards since we should then be
> > > able to revert it without having to revert everything related to
> > > the link status property.
> > >

So currently it reads all the dpcd values on hotplug and then it rereads 
everything on drm_helper_probe_single_connector_modes()
So in case of link failure I set the maxlinkrate and lane count to the values lower
than the values at which link training failed, but when userspace tries to redo a modeset,
it calls drm_helper_probe_single_connector_modes() and calls detect() which calls long_pulse
and rewrites the max link rate and lane count and so instead of training at lower values it
trains again at the same values .
So the other alternative is splitting the intel_dp_long_pulse() and do parts of it like
read the dpcd registers i.e call intel_dp_detect_dpcd() and set the max link rate and
max lane count in intel_dp_hpd_pulse(). That way we set the max values only once at
hotplug and avoid them getting rewritten during mode enumeration.

Any thoughts Ville/Jani/Daniel?

Manasi
> > > > 
> > > > Manasi 
> > > > > > 
> > > > > > Ville/Jani could you please review this patch?
> > > > > > Thi is definitely a bug in the existing codebase and we need to get this merged
> > > > > > soon to get it fixed.
> > > > > > 
> > > > > > Regards
> > > > > > Manasi
> > > > > > 
> > > > > > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > > > > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > > > > > 
> > > > > > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > > > > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > > > > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > > > > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > > > > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > > > > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > > > > > flag after long hpd handling leads to the code being executed again during
> > > > > > > mode enumeration.
> > > > > > > 
> > > > > > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > > > > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > > > > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > > > > > during mode enumeration after a DPCD detect, return the cached connector
> > > > > > > status.
> > > > > > > 
> > > > > > > Resetting the flag in the encoder's reset callback should take care of
> > > > > > > hotplug between suspend/resume.
> > > > > > > 
> > > > > > > v2:
> > > > > > > Allow full detect after encoder reset. (Ville)
> > > > > > > Set the detect_done flag for connector disconnected case too. (DK)
> > > > > > > Commit message changes.
> > > > > > > 
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > > > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > index fb12896..6732c17 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > > > > >  	intel_dp_set_edid(intel_dp);
> > > > > > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > > > > >  		status = connector_status_connected;
> > > > > > > -	intel_dp->detect_done = true;
> > > > > > >  
> > > > > > >  	/* Try to read the source of the interrupt */
> > > > > > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > > > > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > > > > > >  		      connector->base.id, connector->name);
> > > > > > >  
> > > > > > >  	/* If full detect is not performed yet, do a full detect */
> > > > > > > -	if (!intel_dp->detect_done)
> > > > > > > +	if (!intel_dp->detect_done) {
> > > > > > > +		intel_dp->detect_done = true;
> > > > > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > > > -
> > > > > > > -	intel_dp->detect_done = false;
> > > > > > > +	}
> > > > > > >  
> > > > > > >  	return status;
> > > > > >  }
> > > > > > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > > > > >  	if (lspcon->active)
> > > > > > >  		lspcon_resume(lspcon);
> > > > > > >  
> > > > > > > +	intel_dp->detect_done = false;
> > > > > > > +
> > > > > > >  	pps_lock(intel_dp);
> > > > > > >  
> > > > > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > > -- 
> > > > > > > 2.7.4
> > > > > > > 
> > > > > 
> > > > > -- 
> > > > > Ville Syrj�l�
> > > > > Intel OTC
> > > 
> > > -- 
> > > Ville Syrj�l�
> > > Intel OTC
> 
> -- 
> Ville Syrj�l�
> Intel OTC

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

On Thu, Jan 19, 2017 at 09:59:54PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 19, 2017 at 11:38:56AM -0800, Manasi Navare wrote:
> > On Thu, Jan 19, 2017 at 09:26:36PM +0200, Ville Syrjälä wrote:
> > > On Thu, Jan 19, 2017 at 11:01:01AM -0800, Manasi Navare wrote:
> > > > On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > > > > > I have verified this patch with the latest drm-tip and it is also
> > > > > > absolutely necessary to ensure the link gets properly retrained
> > > > > > after link-status is BAD and after anew modeset is triggered by
> > > > > > userspace. Without this patch, since intel_dp_long_pulse gets called
> > > > > > it resets the link failure values and link retraining does not happen
> > > > > > at lower link rates.
> > > > > 
> > > > > Why are you resetting the failure values in this function? You should
> > > > > only do that when a long pulse is actually detected IMO (and yes, 
> > > > > calling the function intel_dp_long_pulse() name is pretty much wrong
> > > > > now).
> > > > >
> > > > 
> > > > Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
> > > > and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
> > > > get called on hotplug and that we need reread the dpcd registers. So setting these to max values
> > > > mean that we have lost the information about the link rate and lane count at which link training failed.
> > > > 
> > > > If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
> > > > then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
> > > > lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
> > > > So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()
> > > 
> > > No. What you're asking for is a change in behaviour (to call
> > > intel_dp_long_pulse() from ->detect() only if it was just preceded by an
> > > actual long pulse). That was never how this code worked. The point of
> > > the flag was to avoid calling it twice when processing the HPD since it
> > > was directly called from intel_dp_hpd_pulse() and then again from
> > > ->detect(). Since we no longer call it directly from
> > > intel_dp_hpd_pulse() the flag is in fact useless.
> > >
> > 
> > No I think there is some confusion. I am not asking the change in behaviour. I dont
> > want to call intel_dp_long_pulse from ->detect() only if it was just preceeded by an actual long pulse.
> 
> That is precisely what you're asking since intel_dp_hpd_pulse() would
> be the only place left that would reset the flag.
> 
> > I want to avoid calling it twice and which exactly was the point of adding that flag.
> 
> The commit that added the flag did exactly what I stated above. It
> didn't avoid any calls via mode enumeration, it only avoid the double
> call during actual hpd handling.
> 
> Which is also how the code behaved before someone tried to move the
> long hpd handling into the hpd pulse handler, and it's how the code
> behaves now that I moved the long hpd handling back into ->detect().
> 
> > The long pulse should
> > be called only when the full detect is not done, we shouldnt be doing full detect twice. In intel_dp_hpd_pulse()
> > we do set the detect_done flag to false and it does a full detect through ->detect(). But then
> > for mode enumeration, we do not need to do a full detect so that ->detect() call should actually not
> > call intel_dp_long_pulse because that is exactly the behavour we were trying to avoid (calling it twice)
> > 
> > Manasi 
> > > While the change you ask for may be desirable, history has shown
> > > that the DP code is very fragile, so I don't think we should make
> > > the link state property depend on something that may need to be
> > > reverted if a regression crops up. And so I think you should just
> > > change your new code to work with the existing scheme. We can push
> > > the detect_done optimization in afterwards since we should then be
> > > able to revert it without having to revert everything related to
> > > the link status property.
> > >

So currently it reads all the dpcd values on hotplug and then it rereads 
everything on drm_helper_probe_single_connector_modes()
So in case of link failure I set the maxlinkrate and lane count to the values lower
than the values at which link training failed, but when userspace tries to redo a modeset,
it calls drm_helper_probe_single_connector_modes() and calls detect() which calls long_pulse
and rewrites the max link rate and lane count and so instead of training at lower values it
trains again at the same values .
So the other alternative is splitting the intel_dp_long_pulse() and do parts of it like
read the dpcd registers i.e call intel_dp_detect_dpcd() and set the max link rate and
max lane count in intel_dp_hpd_pulse(). That way we set the max values only once at
hotplug and avoid them getting rewritten during mode enumeration.

Any thoughts Ville/Jani/Daniel?

Manasi
> > > > 
> > > > Manasi 
> > > > > > 
> > > > > > Ville/Jani could you please review this patch?
> > > > > > Thi is definitely a bug in the existing codebase and we need to get this merged
> > > > > > soon to get it fixed.
> > > > > > 
> > > > > > Regards
> > > > > > Manasi
> > > > > > 
> > > > > > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > > > > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > > > > > 
> > > > > > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > > > > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > > > > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > > > > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > > > > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > > > > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > > > > > flag after long hpd handling leads to the code being executed again during
> > > > > > > mode enumeration.
> > > > > > > 
> > > > > > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > > > > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > > > > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > > > > > during mode enumeration after a DPCD detect, return the cached connector
> > > > > > > status.
> > > > > > > 
> > > > > > > Resetting the flag in the encoder's reset callback should take care of
> > > > > > > hotplug between suspend/resume.
> > > > > > > 
> > > > > > > v2:
> > > > > > > Allow full detect after encoder reset. (Ville)
> > > > > > > Set the detect_done flag for connector disconnected case too. (DK)
> > > > > > > Commit message changes.
> > > > > > > 
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > > > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > index fb12896..6732c17 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > > > > >  	intel_dp_set_edid(intel_dp);
> > > > > > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > > > > >  		status = connector_status_connected;
> > > > > > > -	intel_dp->detect_done = true;
> > > > > > >  
> > > > > > >  	/* Try to read the source of the interrupt */
> > > > > > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > > > > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > > > > > >  		      connector->base.id, connector->name);
> > > > > > >  
> > > > > > >  	/* If full detect is not performed yet, do a full detect */
> > > > > > > -	if (!intel_dp->detect_done)
> > > > > > > +	if (!intel_dp->detect_done) {
> > > > > > > +		intel_dp->detect_done = true;
> > > > > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > > > -
> > > > > > > -	intel_dp->detect_done = false;
> > > > > > > +	}
> > > > > > >  
> > > > > > >  	return status;
> > > > > >  }
> > > > > > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > > > > >  	if (lspcon->active)
> > > > > > >  		lspcon_resume(lspcon);
> > > > > > >  
> > > > > > > +	intel_dp->detect_done = false;
> > > > > > > +
> > > > > > >  	pps_lock(intel_dp);
> > > > > > >  
> > > > > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > > -- 
> > > > > > > 2.7.4
> > > > > > > 
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Ville Syrjälä]

On Thu, Jan 19, 2017 at 11:38:56AM -0800, Manasi Navare wrote:
> On Thu, Jan 19, 2017 at 09:26:36PM +0200, Ville Syrj�l� wrote:
> > On Thu, Jan 19, 2017 at 11:01:01AM -0800, Manasi Navare wrote:
> > > On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrj�l� wrote:
> > > > On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > > > > I have verified this patch with the latest drm-tip and it is also
> > > > > absolutely necessary to ensure the link gets properly retrained
> > > > > after link-status is BAD and after anew modeset is triggered by
> > > > > userspace. Without this patch, since intel_dp_long_pulse gets called
> > > > > it resets the link failure values and link retraining does not happen
> > > > > at lower link rates.
> > > > 
> > > > Why are you resetting the failure values in this function? You should
> > > > only do that when a long pulse is actually detected IMO (and yes, 
> > > > calling the function intel_dp_long_pulse() name is pretty much wrong
> > > > now).
> > > >
> > > 
> > > Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
> > > and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
> > > get called on hotplug and that we need reread the dpcd registers. So setting these to max values
> > > mean that we have lost the information about the link rate and lane count at which link training failed.
> > > 
> > > If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
> > > then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
> > > lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
> > > So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()
> > 
> > No. What you're asking for is a change in behaviour (to call
> > intel_dp_long_pulse() from ->detect() only if it was just preceded by an
> > actual long pulse). That was never how this code worked. The point of
> > the flag was to avoid calling it twice when processing the HPD since it
> > was directly called from intel_dp_hpd_pulse() and then again from
> > ->detect(). Since we no longer call it directly from
> > intel_dp_hpd_pulse() the flag is in fact useless.
> >
> 
> No I think there is some confusion. I am not asking the change in behaviour. I dont
> want to call intel_dp_long_pulse from ->detect() only if it was just preceeded by an actual long pulse.

That is precisely what you're asking since intel_dp_hpd_pulse() would
be the only place left that would reset the flag.

> I want to avoid calling it twice and which exactly was the point of adding that flag.

The commit that added the flag did exactly what I stated above. It
didn't avoid any calls via mode enumeration, it only avoid the double
call during actual hpd handling.

Which is also how the code behaved before someone tried to move the
long hpd handling into the hpd pulse handler, and it's how the code
behaves now that I moved the long hpd handling back into ->detect().

> The long pulse should
> be called only when the full detect is not done, we shouldnt be doing full detect twice. In intel_dp_hpd_pulse()
> we do set the detect_done flag to false and it does a full detect through ->detect(). But then
> for mode enumeration, we do not need to do a full detect so that ->detect() call should actually not
> call intel_dp_long_pulse because that is exactly the behavour we were trying to avoid (calling it twice)
> 
> Manasi 
> > While the change you ask for may be desirable, history has shown
> > that the DP code is very fragile, so I don't think we should make
> > the link state property depend on something that may need to be
> > reverted if a regression crops up. And so I think you should just
> > change your new code to work with the existing scheme. We can push
> > the detect_done optimization in afterwards since we should then be
> > able to revert it without having to revert everything related to
> > the link status property.
> > 
> > > 
> > > Manasi 
> > > > > 
> > > > > Ville/Jani could you please review this patch?
> > > > > Thi is definitely a bug in the existing codebase and we need to get this merged
> > > > > soon to get it fixed.
> > > > > 
> > > > > Regards
> > > > > Manasi
> > > > > 
> > > > > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > > > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > > > > 
> > > > > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > > > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > > > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > > > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > > > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > > > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > > > > flag after long hpd handling leads to the code being executed again during
> > > > > > mode enumeration.
> > > > > > 
> > > > > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > > > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > > > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > > > > during mode enumeration after a DPCD detect, return the cached connector
> > > > > > status.
> > > > > > 
> > > > > > Resetting the flag in the encoder's reset callback should take care of
> > > > > > hotplug between suspend/resume.
> > > > > > 
> > > > > > v2:
> > > > > > Allow full detect after encoder reset. (Ville)
> > > > > > Set the detect_done flag for connector disconnected case too. (DK)
> > > > > > Commit message changes.
> > > > > > 
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index fb12896..6732c17 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > > > >  	intel_dp_set_edid(intel_dp);
> > > > > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > > > >  		status = connector_status_connected;
> > > > > > -	intel_dp->detect_done = true;
> > > > > >  
> > > > > >  	/* Try to read the source of the interrupt */
> > > > > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > > > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > > > > >  		      connector->base.id, connector->name);
> > > > > >  
> > > > > >  	/* If full detect is not performed yet, do a full detect */
> > > > > > -	if (!intel_dp->detect_done)
> > > > > > +	if (!intel_dp->detect_done) {
> > > > > > +		intel_dp->detect_done = true;
> > > > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > > -
> > > > > > -	intel_dp->detect_done = false;
> > > > > > +	}
> > > > > >  
> > > > > >  	return status;
> > > > >  }
> > > > > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > > > >  	if (lspcon->active)
> > > > > >  		lspcon_resume(lspcon);
> > > > > >  
> > > > > > +	intel_dp->detect_done = false;
> > > > > > +
> > > > > >  	pps_lock(intel_dp);
> > > > > >  
> > > > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > -- 
> > > > > > 2.7.4
> > > > > > 
> > > > 
> > > > -- 
> > > > Ville Syrj�l�
> > > > Intel OTC
> > 
> > -- 
> > Ville Syrj�l�
> > Intel OTC

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Ville Syrjälä]

On Thu, Jan 19, 2017 at 11:38:56AM -0800, Manasi Navare wrote:
> On Thu, Jan 19, 2017 at 09:26:36PM +0200, Ville Syrjälä wrote:
> > On Thu, Jan 19, 2017 at 11:01:01AM -0800, Manasi Navare wrote:
> > > On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > > > > I have verified this patch with the latest drm-tip and it is also
> > > > > absolutely necessary to ensure the link gets properly retrained
> > > > > after link-status is BAD and after anew modeset is triggered by
> > > > > userspace. Without this patch, since intel_dp_long_pulse gets called
> > > > > it resets the link failure values and link retraining does not happen
> > > > > at lower link rates.
> > > > 
> > > > Why are you resetting the failure values in this function? You should
> > > > only do that when a long pulse is actually detected IMO (and yes, 
> > > > calling the function intel_dp_long_pulse() name is pretty much wrong
> > > > now).
> > > >
> > > 
> > > Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
> > > and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
> > > get called on hotplug and that we need reread the dpcd registers. So setting these to max values
> > > mean that we have lost the information about the link rate and lane count at which link training failed.
> > > 
> > > If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
> > > then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
> > > lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
> > > So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()
> > 
> > No. What you're asking for is a change in behaviour (to call
> > intel_dp_long_pulse() from ->detect() only if it was just preceded by an
> > actual long pulse). That was never how this code worked. The point of
> > the flag was to avoid calling it twice when processing the HPD since it
> > was directly called from intel_dp_hpd_pulse() and then again from
> > ->detect(). Since we no longer call it directly from
> > intel_dp_hpd_pulse() the flag is in fact useless.
> >
> 
> No I think there is some confusion. I am not asking the change in behaviour. I dont
> want to call intel_dp_long_pulse from ->detect() only if it was just preceeded by an actual long pulse.

That is precisely what you're asking since intel_dp_hpd_pulse() would
be the only place left that would reset the flag.

> I want to avoid calling it twice and which exactly was the point of adding that flag.

The commit that added the flag did exactly what I stated above. It
didn't avoid any calls via mode enumeration, it only avoid the double
call during actual hpd handling.

Which is also how the code behaved before someone tried to move the
long hpd handling into the hpd pulse handler, and it's how the code
behaves now that I moved the long hpd handling back into ->detect().

> The long pulse should
> be called only when the full detect is not done, we shouldnt be doing full detect twice. In intel_dp_hpd_pulse()
> we do set the detect_done flag to false and it does a full detect through ->detect(). But then
> for mode enumeration, we do not need to do a full detect so that ->detect() call should actually not
> call intel_dp_long_pulse because that is exactly the behavour we were trying to avoid (calling it twice)
> 
> Manasi 
> > While the change you ask for may be desirable, history has shown
> > that the DP code is very fragile, so I don't think we should make
> > the link state property depend on something that may need to be
> > reverted if a regression crops up. And so I think you should just
> > change your new code to work with the existing scheme. We can push
> > the detect_done optimization in afterwards since we should then be
> > able to revert it without having to revert everything related to
> > the link status property.
> > 
> > > 
> > > Manasi 
> > > > > 
> > > > > Ville/Jani could you please review this patch?
> > > > > Thi is definitely a bug in the existing codebase and we need to get this merged
> > > > > soon to get it fixed.
> > > > > 
> > > > > Regards
> > > > > Manasi
> > > > > 
> > > > > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > > > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > > > > 
> > > > > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > > > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > > > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > > > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > > > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > > > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > > > > flag after long hpd handling leads to the code being executed again during
> > > > > > mode enumeration.
> > > > > > 
> > > > > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > > > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > > > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > > > > during mode enumeration after a DPCD detect, return the cached connector
> > > > > > status.
> > > > > > 
> > > > > > Resetting the flag in the encoder's reset callback should take care of
> > > > > > hotplug between suspend/resume.
> > > > > > 
> > > > > > v2:
> > > > > > Allow full detect after encoder reset. (Ville)
> > > > > > Set the detect_done flag for connector disconnected case too. (DK)
> > > > > > Commit message changes.
> > > > > > 
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index fb12896..6732c17 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > > > >  	intel_dp_set_edid(intel_dp);
> > > > > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > > > >  		status = connector_status_connected;
> > > > > > -	intel_dp->detect_done = true;
> > > > > >  
> > > > > >  	/* Try to read the source of the interrupt */
> > > > > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > > > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > > > > >  		      connector->base.id, connector->name);
> > > > > >  
> > > > > >  	/* If full detect is not performed yet, do a full detect */
> > > > > > -	if (!intel_dp->detect_done)
> > > > > > +	if (!intel_dp->detect_done) {
> > > > > > +		intel_dp->detect_done = true;
> > > > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > > -
> > > > > > -	intel_dp->detect_done = false;
> > > > > > +	}
> > > > > >  
> > > > > >  	return status;
> > > > >  }
> > > > > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > > > >  	if (lspcon->active)
> > > > > >  		lspcon_resume(lspcon);
> > > > > >  
> > > > > > +	intel_dp->detect_done = false;
> > > > > > +
> > > > > >  	pps_lock(intel_dp);
> > > > > >  
> > > > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > -- 
> > > > > > 2.7.4
> > > > > > 
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel OTC
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

On Thu, Jan 19, 2017 at 09:26:36PM +0200, Ville Syrj�l� wrote:
> On Thu, Jan 19, 2017 at 11:01:01AM -0800, Manasi Navare wrote:
> > On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrj�l� wrote:
> > > On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > > > I have verified this patch with the latest drm-tip and it is also
> > > > absolutely necessary to ensure the link gets properly retrained
> > > > after link-status is BAD and after anew modeset is triggered by
> > > > userspace. Without this patch, since intel_dp_long_pulse gets called
> > > > it resets the link failure values and link retraining does not happen
> > > > at lower link rates.
> > > 
> > > Why are you resetting the failure values in this function? You should
> > > only do that when a long pulse is actually detected IMO (and yes, 
> > > calling the function intel_dp_long_pulse() name is pretty much wrong
> > > now).
> > >
> > 
> > Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
> > and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
> > get called on hotplug and that we need reread the dpcd registers. So setting these to max values
> > mean that we have lost the information about the link rate and lane count at which link training failed.
> > 
> > If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
> > then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
> > lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
> > So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()
> 
> No. What you're asking for is a change in behaviour (to call
> intel_dp_long_pulse() from ->detect() only if it was just preceded by an
> actual long pulse). That was never how this code worked. The point of
> the flag was to avoid calling it twice when processing the HPD since it
> was directly called from intel_dp_hpd_pulse() and then again from
> ->detect(). Since we no longer call it directly from
> intel_dp_hpd_pulse() the flag is in fact useless.
>

No I think there is some confusion. I am not asking the change in behaviour. I dont
want to call intel_dp_long_pulse from ->detect() only if it was just preceeded by an actual long pulse.
I want to avoid calling it twice and which exactly was the point of adding that flag. The long pulse should
be called only when the full detect is not done, we shouldnt be doing full detect twice. In intel_dp_hpd_pulse()
we do set the detect_done flag to false and it does a full detect through ->detect(). But then
for mode enumeration, we do not need to do a full detect so that ->detect() call should actually not
call intel_dp_long_pulse because that is exactly the behavour we were trying to avoid (calling it twice)

Manasi 
> While the change you ask for may be desirable, history has shown
> that the DP code is very fragile, so I don't think we should make
> the link state property depend on something that may need to be
> reverted if a regression crops up. And so I think you should just
> change your new code to work with the existing scheme. We can push
> the detect_done optimization in afterwards since we should then be
> able to revert it without having to revert everything related to
> the link status property.
> 
> > 
> > Manasi 
> > > > 
> > > > Ville/Jani could you please review this patch?
> > > > Thi is definitely a bug in the existing codebase and we need to get this merged
> > > > soon to get it fixed.
> > > > 
> > > > Regards
> > > > Manasi
> > > > 
> > > > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > > > 
> > > > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > > > flag after long hpd handling leads to the code being executed again during
> > > > > mode enumeration.
> > > > > 
> > > > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > > > during mode enumeration after a DPCD detect, return the cached connector
> > > > > status.
> > > > > 
> > > > > Resetting the flag in the encoder's reset callback should take care of
> > > > > hotplug between suspend/resume.
> > > > > 
> > > > > v2:
> > > > > Allow full detect after encoder reset. (Ville)
> > > > > Set the detect_done flag for connector disconnected case too. (DK)
> > > > > Commit message changes.
> > > > > 
> > > > > Cc: stable@vger.kernel.org
> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index fb12896..6732c17 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > > >  	intel_dp_set_edid(intel_dp);
> > > > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > > >  		status = connector_status_connected;
> > > > > -	intel_dp->detect_done = true;
> > > > >  
> > > > >  	/* Try to read the source of the interrupt */
> > > > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > > > >  		      connector->base.id, connector->name);
> > > > >  
> > > > >  	/* If full detect is not performed yet, do a full detect */
> > > > > -	if (!intel_dp->detect_done)
> > > > > +	if (!intel_dp->detect_done) {
> > > > > +		intel_dp->detect_done = true;
> > > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > -
> > > > > -	intel_dp->detect_done = false;
> > > > > +	}
> > > > >  
> > > > >  	return status;
> > > >  }
> > > > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > > >  	if (lspcon->active)
> > > > >  		lspcon_resume(lspcon);
> > > > >  
> > > > > +	intel_dp->detect_done = false;
> > > > > +
> > > > >  	pps_lock(intel_dp);
> > > > >  
> > > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > 
> > > -- 
> > > Ville Syrj�l�
> > > Intel OTC
> 
> -- 
> Ville Syrj�l�
> Intel OTC

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

On Thu, Jan 19, 2017 at 09:26:36PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 19, 2017 at 11:01:01AM -0800, Manasi Navare wrote:
> > On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrjälä wrote:
> > > On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > > > I have verified this patch with the latest drm-tip and it is also
> > > > absolutely necessary to ensure the link gets properly retrained
> > > > after link-status is BAD and after anew modeset is triggered by
> > > > userspace. Without this patch, since intel_dp_long_pulse gets called
> > > > it resets the link failure values and link retraining does not happen
> > > > at lower link rates.
> > > 
> > > Why are you resetting the failure values in this function? You should
> > > only do that when a long pulse is actually detected IMO (and yes, 
> > > calling the function intel_dp_long_pulse() name is pretty much wrong
> > > now).
> > >
> > 
> > Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
> > and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
> > get called on hotplug and that we need reread the dpcd registers. So setting these to max values
> > mean that we have lost the information about the link rate and lane count at which link training failed.
> > 
> > If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
> > then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
> > lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
> > So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()
> 
> No. What you're asking for is a change in behaviour (to call
> intel_dp_long_pulse() from ->detect() only if it was just preceded by an
> actual long pulse). That was never how this code worked. The point of
> the flag was to avoid calling it twice when processing the HPD since it
> was directly called from intel_dp_hpd_pulse() and then again from
> ->detect(). Since we no longer call it directly from
> intel_dp_hpd_pulse() the flag is in fact useless.
>

No I think there is some confusion. I am not asking the change in behaviour. I dont
want to call intel_dp_long_pulse from ->detect() only if it was just preceeded by an actual long pulse.
I want to avoid calling it twice and which exactly was the point of adding that flag. The long pulse should
be called only when the full detect is not done, we shouldnt be doing full detect twice. In intel_dp_hpd_pulse()
we do set the detect_done flag to false and it does a full detect through ->detect(). But then
for mode enumeration, we do not need to do a full detect so that ->detect() call should actually not
call intel_dp_long_pulse because that is exactly the behavour we were trying to avoid (calling it twice)

Manasi 
> While the change you ask for may be desirable, history has shown
> that the DP code is very fragile, so I don't think we should make
> the link state property depend on something that may need to be
> reverted if a regression crops up. And so I think you should just
> change your new code to work with the existing scheme. We can push
> the detect_done optimization in afterwards since we should then be
> able to revert it without having to revert everything related to
> the link status property.
> 
> > 
> > Manasi 
> > > > 
> > > > Ville/Jani could you please review this patch?
> > > > Thi is definitely a bug in the existing codebase and we need to get this merged
> > > > soon to get it fixed.
> > > > 
> > > > Regards
> > > > Manasi
> > > > 
> > > > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > > > 
> > > > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > > > flag after long hpd handling leads to the code being executed again during
> > > > > mode enumeration.
> > > > > 
> > > > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > > > during mode enumeration after a DPCD detect, return the cached connector
> > > > > status.
> > > > > 
> > > > > Resetting the flag in the encoder's reset callback should take care of
> > > > > hotplug between suspend/resume.
> > > > > 
> > > > > v2:
> > > > > Allow full detect after encoder reset. (Ville)
> > > > > Set the detect_done flag for connector disconnected case too. (DK)
> > > > > Commit message changes.
> > > > > 
> > > > > Cc: stable@vger.kernel.org
> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index fb12896..6732c17 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > > >  	intel_dp_set_edid(intel_dp);
> > > > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > > >  		status = connector_status_connected;
> > > > > -	intel_dp->detect_done = true;
> > > > >  
> > > > >  	/* Try to read the source of the interrupt */
> > > > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > > > >  		      connector->base.id, connector->name);
> > > > >  
> > > > >  	/* If full detect is not performed yet, do a full detect */
> > > > > -	if (!intel_dp->detect_done)
> > > > > +	if (!intel_dp->detect_done) {
> > > > > +		intel_dp->detect_done = true;
> > > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > -
> > > > > -	intel_dp->detect_done = false;
> > > > > +	}
> > > > >  
> > > > >  	return status;
> > > >  }
> > > > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > > >  	if (lspcon->active)
> > > > >  		lspcon_resume(lspcon);
> > > > >  
> > > > > +	intel_dp->detect_done = false;
> > > > > +
> > > > >  	pps_lock(intel_dp);
> > > > >  
> > > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Ville Syrjälä]

On Thu, Jan 19, 2017 at 11:01:01AM -0800, Manasi Navare wrote:
> On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrj�l� wrote:
> > On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > > I have verified this patch with the latest drm-tip and it is also
> > > absolutely necessary to ensure the link gets properly retrained
> > > after link-status is BAD and after anew modeset is triggered by
> > > userspace. Without this patch, since intel_dp_long_pulse gets called
> > > it resets the link failure values and link retraining does not happen
> > > at lower link rates.
> > 
> > Why are you resetting the failure values in this function? You should
> > only do that when a long pulse is actually detected IMO (and yes, 
> > calling the function intel_dp_long_pulse() name is pretty much wrong
> > now).
> >
> 
> Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
> and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
> get called on hotplug and that we need reread the dpcd registers. So setting these to max values
> mean that we have lost the information about the link rate and lane count at which link training failed.
> 
> If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
> then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
> lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
> So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()

No. What you're asking for is a change in behaviour (to call
intel_dp_long_pulse() from ->detect() only if it was just preceded by an
actual long pulse). That was never how this code worked. The point of
the flag was to avoid calling it twice when processing the HPD since it
was directly called from intel_dp_hpd_pulse() and then again from
->detect(). Since we no longer call it directly from
intel_dp_hpd_pulse() the flag is in fact useless.

While the change you ask for may be desirable, history has shown
that the DP code is very fragile, so I don't think we should make
the link state property depend on something that may need to be
reverted if a regression crops up. And so I think you should just
change your new code to work with the existing scheme. We can push
the detect_done optimization in afterwards since we should then be
able to revert it without having to revert everything related to
the link status property.

> 
> Manasi 
> > > 
> > > Ville/Jani could you please review this patch?
> > > Thi is definitely a bug in the existing codebase and we need to get this merged
> > > soon to get it fixed.
> > > 
> > > Regards
> > > Manasi
> > > 
> > > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > > 
> > > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > > flag after long hpd handling leads to the code being executed again during
> > > > mode enumeration.
> > > > 
> > > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > > during mode enumeration after a DPCD detect, return the cached connector
> > > > status.
> > > > 
> > > > Resetting the flag in the encoder's reset callback should take care of
> > > > hotplug between suspend/resume.
> > > > 
> > > > v2:
> > > > Allow full detect after encoder reset. (Ville)
> > > > Set the detect_done flag for connector disconnected case too. (DK)
> > > > Commit message changes.
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index fb12896..6732c17 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > >  	intel_dp_set_edid(intel_dp);
> > > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > >  		status = connector_status_connected;
> > > > -	intel_dp->detect_done = true;
> > > >  
> > > >  	/* Try to read the source of the interrupt */
> > > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > > >  		      connector->base.id, connector->name);
> > > >  
> > > >  	/* If full detect is not performed yet, do a full detect */
> > > > -	if (!intel_dp->detect_done)
> > > > +	if (!intel_dp->detect_done) {
> > > > +		intel_dp->detect_done = true;
> > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > > -
> > > > -	intel_dp->detect_done = false;
> > > > +	}
> > > >  
> > > >  	return status;
> > >  }
> > > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > >  	if (lspcon->active)
> > > >  		lspcon_resume(lspcon);
> > > >  
> > > > +	intel_dp->detect_done = false;
> > > > +
> > > >  	pps_lock(intel_dp);
> > > >  
> > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > -- 
> > > > 2.7.4
> > > > 
> > 
> > -- 
> > Ville Syrj�l�
> > Intel OTC

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Ville Syrjälä]

On Thu, Jan 19, 2017 at 11:01:01AM -0800, Manasi Navare wrote:
> On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrjälä wrote:
> > On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > > I have verified this patch with the latest drm-tip and it is also
> > > absolutely necessary to ensure the link gets properly retrained
> > > after link-status is BAD and after anew modeset is triggered by
> > > userspace. Without this patch, since intel_dp_long_pulse gets called
> > > it resets the link failure values and link retraining does not happen
> > > at lower link rates.
> > 
> > Why are you resetting the failure values in this function? You should
> > only do that when a long pulse is actually detected IMO (and yes, 
> > calling the function intel_dp_long_pulse() name is pretty much wrong
> > now).
> >
> 
> Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
> and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
> get called on hotplug and that we need reread the dpcd registers. So setting these to max values
> mean that we have lost the information about the link rate and lane count at which link training failed.
> 
> If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
> then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
> lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
> So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()

No. What you're asking for is a change in behaviour (to call
intel_dp_long_pulse() from ->detect() only if it was just preceded by an
actual long pulse). That was never how this code worked. The point of
the flag was to avoid calling it twice when processing the HPD since it
was directly called from intel_dp_hpd_pulse() and then again from
->detect(). Since we no longer call it directly from
intel_dp_hpd_pulse() the flag is in fact useless.

While the change you ask for may be desirable, history has shown
that the DP code is very fragile, so I don't think we should make
the link state property depend on something that may need to be
reverted if a regression crops up. And so I think you should just
change your new code to work with the existing scheme. We can push
the detect_done optimization in afterwards since we should then be
able to revert it without having to revert everything related to
the link status property.

> 
> Manasi 
> > > 
> > > Ville/Jani could you please review this patch?
> > > Thi is definitely a bug in the existing codebase and we need to get this merged
> > > soon to get it fixed.
> > > 
> > > Regards
> > > Manasi
> > > 
> > > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > > 
> > > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > > flag after long hpd handling leads to the code being executed again during
> > > > mode enumeration.
> > > > 
> > > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > > during mode enumeration after a DPCD detect, return the cached connector
> > > > status.
> > > > 
> > > > Resetting the flag in the encoder's reset callback should take care of
> > > > hotplug between suspend/resume.
> > > > 
> > > > v2:
> > > > Allow full detect after encoder reset. (Ville)
> > > > Set the detect_done flag for connector disconnected case too. (DK)
> > > > Commit message changes.
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index fb12896..6732c17 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > >  	intel_dp_set_edid(intel_dp);
> > > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > > >  		status = connector_status_connected;
> > > > -	intel_dp->detect_done = true;
> > > >  
> > > >  	/* Try to read the source of the interrupt */
> > > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > > >  		      connector->base.id, connector->name);
> > > >  
> > > >  	/* If full detect is not performed yet, do a full detect */
> > > > -	if (!intel_dp->detect_done)
> > > > +	if (!intel_dp->detect_done) {
> > > > +		intel_dp->detect_done = true;
> > > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > > -
> > > > -	intel_dp->detect_done = false;
> > > > +	}
> > > >  
> > > >  	return status;
> > >  }
> > > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > > >  	if (lspcon->active)
> > > >  		lspcon_resume(lspcon);
> > > >  
> > > > +	intel_dp->detect_done = false;
> > > > +
> > > >  	pps_lock(intel_dp);
> > > >  
> > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > -- 
> > > > 2.7.4
> > > > 
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrj�l� wrote:
> On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > I have verified this patch with the latest drm-tip and it is also
> > absolutely necessary to ensure the link gets properly retrained
> > after link-status is BAD and after anew modeset is triggered by
> > userspace. Without this patch, since intel_dp_long_pulse gets called
> > it resets the link failure values and link retraining does not happen
> > at lower link rates.
> 
> Why are you resetting the failure values in this function? You should
> only do that when a long pulse is actually detected IMO (and yes, 
> calling the function intel_dp_long_pulse() name is pretty much wrong
> now).
>

Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
get called on hotplug and that we need reread the dpcd registers. So setting these to max values
mean that we have lost the information about the link rate and lane count at which link training failed.

If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()

Manasi 
> > 
> > Ville/Jani could you please review this patch?
> > Thi is definitely a bug in the existing codebase and we need to get this merged
> > soon to get it fixed.
> > 
> > Regards
> > Manasi
> > 
> > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > 
> > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > flag after long hpd handling leads to the code being executed again during
> > > mode enumeration.
> > > 
> > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > during mode enumeration after a DPCD detect, return the cached connector
> > > status.
> > > 
> > > Resetting the flag in the encoder's reset callback should take care of
> > > hotplug between suspend/resume.
> > > 
> > > v2:
> > > Allow full detect after encoder reset. (Ville)
> > > Set the detect_done flag for connector disconnected case too. (DK)
> > > Commit message changes.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index fb12896..6732c17 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > >  	intel_dp_set_edid(intel_dp);
> > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > >  		status = connector_status_connected;
> > > -	intel_dp->detect_done = true;
> > >  
> > >  	/* Try to read the source of the interrupt */
> > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > >  		      connector->base.id, connector->name);
> > >  
> > >  	/* If full detect is not performed yet, do a full detect */
> > > -	if (!intel_dp->detect_done)
> > > +	if (!intel_dp->detect_done) {
> > > +		intel_dp->detect_done = true;
> > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > -
> > > -	intel_dp->detect_done = false;
> > > +	}
> > >  
> > >  	return status;
> > >  }
> > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > >  	if (lspcon->active)
> > >  		lspcon_resume(lspcon);
> > >  
> > > +	intel_dp->detect_done = false;
> > > +
> > >  	pps_lock(intel_dp);
> > >  
> > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > -- 
> > > 2.7.4
> > > 
> 
> -- 
> Ville Syrj�l�
> Intel OTC

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

On Thu, Jan 19, 2017 at 03:42:34PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> > I have verified this patch with the latest drm-tip and it is also
> > absolutely necessary to ensure the link gets properly retrained
> > after link-status is BAD and after anew modeset is triggered by
> > userspace. Without this patch, since intel_dp_long_pulse gets called
> > it resets the link failure values and link retraining does not happen
> > at lower link rates.
> 
> Why are you resetting the failure values in this function? You should
> only do that when a long pulse is actually detected IMO (and yes, 
> calling the function intel_dp_long_pulse() name is pretty much wrong
> now).
>

Yes the values for link rate nd lane count get set to the intel_dp_max_link_bw()
and drm_dp_max_lane_count() in intel_dp_long_pulse() assuming that this function would
get called on hotplug and that we need reread the dpcd registers. So setting these to max values
mean that we have lost the information about the link rate and lane count at which link training failed.

If detect_done is reset in intel_dp_detect() right after calling intel_dp_long_pulse() which is clearly a bug
then it calls the intel_dp_long_pulse() during mode enumeration before the modeset and it never retrains at the 
lower values since the max link rate/lane count get overwritten in intel_dp_long_pulse().
So it is absolutely necessary to not reset detect_done flag here in intel_dp_detect()

Manasi 
> > 
> > Ville/Jani could you please review this patch?
> > Thi is definitely a bug in the existing codebase and we need to get this merged
> > soon to get it fixed.
> > 
> > Regards
> > Manasi
> > 
> > On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > > 
> > > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > > flag after long hpd handling leads to the code being executed again during
> > > mode enumeration.
> > > 
> > > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > > during mode enumeration after a DPCD detect, return the cached connector
> > > status.
> > > 
> > > Resetting the flag in the encoder's reset callback should take care of
> > > hotplug between suspend/resume.
> > > 
> > > v2:
> > > Allow full detect after encoder reset. (Ville)
> > > Set the detect_done flag for connector disconnected case too. (DK)
> > > Commit message changes.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index fb12896..6732c17 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > >  	intel_dp_set_edid(intel_dp);
> > >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> > >  		status = connector_status_connected;
> > > -	intel_dp->detect_done = true;
> > >  
> > >  	/* Try to read the source of the interrupt */
> > >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> > >  		      connector->base.id, connector->name);
> > >  
> > >  	/* If full detect is not performed yet, do a full detect */
> > > -	if (!intel_dp->detect_done)
> > > +	if (!intel_dp->detect_done) {
> > > +		intel_dp->detect_done = true;
> > >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > > -
> > > -	intel_dp->detect_done = false;
> > > +	}
> > >  
> > >  	return status;
> > >  }
> > > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> > >  	if (lspcon->active)
> > >  		lspcon_resume(lspcon);
> > >  
> > > +	intel_dp->detect_done = false;
> > > +
> > >  	pps_lock(intel_dp);
> > >  
> > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > -- 
> > > 2.7.4
> > > 
> 
> -- 
> Ville Syrjälä
> Intel OTC

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Ville Syrjälä]

On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> I have verified this patch with the latest drm-tip and it is also
> absolutely necessary to ensure the link gets properly retrained
> after link-status is BAD and after anew modeset is triggered by
> userspace. Without this patch, since intel_dp_long_pulse gets called
> it resets the link failure values and link retraining does not happen
> at lower link rates.

Why are you resetting the failure values in this function? You should
only do that when a long pulse is actually detected IMO (and yes, 
calling the function intel_dp_long_pulse() name is pretty much wrong
now).

> 
> Ville/Jani could you please review this patch?
> Thi is definitely a bug in the existing codebase and we need to get this merged
> soon to get it fixed.
> 
> Regards
> Manasi
> 
> On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > 
> > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > flag after long hpd handling leads to the code being executed again during
> > mode enumeration.
> > 
> > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > during mode enumeration after a DPCD detect, return the cached connector
> > status.
> > 
> > Resetting the flag in the encoder's reset callback should take care of
> > hotplug between suspend/resume.
> > 
> > v2:
> > Allow full detect after encoder reset. (Ville)
> > Set the detect_done flag for connector disconnected case too. (DK)
> > Commit message changes.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index fb12896..6732c17 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  	intel_dp_set_edid(intel_dp);
> >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> >  		status = connector_status_connected;
> > -	intel_dp->detect_done = true;
> >  
> >  	/* Try to read the source of the interrupt */
> >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> >  		      connector->base.id, connector->name);
> >  
> >  	/* If full detect is not performed yet, do a full detect */
> > -	if (!intel_dp->detect_done)
> > +	if (!intel_dp->detect_done) {
> > +		intel_dp->detect_done = true;
> >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > -
> > -	intel_dp->detect_done = false;
> > +	}
> >  
> >  	return status;
> >  }
> > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> >  	if (lspcon->active)
> >  		lspcon_resume(lspcon);
> >  
> > +	intel_dp->detect_done = false;
> > +
> >  	pps_lock(intel_dp);
> >  
> >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > -- 
> > 2.7.4
> > 

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Ville Syrjälä]

On Tue, Jan 17, 2017 at 09:40:42AM -0800, Manasi Navare wrote:
> I have verified this patch with the latest drm-tip and it is also
> absolutely necessary to ensure the link gets properly retrained
> after link-status is BAD and after anew modeset is triggered by
> userspace. Without this patch, since intel_dp_long_pulse gets called
> it resets the link failure values and link retraining does not happen
> at lower link rates.

Why are you resetting the failure values in this function? You should
only do that when a long pulse is actually detected IMO (and yes, 
calling the function intel_dp_long_pulse() name is pretty much wrong
now).

> 
> Ville/Jani could you please review this patch?
> Thi is definitely a bug in the existing codebase and we need to get this merged
> soon to get it fixed.
> 
> Regards
> Manasi
> 
> On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> > From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> > 
> > The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> > ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> > detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> > as well as intel_dp_detect(). Later, 'commit 1015811609c0
> > ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> > hpd handling to hotplug work to avoid handling it twice. But, resetting the
> > flag after long hpd handling leads to the code being executed again during
> > mode enumeration.
> > 
> > So, do not reset the detect_done flag to False in intel_dp_detect(). The
> > flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> > the hotplug work does a full DPCD detect. However if ->detect() gets called
> > during mode enumeration after a DPCD detect, return the cached connector
> > status.
> > 
> > Resetting the flag in the encoder's reset callback should take care of
> > hotplug between suspend/resume.
> > 
> > v2:
> > Allow full detect after encoder reset. (Ville)
> > Set the detect_done flag for connector disconnected case too. (DK)
> > Commit message changes.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index fb12896..6732c17 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  	intel_dp_set_edid(intel_dp);
> >  	if (is_edp(intel_dp) || intel_connector->detect_edid)
> >  		status = connector_status_connected;
> > -	intel_dp->detect_done = true;
> >  
> >  	/* Try to read the source of the interrupt */
> >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> >  		      connector->base.id, connector->name);
> >  
> >  	/* If full detect is not performed yet, do a full detect */
> > -	if (!intel_dp->detect_done)
> > +	if (!intel_dp->detect_done) {
> > +		intel_dp->detect_done = true;
> >  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> > -
> > -	intel_dp->detect_done = false;
> > +	}
> >  
> >  	return status;
> >  }
> > @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
> >  	if (lspcon->active)
> >  		lspcon_resume(lspcon);
> >  
> > +	intel_dp->detect_done = false;
> > +
> >  	pps_lock(intel_dp);
> >  
> >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > -- 
> > 2.7.4
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

I have verified this patch with the latest drm-tip and it is also
absolutely necessary to ensure the link gets properly retrained
after link-status is BAD and after anew modeset is triggered by
userspace. Without this patch, since intel_dp_long_pulse gets called
it resets the link failure values and link retraining does not happen
at lower link rates.

Ville/Jani could you please review this patch?
Thi is definitely a bug in the existing codebase and we need to get this merged
soon to get it fixed.

Regards
Manasi

On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> 
> The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> as well as intel_dp_detect(). Later, 'commit 1015811609c0
> ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> hpd handling to hotplug work to avoid handling it twice. But, resetting the
> flag after long hpd handling leads to the code being executed again during
> mode enumeration.
> 
> So, do not reset the detect_done flag to False in intel_dp_detect(). The
> flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> the hotplug work does a full DPCD detect. However if ->detect() gets called
> during mode enumeration after a DPCD detect, return the cached connector
> status.
> 
> Resetting the flag in the encoder's reset callback should take care of
> hotplug between suspend/resume.
> 
> v2:
> Allow full detect after encoder reset. (Ville)
> Set the detect_done flag for connector disconnected case too. (DK)
> Commit message changes.
> 
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fb12896..6732c17 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  	intel_dp_set_edid(intel_dp);
>  	if (is_edp(intel_dp) || intel_connector->detect_edid)
>  		status = connector_status_connected;
> -	intel_dp->detect_done = true;
>  
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  		      connector->base.id, connector->name);
>  
>  	/* If full detect is not performed yet, do a full detect */
> -	if (!intel_dp->detect_done)
> +	if (!intel_dp->detect_done) {
> +		intel_dp->detect_done = true;
>  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> -
> -	intel_dp->detect_done = false;
> +	}
>  
>  	return status;
>  }
> @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
>  	if (lspcon->active)
>  		lspcon_resume(lspcon);
>  
> +	intel_dp->detect_done = false;
> +
>  	pps_lock(intel_dp);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -- 
> 2.7.4
> 

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Manasi Navare]

I have verified this patch with the latest drm-tip and it is also
absolutely necessary to ensure the link gets properly retrained
after link-status is BAD and after anew modeset is triggered by
userspace. Without this patch, since intel_dp_long_pulse gets called
it resets the link failure values and link retraining does not happen
at lower link rates.

Ville/Jani could you please review this patch?
Thi is definitely a bug in the existing codebase and we need to get this merged
soon to get it fixed.

Regards
Manasi

On Thu, Dec 29, 2016 at 11:14:28PM -0800, Dhinakaran Pandiyan wrote:
> From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> 
> The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> as well as intel_dp_detect(). Later, 'commit 1015811609c0
> ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> hpd handling to hotplug work to avoid handling it twice. But, resetting the
> flag after long hpd handling leads to the code being executed again during
> mode enumeration.
> 
> So, do not reset the detect_done flag to False in intel_dp_detect(). The
> flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> the hotplug work does a full DPCD detect. However if ->detect() gets called
> during mode enumeration after a DPCD detect, return the cached connector
> status.
> 
> Resetting the flag in the encoder's reset callback should take care of
> hotplug between suspend/resume.
> 
> v2:
> Allow full detect after encoder reset. (Ville)
> Set the detect_done flag for connector disconnected case too. (DK)
> Commit message changes.
> 
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fb12896..6732c17 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  	intel_dp_set_edid(intel_dp);
>  	if (is_edp(intel_dp) || intel_connector->detect_edid)
>  		status = connector_status_connected;
> -	intel_dp->detect_done = true;
>  
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  		      connector->base.id, connector->name);
>  
>  	/* If full detect is not performed yet, do a full detect */
> -	if (!intel_dp->detect_done)
> +	if (!intel_dp->detect_done) {
> +		intel_dp->detect_done = true;
>  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> -
> -	intel_dp->detect_done = false;
> +	}
>  
>  	return status;
>  }
> @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
>  	if (lspcon->active)
>  		lspcon_resume(lspcon);
>  
> +	intel_dp->detect_done = false;
> +
>  	pps_lock(intel_dp);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -- 
> 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Pandiyan, Dhinakaran]

On Thu, 2016-12-29 at 23:14 -0800, Dhinakaran Pandiyan wrote:

bump for review.


-DK


> From: "Navare, Manasi D" <manasi.d.navare@intel.com>
> 
> The detect_done flag was introduced in the 'commit 7d23e3c37bb3
> ("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
> detects on hotplug where intel_dp_long_pulse() was called from HPD handler
> as well as intel_dp_detect(). Later, 'commit 1015811609c0
> ("drm/i915: Move long hpd handling into the hotplug work")' deferred long
> hpd handling to hotplug work to avoid handling it twice. But, resetting the
> flag after long hpd handling leads to the code being executed again during
> mode enumeration.
> 
> So, do not reset the detect_done flag to False in intel_dp_detect(). The
> flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
> the hotplug work does a full DPCD detect. However if ->detect() gets called
> during mode enumeration after a DPCD detect, return the cached connector
> status.
> 
> Resetting the flag in the encoder's reset callback should take care of
> hotplug between suspend/resume.
> 
> v2:
> Allow full detect after encoder reset. (Ville)
> Set the detect_done flag for connector disconnected case too. (DK)
> Commit message changes.
> 
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fb12896..6732c17 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  	intel_dp_set_edid(intel_dp);
>  	if (is_edp(intel_dp) || intel_connector->detect_edid)
>  		status = connector_status_connected;
> -	intel_dp->detect_done = true;
>  
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> @@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
>  		      connector->base.id, connector->name);
>  
>  	/* If full detect is not performed yet, do a full detect */
> -	if (!intel_dp->detect_done)
> +	if (!intel_dp->detect_done) {
> +		intel_dp->detect_done = true;
>  		status = intel_dp_long_pulse(intel_dp->attached_connector);
> -
> -	intel_dp->detect_done = false;
> +	}
>  
>  	return status;
>  }
> @@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
>  	if (lspcon->active)
>  		lspcon_resume(lspcon);
>  
> +	intel_dp->detect_done = false;
> +
>  	pps_lock(intel_dp);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))

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

[PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Dhinakaran Pandiyan]

From: "Navare, Manasi D" <manasi.d.navare@intel.com>

The detect_done flag was introduced in the 'commit 7d23e3c37bb3
("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
detects on hotplug where intel_dp_long_pulse() was called from HPD handler
as well as intel_dp_detect(). Later, 'commit 1015811609c0
("drm/i915: Move long hpd handling into the hotplug work")' deferred long
hpd handling to hotplug work to avoid handling it twice. But, resetting the
flag after long hpd handling leads to the code being executed again during
mode enumeration.

So, do not reset the detect_done flag to False in intel_dp_detect(). The
flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
the hotplug work does a full DPCD detect. However if ->detect() gets called
during mode enumeration after a DPCD detect, return the cached connector
status.

Resetting the flag in the encoder's reset callback should take care of
hotplug between suspend/resume.

v2:
Allow full detect after encoder reset. (Ville)
Set the detect_done flag for connector disconnected case too. (DK)
Commit message changes.

Cc: stable@vger.kernel.org
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fb12896..6732c17 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4516,7 +4516,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_set_edid(intel_dp);
 	if (is_edp(intel_dp) || intel_connector->detect_edid)
 		status = connector_status_connected;
-	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -4551,10 +4550,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		      connector->base.id, connector->name);
 
 	/* If full detect is not performed yet, do a full detect */
-	if (!intel_dp->detect_done)
+	if (!intel_dp->detect_done) {
+		intel_dp->detect_done = true;
 		status = intel_dp_long_pulse(intel_dp->attached_connector);
-
-	intel_dp->detect_done = false;
+	}
 
 	return status;
 }
@@ -4859,6 +4858,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
 	if (lspcon->active)
 		lspcon_resume(lspcon);
 
+	intel_dp->detect_done = false;
+
 	pps_lock(intel_dp);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-- 
2.7.4

[PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Dhinakaran Pandiyan]

From: "Navare, Manasi D" <manasi.d.navare@intel.com>

The detect_done flag was introduced in the 'commit 7d23e3c37bb3
("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
detects on hotplug where intel_dp_long_pulse() was called from HPD handler
as well as intel_dp_detect(). Later, 'commit 1015811609c0
("drm/i915: Move long hpd handling into the hotplug work")' deferred long
hpd handling to hotplug work to avoid handling it twice. But, resetting the
flag after long hpd handling leads to the code being executed again during
mode enumeration.

So, do not reset the detect_done flag to False in intel_dp_detect(). The
flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
the hotplug work does a full DPCD detect. However if ->detect() gets called
during mode enumeration after a DPCD detect, return the cached connector
status.

Resetting the flag in the encoder's reset callback should take care of
hotplug between suspend/resume.

v2:
Allow full detect after encoder reset. (Ville)
Set the detect_done flag for connector disconnected case too. (DK)
Commit message changes.

Cc: stable@vger.kernel.org
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
Rebased.

 drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 66b5bc8..0927fc2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4515,7 +4515,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_set_edid(intel_dp);
 	if (is_edp(intel_dp) || intel_connector->detect_edid)
 		status = connector_status_connected;
-	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -4550,10 +4549,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		      connector->base.id, connector->name);
 
 	/* If full detect is not performed yet, do a full detect */
-	if (!intel_dp->detect_done)
+	if (!intel_dp->detect_done) {
+		intel_dp->detect_done = true;
 		status = intel_dp_long_pulse(intel_dp->attached_connector);
-
-	intel_dp->detect_done = false;
+	}
 
 	return status;
 }
@@ -4858,6 +4857,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
 	if (lspcon->active)
 		lspcon_resume(lspcon);
 
+	intel_dp->detect_done = false;
+
 	pps_lock(intel_dp);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-- 
2.7.4

[PATCH v3] drm/i915/dp: Do not reset detect_done flag in intel_dp_detect

[Dhinakaran Pandiyan]

From: "Navare, Manasi D" <manasi.d.navare@intel.com>

The detect_done flag was introduced in the 'commit 7d23e3c37bb3
("drm/i915: Cleaning up intel_dp_hpd_pulse")' in order to avoid multiple
detects on hotplug where intel_dp_long_pulse() was called from HPD handler
as well as intel_dp_detect(). Later, 'commit 1015811609c0
("drm/i915: Move long hpd handling into the hotplug work")' deferred long
hpd handling to hotplug work to avoid handling it twice. But, resetting the
flag after long hpd handling leads to the code being executed again during
mode enumeration.

So, do not reset the detect_done flag to False in intel_dp_detect(). The
flag is reset in intel_dp_hpd_pulse() to allow a full detect and set when
the hotplug work does a full DPCD detect. However if ->detect() gets called
during mode enumeration after a DPCD detect, return the cached connector
status.

Resetting the flag in the encoder's reset callback should take care of
hotplug between suspend/resume.

v2:
Allow full detect after encoder reset. (Ville)
Set the detect_done flag for connector disconnected case too. (DK)
Commit message changes.

Cc: stable@vger.kernel.org
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Ander Conselvande Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Fixes: commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
Rebased.

 drivers/gpu/drm/i915/intel_dp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 66b5bc8..0927fc2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4515,7 +4515,6 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_set_edid(intel_dp);
 	if (is_edp(intel_dp) || intel_connector->detect_edid)
 		status = connector_status_connected;
-	intel_dp->detect_done = true;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -4550,10 +4549,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		      connector->base.id, connector->name);
 
 	/* If full detect is not performed yet, do a full detect */
-	if (!intel_dp->detect_done)
+	if (!intel_dp->detect_done) {
+		intel_dp->detect_done = true;
 		status = intel_dp_long_pulse(intel_dp->attached_connector);
-
-	intel_dp->detect_done = false;
+	}
 
 	return status;
 }
@@ -4858,6 +4857,8 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder)
 	if (lspcon->active)
 		lspcon_resume(lspcon);
 
+	intel_dp->detect_done = false;
+
 	pps_lock(intel_dp);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-- 
2.7.4

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