On Thu, May 06, 2021 at 11:47:08AM +0300, Lisovskiy, Stanislav wrote:
> On Thu, May 06, 2021 at 10:38:36AM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > When scanning out NV12 if we at any time have the plane enabled
> > while the scaler is disabled we get a pretty catastrophics
> > underrun.
> >
> > Let's reorder the operations so that we try to avoid that happening
> > even if our vblank evade fails and the scaler enable/disable and
> > the plane enable/disable get latched during two diffent frames.
> >
> > This takes care of the most common cases. I suppose there is still
> > at least a theoretical possibility of hitting this if one plane
> > takes the scaler away from another plane before the second plane
> > had a chance to set up another scaler for its use.
>
> Just curious, how this is possible? Shouldn't the scaler be already
> marked "in_use" if another plane uses it, so we can't start using
> it until it is detached?
The commit is supposed to be atomic so we can shuffle the scalers
around as much as we want, so long as we don't exceed the number
of availalbe scalers.
So we could have a commit that does this for example:
plane 1: disabled -> scaler 0
plane 2: scaler 0 -> disabled
assuming the planes get committed in order, plane 1 grabs the
scaler from plane 2 before plane 2 gets turned off.
Simple cases like that could be handled by committing the
planes in a different order (assuming the dbuf constraints
allow it), but something like
plane 1: scaler 0 -> scaler 1
plane 2: scaler 1 -> scaler 0
would pose more of a challenge.
>
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
>
>
> > But that
> > is starting to get a bit complicated, especially since the plane
> > commit order already has to be carefully sequenced to avoid any
> > dbuf overlaps. So plugging this 100% may prove somewhat hard...
> >
> > Cc: Cooper Chiou <cooper.chiou@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 30 ++++++++++++++-----
> > .../drm/i915/display/skl_universal_plane.c | 11 +++++--
> > 2 files changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index fcd8123ede8e..0c8ca26156b1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -9698,8 +9698,6 @@ static void intel_pipe_fastset(const struct intel_crtc_state *old_crtc_state,
> >
> > /* on skylake this is done by detaching scalers */
> > if (DISPLAY_VER(dev_priv) >= 9) {
> > - skl_detach_scalers(new_crtc_state);
> > -
> > if (new_crtc_state->pch_pfit.enabled)
> > skl_pfit_enable(new_crtc_state);
> > } else if (HAS_PCH_SPLIT(dev_priv)) {
> > @@ -9725,8 +9723,8 @@ static void intel_pipe_fastset(const struct intel_crtc_state *old_crtc_state,
> > icl_set_pipe_chicken(crtc);
> > }
> >
> > -static void commit_pipe_config(struct intel_atomic_state *state,
> > - struct intel_crtc *crtc)
> > +static void commit_pipe_pre_planes(struct intel_atomic_state *state,
> > + struct intel_crtc *crtc)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > const struct intel_crtc_state *old_crtc_state =
> > @@ -9744,9 +9742,6 @@ static void commit_pipe_config(struct intel_atomic_state *state,
> > new_crtc_state->update_pipe)
> > intel_color_commit(new_crtc_state);
> >
> > - if (DISPLAY_VER(dev_priv) >= 9)
> > - skl_detach_scalers(new_crtc_state);
> > -
> > if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > bdw_set_pipemisc(new_crtc_state);
> >
> > @@ -9760,6 +9755,23 @@ static void commit_pipe_config(struct intel_atomic_state *state,
> > dev_priv->display.atomic_update_watermarks(state, crtc);
> > }
> >
> > +static void commit_pipe_post_planes(struct intel_atomic_state *state,
> > + struct intel_crtc *crtc)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > + const struct intel_crtc_state *new_crtc_state =
> > + intel_atomic_get_new_crtc_state(state, crtc);
> > +
> > + /*
> > + * Disable the scaler(s) after the plane(s) so that we don't
> > + * get a catastrophic underrun even if the two operations
> > + * end up happening in two different frames.
> > + */
> > + if (DISPLAY_VER(dev_priv) >= 9 &&
> > + !intel_crtc_needs_modeset(new_crtc_state))
> > + skl_detach_scalers(new_crtc_state);
> > +}
> > +
> > static void intel_enable_crtc(struct intel_atomic_state *state,
> > struct intel_crtc *crtc)
> > {
> > @@ -9811,13 +9823,15 @@ static void intel_update_crtc(struct intel_atomic_state *state,
> > /* Perform vblank evasion around commit operation */
> > intel_pipe_update_start(new_crtc_state);
> >
> > - commit_pipe_config(state, crtc);
> > + commit_pipe_pre_planes(state, crtc);
> >
> > if (DISPLAY_VER(dev_priv) >= 9)
> > skl_update_planes_on_crtc(state, crtc);
> > else
> > i9xx_update_planes_on_crtc(state, crtc);
> >
> > + commit_pipe_post_planes(state, crtc);
> > +
> > intel_pipe_update_end(new_crtc_state);
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 0d34a5ad4e2b..6ad85d7cb219 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1032,6 +1032,14 @@ skl_program_plane(struct intel_plane *plane,
> > if (!drm_atomic_crtc_needs_modeset(&crtc_state->uapi))
> > intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, color_plane);
> >
> > + /*
> > + * Enable the scaler before the plane so that we don't
> > + * get a catastrophic underrun even if the two operations
> > + * end up happening in two different frames.
> > + */
> > + if (plane_state->scaler_id >= 0)
> > + skl_program_plane_scaler(plane, crtc_state, plane_state);
> > +
> > /*
> > * The control register self-arms if the plane was previously
> > * disabled. Try to make the plane enable atomic by writing
> > @@ -1041,9 +1049,6 @@ skl_program_plane(struct intel_plane *plane,
> > intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> > intel_plane_ggtt_offset(plane_state) + surf_addr);
> >
> > - if (plane_state->scaler_id >= 0)
> > - skl_program_plane_scaler(plane, crtc_state, plane_state);
> > -
> > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > }
> >
> > --
> > 2.26.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
