| Message ID | 20200914055633.21109-7-karthik.b.s@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Asynchronous flip implementation for i915 | expand |
On Mon, Sep 14, 2020 at 11:26:31AM +0530, Karthik B S wrote: > In Gen 9 and Gen 10 platforms, async address update enable bit is > double buffered. Due to this, during the transition from async flip > to sync flip we have to wait until this bit is updated before continuing > with the normal commit for sync flip. > > Signed-off-by: Karthik B S <karthik.b.s@intel.com> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 44 ++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index a0c17d94daf3..b7e24dff0772 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -15360,6 +15360,42 @@ static void intel_enable_crtc(struct intel_atomic_state *state, > intel_crtc_enable_pipe_crc(crtc); > } > > +static void skl_toggle_async_sync(struct intel_atomic_state *state, skl_disable_async_flip_wa() maybe? > + struct intel_crtc *crtc, > + struct intel_crtc_state *new_crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct intel_plane *plane; > + struct intel_plane_state *new_plane_state; > + u32 update_mask = new_crtc_state->update_planes; > + u32 plane_ctl, surf_addr; > + enum plane_id plane_id; > + unsigned long irqflags; > + enum pipe pipe; Most of these things are only needed within the loop, so that's where the declarations should be. > + int i; > + > + for_each_new_intel_plane_in_state(state, plane, new_plane_state, i) { > + if (crtc->pipe != plane->pipe || > + !(update_mask & BIT(plane->id))) > + continue; > + > + plane_id = plane->id; > + pipe = plane->pipe; > + I'd take the lock here so the rmw is fully protected. > + plane_ctl = intel_de_read_fw(dev_priv, PLANE_CTL(pipe, plane_id)); > + surf_addr = intel_de_read_fw(dev_priv, PLANE_SURF(pipe, plane_id)); > + > + plane_ctl &= ~PLANE_CTL_ASYNC_FLIP; > + > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > + intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl); > + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), surf_addr); > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > + } > + > + intel_wait_for_vblank(dev_priv, crtc->pipe); > +} > + > static void intel_update_crtc(struct intel_atomic_state *state, > struct intel_crtc *crtc) > { > @@ -15387,6 +15423,14 @@ static void intel_update_crtc(struct intel_atomic_state *state, > else > intel_fbc_enable(state, crtc); > > + /* WA for older platforms where async address update enable bit s/older// > + * is double buffered. "is double buffered and only latched at start of vblank" or something. Otherwise one is left wondering what the fuss is about. > + */ Multiline comment format should be /* * blah * blah */ > + if (old_crtc_state->uapi.async_flip && > + !new_crtc_state->uapi.async_flip && > + INTEL_GEN(dev_priv) <= 10 && INTEL_GEN(dev_priv) >= 9) IS_GEN_RANGE(9, 10) or whatever it's called. I guess we might want to put this call into intel_pre_plane_update() since that's where all kinds of similar wait_for_vblank w/as live. > + skl_toggle_async_sync(state, crtc, new_crtc_state); > + > /* Perform vblank evasion around commit operation */ > intel_pipe_update_start(new_crtc_state); > > -- > 2.22.0
On 9/15/2020 7:49 PM, Ville Syrjälä wrote: > On Mon, Sep 14, 2020 at 11:26:31AM +0530, Karthik B S wrote: >> In Gen 9 and Gen 10 platforms, async address update enable bit is >> double buffered. Due to this, during the transition from async flip >> to sync flip we have to wait until this bit is updated before continuing >> with the normal commit for sync flip. >> >> Signed-off-by: Karthik B S <karthik.b.s@intel.com> >> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 44 ++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index a0c17d94daf3..b7e24dff0772 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -15360,6 +15360,42 @@ static void intel_enable_crtc(struct intel_atomic_state *state, >> intel_crtc_enable_pipe_crc(crtc); >> } >> >> +static void skl_toggle_async_sync(struct intel_atomic_state *state, > > skl_disable_async_flip_wa() maybe? > Thanks for the review. I'll change the name. >> + struct intel_crtc *crtc, >> + struct intel_crtc_state *new_crtc_state) >> +{ >> + struct drm_i915_private *dev_priv = to_i915(state->base.dev); >> + struct intel_plane *plane; >> + struct intel_plane_state *new_plane_state; >> + u32 update_mask = new_crtc_state->update_planes; >> + u32 plane_ctl, surf_addr; >> + enum plane_id plane_id; >> + unsigned long irqflags; >> + enum pipe pipe; > > Most of these things are only needed within the loop, so that's where > the declarations should be. > Sure, I'll move it inside the loop. >> + int i; >> + >> + for_each_new_intel_plane_in_state(state, plane, new_plane_state, i) { >> + if (crtc->pipe != plane->pipe || >> + !(update_mask & BIT(plane->id))) >> + continue; >> + >> + plane_id = plane->id; >> + pipe = plane->pipe; >> + > > I'd take the lock here so the rmw is fully protected. > Sure, I'll move it here. >> + plane_ctl = intel_de_read_fw(dev_priv, PLANE_CTL(pipe, plane_id)); >> + surf_addr = intel_de_read_fw(dev_priv, PLANE_SURF(pipe, plane_id)); >> + >> + plane_ctl &= ~PLANE_CTL_ASYNC_FLIP; >> + >> + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); >> + intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl); >> + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), surf_addr); >> + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); >> + } >> + >> + intel_wait_for_vblank(dev_priv, crtc->pipe); >> +} >> + >> static void intel_update_crtc(struct intel_atomic_state *state, >> struct intel_crtc *crtc) >> { >> @@ -15387,6 +15423,14 @@ static void intel_update_crtc(struct intel_atomic_state *state, >> else >> intel_fbc_enable(state, crtc); >> >> + /* WA for older platforms where async address update enable bit > > s/older// > Noted. >> + * is double buffered. > > "is double buffered and only latched at start of vblank" or > something. Otherwise one is left wondering what the fuss is about. > Sure, I'll update this. >> + */ > > Multiline comment format should be > /* > * blah > * blah > */ > I'll fix this. >> + if (old_crtc_state->uapi.async_flip && >> + !new_crtc_state->uapi.async_flip && >> + INTEL_GEN(dev_priv) <= 10 && INTEL_GEN(dev_priv) >= 9) > > IS_GEN_RANGE(9, 10) or whatever it's called. > Sure, I'll update this. > I guess we might want to put this call into intel_pre_plane_update() > since that's where all kinds of similar wait_for_vblank w/as live. > Sure, I'll move this to intel_pre_plane_update() Thanks, Karthik.B.S >> + skl_toggle_async_sync(state, crtc, new_crtc_state); >> + >> /* Perform vblank evasion around commit operation */ >> intel_pipe_update_start(new_crtc_state); >> >> -- >> 2.22.0 >
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index a0c17d94daf3..b7e24dff0772 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -15360,6 +15360,42 @@ static void intel_enable_crtc(struct intel_atomic_state *state, intel_crtc_enable_pipe_crc(crtc); } +static void skl_toggle_async_sync(struct intel_atomic_state *state, + struct intel_crtc *crtc, + struct intel_crtc_state *new_crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + struct intel_plane *plane; + struct intel_plane_state *new_plane_state; + u32 update_mask = new_crtc_state->update_planes; + u32 plane_ctl, surf_addr; + enum plane_id plane_id; + unsigned long irqflags; + enum pipe pipe; + int i; + + for_each_new_intel_plane_in_state(state, plane, new_plane_state, i) { + if (crtc->pipe != plane->pipe || + !(update_mask & BIT(plane->id))) + continue; + + plane_id = plane->id; + pipe = plane->pipe; + + plane_ctl = intel_de_read_fw(dev_priv, PLANE_CTL(pipe, plane_id)); + surf_addr = intel_de_read_fw(dev_priv, PLANE_SURF(pipe, plane_id)); + + plane_ctl &= ~PLANE_CTL_ASYNC_FLIP; + + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); + intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl); + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), surf_addr); + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); + } + + intel_wait_for_vblank(dev_priv, crtc->pipe); +} + static void intel_update_crtc(struct intel_atomic_state *state, struct intel_crtc *crtc) { @@ -15387,6 +15423,14 @@ static void intel_update_crtc(struct intel_atomic_state *state, else intel_fbc_enable(state, crtc); + /* WA for older platforms where async address update enable bit + * is double buffered. + */ + if (old_crtc_state->uapi.async_flip && + !new_crtc_state->uapi.async_flip && + INTEL_GEN(dev_priv) <= 10 && INTEL_GEN(dev_priv) >= 9) + skl_toggle_async_sync(state, crtc, new_crtc_state); + /* Perform vblank evasion around commit operation */ intel_pipe_update_start(new_crtc_state);