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

[Hogander, Jouni]

On Thu, 2023-01-26 at 13:29 +0200, Luca Coelho wrote:
> On Wed, 2023-01-25 at 12:44 +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.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Cc: José Roberto de Souza <jose.souza at intel.com>
> > > Cc: Mika Kahola <mika.kahola at intel.com>
> > > Cc: Vinod Govindapillai <vinod.govindapillai at intel.com>
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
> > > ---
> 
> Looks fine to me. A couple of nitpicks.
> 
> 
> > >  drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +-
> > >  drivers/gpu/drm/i915/display/intel_psr.c      | 29
> > > ++++++++++++++-----
> > >  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..50232cec48e0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > @@ -532,7 +532,7 @@ 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, 0);
> 
> This goes well over 80 chars.  Even though it's accepted to go over
> that nowadays, I think it's still preferred to keep it shorter and
> this
> line is easily breakable.
> 
> 
> > >         else
> > >                 intel_psr2_disable_plane_sel_fetch(plane,
> > > crtc_state);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 7d4a15a283a0..63b79c611932 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -1559,7 +1559,26 @@ 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,
> > > +                                       int color_plane)
> > > +{
> > > +       struct drm_i915_private *dev_priv = to_i915(plane-
> > > >base.dev);
> 
> Should you use i915 instead of dev_priv? I've heard and read
> elsewhere
> that this is generally a desired change.  Much easier to use always
> the
> same local name for this kind of thing.  Though this file is already
> interspersed with both versions...
> 
> Regardless of these nitpicks (change them if you want):

Thank you Luca for checking my patch. Sent new version addressing your
comments.


> Reviewed-by: Luca Coelho <luciano.coelho at intel.com>
> 
> --
> Cheers,
> Luca.

BR,

Jouni Högander