diff mbox series

drm/i915/display: Skip state verification with TBT-ALT mode

Message ID 20231127154702.979401-1-mika.kahola@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/display: Skip state verification with TBT-ALT mode | expand

Commit Message

Mika Kahola Nov. 27, 2023, 3:47 p.m. UTC
With TBT-ALT mode we are not programming C20 chip PLL's and
hence we don't need to check state verification. We don't
need to program DP link signal levels i.e.pre-emphasis and
voltage swing either.

This patch fixes dmesg errors like this one

"[drm] ERROR PHY F Write 0c06 failed after 3 retries."

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Jani Nikula Nov. 27, 2023, 4:47 p.m. UTC | #1
On Mon, 27 Nov 2023, Mika Kahola <mika.kahola@intel.com> wrote:
> With TBT-ALT mode we are not programming C20 chip PLL's and
> hence we don't need to check state verification. We don't
> need to program DP link signal levels i.e.pre-emphasis and
> voltage swing either.
>
> This patch fixes dmesg errors like this one
>
> "[drm] ERROR PHY F Write 0c06 failed after 3 retries."
>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index a8fa76580802..3a30cffd450c 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -418,6 +418,10 @@ void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder,
>  	u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(i915, encoder);
>  	intel_wakeref_t wakeref;
>  	int n_entries, ln;
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +
> +	if (intel_tc_port_in_tbt_alt_mode(dig_port))
> +		return;
>  
>  	wakeref = intel_cx0_phy_transaction_begin(encoder);
>  
> @@ -3136,6 +3140,9 @@ void intel_cx0pll_state_verify(struct intel_atomic_state *state,
>  	encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
>  	phy = intel_port_to_phy(i915, encoder->port);
>  
> +	if (intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder)))
> +		return;
> +

Shouldn't we read and ensure it's disabled?

>  	intel_cx0pll_readout_hw_state(encoder, &mpll_hw_state);
>  
>  	if (intel_is_c10phy(i915, phy))
Gustavo Sousa Nov. 27, 2023, 6:17 p.m. UTC | #2
Quoting Mika Kahola (2023-11-27 12:47:02-03:00)
>With TBT-ALT mode we are not programming C20 chip PLL's and
>hence we don't need to check state verification. We don't
>need to program DP link signal levels i.e.pre-emphasis and
>voltage swing either.
>
>This patch fixes dmesg errors like this one
>
>"[drm] ERROR PHY F Write 0c06 failed after 3 retries."
>
>Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>index a8fa76580802..3a30cffd450c 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>@@ -418,6 +418,10 @@ void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder,
>         u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(i915, encoder);
>         intel_wakeref_t wakeref;
>         int n_entries, ln;
>+        struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>+
>+        if (intel_tc_port_in_tbt_alt_mode(dig_port))
>+                return;

I think we could make the call to intel_cx0_get_owned_lane_mask() here, to make
sure we do not waste time doing useless MMIO.

With that in place,

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

--
Gustavo Sousa

> 
>         wakeref = intel_cx0_phy_transaction_begin(encoder);
> 
>@@ -3136,6 +3140,9 @@ void intel_cx0pll_state_verify(struct intel_atomic_state *state,
>         encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
>         phy = intel_port_to_phy(i915, encoder->port);
> 
>+        if (intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder)))
>+                return;
>+
>         intel_cx0pll_readout_hw_state(encoder, &mpll_hw_state);
> 
>         if (intel_is_c10phy(i915, phy))
>-- 
>2.34.1
>
Gustavo Sousa Nov. 27, 2023, 6:32 p.m. UTC | #3
Quoting Jani Nikula (2023-11-27 13:47:22-03:00)
>On Mon, 27 Nov 2023, Mika Kahola <mika.kahola@intel.com> wrote:
>> With TBT-ALT mode we are not programming C20 chip PLL's and
>> hence we don't need to check state verification. We don't
>> need to program DP link signal levels i.e.pre-emphasis and
>> voltage swing either.
>>
>> This patch fixes dmesg errors like this one
>>
>> "[drm] ERROR PHY F Write 0c06 failed after 3 retries."
>>
>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> index a8fa76580802..3a30cffd450c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> @@ -418,6 +418,10 @@ void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder,
>>          u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(i915, encoder);
>>          intel_wakeref_t wakeref;
>>          int n_entries, ln;
>> +        struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>> +
>> +        if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> +                return;
>>  
>>          wakeref = intel_cx0_phy_transaction_begin(encoder);
>>  
>> @@ -3136,6 +3140,9 @@ void intel_cx0pll_state_verify(struct intel_atomic_state *state,
>>          encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
>>          phy = intel_port_to_phy(i915, encoder->port);
>>  
>> +        if (intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder)))
>> +                return;
>> +
>
>Shouldn't we read and ensure it's disabled?

In TBT-alt mode, the PHY is owned by the Thunderbold controller, and it could be
in use.

I guess what we could do is verify that PORT_CLOCK_CTL has the expected bits
depending on the mode. That could done here or in a followup series.

--
Gustavo Sousa

