> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Sent: Friday, August 23, 2024 6:15 PM
> To: Manna, Animesh <animesh.manna@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to use
> DSB_WAIT_FOR_VBLANK
>
> On Wed, Aug 21, 2024 at 02:58:20PM +0000, Manna, Animesh wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Sent: Friday, July 5, 2024 11:09 PM
> > > To: Manna, Animesh <animesh.manna@xxxxxxxxx>
> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to
> > > use DSB_WAIT_FOR_VBLANK
> > >
> > > On Fri, Jul 05, 2024 at 03:58:32PM +0000, Manna, Animesh wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On
> > > > > Behalf Of Ville Syrjala
> > > > > Sent: Tuesday, June 25, 2024 12:40 AM
> > > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > Subject: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to
> > > > > use DSB_WAIT_FOR_VBLANK
> > > > >
> > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > >
> > > > > Allow intel_dsb_chain() to start the chained DSB at start of the
> > > > > undelaye vblank. This is slightly more involved than simply
> > > > > setting the bit as we must use the DEwake mechanism to eliminate
> > > > > pkgC latency.
> > > > >
> > > > > And DSB_ENABLE_DEWAKE itself is problematic in that it allows us
> > > > > to configure just a single scanline, and if the current scanline
> > > > > is already past that DSB_ENABLE_DEWAKE won't do anything,
> > > > > rendering the whole thing moot.
> > > > >
> > > > > The current workaround involves checking the pipe's current
> > > > > scanline with the CPU, and if it looks like we're about to miss
> > > > > the configured DEwake scanline we set DSB_FORCE_DEWAKE to
> > > > > immediately assert DEwake. This is somewhat racy since the
> > > > > hardware is making progress all the while we're checking it on the
> CPU.
> > > > >
> > > > > We can make things less racy by chaining two DSBs and handling
> > > > > the DSB_FORCE_DEWAKE stuff entirely without CPU involvement:
> > > > > 1. CPU starts the first DSB immediately 2. First DSB configures
> > > > > the second DSB, including its dewake_scanline 3. First DSB
> > > > > starts the second w/ DSB_WAIT_FOR_VBLANK 4. First DSB asserts
> > > DSB_FORCE_DEWAKE
> > > > > 5. First DSB waits until we're outside the dewake_scanline-
> vblank_start
> > > > > window
> > > > > 6. First DSB deasserts DSB_FORCE_DEWAKE
> > > > >
> > > > > That will guarantee that the we are fully awake when the second
> > > > > DSB starts to actually execute.
> > > > >
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_dsb.c | 43
> > > > > +++++++++++++++++++++---
> > > > > +++++++++++++++++++++drivers/gpu/drm/i915/display/intel_dsb.h |
> > > > > 3 +-
> > > > > 2 files changed, 40 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > index 4c0519c41f16..cf710f0bf430 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > > > @@ -130,8 +130,8 @@ static int dsb_vtotal(struct
> > > > > intel_atomic_state
> > > *state,
> > > > > return intel_mode_vtotal(&crtc_state->hw.adjusted_mode);
> > > > > }
> > > > >
> > > > > -static int dsb_dewake_scanline(struct intel_atomic_state *state,
> > > > > - struct intel_crtc *crtc)
> > > > > +static int dsb_dewake_scanline_start(struct intel_atomic_state
> *state,
> > > > > + struct intel_crtc *crtc)
> > > > > {
> > > > > const struct intel_crtc_state *crtc_state =
> > > > > pre_commit_crtc_state(state, crtc);
> > > > > struct drm_i915_private *i915 = to_i915(state->base.dev); @@
> > > > > -141,6 +141,14 @@ static int dsb_dewake_scanline(struct
> > > > > intel_atomic_state *state,
> > > > > intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode,
> > > > > latency);
> > > > > }
> > > > >
> > > > > +static int dsb_dewake_scanline_end(struct intel_atomic_state *state,
> > > > > + struct intel_crtc *crtc) {
> > > > > + const struct intel_crtc_state *crtc_state =
> > > > > pre_commit_crtc_state(state, crtc);
> > > > > +
> > > > > + return intel_mode_vdisplay(&crtc_state-
> >hw.adjusted_mode);
> > > > > +}
> > > > > +
> > > > > static int dsb_scanline_to_hw(struct intel_atomic_state *state,
> > > > > struct intel_crtc *crtc, int scanline) { @@ -529,19
> > > > > +537,44 @@ static void _intel_dsb_chain(struct
> > > > > +intel_atomic_state
> > > > > *state,
> > > > > dsb_error_int_status(display) |
> > > DSB_PROG_INT_STATUS |
> > > > > dsb_error_int_en(display));
> > > > >
> > > > > + if (ctrl & DSB_WAIT_FOR_VBLANK) {
> > > > > + int dewake_scanline =
> dsb_dewake_scanline_start(state,
> > > > > crtc);
> > > > > + int hw_dewake_scanline =
> dsb_scanline_to_hw(state, crtc,
> > > > > dewake_scanline);
> > > > > +
> > > > > + intel_dsb_reg_write(dsb, DSB_PMCTRL(pipe,
> chained_dsb-
> > > > > >id),
> > > > > + DSB_ENABLE_DEWAKE |
> > > > > +
> > > > > DSB_SCANLINE_FOR_DEWAKE(hw_dewake_scanline));
> > > > > + }
> > > > > +
> > > > > intel_dsb_reg_write(dsb, DSB_HEAD(pipe, chained_dsb->id),
> > > > > intel_dsb_buffer_ggtt_offset(&chained_dsb-
> > > > > >dsb_buf));
> > > > >
> > > > > intel_dsb_reg_write(dsb, DSB_TAIL(pipe, chained_dsb->id),
> > > > > intel_dsb_buffer_ggtt_offset(&chained_dsb-
> > > > > >dsb_buf) + tail);
> > > > > +
> > > > > + if (ctrl & DSB_WAIT_FOR_VBLANK) {
> > > > > + /*
> > > > > + * Keep DEwake alive via the first DSB, in
> > > > > + * case we're already past dewake_scanline,
> > > > > + * and thus DSB_ENABLE_DEWAKE on the second
> > > > > + * DSB won't do its job.
> > > > > + */
> > > > > + intel_dsb_reg_write_masked(dsb,
> DSB_PMCTRL_2(pipe, dsb-
> > > > > >id),
> > > > > + DSB_FORCE_DEWAKE,
> > > > > DSB_FORCE_DEWAKE);
> > > > > +
> > > > > + intel_dsb_wait_scanline_out(state, dsb,
> > > > > +
> dsb_dewake_scanline_start(state,
> > > > > crtc),
> > > > > +
> dsb_dewake_scanline_end(state,
> > > > > crtc));
> > > > > + }
> > > > > }
> > > > >
> > > > > void intel_dsb_chain(struct intel_atomic_state *state,
> > > > > struct intel_dsb *dsb,
> > > > > - struct intel_dsb *chained_dsb)
> > > > > + struct intel_dsb *chained_dsb,
> > > > > + bool wait_for_vblank)
> > > > > {
> > > > > _intel_dsb_chain(state, dsb, chained_dsb,
> > > > > - 0);
> > > > > + wait_for_vblank ? DSB_WAIT_FOR_VBLANK :
> 0);
> > > >
> > > > As per commit description and current implementation always need
> > > DSB_WAIT_FOR_VBLANK. Just wondering is there any scenario where will
> > > pass false through wait_for_vblank flag to intel_dsb_chain()? If no
> > > can we drop the wait_for_vblank flag?
> > >
> > > Shrug. For now I wanted to model it on intel_dsb_commit().
> > > I'm actually thinking of removing the wait_for_vblank stuff from
> > > intel_dsb_commit() after this lands. We could think about making the
> > > wait_for_vblank unconditional for intel_dsb_chain() at that point,
> > > assuming no one comes up with a use case for the immediate start
> version.
> >
> > As I understood the whole concept of intel_dsb_chain() is based on
> DSB_WAIT_FOR_VBLANK which should be unconditional and always true.
>
> You can chain without the wait for vblank just fine. Currently we have no
> actual need to do that, but I do have selftests that check both behaviours.
>
> > Not sure is it really needed to add in this patch series and later again
> removing it. So, I was waiting to see your next patch series related to plane
> update using DSB.
> > For now, with the modification to make it unconditional the other changes
> look good to me. Good to understand your plan on this.
>
> The fact that it matches the current intel_dsb_commit() is all I really care
> about at this point.
Got it, as selftest code is not published leaving it to your discretion.
Reviewed-by: Animesh Manna <animesh.manna@xxxxxxxxx>
>
> --
> Ville Syrjälä
> Intel
