Re: [PATCH 3/3] drm/i915/display: Increase number of fast wake precharge pulses

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Wed, 2024-02-21 at 21:11 +0200, Ville Syrjälä wrote:
> On Wed, Feb 21, 2024 at 09:53:22AM +0200, Jouni Högander wrote:
> > Increasing number of fast wake sync pulses seem to fix problems
> > with
> > certain PSR panels. This should be ok for other panels as well as
> > the eDP
> > specification allows 10...16 precharge pulses and we are still
> > within that
> > range.
> 
> Hmm. Didn't we have some other panel that didn't like the original 16
> precharge pulses? Or maybe that was just because we weren't using the
> formula and thus we had a mismatch wrt. the fast wake stuff?

Original value was 32 for fw pulse len. This was changed to 24
(605f7c731333 drm/i915: Fix fast wake AUX sync len) and back then it
was fixing problem with certain panel model. This didn't match with our
static 42 us io wake time used to calculate io wake line configuration.
Issue with another panel model was fixed by setting it to 18
(b29a20f7c499 drm/i915: Use 18 fast wake AUX sync len) as was expected
by that 42 us io wake time.

> 
> And are we sure this is definitely due to the precharge length
> and not because eg. our idea of the io buffer on latency is too
> short?

In this problematic case io wake / fast wake is ending up to 4 lines.
I.e. io buffer on latency should be increased quite a lot to have any
impact on what is configured as io wake lines in PSR2 CTL. Anyways
based on my testing issue gets fixed by changing the pulse count or
fast wake lines configuration or both. Changing IO wake line
configuration doesn't help.

BR,

Jouni Högander

> 
> > 
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9739
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_aux.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > index fad39b2e3022..4641c5bb8fb9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > @@ -145,7 +145,7 @@ static int intel_dp_aux_sync_len(void)
> >  
> >  static u8 intel_dp_aux_fw_sync_len(void)
> >  {
> > -       u8 precharge = 10; /* 10-16 */
> > +       u8 precharge = 12; /* 10-16 */
> 
> I think we need a comment to remind people why we are using a
> specific
> value.
> 
> >         u8 preamble = 8;
> >  
> >         return precharge + preamble;
> > -- 
> > 2.34.1
>