Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-03-05 Thread Manasi Navare 
On Wed, Feb 28, 2018 at 03:10:24PM -0500, Lyude Paul wrote:
> On Wed, 2018-02-28 at 11:57 -0800, Manasi Navare wrote:
> > On Wed, Feb 28, 2018 at 02:41:06PM -0500, Lyude Paul wrote:
> > > On Wed, 2018-02-28 at 11:27 -0800, Manasi Navare wrote:
> > > > On Wed, Feb 28, 2018 at 02:07:34PM -0500, Lyude Paul wrote:
> > > > > 
> > > > > On Tue, 2018-02-27 at 23:17 -0800, Manasi Navare wrote:
> > > > > > Ville,  thanks for the patch and
> > > > > > Sorry for not being able to review this earlier.
> > > > > > Please find some comments below:
> > > > > > 
> > > > > > On Wed, Jan 31, 2018 at 03:27:10PM +0200, Ville Syrjälä wrote:
> > > > > > > On Tue, Jan 30, 2018 at 06:16:59PM -0500, Lyude Paul wrote:
> > > > > > > > On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> > > > > > > > > From: Ville Syrjälä 
> > > > > > > > > 
> > > > > > > > > Doing link retraining from the short pulse handler is
> > > > > > > > > problematic
> > > > > > > > > since
> > > > > > > > > that might introduce deadlocks with MST sideband processing.
> > > > > > > > > Currently
> > > > > > > > > we don't retrain MST links from this code, but we want to
> > > > > > > > > change
> > > > > > > > > that.
> > > > > > > > > So better to move the entire thing to the hotplug work. We can
> > > > > > > > > utilize
> > > > > > > > > the new encoder->hotplug() hook for this.
> > > > > > > > > 
> > > > > > > > > The only thing we leave in the short pulse handler is the link
> > > > > > > > > status
> > > > > > > > > check. That one still depends on the link parameters stored
> > > > > > > > > under
> > > > > > > > > intel_dp, so no locking around that but races should be mostly
> > > > > > > > > harmless
> > > > > > > > > as the actual retraining code will recheck the link state if
> > > > > > > > > we
> > > > > > > > > end up there by mistake.
> > > > > > > > > 
> > > > > > > > > v2: Rebase due to ->post_hotplug() now being just ->hotplug()
> > > > > > > > > Check the connector type to figure out if we should do
> > > > > > > > > the HDMI thing or the DP think for DDI
> > > > > > > > > 
> > > > > > > > > Cc: Manasi Navare 
> > > > > > > > > Cc: Maarten Lankhorst 
> > > > > > > > > Signed-off-by: Ville Syrjälä 
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/i915/intel_ddi.c |  10 +-
> > > > > > > > >  drivers/gpu/drm/i915/intel_dp.c  | 196
> > > > > > > > > ++--
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/i915/intel_drv.h |   2 +
> > > > > > > > >  3 files changed, 120 insertions(+), 88 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > > index 25793bdc692f..5f3d58f1ae6e 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > > @@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct
> > > > > > > > > intel_encoder
> > > > > > > > > *encoder,
> > > > > > > > >   drm_modeset_acquire_init(, 0);
> > > > > > > > >  
> > > > > > > > >   for (;;) {
> > > > > > > > > - ret = intel_hdmi_reset_link(encoder, );
> > > > > > > > > + if (connector->base.connector_type ==
> > > > > > > > > DRM_MODE_CONNECTOR_HDMIA)
> > > > > > > > > + ret = intel_hdmi_reset_link(encoder,
> > > > > > > > > );
> > > > > > > > > + else
> > > > > > > > > + ret = intel_dp_retrain_link(encoder,
> > > > > > > > > );
> > > > > > > > >  
> > > > > > > > >   if (ret == -EDEADLK) {
> > > > > > > > >   drm_modeset_backoff();
> > > > > > > > > @@ -3007,10 +3010,7 @@ void intel_ddi_init(struct
> > > > > > > > > drm_i915_private
> > > > > > > > > *dev_priv,
> > > > > > > > > enum port port)
> > > > > > > > >   drm_encoder_init(_priv->drm, encoder,
> > > > > > > > > _ddi_funcs,
> > > > > > > > >DRM_MODE_ENCODER_TMDS, "DDI %c",
> > > > > > > > > port_name(port));
> > > > > > > > >  
> > > > > > > > > - if (init_hdmi)
> > > > > > > > > - intel_encoder->hotplug = intel_ddi_hotplug;
> > > > > > > > > - else
> > > > > > > > > - intel_encoder->hotplug =
> > > > > > > > > intel_encoder_hotplug;
> > > > > > > > > + intel_encoder->hotplug = intel_ddi_hotplug;
> > > > > > > > >   intel_encoder->compute_output_type =
> > > > > > > > > intel_ddi_compute_output_type;
> > > > > > > > >   intel_encoder->compute_config =
> > > > > > > > > intel_ddi_compute_config;
> > > > > > > > >   intel_encoder->enable = intel_enable_ddi;
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > > index 6bbf14410c2a..152016e09a11 100644
> > > > >

Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-02-28 Thread Lyude Paul 
On Wed, 2018-02-28 at 11:57 -0800, Manasi Navare wrote:
> On Wed, Feb 28, 2018 at 02:41:06PM -0500, Lyude Paul wrote:
> > On Wed, 2018-02-28 at 11:27 -0800, Manasi Navare wrote:
> > > On Wed, Feb 28, 2018 at 02:07:34PM -0500, Lyude Paul wrote:
> > > > 
> > > > On Tue, 2018-02-27 at 23:17 -0800, Manasi Navare wrote:
> > > > > Ville,  thanks for the patch and
> > > > > Sorry for not being able to review this earlier.
> > > > > Please find some comments below:
> > > > > 
> > > > > On Wed, Jan 31, 2018 at 03:27:10PM +0200, Ville Syrjälä wrote:
> > > > > > On Tue, Jan 30, 2018 at 06:16:59PM -0500, Lyude Paul wrote:
> > > > > > > On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> > > > > > > > From: Ville Syrjälä 
> > > > > > > > 
> > > > > > > > Doing link retraining from the short pulse handler is
> > > > > > > > problematic
> > > > > > > > since
> > > > > > > > that might introduce deadlocks with MST sideband processing.
> > > > > > > > Currently
> > > > > > > > we don't retrain MST links from this code, but we want to
> > > > > > > > change
> > > > > > > > that.
> > > > > > > > So better to move the entire thing to the hotplug work. We can
> > > > > > > > utilize
> > > > > > > > the new encoder->hotplug() hook for this.
> > > > > > > > 
> > > > > > > > The only thing we leave in the short pulse handler is the link
> > > > > > > > status
> > > > > > > > check. That one still depends on the link parameters stored
> > > > > > > > under
> > > > > > > > intel_dp, so no locking around that but races should be mostly
> > > > > > > > harmless
> > > > > > > > as the actual retraining code will recheck the link state if
> > > > > > > > we
> > > > > > > > end up there by mistake.
> > > > > > > > 
> > > > > > > > v2: Rebase due to ->post_hotplug() now being just ->hotplug()
> > > > > > > > Check the connector type to figure out if we should do
> > > > > > > > the HDMI thing or the DP think for DDI
> > > > > > > > 
> > > > > > > > Cc: Manasi Navare 
> > > > > > > > Cc: Maarten Lankhorst 
> > > > > > > > Signed-off-by: Ville Syrjälä 
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_ddi.c |  10 +-
> > > > > > > >  drivers/gpu/drm/i915/intel_dp.c  | 196
> > > > > > > > ++--
> > > > > > > > 
> > > > > > > > 
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_drv.h |   2 +
> > > > > > > >  3 files changed, 120 insertions(+), 88 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > index 25793bdc692f..5f3d58f1ae6e 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > @@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct
> > > > > > > > intel_encoder
> > > > > > > > *encoder,
> > > > > > > > drm_modeset_acquire_init(, 0);
> > > > > > > >  
> > > > > > > > for (;;) {
> > > > > > > > -   ret = intel_hdmi_reset_link(encoder, );
> > > > > > > > +   if (connector->base.connector_type ==
> > > > > > > > DRM_MODE_CONNECTOR_HDMIA)
> > > > > > > > +   ret = intel_hdmi_reset_link(encoder,
> > > > > > > > );
> > > > > > > > +   else
> > > > > > > > +   ret = intel_dp_retrain_link(encoder,
> > > > > > > > );
> > > > > > > >  
> > > > > > > > if (ret == -EDEADLK) {
> > > > > > > > drm_modeset_backoff();
> > > > > > > > @@ -3007,10 +3010,7 @@ void intel_ddi_init(struct
> > > > > > > > drm_i915_private
> > > > > > > > *dev_priv,
> > > > > > > > enum port port)
> > > > > > > > drm_encoder_init(_priv->drm, encoder,
> > > > > > > > _ddi_funcs,
> > > > > > > >  DRM_MODE_ENCODER_TMDS, "DDI %c",
> > > > > > > > port_name(port));
> > > > > > > >  
> > > > > > > > -   if (init_hdmi)
> > > > > > > > -   intel_encoder->hotplug = intel_ddi_hotplug;
> > > > > > > > -   else
> > > > > > > > -   intel_encoder->hotplug =
> > > > > > > > intel_encoder_hotplug;
> > > > > > > > +   intel_encoder->hotplug = intel_ddi_hotplug;
> > > > > > > > intel_encoder->compute_output_type =
> > > > > > > > intel_ddi_compute_output_type;
> > > > > > > > intel_encoder->compute_config =
> > > > > > > > intel_ddi_compute_config;
> > > > > > > > intel_encoder->enable = intel_enable_ddi;
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > index 6bbf14410c2a..152016e09a11 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > @@ -4275,12 +4275,83 @@ intel_dp_check_mst_status(struct
> > > > > > > > intel_dp
> > > > > > > >

Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-02-28 Thread Manasi Navare 
On Wed, Feb 28, 2018 at 02:41:06PM -0500, Lyude Paul wrote:
> On Wed, 2018-02-28 at 11:27 -0800, Manasi Navare wrote:
> > On Wed, Feb 28, 2018 at 02:07:34PM -0500, Lyude Paul wrote:
> > > 
> > > On Tue, 2018-02-27 at 23:17 -0800, Manasi Navare wrote:
> > > > Ville,  thanks for the patch and
> > > > Sorry for not being able to review this earlier.
> > > > Please find some comments below:
> > > > 
> > > > On Wed, Jan 31, 2018 at 03:27:10PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Jan 30, 2018 at 06:16:59PM -0500, Lyude Paul wrote:
> > > > > > On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> > > > > > > From: Ville Syrjälä 
> > > > > > > 
> > > > > > > Doing link retraining from the short pulse handler is problematic
> > > > > > > since
> > > > > > > that might introduce deadlocks with MST sideband processing.
> > > > > > > Currently
> > > > > > > we don't retrain MST links from this code, but we want to change
> > > > > > > that.
> > > > > > > So better to move the entire thing to the hotplug work. We can
> > > > > > > utilize
> > > > > > > the new encoder->hotplug() hook for this.
> > > > > > > 
> > > > > > > The only thing we leave in the short pulse handler is the link
> > > > > > > status
> > > > > > > check. That one still depends on the link parameters stored under
> > > > > > > intel_dp, so no locking around that but races should be mostly
> > > > > > > harmless
> > > > > > > as the actual retraining code will recheck the link state if we
> > > > > > > end up there by mistake.
> > > > > > > 
> > > > > > > v2: Rebase due to ->post_hotplug() now being just ->hotplug()
> > > > > > > Check the connector type to figure out if we should do
> > > > > > > the HDMI thing or the DP think for DDI
> > > > > > > 
> > > > > > > Cc: Manasi Navare 
> > > > > > > Cc: Maarten Lankhorst 
> > > > > > > Signed-off-by: Ville Syrjälä 
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_ddi.c |  10 +-
> > > > > > >  drivers/gpu/drm/i915/intel_dp.c  | 196 ++--
> > > > > > > 
> > > > > > > 
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_drv.h |   2 +
> > > > > > >  3 files changed, 120 insertions(+), 88 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > index 25793bdc692f..5f3d58f1ae6e 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > @@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct
> > > > > > > intel_encoder
> > > > > > > *encoder,
> > > > > > >   drm_modeset_acquire_init(, 0);
> > > > > > >  
> > > > > > >   for (;;) {
> > > > > > > - ret = intel_hdmi_reset_link(encoder, );
> > > > > > > + if (connector->base.connector_type ==
> > > > > > > DRM_MODE_CONNECTOR_HDMIA)
> > > > > > > + ret = intel_hdmi_reset_link(encoder,
> > > > > > > );
> > > > > > > + else
> > > > > > > + ret = intel_dp_retrain_link(encoder,
> > > > > > > );
> > > > > > >  
> > > > > > >   if (ret == -EDEADLK) {
> > > > > > >   drm_modeset_backoff();
> > > > > > > @@ -3007,10 +3010,7 @@ void intel_ddi_init(struct drm_i915_private
> > > > > > > *dev_priv,
> > > > > > > enum port port)
> > > > > > >   drm_encoder_init(_priv->drm, encoder,
> > > > > > > _ddi_funcs,
> > > > > > >DRM_MODE_ENCODER_TMDS, "DDI %c",
> > > > > > > port_name(port));
> > > > > > >  
> > > > > > > - if (init_hdmi)
> > > > > > > - intel_encoder->hotplug = intel_ddi_hotplug;
> > > > > > > - else
> > > > > > > - intel_encoder->hotplug = intel_encoder_hotplug;
> > > > > > > + intel_encoder->hotplug = intel_ddi_hotplug;
> > > > > > >   intel_encoder->compute_output_type =
> > > > > > > intel_ddi_compute_output_type;
> > > > > > >   intel_encoder->compute_config = intel_ddi_compute_config;
> > > > > > >   intel_encoder->enable = intel_enable_ddi;
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > index 6bbf14410c2a..152016e09a11 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > @@ -4275,12 +4275,83 @@ intel_dp_check_mst_status(struct intel_dp
> > > > > > > *intel_dp)
> > > > > > >   return -EINVAL;
> > > > > > >  }
> > > > > > >  
> > > > > > > -static void
> > > > > > > -intel_dp_retrain_link(struct intel_dp *intel_dp)
> > > > > > > +static bool
> > > > > > > +intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
> > > > > > > +{
> > > > > > > + u8 link_status[DP_LINK_STATUS_SIZE];
> > > > > > > +
> > > > > > > + if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > > > > > + DRM_ERROR("Failed to get link status\n");
> > > > > > > +

Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-02-28 Thread Lyude Paul 
On Wed, 2018-02-28 at 11:27 -0800, Manasi Navare wrote:
> On Wed, Feb 28, 2018 at 02:07:34PM -0500, Lyude Paul wrote:
> > 
> > On Tue, 2018-02-27 at 23:17 -0800, Manasi Navare wrote:
> > > Ville,  thanks for the patch and
> > > Sorry for not being able to review this earlier.
> > > Please find some comments below:
> > > 
> > > On Wed, Jan 31, 2018 at 03:27:10PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Jan 30, 2018 at 06:16:59PM -0500, Lyude Paul wrote:
> > > > > On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä 
> > > > > > 
> > > > > > Doing link retraining from the short pulse handler is problematic
> > > > > > since
> > > > > > that might introduce deadlocks with MST sideband processing.
> > > > > > Currently
> > > > > > we don't retrain MST links from this code, but we want to change
> > > > > > that.
> > > > > > So better to move the entire thing to the hotplug work. We can
> > > > > > utilize
> > > > > > the new encoder->hotplug() hook for this.
> > > > > > 
> > > > > > The only thing we leave in the short pulse handler is the link
> > > > > > status
> > > > > > check. That one still depends on the link parameters stored under
> > > > > > intel_dp, so no locking around that but races should be mostly
> > > > > > harmless
> > > > > > as the actual retraining code will recheck the link state if we
> > > > > > end up there by mistake.
> > > > > > 
> > > > > > v2: Rebase due to ->post_hotplug() now being just ->hotplug()
> > > > > > Check the connector type to figure out if we should do
> > > > > > the HDMI thing or the DP think for DDI
> > > > > > 
> > > > > > Cc: Manasi Navare 
> > > > > > Cc: Maarten Lankhorst 
> > > > > > Signed-off-by: Ville Syrjälä 
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_ddi.c |  10 +-
> > > > > >  drivers/gpu/drm/i915/intel_dp.c  | 196 ++--
> > > > > > 
> > > > > > 
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_drv.h |   2 +
> > > > > >  3 files changed, 120 insertions(+), 88 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > index 25793bdc692f..5f3d58f1ae6e 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > @@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct
> > > > > > intel_encoder
> > > > > > *encoder,
> > > > > > drm_modeset_acquire_init(, 0);
> > > > > >  
> > > > > > for (;;) {
> > > > > > -   ret = intel_hdmi_reset_link(encoder, );
> > > > > > +   if (connector->base.connector_type ==
> > > > > > DRM_MODE_CONNECTOR_HDMIA)
> > > > > > +   ret = intel_hdmi_reset_link(encoder,
> > > > > > );
> > > > > > +   else
> > > > > > +   ret = intel_dp_retrain_link(encoder,
> > > > > > );
> > > > > >  
> > > > > > if (ret == -EDEADLK) {
> > > > > > drm_modeset_backoff();
> > > > > > @@ -3007,10 +3010,7 @@ void intel_ddi_init(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > enum port port)
> > > > > > drm_encoder_init(_priv->drm, encoder,
> > > > > > _ddi_funcs,
> > > > > >  DRM_MODE_ENCODER_TMDS, "DDI %c",
> > > > > > port_name(port));
> > > > > >  
> > > > > > -   if (init_hdmi)
> > > > > > -   intel_encoder->hotplug = intel_ddi_hotplug;
> > > > > > -   else
> > > > > > -   intel_encoder->hotplug = intel_encoder_hotplug;
> > > > > > +   intel_encoder->hotplug = intel_ddi_hotplug;
> > > > > > intel_encoder->compute_output_type =
> > > > > > intel_ddi_compute_output_type;
> > > > > > intel_encoder->compute_config = intel_ddi_compute_config;
> > > > > > intel_encoder->enable = intel_enable_ddi;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index 6bbf14410c2a..152016e09a11 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -4275,12 +4275,83 @@ intel_dp_check_mst_status(struct intel_dp
> > > > > > *intel_dp)
> > > > > > return -EINVAL;
> > > > > >  }
> > > > > >  
> > > > > > -static void
> > > > > > -intel_dp_retrain_link(struct intel_dp *intel_dp)
> > > > > > +static bool
> > > > > > +intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > +   u8 link_status[DP_LINK_STATUS_SIZE];
> > > > > > +
> > > > > > +   if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > > > > +   DRM_ERROR("Failed to get link status\n");
> > > > > > +   return false;
> > > > > > +   }
> > > > > > +
> > > > > > +   /*
> > > > > > +* Validate the cached values of intel_dp->link_rate and
> > > > > > +* intel_dp->lane_count before attempting to retrain.
> > > > > > +

Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-02-28 Thread Manasi Navare 
On Wed, Feb 28, 2018 at 02:07:34PM -0500, Lyude Paul wrote:
> 
> On Tue, 2018-02-27 at 23:17 -0800, Manasi Navare wrote:
> > Ville,  thanks for the patch and
> > Sorry for not being able to review this earlier.
> > Please find some comments below:
> > 
> > On Wed, Jan 31, 2018 at 03:27:10PM +0200, Ville Syrjälä wrote:
> > > On Tue, Jan 30, 2018 at 06:16:59PM -0500, Lyude Paul wrote:
> > > > On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä 
> > > > > 
> > > > > Doing link retraining from the short pulse handler is problematic
> > > > > since
> > > > > that might introduce deadlocks with MST sideband processing. Currently
> > > > > we don't retrain MST links from this code, but we want to change that.
> > > > > So better to move the entire thing to the hotplug work. We can utilize
> > > > > the new encoder->hotplug() hook for this.
> > > > > 
> > > > > The only thing we leave in the short pulse handler is the link status
> > > > > check. That one still depends on the link parameters stored under
> > > > > intel_dp, so no locking around that but races should be mostly
> > > > > harmless
> > > > > as the actual retraining code will recheck the link state if we
> > > > > end up there by mistake.
> > > > > 
> > > > > v2: Rebase due to ->post_hotplug() now being just ->hotplug()
> > > > > Check the connector type to figure out if we should do
> > > > > the HDMI thing or the DP think for DDI
> > > > > 
> > > > > Cc: Manasi Navare 
> > > > > Cc: Maarten Lankhorst 
> > > > > Signed-off-by: Ville Syrjälä 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_ddi.c |  10 +-
> > > > >  drivers/gpu/drm/i915/intel_dp.c  | 196 ++--
> > > > > 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_drv.h |   2 +
> > > > >  3 files changed, 120 insertions(+), 88 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > index 25793bdc692f..5f3d58f1ae6e 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > @@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct
> > > > > intel_encoder
> > > > > *encoder,
> > > > >   drm_modeset_acquire_init(, 0);
> > > > >  
> > > > >   for (;;) {
> > > > > - ret = intel_hdmi_reset_link(encoder, );
> > > > > + if (connector->base.connector_type ==
> > > > > DRM_MODE_CONNECTOR_HDMIA)
> > > > > + ret = intel_hdmi_reset_link(encoder, );
> > > > > + else
> > > > > + ret = intel_dp_retrain_link(encoder, );
> > > > >  
> > > > >   if (ret == -EDEADLK) {
> > > > >   drm_modeset_backoff();
> > > > > @@ -3007,10 +3010,7 @@ void intel_ddi_init(struct drm_i915_private
> > > > > *dev_priv,
> > > > > enum port port)
> > > > >   drm_encoder_init(_priv->drm, encoder, _ddi_funcs,
> > > > >DRM_MODE_ENCODER_TMDS, "DDI %c",
> > > > > port_name(port));
> > > > >  
> > > > > - if (init_hdmi)
> > > > > - intel_encoder->hotplug = intel_ddi_hotplug;
> > > > > - else
> > > > > - intel_encoder->hotplug = intel_encoder_hotplug;
> > > > > + intel_encoder->hotplug = intel_ddi_hotplug;
> > > > >   intel_encoder->compute_output_type =
> > > > > intel_ddi_compute_output_type;
> > > > >   intel_encoder->compute_config = intel_ddi_compute_config;
> > > > >   intel_encoder->enable = intel_enable_ddi;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 6bbf14410c2a..152016e09a11 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -4275,12 +4275,83 @@ intel_dp_check_mst_status(struct intel_dp
> > > > > *intel_dp)
> > > > >   return -EINVAL;
> > > > >  }
> > > > >  
> > > > > -static void
> > > > > -intel_dp_retrain_link(struct intel_dp *intel_dp)
> > > > > +static bool
> > > > > +intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
> > > > > +{
> > > > > + u8 link_status[DP_LINK_STATUS_SIZE];
> > > > > +
> > > > > + if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > > > + DRM_ERROR("Failed to get link status\n");
> > > > > + return false;
> > > > > + }
> > > > > +
> > > > > + /*
> > > > > +  * Validate the cached values of intel_dp->link_rate and
> > > > > +  * intel_dp->lane_count before attempting to retrain.
> > > > > +  */
> > > > > + if (!intel_dp_link_params_valid(intel_dp, intel_dp-
> > > > > >link_rate,
> > > > > + intel_dp->lane_count))
> > > > > + return false;
> > > > > +
> > > > > + /* Retrain if Channel EQ or CR not ok */
> > > > > + return

Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-02-28 Thread Lyude Paul 

On Tue, 2018-02-27 at 23:17 -0800, Manasi Navare wrote:
> Ville,  thanks for the patch and
> Sorry for not being able to review this earlier.
> Please find some comments below:
> 
> On Wed, Jan 31, 2018 at 03:27:10PM +0200, Ville Syrjälä wrote:
> > On Tue, Jan 30, 2018 at 06:16:59PM -0500, Lyude Paul wrote:
> > > On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä 
> > > > 
> > > > Doing link retraining from the short pulse handler is problematic
> > > > since
> > > > that might introduce deadlocks with MST sideband processing. Currently
> > > > we don't retrain MST links from this code, but we want to change that.
> > > > So better to move the entire thing to the hotplug work. We can utilize
> > > > the new encoder->hotplug() hook for this.
> > > > 
> > > > The only thing we leave in the short pulse handler is the link status
> > > > check. That one still depends on the link parameters stored under
> > > > intel_dp, so no locking around that but races should be mostly
> > > > harmless
> > > > as the actual retraining code will recheck the link state if we
> > > > end up there by mistake.
> > > > 
> > > > v2: Rebase due to ->post_hotplug() now being just ->hotplug()
> > > > Check the connector type to figure out if we should do
> > > > the HDMI thing or the DP think for DDI
> > > > 
> > > > Cc: Manasi Navare 
> > > > Cc: Maarten Lankhorst 
> > > > Signed-off-by: Ville Syrjälä 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c |  10 +-
> > > >  drivers/gpu/drm/i915/intel_dp.c  | 196 ++--
> > > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_drv.h |   2 +
> > > >  3 files changed, 120 insertions(+), 88 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 25793bdc692f..5f3d58f1ae6e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct
> > > > intel_encoder
> > > > *encoder,
> > > > drm_modeset_acquire_init(, 0);
> > > >  
> > > > for (;;) {
> > > > -   ret = intel_hdmi_reset_link(encoder, );
> > > > +   if (connector->base.connector_type ==
> > > > DRM_MODE_CONNECTOR_HDMIA)
> > > > +   ret = intel_hdmi_reset_link(encoder, );
> > > > +   else
> > > > +   ret = intel_dp_retrain_link(encoder, );
> > > >  
> > > > if (ret == -EDEADLK) {
> > > > drm_modeset_backoff();
> > > > @@ -3007,10 +3010,7 @@ void intel_ddi_init(struct drm_i915_private
> > > > *dev_priv,
> > > > enum port port)
> > > > drm_encoder_init(_priv->drm, encoder, _ddi_funcs,
> > > >  DRM_MODE_ENCODER_TMDS, "DDI %c",
> > > > port_name(port));
> > > >  
> > > > -   if (init_hdmi)
> > > > -   intel_encoder->hotplug = intel_ddi_hotplug;
> > > > -   else
> > > > -   intel_encoder->hotplug = intel_encoder_hotplug;
> > > > +   intel_encoder->hotplug = intel_ddi_hotplug;
> > > > intel_encoder->compute_output_type =
> > > > intel_ddi_compute_output_type;
> > > > intel_encoder->compute_config = intel_ddi_compute_config;
> > > > intel_encoder->enable = intel_enable_ddi;
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 6bbf14410c2a..152016e09a11 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4275,12 +4275,83 @@ intel_dp_check_mst_status(struct intel_dp
> > > > *intel_dp)
> > > > return -EINVAL;
> > > >  }
> > > >  
> > > > -static void
> > > > -intel_dp_retrain_link(struct intel_dp *intel_dp)
> > > > +static bool
> > > > +intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
> > > > +{
> > > > +   u8 link_status[DP_LINK_STATUS_SIZE];
> > > > +
> > > > +   if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > > +   DRM_ERROR("Failed to get link status\n");
> > > > +   return false;
> > > > +   }
> > > > +
> > > > +   /*
> > > > +* Validate the cached values of intel_dp->link_rate and
> > > > +* intel_dp->lane_count before attempting to retrain.
> > > > +*/
> > > > +   if (!intel_dp_link_params_valid(intel_dp, intel_dp-
> > > > >link_rate,
> > > > +   intel_dp->lane_count))
> > > > +   return false;
> > > > +
> > > > +   /* Retrain if Channel EQ or CR not ok */
> > > > +   return !drm_dp_channel_eq_ok(link_status, intel_dp-
> > > > >lane_count);
> > > > +}
> > > > +
> > > > +/*
> > > > + * If display is now connected check links status,
> > > > + * there has been known issues of link loss

Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-02-27 Thread Manasi Navare 
Ville,  thanks for the patch and
Sorry for not being able to review this earlier.
Please find some comments below:

On Wed, Jan 31, 2018 at 03:27:10PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 30, 2018 at 06:16:59PM -0500, Lyude Paul wrote:
> > On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä 
> > > 
> > > Doing link retraining from the short pulse handler is problematic since
> > > that might introduce deadlocks with MST sideband processing. Currently
> > > we don't retrain MST links from this code, but we want to change that.
> > > So better to move the entire thing to the hotplug work. We can utilize
> > > the new encoder->hotplug() hook for this.
> > > 
> > > The only thing we leave in the short pulse handler is the link status
> > > check. That one still depends on the link parameters stored under
> > > intel_dp, so no locking around that but races should be mostly harmless
> > > as the actual retraining code will recheck the link state if we
> > > end up there by mistake.
> > > 
> > > v2: Rebase due to ->post_hotplug() now being just ->hotplug()
> > > Check the connector type to figure out if we should do
> > > the HDMI thing or the DP think for DDI
> > > 
> > > Cc: Manasi Navare 
> > > Cc: Maarten Lankhorst 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c |  10 +-
> > >  drivers/gpu/drm/i915/intel_dp.c  | 196 
> > > ++--
> > > ---
> > >  drivers/gpu/drm/i915/intel_drv.h |   2 +
> > >  3 files changed, 120 insertions(+), 88 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 25793bdc692f..5f3d58f1ae6e 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct intel_encoder
> > > *encoder,
> > >   drm_modeset_acquire_init(, 0);
> > >  
> > >   for (;;) {
> > > - ret = intel_hdmi_reset_link(encoder, );
> > > + if (connector->base.connector_type ==
> > > DRM_MODE_CONNECTOR_HDMIA)
> > > + ret = intel_hdmi_reset_link(encoder, );
> > > + else
> > > + ret = intel_dp_retrain_link(encoder, );
> > >  
> > >   if (ret == -EDEADLK) {
> > >   drm_modeset_backoff();
> > > @@ -3007,10 +3010,7 @@ void intel_ddi_init(struct drm_i915_private 
> > > *dev_priv,
> > > enum port port)
> > >   drm_encoder_init(_priv->drm, encoder, _ddi_funcs,
> > >DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
> > >  
> > > - if (init_hdmi)
> > > - intel_encoder->hotplug = intel_ddi_hotplug;
> > > - else
> > > - intel_encoder->hotplug = intel_encoder_hotplug;
> > > + intel_encoder->hotplug = intel_ddi_hotplug;
> > >   intel_encoder->compute_output_type = intel_ddi_compute_output_type;
> > >   intel_encoder->compute_config = intel_ddi_compute_config;
> > >   intel_encoder->enable = intel_enable_ddi;
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 6bbf14410c2a..152016e09a11 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4275,12 +4275,83 @@ intel_dp_check_mst_status(struct intel_dp 
> > > *intel_dp)
> > >   return -EINVAL;
> > >  }
> > >  
> > > -static void
> > > -intel_dp_retrain_link(struct intel_dp *intel_dp)
> > > +static bool
> > > +intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
> > > +{
> > > + u8 link_status[DP_LINK_STATUS_SIZE];
> > > +
> > > + if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > + DRM_ERROR("Failed to get link status\n");
> > > + return false;
> > > + }
> > > +
> > > + /*
> > > +  * Validate the cached values of intel_dp->link_rate and
> > > +  * intel_dp->lane_count before attempting to retrain.
> > > +  */
> > > + if (!intel_dp_link_params_valid(intel_dp, intel_dp->link_rate,
> > > + intel_dp->lane_count))
> > > + return false;
> > > +
> > > + /* Retrain if Channel EQ or CR not ok */
> > > + return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> > > +}
> > > +
> > > +/*
> > > + * If display is now connected check links status,
> > > + * there has been known issues of link loss triggering
> > > + * long pulse.
> > > + *
> > > + * Some sinks (eg. ASUS PB287Q) seem to perform some
> > > + * weird HPD ping pong during modesets. So we can apparently
> > > + * end up with HPD going low during a modeset, and then
> > > + * going back up soon after. And once that happens we must
> > > + * retrain the link to get a picture. That's in case no
> > > + * userspace component reacted to intermittent HPD dip.
> > > + */
> > > +int intel_dp_retrain_link(struct intel_encoder *encoder,
> > > +

Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-01-31 Thread Ville Syrjälä 
On Tue, Jan 30, 2018 at 06:16:59PM -0500, Lyude Paul wrote:
> On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > Doing link retraining from the short pulse handler is problematic since
> > that might introduce deadlocks with MST sideband processing. Currently
> > we don't retrain MST links from this code, but we want to change that.
> > So better to move the entire thing to the hotplug work. We can utilize
> > the new encoder->hotplug() hook for this.
> > 
> > The only thing we leave in the short pulse handler is the link status
> > check. That one still depends on the link parameters stored under
> > intel_dp, so no locking around that but races should be mostly harmless
> > as the actual retraining code will recheck the link state if we
> > end up there by mistake.
> > 
> > v2: Rebase due to ->post_hotplug() now being just ->hotplug()
> > Check the connector type to figure out if we should do
> > the HDMI thing or the DP think for DDI
> > 
> > Cc: Manasi Navare 
> > Cc: Maarten Lankhorst 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c |  10 +-
> >  drivers/gpu/drm/i915/intel_dp.c  | 196 ++--
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |   2 +
> >  3 files changed, 120 insertions(+), 88 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 25793bdc692f..5f3d58f1ae6e 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct intel_encoder
> > *encoder,
> > drm_modeset_acquire_init(, 0);
> >  
> > for (;;) {
> > -   ret = intel_hdmi_reset_link(encoder, );
> > +   if (connector->base.connector_type ==
> > DRM_MODE_CONNECTOR_HDMIA)
> > +   ret = intel_hdmi_reset_link(encoder, );
> > +   else
> > +   ret = intel_dp_retrain_link(encoder, );
> >  
> > if (ret == -EDEADLK) {
> > drm_modeset_backoff();
> > @@ -3007,10 +3010,7 @@ void intel_ddi_init(struct drm_i915_private 
> > *dev_priv,
> > enum port port)
> > drm_encoder_init(_priv->drm, encoder, _ddi_funcs,
> >  DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
> >  
> > -   if (init_hdmi)
> > -   intel_encoder->hotplug = intel_ddi_hotplug;
> > -   else
> > -   intel_encoder->hotplug = intel_encoder_hotplug;
> > +   intel_encoder->hotplug = intel_ddi_hotplug;
> > intel_encoder->compute_output_type = intel_ddi_compute_output_type;
> > intel_encoder->compute_config = intel_ddi_compute_config;
> > intel_encoder->enable = intel_enable_ddi;
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 6bbf14410c2a..152016e09a11 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4275,12 +4275,83 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> > return -EINVAL;
> >  }
> >  
> > -static void
> > -intel_dp_retrain_link(struct intel_dp *intel_dp)
> > +static bool
> > +intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
> > +{
> > +   u8 link_status[DP_LINK_STATUS_SIZE];
> > +
> > +   if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > +   DRM_ERROR("Failed to get link status\n");
> > +   return false;
> > +   }
> > +
> > +   /*
> > +* Validate the cached values of intel_dp->link_rate and
> > +* intel_dp->lane_count before attempting to retrain.
> > +*/
> > +   if (!intel_dp_link_params_valid(intel_dp, intel_dp->link_rate,
> > +   intel_dp->lane_count))
> > +   return false;
> > +
> > +   /* Retrain if Channel EQ or CR not ok */
> > +   return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> > +}
> > +
> > +/*
> > + * If display is now connected check links status,
> > + * there has been known issues of link loss triggering
> > + * long pulse.
> > + *
> > + * Some sinks (eg. ASUS PB287Q) seem to perform some
> > + * weird HPD ping pong during modesets. So we can apparently
> > + * end up with HPD going low during a modeset, and then
> > + * going back up soon after. And once that happens we must
> > + * retrain the link to get a picture. That's in case no
> > + * userspace component reacted to intermittent HPD dip.
> > + */
> > +int intel_dp_retrain_link(struct intel_encoder *encoder,
> > + struct drm_modeset_acquire_ctx *ctx)
> >  {
> > -   struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > -   struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> > +   struct intel_dp *intel_dp = enc_to_intel_dp(>base);
> > +   struct intel_connector

Re: [Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-01-30 Thread Lyude Paul 
On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Doing link retraining from the short pulse handler is problematic since
> that might introduce deadlocks with MST sideband processing. Currently
> we don't retrain MST links from this code, but we want to change that.
> So better to move the entire thing to the hotplug work. We can utilize
> the new encoder->hotplug() hook for this.
> 
> The only thing we leave in the short pulse handler is the link status
> check. That one still depends on the link parameters stored under
> intel_dp, so no locking around that but races should be mostly harmless
> as the actual retraining code will recheck the link state if we
> end up there by mistake.
> 
> v2: Rebase due to ->post_hotplug() now being just ->hotplug()
> Check the connector type to figure out if we should do
> the HDMI thing or the DP think for DDI
> 
> Cc: Manasi Navare 
> Cc: Maarten Lankhorst 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |  10 +-
>  drivers/gpu/drm/i915/intel_dp.c  | 196 ++--
> ---
>  drivers/gpu/drm/i915/intel_drv.h |   2 +
>  3 files changed, 120 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 25793bdc692f..5f3d58f1ae6e 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct intel_encoder
> *encoder,
>   drm_modeset_acquire_init(, 0);
>  
>   for (;;) {
> - ret = intel_hdmi_reset_link(encoder, );
> + if (connector->base.connector_type ==
> DRM_MODE_CONNECTOR_HDMIA)
> + ret = intel_hdmi_reset_link(encoder, );
> + else
> + ret = intel_dp_retrain_link(encoder, );
>  
>   if (ret == -EDEADLK) {
>   drm_modeset_backoff();
> @@ -3007,10 +3010,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv,
> enum port port)
>   drm_encoder_init(_priv->drm, encoder, _ddi_funcs,
>DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
>  
> - if (init_hdmi)
> - intel_encoder->hotplug = intel_ddi_hotplug;
> - else
> - intel_encoder->hotplug = intel_encoder_hotplug;
> + intel_encoder->hotplug = intel_ddi_hotplug;
>   intel_encoder->compute_output_type = intel_ddi_compute_output_type;
>   intel_encoder->compute_config = intel_ddi_compute_config;
>   intel_encoder->enable = intel_enable_ddi;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6bbf14410c2a..152016e09a11 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4275,12 +4275,83 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>   return -EINVAL;
>  }
>  
> -static void
> -intel_dp_retrain_link(struct intel_dp *intel_dp)
> +static bool
> +intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
> +{
> + u8 link_status[DP_LINK_STATUS_SIZE];
> +
> + if (!intel_dp_get_link_status(intel_dp, link_status)) {
> + DRM_ERROR("Failed to get link status\n");
> + return false;
> + }
> +
> + /*
> +  * Validate the cached values of intel_dp->link_rate and
> +  * intel_dp->lane_count before attempting to retrain.
> +  */
> + if (!intel_dp_link_params_valid(intel_dp, intel_dp->link_rate,
> + intel_dp->lane_count))
> + return false;
> +
> + /* Retrain if Channel EQ or CR not ok */
> + return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> +}
> +
> +/*
> + * If display is now connected check links status,
> + * there has been known issues of link loss triggering
> + * long pulse.
> + *
> + * Some sinks (eg. ASUS PB287Q) seem to perform some
> + * weird HPD ping pong during modesets. So we can apparently
> + * end up with HPD going low during a modeset, and then
> + * going back up soon after. And once that happens we must
> + * retrain the link to get a picture. That's in case no
> + * userspace component reacted to intermittent HPD dip.
> + */
> +int intel_dp_retrain_link(struct intel_encoder *encoder,
> +   struct drm_modeset_acquire_ctx *ctx)
>  {
> - struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> - struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> + struct intel_dp *intel_dp = enc_to_intel_dp(>base);
> + struct intel_connector *connector = intel_dp->attached_connector;
> + struct drm_connector_state *conn_state;
> + struct intel_crtc_state *crtc_state;
> + struct intel_crtc *crtc;
> + int ret;
> +
> + /* FIXME handle

[Intel-gfx] [PATCH v2 3/5] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

2018-01-18 Thread Ville Syrjala 
From: Ville Syrjälä 

Doing link retraining from the short pulse handler is problematic since
that might introduce deadlocks with MST sideband processing. Currently
we don't retrain MST links from this code, but we want to change that.
So better to move the entire thing to the hotplug work. We can utilize
the new encoder->hotplug() hook for this.

The only thing we leave in the short pulse handler is the link status
check. That one still depends on the link parameters stored under
intel_dp, so no locking around that but races should be mostly harmless
as the actual retraining code will recheck the link state if we
end up there by mistake.

v2: Rebase due to ->post_hotplug() now being just ->hotplug()
Check the connector type to figure out if we should do
the HDMI thing or the DP think for DDI

Cc: Manasi Navare 
Cc: Maarten Lankhorst 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_ddi.c |  10 +-
 drivers/gpu/drm/i915/intel_dp.c  | 196 ++-
 drivers/gpu/drm/i915/intel_drv.h |   2 +
 3 files changed, 120 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 25793bdc692f..5f3d58f1ae6e 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2880,7 +2880,10 @@ static bool intel_ddi_hotplug(struct intel_encoder 
*encoder,
drm_modeset_acquire_init(, 0);
 
for (;;) {
-   ret = intel_hdmi_reset_link(encoder, );
+   if (connector->base.connector_type == DRM_MODE_CONNECTOR_HDMIA)
+   ret = intel_hdmi_reset_link(encoder, );
+   else
+   ret = intel_dp_retrain_link(encoder, );
 
if (ret == -EDEADLK) {
drm_modeset_backoff();
@@ -3007,10 +3010,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, 
enum port port)
drm_encoder_init(_priv->drm, encoder, _ddi_funcs,
 DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
 
-   if (init_hdmi)
-   intel_encoder->hotplug = intel_ddi_hotplug;
-   else
-   intel_encoder->hotplug = intel_encoder_hotplug;
+   intel_encoder->hotplug = intel_ddi_hotplug;
intel_encoder->compute_output_type = intel_ddi_compute_output_type;
intel_encoder->compute_config = intel_ddi_compute_config;
intel_encoder->enable = intel_enable_ddi;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6bbf14410c2a..152016e09a11 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4275,12 +4275,83 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
return -EINVAL;
 }
 
-static void
-intel_dp_retrain_link(struct intel_dp *intel_dp)
+static bool
+intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
+{
+   u8 link_status[DP_LINK_STATUS_SIZE];
+
+   if (!intel_dp_get_link_status(intel_dp, link_status)) {
+   DRM_ERROR("Failed to get link status\n");
+   return false;
+   }
+
+   /*
+* Validate the cached values of intel_dp->link_rate and
+* intel_dp->lane_count before attempting to retrain.
+*/
+   if (!intel_dp_link_params_valid(intel_dp, intel_dp->link_rate,
+   intel_dp->lane_count))
+   return false;
+
+   /* Retrain if Channel EQ or CR not ok */
+   return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
+}
+
+/*
+ * If display is now connected check links status,
+ * there has been known issues of link loss triggering
+ * long pulse.
+ *
+ * Some sinks (eg. ASUS PB287Q) seem to perform some
+ * weird HPD ping pong during modesets. So we can apparently
+ * end up with HPD going low during a modeset, and then
+ * going back up soon after. And once that happens we must
+ * retrain the link to get a picture. That's in case no
+ * userspace component reacted to intermittent HPD dip.
+ */
+int intel_dp_retrain_link(struct intel_encoder *encoder,
+ struct drm_modeset_acquire_ctx *ctx)
 {
-   struct intel_encoder *encoder = _to_dig_port(intel_dp)->base;
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-   struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
+   struct intel_dp *intel_dp = enc_to_intel_dp(>base);
+   struct intel_connector *connector = intel_dp->attached_connector;
+   struct drm_connector_state *conn_state;
+   struct intel_crtc_state *crtc_state;
+   struct intel_crtc *crtc;
+   int ret;
+
+   /* FIXME handle the MST connectors as well */
+
+   if (!connector || connector->base.status != connector_status_connected)
+   return 0;
+
+   ret =