[Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[José Roberto de Souza]

Right now dp.regs.dp_tp_ctl/status are only set during the encoder
pre_enable() hook, what is causing all reads and writes to those
registers to go to offset 0x0 before pre_enable() is executed.

So if i915 takes the BIOS state and don't do a modeset any following
link retraing will fail.

In the case that i915 needs to do a modeset, the DDI disable sequence
will write to a wrong register not disabling DP 'Transport Enable' in
DP_TP_CTL, making a HDMI modeset in the same port/transcoder to
not light up the monitor.

So here for GENs older than 12, that have those registers fixed at
port offset range it is loading at encoder/port init while for GEN12
it will keep setting it at encoder pre_enable() and during HW state
readout.

Fixes: 4444df6e205b ("drm/i915/tgl: move DP_TP_* to transcoder")
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 14 +++++++++++---
 drivers/gpu/drm/i915/display/intel_dp.c  |  5 ++---
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index be6c61bcbc9c..1aab93a94f40 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3252,9 +3252,6 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
 				 crtc_state->lane_count, is_mst);
 
-	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
-	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
-
 	intel_edp_panel_on(intel_dp);
 
 	intel_ddi_clk_select(encoder, crtc_state);
@@ -4061,12 +4058,18 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->uapi.crtc);
 	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	u32 temp, flags = 0;
 
 	/* XXX: DSI transcoder paranoia */
 	if (drm_WARN_ON(&dev_priv->drm, transcoder_is_dsi(cpu_transcoder)))
 		return;
 
+	if (INTEL_GEN(dev_priv) >= 12) {
+		intel_dp->regs.dp_tp_ctl = TGL_DP_TP_CTL(cpu_transcoder);
+		intel_dp->regs.dp_tp_status = TGL_DP_TP_STATUS(cpu_transcoder);
+	}
+
 	intel_dsc_get_config(encoder, pipe_config);
 
 	temp = intel_de_read(dev_priv, TRANS_DDI_FUNC_CTL(cpu_transcoder));
