Re: [Intel-gfx] [PATCH v2] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

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

On Fri, 2023-01-27 at 14:00 +0000, Souza, Jose wrote:
> On Fri, 2023-01-27 at 10:27 +0200, Jouni Högander wrote:
> > SEL_FETCH_CTL registers are armed immediately when plane is
> > disabled.
> > SEL_FETCH_* instances of plane configuration are used when doing
> > selective update and normal plane register instances for full
> > updates.
> > Currently all SEL_FETCH_* registers are written as a part of noarm
> > plane configuration. If noarm and arm plane configuration are not
> > happening within same vblank we may end up having plane as a part
> > of
> > selective update before it's PLANE_SURF register is written.
> > 
> > Fix this by splitting plane selective fetch configuration into arm
> > and
> > noarm versions and call them accordingly. Write SEL_FETCH_CTL in
> > arm
> > version.
> 
> Does this helps to revert the set of SFF and CFF at the same time?

No, this one is a separate issue.

> 
> > 
> > v2:
> >  - drop color_plane parameter from arm part
> >  - dev_priv -> i915 in arm part
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Cc: Luca Coelho <luciano.coelho@intel.com>
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cursor.c   |  3 +-
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 28 +++++++++++++--
> > ----
> >  drivers/gpu/drm/i915/display/intel_psr.h      |  6 +++-
> >  .../drm/i915/display/skl_universal_plane.c    |  4 ++-
> >  4 files changed, 30 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index d190fa0d393b..ae9f0b6c92db 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -532,7 +532,8 @@ static void i9xx_cursor_update_arm(struct
> > intel_plane *plane,
> >                 skl_write_cursor_wm(plane, crtc_state);
> >  
> >         if (plane_state)
> > -               intel_psr2_program_plane_sel_fetch(plane,
> > crtc_state, plane_state, 0);
> > +               intel_psr2_program_plane_sel_fetch_arm(plane,
> > crtc_state,
> > +                                                     
> > plane_state);
> >         else
> >                 intel_psr2_disable_plane_sel_fetch(plane,
> > crtc_state);
> 
> Missing rename intel_psr2_disable_plane_sel_fetch() to
> intel_psr2_disable_plane_sel_fetch_arm().

Yes, this makes sense. I will update the patch. Thank you for the
review.

> 
> With this LGTM.
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> 
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 7a72e15e6836..a3f4451eb66d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1559,7 +1559,25 @@ void
> > intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> >         intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id), 0);
> >  }
> >  
> > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane
> > *plane,
> > +                                       const struct
> > intel_crtc_state *crtc_state,
> > +                                       const struct
> > intel_plane_state *plane_state)
> > +{
> > +       struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +       enum pipe pipe = plane->pipe;
> > +
> > +       if (!crtc_state->enable_psr2_sel_fetch)
> > +               return;
> > +
> > +       if (plane->id == PLANE_CURSOR)
> > +               intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id),
> > +                                 plane_state->ctl);
> > +       else
> > +               intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id),
> > +                                 PLANE_SEL_FETCH_CTL_ENABLE);
> > +}
> > +
> > +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane
> > *plane,
> >                                         const struct
> > intel_crtc_state *crtc_state,
> >                                         const struct
> > intel_plane_state *plane_state,
> >                                         int color_plane)
> > @@ -1573,11 +1591,8 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> >         if (!crtc_state->enable_psr2_sel_fetch)
> >                 return;
> >  
> > -       if (plane->id == PLANE_CURSOR) {
> > -               intel_de_write_fw(dev_priv,
> > PLANE_SEL_FETCH_CTL(pipe, plane->id),
> > -                                 plane_state->ctl);
> > +       if (plane->id == PLANE_CURSOR)
> >                 return;
> > -       }
> >  
> >         clip = &plane_state->psr2_sel_fetch_area;
> >  
> > @@ -1605,9 +1620,6 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> >         val = (drm_rect_height(clip) - 1) << 16;
> >         val |= (drm_rect_width(&plane_state->uapi.src) >> 16) - 1;
> >         intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe,
> > plane->id), val);
> > -
> > -       intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id),
> > -                         PLANE_SEL_FETCH_CTL_ENABLE);
> >  }
> >  
> >  void intel_psr2_program_trans_man_trk_ctl(const struct
> > intel_crtc_state *crtc_state)
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > b/drivers/gpu/drm/i915/display/intel_psr.h
> > index 2ac3a46cccc5..c87ae2e6ee6c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > @@ -46,10 +46,14 @@ bool intel_psr_enabled(struct intel_dp
> > *intel_dp);
> >  int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
> >                                 struct intel_crtc *crtc);
> >  void intel_psr2_program_trans_man_trk_ctl(const struct
> > intel_crtc_state *crtc_state);
> > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane
> > *plane,
> >                                         const struct
> > intel_crtc_state *crtc_state,
> >                                         const struct
> > intel_plane_state *plane_state,
> >                                         int color_plane);
> > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane
> > *plane,
> > +                                       const struct
> > intel_crtc_state *crtc_state,
> > +                                       const struct
> > intel_plane_state *plane_state);
> > +
> >  void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> >                                         const struct
> > intel_crtc_state *crtc_state);
> >  void intel_psr_pause(struct intel_dp *intel_dp);
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 9b172a1e90de..296da4bc297c 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1260,7 +1260,7 @@ icl_plane_update_noarm(struct intel_plane
> > *plane,
> >         if (plane_state->force_black)
> >                 icl_plane_csc_load_black(plane);
> >  
> > -       intel_psr2_program_plane_sel_fetch(plane, crtc_state,
> > plane_state, color_plane);
> > +       intel_psr2_program_plane_sel_fetch_noarm(plane, crtc_state,
> > plane_state, color_plane);
> >  }
> >  
> >  static void
> > @@ -1287,6 +1287,8 @@ icl_plane_update_arm(struct intel_plane
> > *plane,
> >         if (plane_state->scaler_id >= 0)
> >                 skl_program_plane_scaler(plane, crtc_state,
> > plane_state);
> >  
> > +       intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state,
> > plane_state);
> > +
> >         /*
> >          * The control register self-arms if the plane was
> > previously
> >          * disabled. Try to make the plane enable atomic by writing
> 

BR,

Jouni Högander