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@xxxxxxxxxxxxxxx>
> > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > Cc: Vinod Govindapillai <vinod.govindapillai@xxxxxxxxx>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > ---
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):
Reviewed-by: Luca Coelho <luciano.coelho@xxxxxxxxx>
--
Cheers,
Luca.
