| Message ID | 20240624191032.27333-12-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dsb: Use chained DSBs for LUT programming | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@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.syrjala@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.syrjala@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? Regards, Animesh > } > > static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl, > @@ -699,7 +732,7 @@ struct intel_dsb *intel_dsb_prepare(struct > intel_atomic_state *state, > > dsb->chicken = dsb_chicken(state, crtc); > dsb->hw_dewake_scanline = > - dsb_scanline_to_hw(state, crtc, dsb_dewake_scanline(state, > crtc)); > + dsb_scanline_to_hw(state, crtc, > dsb_dewake_scanline_start(state, crtc)); > > return dsb; > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h > b/drivers/gpu/drm/i915/display/intel_dsb.h > index e59fd7da0fc0..c352c12aa59f 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsb.h > +++ b/drivers/gpu/drm/i915/display/intel_dsb.h > @@ -47,7 +47,8 @@ void intel_dsb_wait_scanline_out(struct > intel_atomic_state *state, > int lower, int upper); > 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); > > void intel_dsb_commit(struct intel_dsb *dsb, > bool wait_for_vblank); > -- > 2.44.2
> -----Original Message----- > From: Manna, Animesh > Sent: Friday, July 5, 2024 9:29 PM > To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel- > gfx@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 <intel-gfx-bounces@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.syrjala@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.syrjala@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)); One quick check: As per bspec DSB_PMCTRL can be updated only before the DSB_CTRL register is programmed to enable the DSB engine. Here programming is done later, not sure if it causes any negative impact. Regards, Animesh > > + } > > + > > 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? > > Regards, > Animesh > > } > > > > static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl, > > @@ -699,7 +732,7 @@ struct intel_dsb *intel_dsb_prepare(struct > > intel_atomic_state *state, > > > > dsb->chicken = dsb_chicken(state, crtc); > > dsb->hw_dewake_scanline = > > - dsb_scanline_to_hw(state, crtc, dsb_dewake_scanline(state, > > crtc)); > > + dsb_scanline_to_hw(state, crtc, > > dsb_dewake_scanline_start(state, crtc)); > > > > return dsb; > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h > > b/drivers/gpu/drm/i915/display/intel_dsb.h > > index e59fd7da0fc0..c352c12aa59f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dsb.h > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.h > > @@ -47,7 +47,8 @@ void intel_dsb_wait_scanline_out(struct > > intel_atomic_state *state, > > int lower, int upper); > > 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); > > > > void intel_dsb_commit(struct intel_dsb *dsb, > > bool wait_for_vblank); > > -- > > 2.44.2
On Fri, Jul 05, 2024 at 04:09:43PM +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Manna, Animesh > > Sent: Friday, July 5, 2024 9:29 PM > > To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel- > > gfx@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 <intel-gfx-bounces@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.syrjala@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.syrjala@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)); > > One quick check: As per bspec DSB_PMCTRL can be updated only before the DSB_CTRL register is programmed to enable the DSB engine. Here programming is done later, not sure if it causes any negative impact. It seems to say that in a bunch of places, and then contradicts itself in other places where it says to reprogram the registers from the DSB commands themselves. A lot of the interesting bits are spread around on byte boundaries, which implies they are very much meant to be updated using DSB register writes with byte enables. Anyways, I've not observed any behavioural differences when updating this stuff before/after the DSB enable bit has been set. And IIRC some DSB registers even stop working when the DSB is disabled (might have been the head/tail pointers at least, not sure about the rest). I do know DSB_PMCTRL_2 at least is accesible when the DSB is not enabled, so technically we could reoder some of this at least. But we'd need to double check the behaviour of each register to be sure that it still works.
On Fri, Jul 05, 2024 at 03:58:32PM +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@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.syrjala@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.syrjala@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.
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Friday, July 5, 2024 11:09 PM > To: Manna, Animesh <animesh.manna@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-bounces@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.syrjala@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.syrjala@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. 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. Regards, Animesh > > -- > Ville Syrjälä > Intel
On Wed, Aug 21, 2024 at 02:58:20PM +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Friday, July 5, 2024 11:09 PM > > To: Manna, Animesh <animesh.manna@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-bounces@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.syrjala@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.syrjala@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.
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Friday, August 23, 2024 6:15 PM > To: Manna, Animesh <animesh.manna@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 Wed, Aug 21, 2024 at 02:58:20PM +0000, Manna, Animesh wrote: > > > > > > > -----Original Message----- > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Sent: Friday, July 5, 2024 11:09 PM > > > To: Manna, Animesh <animesh.manna@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-bounces@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.syrjala@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.syrjala@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. Got it, as selftest code is not published leaving it to your discretion. Reviewed-by: Animesh Manna <animesh.manna@intel.com> > > -- > Ville Syrjälä > Intel
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); } static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl, @@ -699,7 +732,7 @@ struct intel_dsb *intel_dsb_prepare(struct intel_atomic_state *state, dsb->chicken = dsb_chicken(state, crtc); dsb->hw_dewake_scanline = - dsb_scanline_to_hw(state, crtc, dsb_dewake_scanline(state, crtc)); + dsb_scanline_to_hw(state, crtc, dsb_dewake_scanline_start(state, crtc)); return dsb; diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h index e59fd7da0fc0..c352c12aa59f 100644 --- a/drivers/gpu/drm/i915/display/intel_dsb.h +++ b/drivers/gpu/drm/i915/display/intel_dsb.h @@ -47,7 +47,8 @@ void intel_dsb_wait_scanline_out(struct intel_atomic_state *state, int lower, int upper); 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); void intel_dsb_commit(struct intel_dsb *dsb, bool wait_for_vblank);