Re: [Intel-gfx] [PATCH 2/7] drm/i915/display: move more scanline functions to intel_vblank.[ch]

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 15 Dec 2022, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula@intel.com>
>> Sent: Wednesday, December 14, 2022 2:45 PM
>> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Subject: RE: [Intel-gfx] [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@intel.com> wrote:
>> >> -----Original Message-----
>> >> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
>> >> Of Jani Nikula
>> >> Sent: Monday, December 12, 2022 7:59 PM
>> >> To: intel-gfx@lists.freedesktop.org
>> >> Cc: Nikula, Jani <jani.nikula@intel.com>
>> >> Subject: [Intel-gfx] [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@linux.intel.com>
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> ---
>> >>  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@intel.com>
>
>> 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