Re: [PATCH v6 11/26] drm/i915/psr: Move vblank length check to separate function

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

On Fri, 2024-06-07 at 16:19 +0300, Hogander, Jouni wrote:
> On Fri, 2024-06-07 at 11:09 +0000, Manna, Animesh wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Hogander, Jouni <jouni.hogander@intel.com>
> > > Sent: Thursday, June 6, 2024 9:12 PM
> > > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > > gfx@lists.freedesktop.org
> > > Cc: Kahola, Mika <mika.kahola@intel.com>
> > > Subject: Re: [PATCH v6 11/26] drm/i915/psr: Move vblank length
> > > check to
> > > separate function
> > > 
> > > On Thu, 2024-06-06 at 14:58 +0000, Manna, Animesh wrote:
> > > > 
> > > > 
> > > > > -----Original Message-----
> > > > > From: Hogander, Jouni <jouni.hogander@intel.com>
> > > > > Sent: Wednesday, June 5, 2024 3:56 PM
> > > > > To: intel-gfx@lists.freedesktop.org
> > > > > Cc: Manna, Animesh <animesh.manna@intel.com>; Kahola, Mika
> > > > > <mika.kahola@intel.com>; Hogander, Jouni
> > > > > <jouni.hogander@intel.com>
> > > > > Subject: [PATCH v6 11/26] drm/i915/psr: Move vblank length
> > > > > check to
> > > > > separate function
> > > > > 
> > > > > We are about to add more complexity to vblank length check.
> > > > > It
> > > > > makes
> > > > > sense to move it to separate function for sake of clarity.
> > > > > 
> > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_psr.c | 18
> > > > > +++++++++++++++-
> > > > > --
> > > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 3530e5f44096..23c3fed1f983 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -1243,6 +1243,20 @@ static int
> > > > > intel_psr_entry_setup_frames(struct
> > > > > intel_dp *intel_dp,
> > > > >         return entry_setup_frames;
> > > > >  }
> > > > > 
> > > > > +static bool vblank_length_valid(struct intel_dp *intel_dp,
> > > > > +                               const struct intel_crtc_state
> > > > > *crtc_state) {
> > > > 
> > > > As this function specific to psr2, maybe good to have name as
> > > > psr2_vblank_length_valid(). Otherwise the changes looks ok to
> > > > me.
> > > 
> > > Please check patch 19. That is actually moving this to be common
> > > for Panel
> > > Replay and PSR.
> > 
> > How about su_vblank_length_valid() ? this function is specific to
> > psr2/pr and the name sounds generic to me.
> 
> Ok, I will try to figure out something else...

This actually revealed that patch 19 is wrong. This is not SU specific.
We should check this for eDP PR full frame update as well. I will take
care of fixing patch 19. Here I will change name to
wake_lines_fit_into_vblank.

BR,

Jouni Högander

> 
> BR,
> 
> Jouni Högander
> 
> > 
> > Regards,
> > Animesh
> > 
> > > 
> > > BR,
> > > 
> > > Jouni Högander
> > > 
> > > > 
> > > > Regards,
> > > > Animesh
> > > > > +       int vblank = crtc_state-
> > > > > > hw.adjusted_mode.crtc_vblank_end -
> > > > > +               crtc_state-
> > > > > >hw.adjusted_mode.crtc_vblank_start;
> > > > > +       int wake_lines = psr2_block_count_lines(intel_dp);
> > > > > +
> > > > > +       /* Vblank >= PSR2_CTL Block Count Number maximum line
> > > > > count
> > > > > */
> > > > > +       if (vblank < wake_lines)
> > > > > +               return false;
> > > > > +
> > > > > +       return true;
> > > > > +}
> > > > > +
> > > > >  static bool intel_psr2_config_valid(struct intel_dp
> > > > > *intel_dp,
> > > > >                                     struct intel_crtc_state
> > > > > *crtc_state)  { @@ -
> > > > > 1333,9 +1347,7 @@ static bool intel_psr2_config_valid(struct
> > > > > intel_dp *intel_dp,
> > > > >         }
> > > > > 
> > > > >         /* Vblank >= PSR2_CTL Block Count Number maximum line
> > > > > count
> > > > > */
> > > > > -       if (crtc_state->hw.adjusted_mode.crtc_vblank_end -
> > > > > -           crtc_state->hw.adjusted_mode.crtc_vblank_start <
> > > > > -           psr2_block_count_lines(intel_dp)) {
> > > > > +       if (!vblank_length_valid(intel_dp, crtc_state)) {
> > > > >                 drm_dbg_kms(&dev_priv->drm,
> > > > >                             "PSR2 not enabled, too short
> > > > > vblank
> > > > > time\n");
> > > > >                 return false;
> > > > > --
> > > > > 2.34.1
> > > > 
> > 
>