Re: [Intel-gfx] [PATCH 2/4] drm/i915: Release DDI power well references in MST ports

[Imre Deak]

On Fri, Nov 02, 2018 at 01:39:22PM -0700, José Roberto de Souza wrote:
> MST ports did not had the post_pll_disable() hook causing the
> references get in pre_pll_enable() never being released causing
> DDI and AUX CH being enabled all the times.
> 
> Cc: Imre Deak 
> Cc: Manasi Navare 
> Signed-off-by: José Roberto de Souza 

Could you resend this and the first patch separately?

Thanks,
Imre

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8b71d64ebd9d..8e3d0b04580b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -215,6 +215,20 @@ static void intel_mst_pre_pll_enable_dp(struct 
> intel_encoder *encoder,
>   pipe_config, NULL);
>  }
>  
> +static void intel_mst_post_pll_disable_dp(struct intel_encoder *encoder,
> +   const struct intel_crtc_state 
> *pipe_config,
> +   const struct drm_connector_state 
> *conn_state)
> +{
> + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> + struct intel_digital_port *intel_dig_port = intel_mst->primary;
> + struct intel_dp *intel_dp = &intel_dig_port->dp;
> +
> + if (intel_dp->active_mst_links == 0 &&
> + intel_dig_port->base.post_pll_disable)
> + intel_dig_port->base.post_pll_disable(&intel_dig_port->base,
> +   pipe_config, NULL);
> +}
> +
>  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>   const struct intel_crtc_state *pipe_config,
>   const struct drm_connector_state 
> *conn_state)
> @@ -549,6 +563,7 @@ intel_dp_create_fake_mst_encoder(struct 
> intel_digital_port *intel_dig_port, enum
>   intel_encoder->disable = intel_mst_disable_dp;
>   intel_encoder->post_disable = intel_mst_post_disable_dp;
>   intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
> + intel_encoder->post_pll_disable = intel_mst_post_pll_disable_dp;
>   intel_encoder->pre_enable = intel_mst_pre_enable_dp;
>   intel_encoder->enable = intel_mst_enable_dp;
>   intel_encoder->get_hw_state = intel_dp_mst_enc_get_hw_state;
> -- 
> 2.19.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Release DDI power well references in MST ports

[Imre Deak]

On Fri, Nov 02, 2018 at 01:39:22PM -0700, José Roberto de Souza wrote:
> MST ports did not had the post_pll_disable() hook causing the
> references get in pre_pll_enable() never being released causing
> DDI and AUX CH being enabled all the times.
> 
> Cc: Imre Deak 
> Cc: Manasi Navare 
> Signed-off-by: José Roberto de Souza 

Err, thanks for catching it. We really need more testing on MST.. You
could also add the Fixes: line.

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8b71d64ebd9d..8e3d0b04580b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -215,6 +215,20 @@ static void intel_mst_pre_pll_enable_dp(struct 
> intel_encoder *encoder,
>   pipe_config, NULL);
>  }
>  
> +static void intel_mst_post_pll_disable_dp(struct intel_encoder *encoder,
> +   const struct intel_crtc_state 
> *pipe_config,
> +   const struct drm_connector_state 
> *conn_state)
> +{

Nit: these are better called old_crtc_state, old_conn_state.

> + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> + struct intel_digital_port *intel_dig_port = intel_mst->primary;
> + struct intel_dp *intel_dp = &intel_dig_port->dp;
> +
> + if (intel_dp->active_mst_links == 0 &&
> + intel_dig_port->base.post_pll_disable)

Since by now all DDI platforms (which are the only platforms supporting
MST) define the .pre_pll_enable() .post_pll_disable() hooks you could
remove the NULL check for them (in a follow-up patch).

Either with or without changing the above nit:

Reviewed-by: Imre Deak 

> + intel_dig_port->base.post_pll_disable(&intel_dig_port->base,
> +   pipe_config, NULL);
> +}
> +
>  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>   const struct intel_crtc_state *pipe_config,
>   const struct drm_connector_state 
> *conn_state)
> @@ -549,6 +563,7 @@ intel_dp_create_fake_mst_encoder(struct 
> intel_digital_port *intel_dig_port, enum
>   intel_encoder->disable = intel_mst_disable_dp;
>   intel_encoder->post_disable = intel_mst_post_disable_dp;
>   intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
> + intel_encoder->post_pll_disable = intel_mst_post_pll_disable_dp;
>   intel_encoder->pre_enable = intel_mst_pre_enable_dp;
>   intel_encoder->enable = intel_mst_enable_dp;
>   intel_encoder->get_hw_state = intel_dp_mst_enc_get_hw_state;
> -- 
> 2.19.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/4] drm/i915: Release DDI power well references in MST ports

