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.
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
