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 +, Manna, Animesh wrote:
>
>
> > -Original Message-
> > From: Intel-gfx 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ä
> >
> > 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ä
> > ---
> > 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(_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(_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(_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(_dsb-
> > >dsb_buf));
> >
> > intel_dsb_reg_write(dsb, DSB_TAIL(pipe, chained_dsb->id),
> > intel_dsb_buffer_ggtt_offset(_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);
> > +
> > +
Re: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to use DSB_WAIT_FOR_VBLANK
On Fri, Jul 05, 2024 at 04:09:43PM +, Manna, Animesh wrote:
>
>
> > -Original Message-
> > From: Manna, Animesh
> > Sent: Friday, July 5, 2024 9:29 PM
> > To: Ville Syrjala ; intel-
> > g...@lists.freedesktop.org
> > Subject: RE: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to use
> > DSB_WAIT_FOR_VBLANK
> >
> >
> >
> > > -Original Message-
> > > From: Intel-gfx 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ä
> > >
> > > 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ä
> > > ---
> > > 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(_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(_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(_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 |
> > >
RE: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to use DSB_WAIT_FOR_VBLANK
> -Original Message-
> From: Manna, Animesh
> Sent: Friday, July 5, 2024 9:29 PM
> To: Ville Syrjala ; intel-
> g...@lists.freedesktop.org
> Subject: RE: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to use
> DSB_WAIT_FOR_VBLANK
>
>
>
> > -Original Message-
> > From: Intel-gfx 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ä
> >
> > 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ä
> > ---
> > 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(_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(_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(_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));
One qu
RE: [PATCH 11/14] drm/i915/dsb: Allow intel_dsb_chain() to use DSB_WAIT_FOR_VBLANK
> -Original Message-
> From: Intel-gfx 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ä
>
> 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ä
> ---
> 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(_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(_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(_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(_dsb-
> >dsb_buf));
>
> intel_dsb_reg_write(dsb, DSB_TAIL(pipe, chained_dsb->id),
> intel_dsb_buffer_ggtt_offset(_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
