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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Thursday, December 15, 2022 2:43 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 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?

static void intel_wait_for_pipe_scanline_moving(struct intel_crtc *crtc) {
	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));
}

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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux