> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Wednesday, December 14, 2022 2:45 PM
> To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH 2/7] drm/i915/display: move more scanline
> functions to intel_vblank.[ch]
>
> On Wed, 14 Dec 2022, "Murthy, Arun R" <arun.r.murthy@xxxxxxxxx> wrote:
> >> -----Original Message-----
> >> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf
> >> Of Jani Nikula
> >> Sent: Monday, December 12, 2022 7:59 PM
> >> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>
> >> Subject: [PATCH 2/7] drm/i915/display: move more scanline
> >> functions to intel_vblank.[ch]
> >>
> >> Reduce clutter in intel_display.c by moving the scanline
> >> moving/stopped wait functions to intel_vblank.[ch].
> >>
> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/display/intel_display.c | 36
> >> +------------------- drivers/gpu/drm/i915/display/intel_vblank.c |
> >> 35 +++++++++++++++++++ drivers/gpu/drm/i915/display/intel_vblank.h |
> >> 2 ++
> >> 3 files changed, 38 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >> b/drivers/gpu/drm/i915/display/intel_display.c
> >> index 6cdfdae2c712..0cdb514d7ee0 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -115,6 +115,7 @@
> >> #include "intel_quirks.h"
> >> #include "intel_sprite.h"
> >> #include "intel_tc.h"
> >> +#include "intel_vblank.h"
> >> #include "intel_vga.h"
> >> #include "i9xx_plane.h"
> >> #include "skl_scaler.h"
> >> @@ -386,41 +387,6 @@ struct intel_crtc *intel_master_crtc(const
> >> struct intel_crtc_state *crtc_state)
> >> return to_intel_crtc(crtc_state->uapi.crtc);
> >> }
> >>
> >> -static bool pipe_scanline_is_moving(struct drm_i915_private *dev_priv,
> >> - enum pipe pipe)
> >> -{
> >> - i915_reg_t reg = PIPEDSL(pipe);
> >> - u32 line1, line2;
> >> -
> >> - line1 = intel_de_read(dev_priv, reg) & PIPEDSL_LINE_MASK;
> >> - msleep(5);
> >> - line2 = intel_de_read(dev_priv, reg) & PIPEDSL_LINE_MASK;
> >> -
> >> - return line1 != line2;
> >> -}
> >> -
> >> -static void wait_for_pipe_scanline_moving(struct intel_crtc *crtc,
> >> bool state) -{
> >> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> - enum pipe pipe = crtc->pipe;
> >> -
> >> - /* Wait for the display line to settle/start moving */
> >> - if (wait_for(pipe_scanline_is_moving(dev_priv, pipe) == state, 100))
> >> - drm_err(&dev_priv->drm,
> >> - "pipe %c scanline %s wait timed out\n",
> >> - pipe_name(pipe), str_on_off(state));
> >> -}
> >> -
> >> -static void intel_wait_for_pipe_scanline_stopped(struct intel_crtc *crtc) -{
> >> - wait_for_pipe_scanline_moving(crtc, false);
> >> -}
> >> -
> >> -static void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc) -{
> >> - wait_for_pipe_scanline_moving(crtc, true);
> >> -}
> >> -
> >> static void
> >> intel_wait_for_pipe_off(const struct intel_crtc_state
> >> *old_crtc_state) { diff -- git
> >> a/drivers/gpu/drm/i915/display/intel_vblank.c
> >> b/drivers/gpu/drm/i915/display/intel_vblank.c
> >> index 78a579496ad1..f25ec643a0a3 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> >> @@ -417,3 +417,38 @@ int intel_get_crtc_scanline(struct intel_crtc
> >> *crtc)
> >>
> >> return position;
> >> }
> >> +
> >> +static bool pipe_scanline_is_moving(struct drm_i915_private *dev_priv,
> >> + enum pipe pipe) {
> >> + i915_reg_t reg = PIPEDSL(pipe);
> >> + u32 line1, line2;
> >> +
> >> + line1 = intel_de_read(dev_priv, reg) & PIPEDSL_LINE_MASK;
> >> + msleep(5);
> >> + line2 = intel_de_read(dev_priv, reg) & PIPEDSL_LINE_MASK;
> >> +
> >> + return line1 != line2;
> >> +}
> >> +
> >> +static void wait_for_pipe_scanline_moving(struct intel_crtc *crtc,
> >> +bool
> >> +state) {
> >> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> + enum pipe pipe = crtc->pipe;
> >> +
> >> + /* Wait for the display line to settle/start moving */
> >> + if (wait_for(pipe_scanline_is_moving(dev_priv, pipe) == state, 100))
> >> + drm_err(&dev_priv->drm,
> >> + "pipe %c scanline %s wait timed out\n",
> >> + pipe_name(pipe), str_on_off(state)); }
> >> +
> >> +void intel_wait_for_pipe_scanline_stopped(struct intel_crtc *crtc) {
> >> + wait_for_pipe_scanline_moving(crtc, false); }
> >> +
> > Is this wrapper function required, since nothing else is being other
> > than calling the function wait_for_pipe_scanline_moving, can this be
> > replaced?
>
> Well, first, this patch is only about code *movement*. Any changes like that
> would have to be separate.
>
Ok.
Reviewed-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> And how would you propose to drop the wrapper? The wrappers are all
> about readability in the caller side:
>
I didn’t mean to drop the wrapper, understand that wrapper is more readable, what I meant is to replace wait_for_pipe_scanline_moving/stopped with its function contents.
Thanks and Regards,
Arun R Murthy
--------------------
> intel_wait_for_pipe_scanline_stopped(crtc)
>
> vs.
>
> intel_wait_for_pipe_scanline_moving(crtc, false)
>
> BR,
> Jani.
>
> >
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> >
> >> +void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc) {
> >> + wait_for_pipe_scanline_moving(crtc, true); }
> >> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.h
> >> b/drivers/gpu/drm/i915/display/intel_vblank.h
> >> index 9c0034d7454d..54870cabd734 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_vblank.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_vblank.h
> >> @@ -17,5 +17,7 @@ u32 g4x_get_vblank_counter(struct drm_crtc *crtc);
> >> bool intel_crtc_get_vblank_timestamp(struct drm_crtc *crtc, int
> *max_error,
> >> ktime_t *vblank_time, bool
> >> in_vblank_irq); int intel_get_crtc_scanline(struct intel_crtc *crtc);
> >> +void intel_wait_for_pipe_scanline_stopped(struct intel_crtc *crtc);
> >> +void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc);
> >>
> >> #endif /* __INTEL_VBLANK_H__ */
> >> --
> >> 2.34.1
> >
>
> --
> Jani Nikula, Intel Open Source Graphics Center
