On Thu, 15 Dec 2022, "Murthy, Arun R" <arun.r.murthy@xxxxxxxxx> wrote:
>> -----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.
Why should we duplicate the code?
BR,
Jani.
>
> 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
--
Jani Nikula, Intel Open Source Graphics Center
