On Wed, May 18, 2022 at 03:05:44PM +0300, Lisovskiy, Stanislav wrote:
> On Wed, May 18, 2022 at 02:44:30PM +0300, Jani Nikula wrote:
> > On Wed, 18 May 2022, Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx> wrote:
> > > Otherwise we seem to get FIFO underruns. It is being disabled
> > > anyway, so kind of logical to write those as zeroes, even if
> > > disabling is temporary.
> > >
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > > ---
> > > .../drm/i915/display/skl_universal_plane.c | 2 +-
> > > drivers/gpu/drm/i915/intel_pm.c | 46 +++++++++++++++++++
> > > drivers/gpu/drm/i915/intel_pm.h | 2 +
> > > 3 files changed, 49 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > index caa03324a733..c0251189c042 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > @@ -633,7 +633,7 @@ icl_plane_disable_arm(struct intel_plane *plane,
> > > if (icl_is_hdr_plane(dev_priv, plane_id))
> > > intel_de_write_fw(dev_priv, PLANE_CUS_CTL(pipe, plane_id), 0);
> > >
> > > - skl_write_plane_wm(plane, crtc_state);
> > > + skl_write_zero_plane_wm(plane, crtc_state);
> > >
> > > intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
> > > intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), 0);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index ee0047fdc95d..2470c037bfae 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5885,6 +5885,52 @@ void skl_write_plane_wm(struct intel_plane *plane,
> > > PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y);
> > > }
> > >
> > > +void skl_write_zero_plane_wm(struct intel_plane *plane,
> > > + const struct intel_crtc_state *crtc_state)
> > > +{
> > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > + int level, max_level = ilk_wm_max_level(dev_priv);
> > > + enum plane_id plane_id = plane->id;
> > > + enum pipe pipe = plane->pipe;
> > > + struct skl_pipe_wm pipe_wm;
> > > + const struct skl_ddb_entry *ddb =
> > > + &crtc_state->wm.skl.plane_ddb[plane_id];
> > > + const struct skl_ddb_entry *ddb_y =
> > > + &crtc_state->wm.skl.plane_ddb_y[plane_id];
> > > +
> > > + for (level = 0; level <= max_level; level++) {
> >
> > Not your doing here, but why have we adopted this error prone inclusive
> > max that always makes you take a double look in the for loops?!
> >
> > BR,
> > Jani.
>
> No clue, really. It seems to be used same way all over the place in
> intel_pm.c. I was suggesting there is some smart reasoning behind this,
> so simply follow the common approach.
>
> For me it is more confusing that apparently it is called "ilk_*" which
> is IronLake probably, however we use it everywhere.
> I would call it simple i915_wm_max_level and make it determine itself based on
> dev_priv which platform it is..
There are loads of (old by now) patches on the list for cleaning
up all kinds of stuff in the wm code:
https://patchwork.freedesktop.org/series/50802/
https://patchwork.freedesktop.org/series/83289/
https://patchwork.freedesktop.org/series/90164/
--
Ville Syrjälä
Intel