>
>>          intel_cx0pll_readout_hw_state(encoder, &mpll_hw_state);
>>  
>>          if (intel_is_c10phy(i915, phy))
>
>-- 
>Jani Nikula, Intel
Mika Kahola Nov. 28, 2023, 10:50 a.m. UTC | #4
> -----Original Message-----
> From: Sousa, Gustavo <gustavo.sousa@intel.com>
> Sent: Monday, November 27, 2023 8:18 PM
> To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Skip state verification with TBT-ALT mode
> 
> Quoting Mika Kahola (2023-11-27 12:47:02-03:00)
> >With TBT-ALT mode we are not programming C20 chip PLL's and hence we
> >don't need to check state verification. We don't need to program DP
> >link signal levels i.e.pre-emphasis and voltage swing either.
> >
> >This patch fixes dmesg errors like this one
> >
> >"[drm] ERROR PHY F Write 0c06 failed after 3 retries."
> >
> >Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >index a8fa76580802..3a30cffd450c 100644
> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >@@ -418,6 +418,10 @@ void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder,
> >         u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(i915, encoder);
> >         intel_wakeref_t wakeref;
> >         int n_entries, ln;
> >+        struct intel_digital_port *dig_port =
> >+ enc_to_dig_port(encoder);
> >+
> >+        if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >+                return;
> 
> I think we could make the call to intel_cx0_get_owned_lane_mask() here, to make sure we do not waste time doing useless
> MMIO.

That's true, thanks!. I will move this intel_cx0_get_owned_lane_mask() call here. This way it is not called if we need to bail out early from this function.

-Mika-

> 
> With that in place,
> 
> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
> 
> --
> Gustavo Sousa
> 
> >
> >         wakeref = intel_cx0_phy_transaction_begin(encoder);
> >
> >@@ -3136,6 +3140,9 @@ void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> >         encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
> >         phy = intel_port_to_phy(i915, encoder->port);
> >
> >+        if (intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder)))
> >+                return;
> >+
> >         intel_cx0pll_readout_hw_state(encoder, &mpll_hw_state);
> >
> >         if (intel_is_c10phy(i915, phy))
> >--
> >2.34.1
> >
Mika Kahola Nov. 28, 2023, 11:13 a.m. UTC | #5
> -----Original Message-----
> From: Sousa, Gustavo <gustavo.sousa@intel.com>
> Sent: Monday, November 27, 2023 8:33 PM
> To: Jani Nikula <jani.nikula@linux.intel.com>; Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Skip state verification with TBT-ALT mode
> 
> Quoting Jani Nikula (2023-11-27 13:47:22-03:00)
> >On Mon, 27 Nov 2023, Mika Kahola <mika.kahola@intel.com> wrote:
> >> With TBT-ALT mode we are not programming C20 chip PLL's and hence we
> >> don't need to check state verification. We don't need to program DP
> >> link signal levels i.e.pre-emphasis and voltage swing either.
> >>
> >> This patch fixes dmesg errors like this one
> >>
> >> "[drm] ERROR PHY F Write 0c06 failed after 3 retries."
> >>
> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> index a8fa76580802..3a30cffd450c 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> @@ -418,6 +418,10 @@ void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder,
> >>          u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(i915, encoder);
> >>          intel_wakeref_t wakeref;
> >>          int n_entries, ln;
> >> +        struct intel_digital_port *dig_port =
> >> + enc_to_dig_port(encoder);
> >> +
> >> +        if (intel_tc_port_in_tbt_alt_mode(dig_port))
> >> +                return;
> >>
> >>          wakeref = intel_cx0_phy_transaction_begin(encoder);
> >>
> >> @@ -3136,6 +3140,9 @@ void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> >>          encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
> >>          phy = intel_port_to_phy(i915, encoder->port);
> >>
> >> +        if (intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder)))
> >> +                return;
> >> +
> >
> >Shouldn't we read and ensure it's disabled?
> 
> In TBT-alt mode, the PHY is owned by the Thunderbold controller, and it could be in use.
> 
> I guess what we could do is verify that PORT_CLOCK_CTL has the expected bits depending on the mode. That could done here or in
> a followup series.

From PORT_CLOCK_CTL we could check if DDI or any transcoder directed to DDI is enabled. I could do some testing with this. Probably this is a subject for a follow up patch.

-Mika-
> 
> --
> Gustavo Sousa
> 
> >
> >>          intel_cx0pll_readout_hw_state(encoder, &mpll_hw_state);
> >>
> >>          if (intel_is_c10phy(i915, phy))
> >
> >--
> >Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index a8fa76580802..3a30cffd450c 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -418,6 +418,10 @@  void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder,
 	u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(i915, encoder);
 	intel_wakeref_t wakeref;
 	int n_entries, ln;
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+
+	if (intel_tc_port_in_tbt_alt_mode(dig_port))
+		return;
 
 	wakeref = intel_cx0_phy_transaction_begin(encoder);
 
@@ -3136,6 +3140,9 @@  void intel_cx0pll_state_verify(struct intel_atomic_state *state,
 	encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
 	phy = intel_port_to_phy(i915, encoder->port);
 
+	if (intel_tc_port_in_tbt_alt_mode(enc_to_dig_port(encoder)))
+		return;
+
 	intel_cx0pll_readout_hw_state(encoder, &mpll_hw_state);
 
 	if (intel_is_c10phy(i915, phy))