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

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

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.

And how would you propose to drop the wrapper? The wrappers are all
about readability in the caller side:

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