[PATCH 0/2] Try to fix MST regression with DDI IO power domains

[PATCH 0/2] Try to fix MST regression with DDI IO power domains

[Ander Conselvan de Oliveira]

Hi,

Commit 62b695662a24 ("drm/i915: Only enable DDI IO power domains after
enabling DPLL") seems to have broken MST. Here's an untested fix (I
don't have the right hardware to test this at the moment).

Thanks,
Ander

Ander Conselvan de Oliveira (2):
  drm/i915: Enable DDI IO power domains in the DP MST path
  drm/i915: Remove duplicate DDI enabling logic from MST path

 drivers/gpu/drm/i915/intel_ddi.c    |  2 ++
 drivers/gpu/drm/i915/intel_dp_mst.c | 20 +++-----------------
 2 files changed, 5 insertions(+), 17 deletions(-)

-- 
2.9.3

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

[PATCH 1/2] drm/i915: Enable DDI IO power domains in the DP MST path

[Ander Conselvan de Oliveira]

Commit 62b695662a24 ("drm/i915: Only enable DDI IO power domains after
enabling DPLL") changed how the DDI IO power domains get enabled, but
neglected the need to enable those domains when enabling a DP connector
with MST enabled, leading to

    Kernel panic - not syncing: Timeout: Not all CPUs entered broadcast exception handler

Fixes: 62b695662a24 ("drm/i915: Only enable DDI IO power domains after enabling DPLL")
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index d94fd4b..a8334e1 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -163,6 +163,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 		intel_ddi_clk_select(&intel_dig_port->base,
 				     pipe_config->shared_dpll);
 
+		intel_display_power_get(dev_priv,
+					intel_dig_port->ddi_io_power_domain);
+
 		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
 		intel_dp_set_link_params(intel_dp,
 					 pipe_config->port_clock,
-- 
2.9.3

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

Re: [PATCH 1/2] drm/i915: Enable DDI IO power domains in the DP MST path

[Imre Deak]

On Tue, Feb 28, 2017 at 09:21:12AM +0200, Ander Conselvan de Oliveira wrote:
> Commit 62b695662a24 ("drm/i915: Only enable DDI IO power domains after
> enabling DPLL") changed how the DDI IO power domains get enabled, but
> neglected the need to enable those domains when enabling a DP connector
> with MST enabled, leading to
> 
>     Kernel panic - not syncing: Timeout: Not all CPUs entered broadcast exception handler
> 
> Fixes: 62b695662a24 ("drm/i915: Only enable DDI IO power domains after enabling DPLL")
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index d94fd4b..a8334e1 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -163,6 +163,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  		intel_ddi_clk_select(&intel_dig_port->base,
>  				     pipe_config->shared_dpll);
>  
> +		intel_display_power_get(dev_priv,
> +					intel_dig_port->ddi_io_power_domain);
> +
>  		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
>  		intel_dp_set_link_params(intel_dp,
>  					 pipe_config->port_clock,
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Remove duplicate DDI enabling logic from MST path

[Ander Conselvan de Oliveira]

The logic to enable a DDI in intel_mst_pre_enable_dp() is essentially
the same as in intel_ddi_pre_enable_dp(). So reuse the latter function
by calling the post_disable hook on the intel_dig_port instead of
duplicating that code.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c    |  2 ++
 drivers/gpu/drm/i915/intel_dp_mst.c | 23 +++--------------------
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index e9013f1..71aaddf 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1695,6 +1695,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 	enum port port = intel_ddi_get_encoder_port(encoder);
 	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
 
+	WARN_ON(link_mst && port == PORT_A);
+
 	intel_dp_set_link_params(intel_dp, link_rate, lane_count,
 				 link_mst);
 	if (encoder->type == INTEL_OUTPUT_EDP)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index a8334e1..094cbdc 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -159,26 +159,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
 
-	if (intel_dp->active_mst_links == 0) {
-		intel_ddi_clk_select(&intel_dig_port->base,
-				     pipe_config->shared_dpll);
-
-		intel_display_power_get(dev_priv,
-					intel_dig_port->ddi_io_power_domain);
-
-		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
-		intel_dp_set_link_params(intel_dp,
-					 pipe_config->port_clock,
-					 pipe_config->lane_count,
-					 true);
-
-		intel_ddi_init_dp_buf_reg(&intel_dig_port->base);
-
-		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
-
-		intel_dp_start_link_train(intel_dp);
-		intel_dp_stop_link_train(intel_dp);
-	}
+	if (intel_dp->active_mst_links == 0)
+		intel_dig_port->base.pre_enable(&intel_dig_port->base,
+						pipe_config, NULL);
 
 	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
 				       connector->port,
-- 
2.9.3

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

Re: [PATCH 2/2] drm/i915: Remove duplicate DDI enabling logic from MST path

[Imre Deak]

On Tue, Feb 28, 2017 at 09:21:13AM +0200, Ander Conselvan de Oliveira wrote:
> The logic to enable a DDI in intel_mst_pre_enable_dp() is essentially
> the same as in intel_ddi_pre_enable_dp(). So reuse the latter function
> by calling the post_disable hook on the intel_dig_port instead of
                 ^pre_enable
> duplicating that code.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c    |  2 ++
>  drivers/gpu/drm/i915/intel_dp_mst.c | 23 +++--------------------
>  2 files changed, 5 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index e9013f1..71aaddf 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1695,6 +1695,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  	enum port port = intel_ddi_get_encoder_port(encoder);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>  
> +	WARN_ON(link_mst && port == PORT_A);
> +

So IIUC, this wasn't used before for MST and so link_mst was always
false. Also link_mst=true implies type == INTEL_OUTPUT_DP, as this will
be called for the primary encoder. After the change crtc->config will be
used instead of the (new) pipe_config, but this is fine since the two
are the same in the enable hooks. Looks ok to me:

Reviewed-by: Imre Deak <imre.deak@intel.com>

>  	intel_dp_set_link_params(intel_dp, link_rate, lane_count,
>  				 link_mst);
>  	if (encoder->type == INTEL_OUTPUT_EDP)
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index a8334e1..094cbdc 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -159,26 +159,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  
>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>  
> -	if (intel_dp->active_mst_links == 0) {
> -		intel_ddi_clk_select(&intel_dig_port->base,
> -				     pipe_config->shared_dpll);
> -
> -		intel_display_power_get(dev_priv,
> -					intel_dig_port->ddi_io_power_domain);
> -
> -		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
> -		intel_dp_set_link_params(intel_dp,
> -					 pipe_config->port_clock,
> -					 pipe_config->lane_count,
> -					 true);
> -
> -		intel_ddi_init_dp_buf_reg(&intel_dig_port->base);
> -
> -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> -
> -		intel_dp_start_link_train(intel_dp);
> -		intel_dp_stop_link_train(intel_dp);
> -	}
> +	if (intel_dp->active_mst_links == 0)
> +		intel_dig_port->base.pre_enable(&intel_dig_port->base,
> +						pipe_config, NULL);
>  
>  	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
>  				       connector->port,
> -- 
> 2.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Remove duplicate DDI enabling logic from MST path

[Ville Syrjälä]

On Tue, Feb 28, 2017 at 03:59:52PM +0200, Imre Deak wrote:
> On Tue, Feb 28, 2017 at 09:21:13AM +0200, Ander Conselvan de Oliveira wrote:
> > The logic to enable a DDI in intel_mst_pre_enable_dp() is essentially
> > the same as in intel_ddi_pre_enable_dp(). So reuse the latter function
> > by calling the post_disable hook on the intel_dig_port instead of
>                  ^pre_enable
> > duplicating that code.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c    |  2 ++
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 23 +++--------------------
> >  2 files changed, 5 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index e9013f1..71aaddf 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1695,6 +1695,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >  	enum port port = intel_ddi_get_encoder_port(encoder);
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> >  
> > +	WARN_ON(link_mst && port == PORT_A);
> > +
> 
> So IIUC, this wasn't used before for MST and so link_mst was always
> false.

The MST code explicitly passes 'true' here.

> Also link_mst=true implies type == INTEL_OUTPUT_DP, as this will
> be called for the primary encoder. After the change crtc->config will be
> used instead of the (new) pipe_config, but this is fine since the two
> are the same in the enable hooks.

Nope. AFAICS this should explode totally since encoder->crtc for the
primary will be NULL. I guess Maarten didn't yet nuke all crtc->config
usage from the ddi code (or he did but the patches didn't get reviewed).

> Looks ok to me:
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> >  	intel_dp_set_link_params(intel_dp, link_rate, lane_count,
> >  				 link_mst);
> >  	if (encoder->type == INTEL_OUTPUT_EDP)
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index a8334e1..094cbdc 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -159,26 +159,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> >  
> >  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> >  
> > -	if (intel_dp->active_mst_links == 0) {
> > -		intel_ddi_clk_select(&intel_dig_port->base,
> > -				     pipe_config->shared_dpll);
> > -
> > -		intel_display_power_get(dev_priv,
> > -					intel_dig_port->ddi_io_power_domain);
> > -
> > -		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
> > -		intel_dp_set_link_params(intel_dp,
> > -					 pipe_config->port_clock,
> > -					 pipe_config->lane_count,
> > -					 true);
> > -
> > -		intel_ddi_init_dp_buf_reg(&intel_dig_port->base);
> > -
> > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > -
> > -		intel_dp_start_link_train(intel_dp);
> > -		intel_dp_stop_link_train(intel_dp);
> > -	}
> > +	if (intel_dp->active_mst_links == 0)
> > +		intel_dig_port->base.pre_enable(&intel_dig_port->base,
> > +						pipe_config, NULL);
> >  
> >  	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
> >  				       connector->port,
> > -- 
> > 2.9.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/2] drm/i915: Remove duplicate DDI enabling logic from MST path

[Imre Deak]

On Tue, Feb 28, 2017 at 04:09:31PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 28, 2017 at 03:59:52PM +0200, Imre Deak wrote:
> > On Tue, Feb 28, 2017 at 09:21:13AM +0200, Ander Conselvan de Oliveira wrote:
> > > The logic to enable a DDI in intel_mst_pre_enable_dp() is essentially
> > > the same as in intel_ddi_pre_enable_dp(). So reuse the latter function
> > > by calling the post_disable hook on the intel_dig_port instead of
> >                  ^pre_enable
> > > duplicating that code.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c    |  2 ++
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 23 +++--------------------
> > >  2 files changed, 5 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index e9013f1..71aaddf 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1695,6 +1695,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > >  	enum port port = intel_ddi_get_encoder_port(encoder);
> > >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> > >  
> > > +	WARN_ON(link_mst && port == PORT_A);
> > > +
> > 
> > So IIUC, this wasn't used before for MST and so link_mst was always
> > false.
> 
> The MST code explicitly passes 'true' here.

Yes, after the change. I was just wondering why this wasn't used for MST
before.

> 
> > Also link_mst=true implies type == INTEL_OUTPUT_DP, as this will
> > be called for the primary encoder. After the change crtc->config will be
> > used instead of the (new) pipe_config, but this is fine since the two
> > are the same in the enable hooks.
> 
> Nope. AFAICS this should explode totally since encoder->crtc for the
> primary will be NULL. I guess Maarten didn't yet nuke all crtc->config
> usage from the ddi code (or he did but the patches didn't get reviewed).

Err, right. So this would need first changing to use crtc->config to
pipe_config in intel_ddi_pre_enable.

--Imre

> 
> > Looks ok to me:
> > 
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > >  	intel_dp_set_link_params(intel_dp, link_rate, lane_count,
> > >  				 link_mst);
> > >  	if (encoder->type == INTEL_OUTPUT_EDP)
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index a8334e1..094cbdc 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -159,26 +159,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > >  
> > >  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > >  
> > > -	if (intel_dp->active_mst_links == 0) {
> > > -		intel_ddi_clk_select(&intel_dig_port->base,
> > > -				     pipe_config->shared_dpll);
> > > -
> > > -		intel_display_power_get(dev_priv,
> > > -					intel_dig_port->ddi_io_power_domain);
> > > -
> > > -		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
> > > -		intel_dp_set_link_params(intel_dp,
> > > -					 pipe_config->port_clock,
> > > -					 pipe_config->lane_count,
> > > -					 true);
> > > -
> > > -		intel_ddi_init_dp_buf_reg(&intel_dig_port->base);
> > > -
> > > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > -
> > > -		intel_dp_start_link_train(intel_dp);
> > > -		intel_dp_stop_link_train(intel_dp);
> > > -	}
> > > +	if (intel_dp->active_mst_links == 0)
> > > +		intel_dig_port->base.pre_enable(&intel_dig_port->base,
> > > +						pipe_config, NULL);
> > >  
> > >  	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
> > >  				       connector->port,
> > > -- 
> > > 2.9.3
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Remove duplicate DDI enabling logic from MST path

[Ville Syrjälä]

On Tue, Feb 28, 2017 at 04:33:46PM +0200, Imre Deak wrote:
> On Tue, Feb 28, 2017 at 04:09:31PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 28, 2017 at 03:59:52PM +0200, Imre Deak wrote:
> > > On Tue, Feb 28, 2017 at 09:21:13AM +0200, Ander Conselvan de Oliveira wrote:
> > > > The logic to enable a DDI in intel_mst_pre_enable_dp() is essentially
> > > > the same as in intel_ddi_pre_enable_dp(). So reuse the latter function
> > > > by calling the post_disable hook on the intel_dig_port instead of
> > >                  ^pre_enable
> > > > duplicating that code.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c    |  2 ++
> > > >  drivers/gpu/drm/i915/intel_dp_mst.c | 23 +++--------------------
> > > >  2 files changed, 5 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index e9013f1..71aaddf 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -1695,6 +1695,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > > >  	enum port port = intel_ddi_get_encoder_port(encoder);
> > > >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> > > >  
> > > > +	WARN_ON(link_mst && port == PORT_A);
> > > > +
> > > 
> > > So IIUC, this wasn't used before for MST and so link_mst was always
> > > false.
> > 
> > The MST code explicitly passes 'true' here.
> 
> Yes, after the change.

It was 'true' before. After the change it'll be 'intel_crtc_has_type(MST)'

> I was just wondering why this wasn't used for MST
> before.
> 
> > 
> > > Also link_mst=true implies type == INTEL_OUTPUT_DP, as this will
> > > be called for the primary encoder. After the change crtc->config will be
> > > used instead of the (new) pipe_config, but this is fine since the two
> > > are the same in the enable hooks.
> > 
> > Nope. AFAICS this should explode totally since encoder->crtc for the
> > primary will be NULL. I guess Maarten didn't yet nuke all crtc->config
> > usage from the ddi code (or he did but the patches didn't get reviewed).
> 
> Err, right. So this would need first changing to use crtc->config to
> pipe_config in intel_ddi_pre_enable.

+ review all the code that gets called from there to make sure no more
users are lurking somewhere deeper.

> 
> --Imre
> 
> > 
> > > Looks ok to me:
> > > 
> > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > >  	intel_dp_set_link_params(intel_dp, link_rate, lane_count,
> > > >  				 link_mst);
> > > >  	if (encoder->type == INTEL_OUTPUT_EDP)
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > index a8334e1..094cbdc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > @@ -159,26 +159,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > > >  
> > > >  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > > >  
> > > > -	if (intel_dp->active_mst_links == 0) {
> > > > -		intel_ddi_clk_select(&intel_dig_port->base,
> > > > -				     pipe_config->shared_dpll);
> > > > -
> > > > -		intel_display_power_get(dev_priv,
> > > > -					intel_dig_port->ddi_io_power_domain);
> > > > -
> > > > -		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
> > > > -		intel_dp_set_link_params(intel_dp,
> > > > -					 pipe_config->port_clock,
> > > > -					 pipe_config->lane_count,
> > > > -					 true);
> > > > -
> > > > -		intel_ddi_init_dp_buf_reg(&intel_dig_port->base);
> > > > -
> > > > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > > -
> > > > -		intel_dp_start_link_train(intel_dp);
> > > > -		intel_dp_stop_link_train(intel_dp);
> > > > -	}
> > > > +	if (intel_dp->active_mst_links == 0)
> > > > +		intel_dig_port->base.pre_enable(&intel_dig_port->base,
> > > > +						pipe_config, NULL);
> > > >  
> > > >  	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
> > > >  				       connector->port,
> > > > -- 
> > > > 2.9.3
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > 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 2/2] drm/i915: Remove duplicate DDI enabling logic from MST path

[Ville Syrjälä]

On Tue, Feb 28, 2017 at 04:47:51PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 28, 2017 at 04:33:46PM +0200, Imre Deak wrote:
> > On Tue, Feb 28, 2017 at 04:09:31PM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 28, 2017 at 03:59:52PM +0200, Imre Deak wrote:
> > > > On Tue, Feb 28, 2017 at 09:21:13AM +0200, Ander Conselvan de Oliveira wrote:
> > > > > The logic to enable a DDI in intel_mst_pre_enable_dp() is essentially
> > > > > the same as in intel_ddi_pre_enable_dp(). So reuse the latter function
> > > > > by calling the post_disable hook on the intel_dig_port instead of
> > > >                  ^pre_enable
> > > > > duplicating that code.
> > > > > 
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_ddi.c    |  2 ++
> > > > >  drivers/gpu/drm/i915/intel_dp_mst.c | 23 +++--------------------
> > > > >  2 files changed, 5 insertions(+), 20 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > index e9013f1..71aaddf 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > @@ -1695,6 +1695,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > > > >  	enum port port = intel_ddi_get_encoder_port(encoder);
> > > > >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> > > > >  
> > > > > +	WARN_ON(link_mst && port == PORT_A);
> > > > > +
> > > > 
> > > > So IIUC, this wasn't used before for MST and so link_mst was always
> > > > false.
> > > 
> > > The MST code explicitly passes 'true' here.
> > 
> > Yes, after the change.
> 
> It was 'true' before. After the change it'll be 'intel_crtc_has_type(MST)'

Ahem. So I thought we were talking about intel_dp_set_link_params(), but
really this was about intel_ddi_pre_enable_dp(). So yes, you're correct
that intel_ddi_pre_enable_dp() was not actually used for MST.

> 
> > I was just wondering why this wasn't used for MST
> > before.
> > 
> > > 
> > > > Also link_mst=true implies type == INTEL_OUTPUT_DP, as this will
> > > > be called for the primary encoder. After the change crtc->config will be
> > > > used instead of the (new) pipe_config, but this is fine since the two
> > > > are the same in the enable hooks.
> > > 
> > > Nope. AFAICS this should explode totally since encoder->crtc for the
> > > primary will be NULL. I guess Maarten didn't yet nuke all crtc->config
> > > usage from the ddi code (or he did but the patches didn't get reviewed).
> > 
> > Err, right. So this would need first changing to use crtc->config to
> > pipe_config in intel_ddi_pre_enable.
> 
> + review all the code that gets called from there to make sure no more
> users are lurking somewhere deeper.
> 
> > 
> > --Imre
> > 
> > > 
> > > > Looks ok to me:
> > > > 
> > > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > > > 
> > > > >  	intel_dp_set_link_params(intel_dp, link_rate, lane_count,
> > > > >  				 link_mst);
> > > > >  	if (encoder->type == INTEL_OUTPUT_EDP)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > index a8334e1..094cbdc 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > @@ -159,26 +159,9 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > > > >  
> > > > >  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> > > > >  
> > > > > -	if (intel_dp->active_mst_links == 0) {
> > > > > -		intel_ddi_clk_select(&intel_dig_port->base,
> > > > > -				     pipe_config->shared_dpll);
> > > > > -
> > > > > -		intel_display_power_get(dev_priv,
> > > > > -					intel_dig_port->ddi_io_power_domain);
> > > > > -
> > > > > -		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
> > > > > -		intel_dp_set_link_params(intel_dp,
> > > > > -					 pipe_config->port_clock,
> > > > > -					 pipe_config->lane_count,
> > > > > -					 true);
> > > > > -
> > > > > -		intel_ddi_init_dp_buf_reg(&intel_dig_port->base);
> > > > > -
> > > > > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > > > -
> > > > > -		intel_dp_start_link_train(intel_dp);
> > > > > -		intel_dp_stop_link_train(intel_dp);
> > > > > -	}
> > > > > +	if (intel_dp->active_mst_links == 0)
> > > > > +		intel_dig_port->base.pre_enable(&intel_dig_port->base,
> > > > > +						pipe_config, NULL);
> > > > >  
> > > > >  	ret = drm_dp_mst_allocate_vcpi(&intel_dp->mst_mgr,
> > > > >  				       connector->port,
> > > > > -- 
> > > > > 2.9.3
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > 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

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

✗ Fi.CI.BAT: failure for Try to fix MST regression with DDI IO power domains

[Patchwork]

== Series Details ==

Series: Try to fix MST regression with DDI IO power domains
URL   : https://patchwork.freedesktop.org/series/20345/
State : failure

== Summary ==

Series 20345v1 Try to fix MST regression with DDI IO power domains
https://patchwork.freedesktop.org/api/1.0/series/20345/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600)
Test gem_exec_parallel:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-skl-6700hq)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:260  dwarn:1   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29 

1a8bd0fb40e5d02f827f925b7702ed6f64fadce2 drm-tip: 2017y-02m-27d-22h-04m-19s UTC integration manifest
a225efb drm/i915: Remove duplicate DDI enabling logic from MST path
a6623f1 drm/i915: Enable DDI IO power domains in the DP MST path

== Logs ==

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

Re: ✗ Fi.CI.BAT: failure for Try to fix MST regression with DDI IO power domains

[Ander Conselvan De Oliveira]

On Tue, 2017-02-28 at 07:53 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: Try to fix MST regression with DDI IO power domains
> URL   : https://patchwork.freedesktop.org/series/20345/
> State : failure
> 
> == Summary ==
> 
> Series 20345v1 Try to fix MST regression with DDI IO power domains
> https://patchwork.freedesktop.org/api/1.0/series/20345/revisions/1/mbox/
> 
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-uc:
>                 pass       -> FAIL       (fi-snb-2600)

I filed a bug for this one, it happened a couple of times in the past CI rounds.

https://bugs.freedesktop.org/show_bug.cgi?id=100007


> Test gem_exec_parallel:
>         Subgroup basic:
>                 pass       -> DMESG-WARN (fi-skl-6700hq)

I'm not sure what to make of this:

[  276.628178] BUG: spinlock already unlocked on CPU#6, gem_exec_parall/6632
[  276.628185]  lock: 0xffff8801a0f53220, .magic: dead4ead, .owner: <none>/-1, .owner_cpu: -1
[  276.628190] CPU: 6 PID: 6632 Comm: gem_exec_parall Not tainted 4.10.0-CI-Patchwork_3992+ #1
[  276.628193] Hardware name: TOSHIBA SATELLITE P50-C/06F4                            , BIOS 1.20 10/08/2015
[  276.628197] Call Trace:
[  276.628198]  <IRQ>
[  276.628202]  dump_stack+0x67/0x92
[  276.628205]  spin_dump+0x73/0xc0
[  276.628208]  do_raw_spin_unlock+0x79/0xb0
[  276.628212]  _raw_spin_unlock_irqrestore+0x27/0x60
[  276.628216]  dma_fence_signal+0x160/0x230
[  276.628242]  notify_ring+0xae/0x2e0 [i915]
[  276.628255]  ? ibx_hpd_irq_handler+0xc0/0xc0 [i915]
[  276.628269]  gen8_gt_irq_handler+0x219/0x290 [i915]
[  276.628283]  gen8_irq_handler+0x8e/0x6b0 [i915]
[  276.628286]  __handle_irq_event_percpu+0x58/0x370
[  276.628289]  handle_irq_event_percpu+0x1e/0x50
[  276.628292]  handle_irq_event+0x34/0x60
[  276.628294]  handle_edge_irq+0xbe/0x150
[  276.628299]  handle_irq+0x15/0x20
[  276.628302]  do_IRQ+0x63/0x130
[  276.628306]  common_interrupt+0x90/0x90
[  276.628310] RIP: 0010:osq_lock+0x77/0x110
[  276.628313] RSP: 0018:ffffc90009113a80 EFLAGS: 00000246 ORIG_RAX: ffffffffffffffad
[  276.628318] RAX: 0000000000000000 RBX: ffff880281d98d40 RCX: ffff88022181d758
[  276.628322] RDX: ffff88022181cf40 RSI: ffffffff81c65a58 RDI: ffffffff81c73840
[  276.628325] RBP: ffffc90009113a98 R08: 000000003f352d81 R09: 3e2d9cbd00000000
[  276.628329] R10: 0000000000000001 R11: 0000000000000001 R12: ffff880281c18d40
[  276.628333] R13: ffff88026ab400b0 R14: 0000000000000000 R15: ffff88026ab40070
[  276.628336]  </IRQ>
[  276.628340]  mutex_optimistic_spin+0xf4/0x2a0
[  276.628343]  ? mutex_optimistic_spin+0x39/0x2a0
[  276.628362]  ? i915_mutex_lock_interruptible+0x39/0x140 [i915]
[  276.628366]  mutex_lock_interruptible_nested+0xa9/0x920
[  276.628399]  ? i915_mutex_lock_interruptible+0x39/0x140 [i915]
[  276.628416]  ? i915_mutex_lock_interruptible+0x39/0x140 [i915]
[  276.628433]  i915_mutex_lock_interruptible+0x39/0x140 [i915]
[  276.628437]  ? __pm_runtime_resume+0x56/0x80
[  276.628453]  i915_gem_do_execbuffer.isra.15+0x419/0x1ba0 [i915]
[  276.628457]  ? __lock_acquire+0x449/0x1b50
[  276.628461]  ? __might_fault+0x3e/0x90
[  276.628464]  ? __might_fault+0x87/0x90
[  276.628467]  ? __might_fault+0x3e/0x90
[  276.628482]  i915_gem_execbuffer2+0xb5/0x220 [i915]
[  276.628487]  drm_ioctl+0x200/0x450
[  276.628502]  ? i915_gem_execbuffer+0x330/0x330 [i915]
[  276.628507]  do_vfs_ioctl+0x90/0x6e0
[  276.628510]  ? __fget+0x108/0x200
[  276.628513]  ? expand_files+0x2b0/0x2b0
[  276.628517]  SyS_ioctl+0x3c/0x70
[  276.628520]  entry_SYSCALL_64_fastpath+0x1c/0xb1
[  276.628523] RIP: 0033:0x7f5914d87357
[  276.628526] RSP: 002b:00007f58ed475bf8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[  276.628531] RAX: ffffffffffffffda RBX: ffffffff81474ff3 RCX: 00007f5914d87357
[  276.628534] RDX: 00007f58ed475c70 RSI: 0000000040406469 RDI: 0000000000000003
[  276.628538] RBP: ffffc90009113f88 R08: 0000000000000040 R09: 00000000000003f7
[  276.628541] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[  276.628545] R13: 0000000000000003 R14: 0000000040406469 R15: 00000000024105f0
[  276.628549]  ? __this_cpu_preempt_check+0x13/0x20


Ander

> fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
> fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
> fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
> fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
> fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
> fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
> fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
> fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
> fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
> fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
> fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
> fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
> fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
> fi-skl-6700hq    total:278  pass:260  dwarn:1   dfail:0   fail:0   skip:17 
> fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
> fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
> fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
> fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29 
> 
> 1a8bd0fb40e5d02f827f925b7702ed6f64fadce2 drm-tip: 2017y-02m-27d-22h-04m-19s UTC integration manifest
> a225efb drm/i915: Remove duplicate DDI enabling logic from MST path
> a6623f1 drm/i915: Enable DDI IO power domains in the DP MST path
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3992/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx