On Wed, Aug 21, 2024 at 02:58:20PM +0000, Manna, Animesh wrote:
>
>
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > Sent: Friday, July 5, 2024 11:09 PM
> > To: Manna, Animesh <animesh.ma...@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > 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-boun...@lists.freedesktop.org> On Behalf
> > > > Of Ville Syrjala
> > > > Sent: Tuesday, June 25, 2024 12:40 AM
> > > > To: intel-gfx@lists.freedesktop.org
> > > > Subject: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to use
> > > > DSB_WAIT_FOR_VBLANK
> > > >
> > > > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > >
> > > > 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.syrj...@linux.intel.com>
> > > > ---
> > > > 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.
--
Ville Syrjälä
Intel