[Ville Syrjälä]

On Fri, Nov 02, 2018 at 01:39:22PM -0700, José Roberto de Souza wrote:
> MST ports did not had the post_pll_disable() hook causing the
> references get in pre_pll_enable() never being released causing
> DDI and AUX CH being enabled all the times.
> 
> Cc: Imre Deak 
> Cc: Manasi Navare 
> Signed-off-by: José Roberto de Souza 

Patches 1 and 2 are 
Reviewed-by: Ville Syrjälä 

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8b71d64ebd9d..8e3d0b04580b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -215,6 +215,20 @@ static void intel_mst_pre_pll_enable_dp(struct 
> intel_encoder *encoder,
>   pipe_config, NULL);
>  }
>  
> +static void intel_mst_post_pll_disable_dp(struct intel_encoder *encoder,
> +   const struct intel_crtc_state 
> *pipe_config,
> +   const struct drm_connector_state 
> *conn_state)
> +{
> + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> + struct intel_digital_port *intel_dig_port = intel_mst->primary;
> + struct intel_dp *intel_dp = &intel_dig_port->dp;
> +
> + if (intel_dp->active_mst_links == 0 &&
> + intel_dig_port->base.post_pll_disable)
> + intel_dig_port->base.post_pll_disable(&intel_dig_port->base,
> +   pipe_config, NULL);
> +}
> +
>  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>   const struct intel_crtc_state *pipe_config,
>   const struct drm_connector_state 
> *conn_state)
> @@ -549,6 +563,7 @@ intel_dp_create_fake_mst_encoder(struct 
> intel_digital_port *intel_dig_port, enum
>   intel_encoder->disable = intel_mst_disable_dp;
>   intel_encoder->post_disable = intel_mst_post_disable_dp;
>   intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
> + intel_encoder->post_pll_disable = intel_mst_post_pll_disable_dp;
>   intel_encoder->pre_enable = intel_mst_pre_enable_dp;
>   intel_encoder->enable = intel_mst_enable_dp;
>   intel_encoder->get_hw_state = intel_dp_mst_enc_get_hw_state;
> -- 
> 2.19.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Intel-gfx] [PATCH 2/4] drm/i915: Release DDI power well references in MST ports

[José Roberto de Souza]

MST ports did not had the post_pll_disable() hook causing the
references get in pre_pll_enable() never being released causing
DDI and AUX CH being enabled all the times.

Cc: Imre Deak 
Cc: Manasi Navare 
Signed-off-by: José Roberto de Souza 
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8b71d64ebd9d..8e3d0b04580b 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -215,6 +215,20 @@ static void intel_mst_pre_pll_enable_dp(struct 
intel_encoder *encoder,
pipe_config, NULL);
 }
 
+static void intel_mst_post_pll_disable_dp(struct intel_encoder *encoder,
+ const struct intel_crtc_state 
*pipe_config,
+ const struct drm_connector_state 
*conn_state)
+{
+   struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
+   struct intel_digital_port *intel_dig_port = intel_mst->primary;
+   struct intel_dp *intel_dp = &intel_dig_port->dp;
+
+   if (intel_dp->active_mst_links == 0 &&
+   intel_dig_port->base.post_pll_disable)
+   intel_dig_port->base.post_pll_disable(&intel_dig_port->base,
+ pipe_config, NULL);
+}
+
 static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
const struct intel_crtc_state *pipe_config,
const struct drm_connector_state 
*conn_state)
@@ -549,6 +563,7 @@ intel_dp_create_fake_mst_encoder(struct intel_digital_port 
*intel_dig_port, enum
intel_encoder->disable = intel_mst_disable_dp;
intel_encoder->post_disable = intel_mst_post_disable_dp;
intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
+   intel_encoder->post_pll_disable = intel_mst_post_pll_disable_dp;
intel_encoder->pre_enable = intel_mst_pre_enable_dp;
intel_encoder->enable = intel_mst_enable_dp;
intel_encoder->get_hw_state = intel_dp_mst_enc_get_hw_state;
-- 
2.19.1

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