Re: [Intel-gfx] [PATCH 2/2] drm/i915: Explain the magic numbers for AUX SYNC/precharge length
On Thu, 2023-03-30 at 07:22 +, Hogander, Jouni wrote:
> On Wed, 2023-03-29 at 20:24 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Replace the hardcoded final numbers in the AUX SYNC/precharge
> > setup, and derive those from numbers from the (e)DP specs.
> >
> > The new functions can serve as the single point of truth for
> > the number of SYNC pulses we use.
> >
> > Cc: Jouni Högander
> > Signed-off-by: Ville Syrjälä
Let's take care of that io/fast wake calculation separately. I think we
want to have these in as currently fast wake SYNC pulse count is not
according to spec.
Reviewed-by: Jouni Högander
> > ---
> > drivers/gpu/drm/i915/display/intel_dp_aux.c | 32
> > +++--
> > 1 file changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > index ad0aac707219..374492293392 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > @@ -118,6 +118,32 @@ static u32 skl_get_aux_clock_divider(struct
> > intel_dp *intel_dp, int index)
> > return index ? 0 : 1;
> > }
> >
> > +static int intel_dp_aux_sync_len(void)
> > +{
> > + int precharge = 16; /* 10-16 */
> > + int preamble = 16;
> > +
> > + return precharge + preamble;
> > +}
> > +
> > +static int intel_dp_aux_fw_sync_len(void)
> > +{
> > + int precharge = 16; /* 10-16 */
> > + int preamble = 8;
> > +
> > + return precharge + preamble;
> > +}
>
> What do you think if we move this into intel_dp_aux.h and use that to
> calculate io wake time and fast wake time in
> intel_psr.c:_compute_psr2_wake_time.
>
> > +
> > +static int g4x_dp_aux_precharge_len(void)
> > +{
> > + int precharge_min = 10;
> > + int preamble = 16;
> > +
> > + /* HW wants the length of the extra precharge in 2us units
> > */
> > + return (intel_dp_aux_sync_len() -
> > + precharge_min - preamble) / 2;
> > +}
> > +
> > static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> > int send_bytes,
> > u32 aux_clock_divider)
> > @@ -140,7 +166,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp
> > *intel_dp,
> > timeout |
> > DP_AUX_CH_CTL_RECEIVE_ERROR |
> > (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> > - (3 << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> > + (g4x_dp_aux_precharge_len() <<
> > DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> > (aux_clock_divider <<
> > DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT);
> > }
> >
> > @@ -164,8 +190,8 @@ static u32 skl_get_aux_send_ctl(struct intel_dp
> > *intel_dp,
> > DP_AUX_CH_CTL_TIME_OUT_MAX |
> > DP_AUX_CH_CTL_RECEIVE_ERROR |
> > (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> > - DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(24) |
> > - DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> > +
> > DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len()) |
> > +
> > DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len());
> >
> > if (intel_tc_port_in_tbt_alt_mode(dig_port))
> > ret |= DP_AUX_CH_CTL_TBT_IO;
>
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Explain the magic numbers for AUX SYNC/precharge length
On Wed, 2023-03-29 at 20:24 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Replace the hardcoded final numbers in the AUX SYNC/precharge
> setup, and derive those from numbers from the (e)DP specs.
>
> The new functions can serve as the single point of truth for
> the number of SYNC pulses we use.
>
> Cc: Jouni Högander
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_dp_aux.c | 32
> +++--
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> index ad0aac707219..374492293392 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -118,6 +118,32 @@ static u32 skl_get_aux_clock_divider(struct
> intel_dp *intel_dp, int index)
> return index ? 0 : 1;
> }
>
> +static int intel_dp_aux_sync_len(void)
> +{
> + int precharge = 16; /* 10-16 */
> + int preamble = 16;
> +
> + return precharge + preamble;
> +}
> +
> +static int intel_dp_aux_fw_sync_len(void)
> +{
> + int precharge = 16; /* 10-16 */
> + int preamble = 8;
> +
> + return precharge + preamble;
> +}
What do you think if we move this into intel_dp_aux.h and use that to
calculate io wake time and fast wake time in
intel_psr.c:_compute_psr2_wake_time.
> +
> +static int g4x_dp_aux_precharge_len(void)
> +{
> + int precharge_min = 10;
> + int preamble = 16;
> +
> + /* HW wants the length of the extra precharge in 2us units */
> + return (intel_dp_aux_sync_len() -
> + precharge_min - preamble) / 2;
> +}
> +
> static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> int send_bytes,
> u32 aux_clock_divider)
> @@ -140,7 +166,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp
> *intel_dp,
> timeout |
> DP_AUX_CH_CTL_RECEIVE_ERROR |
> (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> - (3 << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> + (g4x_dp_aux_precharge_len() <<
> DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> (aux_clock_divider <<
> DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT);
> }
>
> @@ -164,8 +190,8 @@ static u32 skl_get_aux_send_ctl(struct intel_dp
> *intel_dp,
> DP_AUX_CH_CTL_TIME_OUT_MAX |
> DP_AUX_CH_CTL_RECEIVE_ERROR |
> (send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> - DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(24) |
> - DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> +
> DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len()) |
> + DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len());
>
> if (intel_tc_port_in_tbt_alt_mode(dig_port))
> ret |= DP_AUX_CH_CTL_TBT_IO;
[Intel-gfx] [PATCH 2/2] drm/i915: Explain the magic numbers for AUX SYNC/precharge length
From: Ville Syrjälä
Replace the hardcoded final numbers in the AUX SYNC/precharge
setup, and derive those from numbers from the (e)DP specs.
The new functions can serve as the single point of truth for
the number of SYNC pulses we use.
Cc: Jouni Högander
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_dp_aux.c | 32 +++--
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
b/drivers/gpu/drm/i915/display/intel_dp_aux.c
index ad0aac707219..374492293392 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
@@ -118,6 +118,32 @@ static u32 skl_get_aux_clock_divider(struct intel_dp
*intel_dp, int index)
return index ? 0 : 1;
}
+static int intel_dp_aux_sync_len(void)
+{
+ int precharge = 16; /* 10-16 */
+ int preamble = 16;
+
+ return precharge + preamble;
+}
+
+static int intel_dp_aux_fw_sync_len(void)
+{
+ int precharge = 16; /* 10-16 */
+ int preamble = 8;
+
+ return precharge + preamble;
+}
+
+static int g4x_dp_aux_precharge_len(void)
+{
+ int precharge_min = 10;
+ int preamble = 16;
+
+ /* HW wants the length of the extra precharge in 2us units */
+ return (intel_dp_aux_sync_len() -
+ precharge_min - preamble) / 2;
+}
+
static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
int send_bytes,
u32 aux_clock_divider)
@@ -140,7 +166,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
timeout |
DP_AUX_CH_CTL_RECEIVE_ERROR |
(send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
- (3 << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
+ (g4x_dp_aux_precharge_len() <<
DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
(aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT);
}
@@ -164,8 +190,8 @@ static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
DP_AUX_CH_CTL_TIME_OUT_MAX |
DP_AUX_CH_CTL_RECEIVE_ERROR |
(send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
- DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(24) |
- DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
+ DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len()) |
+ DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len());
if (intel_tc_port_in_tbt_alt_mode(dig_port))
ret |= DP_AUX_CH_CTL_TBT_IO;
--
2.39.2