@@ -4396,6 +4399,7 @@ static const struct drm_encoder_funcs intel_ddi_funcs = {
 static struct intel_connector *
 intel_ddi_init_dp_connector(struct intel_digital_port *intel_dig_port)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
 	struct intel_connector *connector;
 	enum port port = intel_dig_port->base.port;
 
@@ -4406,6 +4410,10 @@ intel_ddi_init_dp_connector(struct intel_digital_port *intel_dig_port)
 	intel_dig_port->dp.output_reg = DDI_BUF_CTL(port);
 	intel_dig_port->dp.prepare_link_retrain =
 		intel_ddi_prepare_link_retrain;
+	if (INTEL_GEN(dev_priv) < 12) {
+		intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
+		intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
+	}
 
 	if (!intel_dp_init_connector(intel_dig_port, connector)) {
 		kfree(connector);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index d4fcc9583869..03591ab76b0d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2671,9 +2671,6 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
 				 intel_crtc_has_type(pipe_config,
 						     INTEL_OUTPUT_DP_MST));
 
-	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
-	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
-
 	/*
 	 * There are four kinds of DP registers:
 	 *
@@ -8470,6 +8467,8 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
 
 	intel_dig_port->dp.output_reg = output_reg;
 	intel_dig_port->max_lanes = 4;
+	intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
+	intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
 
 	intel_encoder->type = INTEL_OUTPUT_DP;
 	intel_encoder->power_domain = intel_port_to_power_domain(port);
-- 
2.26.0

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

[Intel-gfx] [PATCH 2/3] drm/i915/hdmi: Get digital_port only once in intel_ddi_pre_enable_hdmi()

[José Roberto de Souza]

Getting it only once also removing intel_hdmi that is used only once
and can be easily accessed by dig_port->hdmi.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 1aab93a94f40..de5cb25053e3 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3328,13 +3328,11 @@ static void intel_ddi_pre_enable_hdmi(struct intel_atomic_state *state,
 				      const struct intel_crtc_state *crtc_state,
 				      const struct drm_connector_state *conn_state)
 {
-	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
-	struct intel_hdmi *intel_hdmi = &intel_dig_port->hdmi;
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	int level = intel_ddi_hdmi_level(encoder);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	int level = intel_ddi_hdmi_level(encoder);
 
-	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
+	intel_dp_dual_mode_set_tmds_output(&dig_port->hdmi, true);
 	intel_ddi_clk_select(encoder, crtc_state);
 
 	intel_display_power_get(dev_priv, dig_port->ddi_io_power_domain);
@@ -3359,9 +3357,8 @@ static void intel_ddi_pre_enable_hdmi(struct intel_atomic_state *state,
 
 	intel_ddi_enable_pipe_clock(crtc_state);
 
-	intel_dig_port->set_infoframes(encoder,
-				       crtc_state->has_infoframe,
-				       crtc_state, conn_state);
+	dig_port->set_infoframes(encoder, crtc_state->has_infoframe, crtc_state,
+				 conn_state);
 }
 
 static void intel_ddi_pre_enable(struct intel_atomic_state *state,
-- 
2.26.0

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

[Intel-gfx] [PATCH 3/3] drm/i915/hdmi: Add missing sequence

[José Roberto de Souza]

It was missing the step 7.b - "If not type-C static connection,
configure PORT_CL_DW10 Static Power Down to power up all lanes of the
DDI".

BSpec: 53339
BSpec: 49191
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index de5cb25053e3..94fa37d22e2c 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3330,6 +3330,7 @@ static void intel_ddi_pre_enable_hdmi(struct intel_atomic_state *state,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
 	int level = intel_ddi_hdmi_level(encoder);
 
 	intel_dp_dual_mode_set_tmds_output(&dig_port->hdmi, true);
@@ -3357,6 +3358,15 @@ static void intel_ddi_pre_enable_hdmi(struct intel_atomic_state *state,
 
 	intel_ddi_enable_pipe_clock(crtc_state);
 
+	if (intel_phy_is_combo(dev_priv, phy)) {
+		bool lane_reversal =
+			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
+
+		intel_combo_phy_power_up_lanes(dev_priv, phy, false,
+					       crtc_state->lane_count,
+					       lane_reversal);
+	}
+
 	dig_port->set_infoframes(encoder, crtc_state->has_infoframe, crtc_state,
 				 conn_state);
 }
-- 
2.26.0

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

[Intel-gfx] ✗ Fi.CI.DOCS: warning for series starting with [1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it
URL   : https://patchwork.freedesktop.org/series/75946/
State : warning

== Summary ==

$ make htmldocs 2>&1 > /dev/null | grep i915
/home/cidrm/kernel/Documentation/gpu/i915.rst:610: WARNING: duplicate label gpu/i915:layout, other instance in /home/cidrm/kernel/Documentation/gpu/i915.rst

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

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it
URL   : https://patchwork.freedesktop.org/series/75946/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8298 -> Patchwork_17303
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/index.html

Known issues
------------

  Here are the changes found in Patchwork_17303 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@uncore:
    - fi-bwr-2160:        [PASS][1] -> [INCOMPLETE][2] ([i915#489])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/fi-bwr-2160/igt@i915_selftest@live@uncore.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/fi-bwr-2160/igt@i915_selftest@live@uncore.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-cml-u2:          [PASS][3] -> [DMESG-WARN][4] ([IGT#4])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/fi-cml-u2/igt@kms_chamelium@common-hpd-after-suspend.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/fi-cml-u2/igt@kms_chamelium@common-hpd-after-suspend.html

  
  [IGT#4]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/4
  [i915#489]: https://gitlab.freedesktop.org/drm/intel/issues/489


Participating hosts (48 -> 43)
------------------------------

  Missing    (5): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-kbl-7560u fi-byt-clapper 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8298 -> Patchwork_17303

  CI-20190529: 20190529
  CI_DRM_8298: 17f82f0c2857d0b442adbdb62eb44b61d0f5b775 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5589: 31962324ac86f029e2841e56e97c42cf9d572956 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17303: 56321c219613c62da1a54b30119b050c3d658428 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

56321c219613 drm/i915/hdmi: Add missing sequence
67fcd4a8fc25 drm/i915/hdmi: Get digital_port only once in intel_ddi_pre_enable_hdmi()
aa6379aaebea drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Lucas De Marchi]

On Tue, Apr 14, 2020 at 4:03 PM José Roberto de Souza
<jose.souza@intel.com> wrote:
>
> Right now dp.regs.dp_tp_ctl/status are only set during the encoder
> pre_enable() hook, what is causing all reads and writes to those
> registers to go to offset 0x0 before pre_enable() is executed.
>
> So if i915 takes the BIOS state and don't do a modeset any following
> link retraing will fail.
>
> In the case that i915 needs to do a modeset, the DDI disable sequence
> will write to a wrong register not disabling DP 'Transport Enable' in
> DP_TP_CTL, making a HDMI modeset in the same port/transcoder to
> not light up the monitor.
>
> So here for GENs older than 12, that have those registers fixed at
> port offset range it is loading at encoder/port init while for GEN12
> it will keep setting it at encoder pre_enable() and during HW state
> readout.
>
> Fixes: 4444df6e205b ("drm/i915/tgl: move DP_TP_* to transcoder")
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 14 +++++++++++---
>  drivers/gpu/drm/i915/display/intel_dp.c  |  5 ++---
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index be6c61bcbc9c..1aab93a94f40 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3252,9 +3252,6 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>         intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
>                                  crtc_state->lane_count, is_mst);
>
> -       intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> -       intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);

reason to be where it was is because of MST. I think what you are
doing here will break it since now this is set for the port and not
transcoder.
intel_mst_pre_enable_dp() would call here only for the first stream,
so all the others would use this same transcoder.

Lucas De Marchi

> -
>         intel_edp_panel_on(intel_dp);
>
>         intel_ddi_clk_select(encoder, crtc_state);
> @@ -4061,12 +4058,18 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>         struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>         struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->uapi.crtc);
>         enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> +       struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>         u32 temp, flags = 0;
>
>         /* XXX: DSI transcoder paranoia */
>         if (drm_WARN_ON(&dev_priv->drm, transcoder_is_dsi(cpu_transcoder)))
>                 return;
>
> +       if (INTEL_GEN(dev_priv) >= 12) {
> +               intel_dp->regs.dp_tp_ctl = TGL_DP_TP_CTL(cpu_transcoder);
> +               intel_dp->regs.dp_tp_status = TGL_DP_TP_STATUS(cpu_transcoder);
> +       }
> +
>         intel_dsc_get_config(encoder, pipe_config);
>
>         temp = intel_de_read(dev_priv, TRANS_DDI_FUNC_CTL(cpu_transcoder));
> @@ -4396,6 +4399,7 @@ static const struct drm_encoder_funcs intel_ddi_funcs = {
>  static struct intel_connector *
>  intel_ddi_init_dp_connector(struct intel_digital_port *intel_dig_port)
>  {
> +       struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
>         struct intel_connector *connector;
>         enum port port = intel_dig_port->base.port;
>
> @@ -4406,6 +4410,10 @@ intel_ddi_init_dp_connector(struct intel_digital_port *intel_dig_port)
>         intel_dig_port->dp.output_reg = DDI_BUF_CTL(port);
>         intel_dig_port->dp.prepare_link_retrain =
>                 intel_ddi_prepare_link_retrain;
> +       if (INTEL_GEN(dev_priv) < 12) {
> +               intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> +               intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
> +       }
>
>         if (!intel_dp_init_connector(intel_dig_port, connector)) {
>                 kfree(connector);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index d4fcc9583869..03591ab76b0d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2671,9 +2671,6 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
>                                  intel_crtc_has_type(pipe_config,
>                                                      INTEL_OUTPUT_DP_MST));
>
> -       intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> -       intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> -
>         /*
>          * There are four kinds of DP registers:
>          *
> @@ -8470,6 +8467,8 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
>
>         intel_dig_port->dp.output_reg = output_reg;
>         intel_dig_port->max_lanes = 4;
> +       intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> +       intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
>
>         intel_encoder->type = INTEL_OUTPUT_DP;
>         intel_encoder->power_domain = intel_port_to_power_domain(port);
> --
> 2.26.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Souza, Jose]

On Tue, 2020-04-14 at 22:33 -0700, Lucas De Marchi wrote:
> On Tue, Apr 14, 2020 at 4:03 PM José Roberto de Souza
> <jose.souza@intel.com> wrote:
> > Right now dp.regs.dp_tp_ctl/status are only set during the encoder
> > pre_enable() hook, what is causing all reads and writes to those
> > registers to go to offset 0x0 before pre_enable() is executed.
> > 
> > So if i915 takes the BIOS state and don't do a modeset any
> > following
> > link retraing will fail.
> > 
> > In the case that i915 needs to do a modeset, the DDI disable
> > sequence
> > will write to a wrong register not disabling DP 'Transport Enable'
> > in
> > DP_TP_CTL, making a HDMI modeset in the same port/transcoder to
> > not light up the monitor.
> > 
> > So here for GENs older than 12, that have those registers fixed at
> > port offset range it is loading at encoder/port init while for
> > GEN12
> > it will keep setting it at encoder pre_enable() and during HW state
> > readout.
> > 
> > Fixes: 4444df6e205b ("drm/i915/tgl: move DP_TP_* to transcoder")
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 14 +++++++++++---
> >  drivers/gpu/drm/i915/display/intel_dp.c  |  5 ++---
> >  2 files changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index be6c61bcbc9c..1aab93a94f40 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3252,9 +3252,6 @@ static void hsw_ddi_pre_enable_dp(struct
> > intel_atomic_state *state,
> >         intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> >                                  crtc_state->lane_count, is_mst);
> > 
> > -       intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > -       intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> 
> reason to be where it was is because of MST. I think what you are
> doing here will break it since now this is set for the port and not
> transcoder.
> intel_mst_pre_enable_dp() would call here only for the first stream,
> so all the others would use this same transcoder.

For TGL+ it moved to transcoder but for other it is still on port and
it is kept in this patch. The fix here for TGL+ is load those 2 during
HW state readout.
Inside MST code it will continue to get from
intel_mst->primary.dp.

> 
> Lucas De Marchi
> 
> > -
> >         intel_edp_panel_on(intel_dp);
> > 
> >         intel_ddi_clk_select(encoder, crtc_state);
> > @@ -4061,12 +4058,18 @@ void intel_ddi_get_config(struct
> > intel_encoder *encoder,
> >         struct drm_i915_private *dev_priv = to_i915(encoder-
> > >base.dev);
> >         struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config-
> > >uapi.crtc);
> >         enum transcoder cpu_transcoder = pipe_config-
> > >cpu_transcoder;
> > +       struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >         u32 temp, flags = 0;
> > 
> >         /* XXX: DSI transcoder paranoia */
> >         if (drm_WARN_ON(&dev_priv->drm,
> > transcoder_is_dsi(cpu_transcoder)))
> >                 return;
> > 
> > +       if (INTEL_GEN(dev_priv) >= 12) {
> > +               intel_dp->regs.dp_tp_ctl =
> > TGL_DP_TP_CTL(cpu_transcoder);
> > +               intel_dp->regs.dp_tp_status =
> > TGL_DP_TP_STATUS(cpu_transcoder);
> > +       }
> > +
> >         intel_dsc_get_config(encoder, pipe_config);
> > 
> >         temp = intel_de_read(dev_priv,
> > TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > @@ -4396,6 +4399,7 @@ static const struct drm_encoder_funcs
> > intel_ddi_funcs = {
> >  static struct intel_connector *
> >  intel_ddi_init_dp_connector(struct intel_digital_port
> > *intel_dig_port)
> >  {
> > +       struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> > >base.base.dev);
> >         struct intel_connector *connector;
> >         enum port port = intel_dig_port->base.port;
> > 
> > @@ -4406,6 +4410,10 @@ intel_ddi_init_dp_connector(struct
> > intel_digital_port *intel_dig_port)
> >         intel_dig_port->dp.output_reg = DDI_BUF_CTL(port);
> >         intel_dig_port->dp.prepare_link_retrain =
> >                 intel_ddi_prepare_link_retrain;
> > +       if (INTEL_GEN(dev_priv) < 12) {
> > +               intel_dig_port->dp.regs.dp_tp_ctl =
> > DP_TP_CTL(port);
> > +               intel_dig_port->dp.regs.dp_tp_status =
> > DP_TP_STATUS(port);
> > +       }
> > 
> >         if (!intel_dp_init_connector(intel_dig_port, connector)) {
> >                 kfree(connector);
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index d4fcc9583869..03591ab76b0d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2671,9 +2671,6 @@ static void intel_dp_prepare(struct
> > intel_encoder *encoder,
> >                                  intel_crtc_has_type(pipe_config,
> >                                                      INTEL_OUTPUT_D
> > P_MST));
> > 
> > -       intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > -       intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > -
> >         /*
> >          * There are four kinds of DP registers:
> >          *
> > @@ -8470,6 +8467,8 @@ bool intel_dp_init(struct drm_i915_private
> > *dev_priv,
> > 
> >         intel_dig_port->dp.output_reg = output_reg;
> >         intel_dig_port->max_lanes = 4;
> > +       intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> > +       intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
> > 
> >         intel_encoder->type = INTEL_OUTPUT_DP;
> >         intel_encoder->power_domain =
> > intel_port_to_power_domain(port);
> > --
> > 2.26.0
> > 
> > _______________________________________________
> > 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

[Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it
URL   : https://patchwork.freedesktop.org/series/75946/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8298_full -> Patchwork_17303_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

New tests
---------

  New tests have been introduced between CI_DRM_8298_full and Patchwork_17303_full:

### New IGT tests (27) ###

  * igt@kms_plane_cursor@pipe-a-overlay-size-128:
    - Statuses : 8 pass(s)
    - Exec time: [1.66, 3.65] s

  * igt@kms_plane_cursor@pipe-a-overlay-size-256:
    - Statuses : 8 pass(s)
    - Exec time: [1.65, 3.69] s

  * igt@kms_plane_cursor@pipe-a-overlay-size-64:
    - Statuses : 8 pass(s)
    - Exec time: [1.65, 3.68] s

  * igt@kms_plane_cursor@pipe-a-primary-size-128:
    - Statuses : 8 pass(s)
    - Exec time: [1.63, 3.38] s

  * igt@kms_plane_cursor@pipe-a-primary-size-256:
    - Statuses : 8 pass(s)
    - Exec time: [1.63, 3.45] s

  * igt@kms_plane_cursor@pipe-a-primary-size-64:
    - Statuses : 8 pass(s)
    - Exec time: [1.63, 3.41] s

  * igt@kms_plane_cursor@pipe-a-viewport-size-128:
    - Statuses : 8 pass(s)
    - Exec time: [1.65, 3.67] s

  * igt@kms_plane_cursor@pipe-a-viewport-size-256:
    - Statuses : 8 pass(s)
    - Exec time: [1.64, 3.64] s

  * igt@kms_plane_cursor@pipe-a-viewport-size-64:
    - Statuses : 8 pass(s)
    - Exec time: [1.65, 3.69] s

  * igt@kms_plane_cursor@pipe-b-overlay-size-128:
    - Statuses : 8 pass(s)
    - Exec time: [2.22, 4.84] s

  * igt@kms_plane_cursor@pipe-b-overlay-size-256:
    - Statuses : 8 pass(s)
    - Exec time: [2.23, 4.89] s

  * igt@kms_plane_cursor@pipe-b-overlay-size-64:
    - Statuses : 8 pass(s)
    - Exec time: [2.23, 4.85] s

  * igt@kms_plane_cursor@pipe-b-primary-size-128:
    - Statuses : 8 pass(s)
    - Exec time: [2.20, 4.62] s

  * igt@kms_plane_cursor@pipe-b-primary-size-256:
    - Statuses : 8 pass(s)
    - Exec time: [2.21, 4.61] s

  * igt@kms_plane_cursor@pipe-b-primary-size-64:
    - Statuses : 8 pass(s)
    - Exec time: [2.21, 4.70] s

  * igt@kms_plane_cursor@pipe-b-viewport-size-128:
    - Statuses :
    - Exec time: [None] s

  * igt@kms_plane_cursor@pipe-b-viewport-size-256:
    - Statuses : 8 pass(s)
    - Exec time: [2.23, 4.88] s

  * igt@kms_plane_cursor@pipe-b-viewport-size-64:
    - Statuses : 8 pass(s)
    - Exec time: [2.23, 4.82] s

  * igt@kms_plane_cursor@pipe-c-overlay-size-128:
    - Statuses : 6 pass(s) 1 skip(s)
    - Exec time: [0.0, 4.83] s

  * igt@kms_plane_cursor@pipe-c-overlay-size-256:
    - Statuses : 7 pass(s) 1 skip(s)
    - Exec time: [0.0, 5.04] s

  * igt@kms_plane_cursor@pipe-c-overlay-size-64:
    - Statuses : 6 pass(s) 1 skip(s)
    - Exec time: [0.0, 3.60] s

  * igt@kms_plane_cursor@pipe-c-primary-size-128:
    - Statuses : 7 pass(s) 1 skip(s)
    - Exec time: [0.0, 4.57] s

  * igt@kms_plane_cursor@pipe-c-primary-size-256:
    - Statuses : 7 pass(s) 1 skip(s)
    - Exec time: [0.0, 4.63] s

  * igt@kms_plane_cursor@pipe-c-primary-size-64:
    - Statuses : 7 pass(s) 1 skip(s)
    - Exec time: [0.0, 4.60] s

  * igt@kms_plane_cursor@pipe-c-viewport-size-128:
    - Statuses : 7 pass(s) 1 skip(s)
    - Exec time: [0.0, 4.83] s

  * igt@kms_plane_cursor@pipe-c-viewport-size-256:
    - Statuses : 7 pass(s) 1 skip(s)
    - Exec time: [0.0, 4.82] s

  * igt@kms_plane_cursor@pipe-c-viewport-size-64:
    - Statuses : 7 pass(s) 1 skip(s)
    - Exec time: [0.0, 4.88] s

  

Known issues
------------

  Here are the changes found in Patchwork_17303_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          [PASS][1] -> [DMESG-WARN][2] ([i915#180])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-apl4/igt@gem_softpin@noreloc-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-apl4/igt@gem_softpin@noreloc-s3.html

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-snb:          [PASS][3] -> [FAIL][4] ([i915#1066])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-snb2/igt@i915_pm_rc6_residency@rc6-idle.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-snb5/igt@i915_pm_rc6_residency@rc6-idle.html
    - shard-hsw:          [PASS][5] -> [FAIL][6] ([i915#1066])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-hsw6/igt@i915_pm_rc6_residency@rc6-idle.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-hsw4/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [PASS][7] -> [DMESG-WARN][8] ([i915#180]) +5 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-kbl3/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_cursor_legacy@pipe-a-torture-bo:
    - shard-hsw:          [PASS][9] -> [INCOMPLETE][10] ([i915#61])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-hsw1/igt@kms_cursor_legacy@pipe-a-torture-bo.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-hsw4/igt@kms_cursor_legacy@pipe-a-torture-bo.html

  * igt@kms_flip@flip-vs-wf_vblank-interruptible:
    - shard-glk:          [PASS][11] -> [FAIL][12] ([i915#34])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-glk2/igt@kms_flip@flip-vs-wf_vblank-interruptible.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-glk5/igt@kms_flip@flip-vs-wf_vblank-interruptible.html

  * igt@kms_flip@plain-flip-fb-recreate:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([i915#34]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-skl8/igt@kms_flip@plain-flip-fb-recreate.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-skl3/igt@kms_flip@plain-flip-fb-recreate.html

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         [PASS][15] -> [SKIP][16] ([fdo#109441])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-iclb6/igt@kms_psr@psr2_primary_page_flip.html

  * igt@kms_setmode@basic:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([i915#31])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-skl7/igt@kms_setmode@basic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-skl10/igt@kms_setmode@basic.html

  
#### Possible fixes ####

  * {igt@gem_wait@write-wait@all}:
    - shard-skl:          [FAIL][19] ([i915#1676]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-skl2/igt@gem_wait@write-wait@all.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-skl10/igt@gem_wait@write-wait@all.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [FAIL][21] ([i915#79]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-skl8/igt@kms_flip@flip-vs-expired-vblank.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-skl3/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_hdr@bpc-switch-dpms:
    - shard-skl:          [FAIL][23] ([i915#1188]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-skl10/igt@kms_hdr@bpc-switch-dpms.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-skl6/igt@kms_hdr@bpc-switch-dpms.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-kbl:          [DMESG-WARN][25] ([i915#180]) -> [PASS][26] +2 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-kbl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-kbl4/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
    - shard-skl:          [FAIL][27] ([fdo#108145] / [i915#265]) -> [PASS][28] +1 similar issue
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html

  * igt@kms_psr@psr2_sprite_plane_move:
    - shard-iclb:         [SKIP][29] ([fdo#109441]) -> [PASS][30] +2 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-iclb5/igt@kms_psr@psr2_sprite_plane_move.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html

  * igt@kms_vblank@pipe-b-ts-continuation-suspend:
    - shard-apl:          [DMESG-WARN][31] ([i915#180]) -> [PASS][32] +3 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-apl4/igt@kms_vblank@pipe-b-ts-continuation-suspend.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-apl2/igt@kms_vblank@pipe-b-ts-continuation-suspend.html

  * {igt@perf@polling-parameterized}:
    - shard-hsw:          [FAIL][33] ([i915#1542]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-hsw6/igt@perf@polling-parameterized.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-hsw7/igt@perf@polling-parameterized.html

  
#### Warnings ####

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-kbl:          [DMESG-FAIL][35] ([i915#180] / [i915#95]) -> [FAIL][36] ([i915#93] / [i915#95])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-kbl6/igt@kms_fbcon_fbt@fbc-suspend.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-kbl7/igt@kms_fbcon_fbt@fbc-suspend.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-basic:
    - shard-apl:          [FAIL][37] ([fdo#108145] / [i915#265]) -> [FAIL][38] ([fdo#108145] / [i915#265] / [i915#95])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-apl1/igt@kms_plane_alpha_blend@pipe-a-alpha-basic.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-apl3/igt@kms_plane_alpha_blend@pipe-a-alpha-basic.html

  * igt@kms_plane_alpha_blend@pipe-c-alpha-7efc:
    - shard-kbl:          [FAIL][39] ([fdo#108145] / [i915#265] / [i915#93] / [i915#95]) -> [FAIL][40] ([fdo#108145] / [i915#265])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-kbl2/igt@kms_plane_alpha_blend@pipe-c-alpha-7efc.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-kbl4/igt@kms_plane_alpha_blend@pipe-c-alpha-7efc.html
    - shard-apl:          [FAIL][41] ([fdo#108145] / [i915#265] / [i915#95]) -> [FAIL][42] ([fdo#108145] / [i915#265])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/shard-apl4/igt@kms_plane_alpha_blend@pipe-c-alpha-7efc.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/shard-apl8/igt@kms_plane_alpha_blend@pipe-c-alpha-7efc.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [i915#1066]: https://gitlab.freedesktop.org/drm/intel/issues/1066
  [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
  [i915#1459]: https://gitlab.freedesktop.org/drm/intel/issues/1459
  [i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
  [i915#1676]: https://gitlab.freedesktop.org/drm/intel/issues/1676
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [i915#61]: https://gitlab.freedesktop.org/drm/intel/issues/61
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#93]: https://gitlab.freedesktop.org/drm/intel/issues/93
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8298 -> Patchwork_17303

  CI-20190529: 20190529
  CI_DRM_8298: 17f82f0c2857d0b442adbdb62eb44b61d0f5b775 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5589: 31962324ac86f029e2841e56e97c42cf9d572956 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17303: 56321c219613c62da1a54b30119b050c3d658428 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17303/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Matt Roper]

On Tue, Apr 14, 2020 at 04:04:40PM -0700, José Roberto de Souza wrote:
> Right now dp.regs.dp_tp_ctl/status are only set during the encoder
> pre_enable() hook, what is causing all reads and writes to those
> registers to go to offset 0x0 before pre_enable() is executed.
> 
> So if i915 takes the BIOS state and don't do a modeset any following
> link retraing will fail.
> 
> In the case that i915 needs to do a modeset, the DDI disable sequence
> will write to a wrong register not disabling DP 'Transport Enable' in
> DP_TP_CTL, making a HDMI modeset in the same port/transcoder to
> not light up the monitor.

So to clarify I understand the problematic sequence properly:
 * i915 inherits already-enabled display from BIOS; pre_enable is never
   called
 * we do a modeset, so we have to disable the display and then try to re-enable
     - intel_disable_ddi_buf() writes to offset 0 rather than the proper
       register offset when attempting to reprogram DP_TP_CTL and
       disable DP
     - when we re-enable, the old, still-active DP settings in hardware
       cause problems and the display doesn't light up

A couple clarifying questions:
 - It seems like we should have seen unclaimed register warnings from
   the bogus writes to offset 0 during disable.  Any idea why those
   didn't show up?
 - In the commit message you mention that it's the DP Transport Enable
   that's the culprit here, which breaks future attempts to light up
   HDMI.   I assume this means that we're also switching which pipe
   we're driving the port with, not just doing any modeset?  Otherwise
   DP would stay DP, HDMI would stay HDMI, and we wouldn't see this
   problem with DP being active on an HDMI monitor (although we'd still
   be writing to an invalid register offset during our first DP disable
   which might cause other problems).  Might be worth adding a mention
   of the pipe change to the commit message to clarify.

The changes here look correct to me; we'll ensure the DP registers have
proper offsets before we do our first modeset, and then the rest of the
runtime behavior thereafter should be unchanged.  So

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


> 
> So here for GENs older than 12, that have those registers fixed at
> port offset range it is loading at encoder/port init while for GEN12
> it will keep setting it at encoder pre_enable() and during HW state
> readout.
> 
> Fixes: 4444df6e205b ("drm/i915/tgl: move DP_TP_* to transcoder")
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 14 +++++++++++---
>  drivers/gpu/drm/i915/display/intel_dp.c  |  5 ++---
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index be6c61bcbc9c..1aab93a94f40 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3252,9 +3252,6 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
>  				 crtc_state->lane_count, is_mst);
>  
> -	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> -	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> -
>  	intel_edp_panel_on(intel_dp);
>  
>  	intel_ddi_clk_select(encoder, crtc_state);
> @@ -4061,12 +4058,18 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->uapi.crtc);
>  	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	u32 temp, flags = 0;
>  
>  	/* XXX: DSI transcoder paranoia */
>  	if (drm_WARN_ON(&dev_priv->drm, transcoder_is_dsi(cpu_transcoder)))
>  		return;
>  
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		intel_dp->regs.dp_tp_ctl = TGL_DP_TP_CTL(cpu_transcoder);
> +		intel_dp->regs.dp_tp_status = TGL_DP_TP_STATUS(cpu_transcoder);
> +	}
> +
>  	intel_dsc_get_config(encoder, pipe_config);
>  
>  	temp = intel_de_read(dev_priv, TRANS_DDI_FUNC_CTL(cpu_transcoder));
> @@ -4396,6 +4399,7 @@ static const struct drm_encoder_funcs intel_ddi_funcs = {
>  static struct intel_connector *
>  intel_ddi_init_dp_connector(struct intel_digital_port *intel_dig_port)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
>  	struct intel_connector *connector;
>  	enum port port = intel_dig_port->base.port;
>  
> @@ -4406,6 +4410,10 @@ intel_ddi_init_dp_connector(struct intel_digital_port *intel_dig_port)
>  	intel_dig_port->dp.output_reg = DDI_BUF_CTL(port);
>  	intel_dig_port->dp.prepare_link_retrain =
>  		intel_ddi_prepare_link_retrain;
> +	if (INTEL_GEN(dev_priv) < 12) {
> +		intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> +		intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
> +	}
>  
>  	if (!intel_dp_init_connector(intel_dig_port, connector)) {
>  		kfree(connector);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index d4fcc9583869..03591ab76b0d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2671,9 +2671,6 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
>  				 intel_crtc_has_type(pipe_config,
>  						     INTEL_OUTPUT_DP_MST));
>  
> -	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> -	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> -
>  	/*
>  	 * There are four kinds of DP registers:
>  	 *
> @@ -8470,6 +8467,8 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
>  
>  	intel_dig_port->dp.output_reg = output_reg;
>  	intel_dig_port->max_lanes = 4;
> +	intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> +	intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
>  
>  	intel_encoder->type = INTEL_OUTPUT_DP;
>  	intel_encoder->power_domain = intel_port_to_power_domain(port);
> -- 
> 2.26.0
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Souza, Jose]

On Wed, 2020-04-15 at 20:56 -0700, Matt Roper wrote:
> On Tue, Apr 14, 2020 at 04:04:40PM -0700, José Roberto de Souza
> wrote:
> > Right now dp.regs.dp_tp_ctl/status are only set during the encoder
> > pre_enable() hook, what is causing all reads and writes to those
> > registers to go to offset 0x0 before pre_enable() is executed.
> > 
> > So if i915 takes the BIOS state and don't do a modeset any
> > following
> > link retraing will fail.
> > 
> > In the case that i915 needs to do a modeset, the DDI disable
> > sequence
> > will write to a wrong register not disabling DP 'Transport Enable'
> > in
> > DP_TP_CTL, making a HDMI modeset in the same port/transcoder to
> > not light up the monitor.
> 
> So to clarify I understand the problematic sequence properly:
>  * i915 inherits already-enabled display from BIOS; pre_enable is
> never
>    called
>  * we do a modeset, so we have to disable the display and then try to
> re-enable
>      - intel_disable_ddi_buf() writes to offset 0 rather than the
> proper
>        register offset when attempting to reprogram DP_TP_CTL and
>        disable DP
>      - when we re-enable, the old, still-active DP settings in
> hardware
>        cause problems and the display doesn't light up
> 
> A couple clarifying questions:
>  - It seems like we should have seen unclaimed register warnings from
>    the bogus writes to offset 0 during disable.  Any idea why those
>    didn't show up?

Offset 0x0 is valid BSpec: 29316

>  - In the commit message you mention that it's the DP Transport
> Enable
>    that's the culprit here, which breaks future attempts to light up
>    HDMI.   I assume this means that we're also switching which pipe
>    we're driving the port with, not just doing any
> modeset?  Otherwise
>    DP would stay DP, HDMI would stay HDMI, and we wouldn't see this
>    problem with DP being active on an HDMI monitor (although we'd
> still
>    be writing to an invalid register offset during our first DP
> disable
>    which might cause other problems).  Might be worth adding a
> mention
>    of the pipe change to the commit message to clarify.

Got this state in a system were BIOS is lighing up DP in pipe B when
HDMI is also connected and leaving pipe A off, no idea why BIOS decided
this.
But when i915 takes over we do the modeset to light up HDMI and
reorder the pipes.

> 
> The changes here look correct to me; we'll ensure the DP registers
> have
> proper offsets before we do our first modeset, and then the rest of
> the
> runtime behavior thereafter should be unchanged.  So
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> > So here for GENs older than 12, that have those registers fixed at
> > port offset range it is loading at encoder/port init while for
> > GEN12
> > it will keep setting it at encoder pre_enable() and during HW state
> > readout.
> > 
> > Fixes: 4444df6e205b ("drm/i915/tgl: move DP_TP_* to transcoder")
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 14 +++++++++++---
> >  drivers/gpu/drm/i915/display/intel_dp.c  |  5 ++---
> >  2 files changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index be6c61bcbc9c..1aab93a94f40 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3252,9 +3252,6 @@ static void hsw_ddi_pre_enable_dp(struct
> > intel_atomic_state *state,
> >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> >  				 crtc_state->lane_count, is_mst);
> >  
> > -	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > -	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > -
> >  	intel_edp_panel_on(intel_dp);
> >  
> >  	intel_ddi_clk_select(encoder, crtc_state);
> > @@ -4061,12 +4058,18 @@ void intel_ddi_get_config(struct
> > intel_encoder *encoder,
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config-
> > >uapi.crtc);
> >  	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> > +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  	u32 temp, flags = 0;
> >  
> >  	/* XXX: DSI transcoder paranoia */
> >  	if (drm_WARN_ON(&dev_priv->drm,
> > transcoder_is_dsi(cpu_transcoder)))
> >  		return;
> >  
> > +	if (INTEL_GEN(dev_priv) >= 12) {
> > +		intel_dp->regs.dp_tp_ctl =
> > TGL_DP_TP_CTL(cpu_transcoder);
> > +		intel_dp->regs.dp_tp_status =
> > TGL_DP_TP_STATUS(cpu_transcoder);
> > +	}
> > +
> >  	intel_dsc_get_config(encoder, pipe_config);
> >  
> >  	temp = intel_de_read(dev_priv,
> > TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > @@ -4396,6 +4399,7 @@ static const struct drm_encoder_funcs
> > intel_ddi_funcs = {
> >  static struct intel_connector *
> >  intel_ddi_init_dp_connector(struct intel_digital_port
> > *intel_dig_port)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> > >base.base.dev);
> >  	struct intel_connector *connector;
> >  	enum port port = intel_dig_port->base.port;
> >  
> > @@ -4406,6 +4410,10 @@ intel_ddi_init_dp_connector(struct
> > intel_digital_port *intel_dig_port)
> >  	intel_dig_port->dp.output_reg = DDI_BUF_CTL(port);
> >  	intel_dig_port->dp.prepare_link_retrain =
> >  		intel_ddi_prepare_link_retrain;
> > +	if (INTEL_GEN(dev_priv) < 12) {
> > +		intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> > +		intel_dig_port->dp.regs.dp_tp_status =
> > DP_TP_STATUS(port);
> > +	}
> >  
> >  	if (!intel_dp_init_connector(intel_dig_port, connector)) {
> >  		kfree(connector);
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index d4fcc9583869..03591ab76b0d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2671,9 +2671,6 @@ static void intel_dp_prepare(struct
> > intel_encoder *encoder,
> >  				 intel_crtc_has_type(pipe_config,
> >  						     INTEL_OUTPUT_DP_MS
> > T));
> >  
> > -	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > -	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > -
> >  	/*
> >  	 * There are four kinds of DP registers:
> >  	 *
> > @@ -8470,6 +8467,8 @@ bool intel_dp_init(struct drm_i915_private
> > *dev_priv,
> >  
> >  	intel_dig_port->dp.output_reg = output_reg;
> >  	intel_dig_port->max_lanes = 4;
> > +	intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> > +	intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
> >  
> >  	intel_encoder->type = INTEL_OUTPUT_DP;
> >  	intel_encoder->power_domain = intel_port_to_power_domain(port);
> > -- 
> > 2.26.0
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Ville Syrjälä]

On Wed, Apr 15, 2020 at 04:53:19PM +0000, Souza, Jose wrote:
> On Tue, 2020-04-14 at 22:33 -0700, Lucas De Marchi wrote:
> > On Tue, Apr 14, 2020 at 4:03 PM José Roberto de Souza
> > <jose.souza@intel.com> wrote:
> > > Right now dp.regs.dp_tp_ctl/status are only set during the encoder
> > > pre_enable() hook, what is causing all reads and writes to those
> > > registers to go to offset 0x0 before pre_enable() is executed.
> > > 
> > > So if i915 takes the BIOS state and don't do a modeset any
> > > following
> > > link retraing will fail.
> > > 
> > > In the case that i915 needs to do a modeset, the DDI disable
> > > sequence
> > > will write to a wrong register not disabling DP 'Transport Enable'
> > > in
> > > DP_TP_CTL, making a HDMI modeset in the same port/transcoder to
> > > not light up the monitor.
> > > 
> > > So here for GENs older than 12, that have those registers fixed at
> > > port offset range it is loading at encoder/port init while for
> > > GEN12
> > > it will keep setting it at encoder pre_enable() and during HW state
> > > readout.
> > > 
> > > Fixes: 4444df6e205b ("drm/i915/tgl: move DP_TP_* to transcoder")
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 14 +++++++++++---
> > >  drivers/gpu/drm/i915/display/intel_dp.c  |  5 ++---
> > >  2 files changed, 13 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index be6c61bcbc9c..1aab93a94f40 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -3252,9 +3252,6 @@ static void hsw_ddi_pre_enable_dp(struct
> > > intel_atomic_state *state,
> > >         intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > >                                  crtc_state->lane_count, is_mst);
> > > 
> > > -       intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > > -       intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > 
> > reason to be where it was is because of MST. I think what you are
> > doing here will break it since now this is set for the port and not
> > transcoder.
> > intel_mst_pre_enable_dp() would call here only for the first stream,
> > so all the others would use this same transcoder.
> 
> For TGL+ it moved to transcoder but for other it is still on port and
> it is kept in this patch. The fix here for TGL+ is load those 2 during
> HW state readout.
> Inside MST code it will continue to get from
> intel_mst->primary.dp.

FYI looks like I have a reasonable way to get rid of this by finally 
plumbing the crtc_state all the way down into link training code
(has been on the TODO list for years). This should also allow us to
elminate the encoder->type mess in the ddi_buf_trans code. And we
get rid of some crtc->config stuff as well.

MST retraining was the main obstacle left. I think I mostly solved
it with the patch I just sent today. The one remaining issue I
recall is the lane_mask for drm_dp_channel_eq_ok(). So far I don't
have a better plan than to keep intel_dp->lane_count. But most of
the other ad-hoc state under intel_dp can hopefully be nuked.

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

Re: [Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Souza, Jose]

On Fri, 2020-04-17 at 21:01 +0300, Ville Syrjälä wrote:
> On Wed, Apr 15, 2020 at 04:53:19PM +0000, Souza, Jose wrote:
> > On Tue, 2020-04-14 at 22:33 -0700, Lucas De Marchi wrote:
> > > On Tue, Apr 14, 2020 at 4:03 PM José Roberto de Souza
> > > <jose.souza@intel.com> wrote:
> > > > Right now dp.regs.dp_tp_ctl/status are only set during the
> > > > encoder
> > > > pre_enable() hook, what is causing all reads and writes to
> > > > those
> > > > registers to go to offset 0x0 before pre_enable() is executed.
> > > > 
> > > > So if i915 takes the BIOS state and don't do a modeset any
> > > > following
> > > > link retraing will fail.
> > > > 
> > > > In the case that i915 needs to do a modeset, the DDI disable
> > > > sequence
> > > > will write to a wrong register not disabling DP 'Transport
> > > > Enable'
> > > > in
> > > > DP_TP_CTL, making a HDMI modeset in the same port/transcoder to
> > > > not light up the monitor.
> > > > 
> > > > So here for GENs older than 12, that have those registers fixed
> > > > at
> > > > port offset range it is loading at encoder/port init while for
> > > > GEN12
> > > > it will keep setting it at encoder pre_enable() and during HW
> > > > state
> > > > readout.
> > > > 
> > > > Fixes: 4444df6e205b ("drm/i915/tgl: move DP_TP_* to
> > > > transcoder")
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_ddi.c | 14 +++++++++++---
> > > >  drivers/gpu/drm/i915/display/intel_dp.c  |  5 ++---
> > > >  2 files changed, 13 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > index be6c61bcbc9c..1aab93a94f40 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > @@ -3252,9 +3252,6 @@ static void hsw_ddi_pre_enable_dp(struct
> > > > intel_atomic_state *state,
> > > >         intel_dp_set_link_params(intel_dp, crtc_state-
> > > > >port_clock,
> > > >                                  crtc_state->lane_count,
> > > > is_mst);
> > > > 
> > > > -       intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > > > -       intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > > 
> > > reason to be where it was is because of MST. I think what you are
> > > doing here will break it since now this is set for the port and
> > > not
> > > transcoder.
> > > intel_mst_pre_enable_dp() would call here only for the first
> > > stream,
> > > so all the others would use this same transcoder.
> > 
> > For TGL+ it moved to transcoder but for other it is still on port
> > and
> > it is kept in this patch. The fix here for TGL+ is load those 2
> > during
> > HW state readout.
> > Inside MST code it will continue to get from
> > intel_mst->primary.dp.
> 
> FYI looks like I have a reasonable way to get rid of this by finally 
> plumbing the crtc_state all the way down into link training code
> (has been on the TODO list for years). This should also allow us to
> elminate the encoder->type mess in the ddi_buf_trans code. And we
> get rid of some crtc->config stuff as well.

Cool and Chris was faster and already reviewed it.

> 
> MST retraining was the main obstacle left. I think I mostly solved
> it with the patch I just sent today. The one remaining issue I
> recall is the lane_mask for drm_dp_channel_eq_ok(). So far I don't
> have a better plan than to keep intel_dp->lane_count. But most of
> the other ad-hoc state under intel_dp can hopefully be nuked.

Okay but will merge this to fix issues that we have right now, please
CC me when you send those.

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

Re: [Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Souza, Jose]

Pushed the first one, thanks for the review Matt.

On Thu, 2020-04-16 at 16:10 +0000, Souza, Jose wrote:
> On Wed, 2020-04-15 at 20:56 -0700, Matt Roper wrote:
> > On Tue, Apr 14, 2020 at 04:04:40PM -0700, José Roberto de Souza
> > wrote:
> > > Right now dp.regs.dp_tp_ctl/status are only set during the
> > > encoder
> > > pre_enable() hook, what is causing all reads and writes to those
> > > registers to go to offset 0x0 before pre_enable() is executed.
> > > 
> > > So if i915 takes the BIOS state and don't do a modeset any
> > > following
> > > link retraing will fail.
> > > 
> > > In the case that i915 needs to do a modeset, the DDI disable
> > > sequence
> > > will write to a wrong register not disabling DP 'Transport
> > > Enable'
> > > in
> > > DP_TP_CTL, making a HDMI modeset in the same port/transcoder to
> > > not light up the monitor.
> > 
> > So to clarify I understand the problematic sequence properly:
> >  * i915 inherits already-enabled display from BIOS; pre_enable is
> > never
> >    called
> >  * we do a modeset, so we have to disable the display and then try
> > to
> > re-enable
> >      - intel_disable_ddi_buf() writes to offset 0 rather than the
> > proper
> >        register offset when attempting to reprogram DP_TP_CTL and
> >        disable DP
> >      - when we re-enable, the old, still-active DP settings in
> > hardware
> >        cause problems and the display doesn't light up
> > 
> > A couple clarifying questions:
> >  - It seems like we should have seen unclaimed register warnings
> > from
> >    the bogus writes to offset 0 during disable.  Any idea why those
> >    didn't show up?
> 
> Offset 0x0 is valid BSpec: 29316
> 
> >  - In the commit message you mention that it's the DP Transport
> > Enable
> >    that's the culprit here, which breaks future attempts to light
> > up
> >    HDMI.   I assume this means that we're also switching which pipe
> >    we're driving the port with, not just doing any
> > modeset?  Otherwise
> >    DP would stay DP, HDMI would stay HDMI, and we wouldn't see this
> >    problem with DP being active on an HDMI monitor (although we'd
> > still
> >    be writing to an invalid register offset during our first DP
> > disable
> >    which might cause other problems).  Might be worth adding a
> > mention
> >    of the pipe change to the commit message to clarify.
> 
> Got this state in a system were BIOS is lighing up DP in pipe B when
> HDMI is also connected and leaving pipe A off, no idea why BIOS
> decided
> this.
> But when i915 takes over we do the modeset to light up HDMI and
> reorder the pipes.
> 
> > The changes here look correct to me; we'll ensure the DP registers
> > have
> > proper offsets before we do our first modeset, and then the rest of
> > the
> > runtime behavior thereafter should be unchanged.  So
> > 
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > 
> > > So here for GENs older than 12, that have those registers fixed
> > > at
> > > port offset range it is loading at encoder/port init while for
> > > GEN12
> > > it will keep setting it at encoder pre_enable() and during HW
> > > state
> > > readout.
> > > 
> > > Fixes: 4444df6e205b ("drm/i915/tgl: move DP_TP_* to transcoder")
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 14 +++++++++++---
> > >  drivers/gpu/drm/i915/display/intel_dp.c  |  5 ++---
> > >  2 files changed, 13 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index be6c61bcbc9c..1aab93a94f40 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -3252,9 +3252,6 @@ static void hsw_ddi_pre_enable_dp(struct
> > > intel_atomic_state *state,
> > >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > >  				 crtc_state->lane_count, is_mst);
> > >  
> > > -	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > > -	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > > -
> > >  	intel_edp_panel_on(intel_dp);
> > >  
> > >  	intel_ddi_clk_select(encoder, crtc_state);
> > > @@ -4061,12 +4058,18 @@ void intel_ddi_get_config(struct
> > > intel_encoder *encoder,
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config-
> > > > uapi.crtc);
> > >  	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> > > +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > >  	u32 temp, flags = 0;
> > >  
> > >  	/* XXX: DSI transcoder paranoia */
> > >  	if (drm_WARN_ON(&dev_priv->drm,
> > > transcoder_is_dsi(cpu_transcoder)))
> > >  		return;
> > >  
> > > +	if (INTEL_GEN(dev_priv) >= 12) {
> > > +		intel_dp->regs.dp_tp_ctl =
> > > TGL_DP_TP_CTL(cpu_transcoder);
> > > +		intel_dp->regs.dp_tp_status =
> > > TGL_DP_TP_STATUS(cpu_transcoder);
> > > +	}
> > > +
> > >  	intel_dsc_get_config(encoder, pipe_config);
> > >  
> > >  	temp = intel_de_read(dev_priv,
> > > TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > > @@ -4396,6 +4399,7 @@ static const struct drm_encoder_funcs
> > > intel_ddi_funcs = {
> > >  static struct intel_connector *
> > >  intel_ddi_init_dp_connector(struct intel_digital_port
> > > *intel_dig_port)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> > > > base.base.dev);
> > >  	struct intel_connector *connector;
> > >  	enum port port = intel_dig_port->base.port;
> > >  
> > > @@ -4406,6 +4410,10 @@ intel_ddi_init_dp_connector(struct
> > > intel_digital_port *intel_dig_port)
> > >  	intel_dig_port->dp.output_reg = DDI_BUF_CTL(port);
> > >  	intel_dig_port->dp.prepare_link_retrain =
> > >  		intel_ddi_prepare_link_retrain;
> > > +	if (INTEL_GEN(dev_priv) < 12) {
> > > +		intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> > > +		intel_dig_port->dp.regs.dp_tp_status =
> > > DP_TP_STATUS(port);
> > > +	}
> > >  
> > >  	if (!intel_dp_init_connector(intel_dig_port, connector)) {
> > >  		kfree(connector);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index d4fcc9583869..03591ab76b0d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -2671,9 +2671,6 @@ static void intel_dp_prepare(struct
> > > intel_encoder *encoder,
> > >  				 intel_crtc_has_type(pipe_config,
> > >  						     INTEL_OUTPUT_DP_MS
> > > T));
> > >  
> > > -	intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > > -	intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > > -
> > >  	/*
> > >  	 * There are four kinds of DP registers:
> > >  	 *
> > > @@ -8470,6 +8467,8 @@ bool intel_dp_init(struct drm_i915_private
> > > *dev_priv,
> > >  
> > >  	intel_dig_port->dp.output_reg = output_reg;
> > >  	intel_dig_port->max_lanes = 4;
> > > +	intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> > > +	intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
> > >  
> > >  	intel_encoder->type = INTEL_OUTPUT_DP;
> > >  	intel_encoder->power_domain = intel_port_to_power_domain(port);
> > > -- 
> > > 2.26.0
> > > 
> _______________________________________________
> 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