Re: [Intel-gfx] [PATCH 2/6] drm/i915: Change intel_pipe_update_{start, end}() calling convention

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Aug 28, 2023 at 11:30:25AM -0700, Manasi Navare wrote:
> On Sun, Aug 27, 2023 at 10:41 PM Ville Syrjala
> <ville.syrjala@linux.intel.com> wrote:
> >
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We'll need to also look at the old crtc state in
> > intel_pipe_update_start() so change the calling convention to
> > just plumb in the full atomic state instead.
> 
> I am guessing we would need the old crtc state to look at if VRR parameters
> were changed?
> Could we elaborate why we would need old crtc state so we better understand this
> change in the patch?

Details are in the patch that does those changes.

> 
> Manasi
> 
> >
> > Cc: Manasi Navare <navaremanasi@chromium.org>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_crtc.c    | 18 ++++++++++++------
> >  drivers/gpu/drm/i915/display/intel_crtc.h    |  6 ++++--
> >  drivers/gpu/drm/i915/display/intel_display.c |  4 ++--
> >  3 files changed, 18 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index 5caa928e5ce9..461949b48411 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -470,7 +470,8 @@ static int intel_mode_vblank_start(const struct drm_display_mode *mode)
> >
> >  /**
> >   * intel_pipe_update_start() - start update of a set of display registers
> > - * @new_crtc_state: the new crtc state
> > + * @state: the atomic state
> > + * @crtc: the crtc
> >   *
> >   * Mark the start of an update to pipe registers that should be updated
> >   * atomically regarding vblank. If the next vblank will happens within
> > @@ -480,10 +481,12 @@ static int intel_mode_vblank_start(const struct drm_display_mode *mode)
> >   * until a subsequent call to intel_pipe_update_end(). That is done to
> >   * avoid random delays.
> >   */
> > -void intel_pipe_update_start(struct intel_crtc_state *new_crtc_state)
> > +void intel_pipe_update_start(struct intel_atomic_state *state,
> > +                            struct intel_crtc *crtc)
> >  {
> > -       struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
> >         struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +       struct intel_crtc_state *new_crtc_state =
> > +               intel_atomic_get_new_crtc_state(state, crtc);
> >         const struct drm_display_mode *adjusted_mode = &new_crtc_state->hw.adjusted_mode;
> >         long timeout = msecs_to_jiffies_timeout(1);
> >         int scanline, min, max, vblank_start;
> > @@ -631,15 +634,18 @@ static void dbg_vblank_evade(struct intel_crtc *crtc, ktime_t end) {}
> >
> >  /**
> >   * intel_pipe_update_end() - end update of a set of display registers
> > - * @new_crtc_state: the new crtc state
> > + * @state: the atomic state
> > + * @crtc: the crtc
> >   *
> >   * Mark the end of an update started with intel_pipe_update_start(). This
> >   * re-enables interrupts and verifies the update was actually completed
> >   * before a vblank.
> >   */
> > -void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
> > +void intel_pipe_update_end(struct intel_atomic_state *state,
> > +                          struct intel_crtc *crtc)
> >  {
> > -       struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
> > +       struct intel_crtc_state *new_crtc_state =
> > +               intel_atomic_get_new_crtc_state(state, crtc);
> >         enum pipe pipe = crtc->pipe;
> >         int scanline_end = intel_get_crtc_scanline(crtc);
> >         u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h
> > index 51a4c8df9e65..22d7993d1f0b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.h
> > @@ -36,8 +36,10 @@ void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
> >  u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
> >  void intel_crtc_vblank_on(const struct intel_crtc_state *crtc_state);
> >  void intel_crtc_vblank_off(const struct intel_crtc_state *crtc_state);
> > -void intel_pipe_update_start(struct intel_crtc_state *new_crtc_state);
> > -void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state);
> > +void intel_pipe_update_start(struct intel_atomic_state *state,
> > +                            struct intel_crtc *crtc);
> > +void intel_pipe_update_end(struct intel_atomic_state *state,
> > +                          struct intel_crtc *crtc);
> >  void intel_wait_for_vblank_workers(struct intel_atomic_state *state);
> >  struct intel_crtc *intel_first_crtc(struct drm_i915_private *i915);
> >  struct intel_crtc *intel_crtc_for_pipe(struct drm_i915_private *i915,
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index f6397462e4c2..cfad967b5684 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6559,7 +6559,7 @@ static void intel_update_crtc(struct intel_atomic_state *state,
> >         intel_crtc_planes_update_noarm(state, crtc);
> >
> >         /* Perform vblank evasion around commit operation */
> > -       intel_pipe_update_start(new_crtc_state);
> > +       intel_pipe_update_start(state, crtc);
> >
> >         commit_pipe_pre_planes(state, crtc);
> >
> > @@ -6567,7 +6567,7 @@ static void intel_update_crtc(struct intel_atomic_state *state,
> >
> >         commit_pipe_post_planes(state, crtc);
> >
> > -       intel_pipe_update_end(new_crtc_state);
> > +       intel_pipe_update_end(state, crtc);
> >
> >         /*
> >          * We usually enable FIFO underrun interrupts as part of the
> > --
> > 2.41.0
> >

-- 
Ville Syrjälä
Intel