| Message ID | 20220518105946.28179-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Write zero wms if we disable planes for icl+ | expand |
On Wed, 18 May 2022, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> 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@intel.com> > --- > .../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. > + pipe_wm.planes[plane_id].wm[level].blocks = 0; > + pipe_wm.planes[plane_id].wm[level].lines = 0; > + } > + > + pipe_wm.planes[plane_id].trans_wm.blocks = 0; > + pipe_wm.planes[plane_id].trans_wm.lines = 0; > + > + for (level = 0; level <= max_level; level++) > + skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level), > + skl_plane_wm_level(&pipe_wm, plane_id, level)); > + > + skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id), > + skl_plane_trans_wm(&pipe_wm, plane_id)); > + > + if (HAS_HW_SAGV_WM(dev_priv)) { > + const struct skl_plane_wm *wm = &pipe_wm.planes[plane_id]; > + > + skl_write_wm_level(dev_priv, PLANE_WM_SAGV(pipe, plane_id), > + &wm->sagv.wm0); > + skl_write_wm_level(dev_priv, PLANE_WM_SAGV_TRANS(pipe, plane_id), > + &wm->sagv.trans_wm); > + } > + > + skl_ddb_entry_write(dev_priv, > + PLANE_BUF_CFG(pipe, plane_id), ddb); > + > + if (DISPLAY_VER(dev_priv) < 11) > + skl_ddb_entry_write(dev_priv, > + PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y); > +} > + > + > void skl_write_cursor_wm(struct intel_plane *plane, > const struct intel_crtc_state *crtc_state) > { > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > index 50604cf7398c..fb0ac4f143ab 100644 > --- a/drivers/gpu/drm/i915/intel_pm.h > +++ b/drivers/gpu/drm/i915/intel_pm.h > @@ -61,6 +61,8 @@ bool skl_wm_level_equals(const struct skl_wm_level *l1, > bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb, > const struct skl_ddb_entry *entries, > int num_entries, int ignore_idx); > +void skl_write_zero_plane_wm(struct intel_plane *plane, > + const struct intel_crtc_state *crtc_state); > void skl_write_plane_wm(struct intel_plane *plane, > const struct intel_crtc_state *crtc_state); > void skl_write_cursor_wm(struct intel_plane *plane,
On Wed, May 18, 2022 at 02:44:30PM +0300, Jani Nikula wrote: > On Wed, 18 May 2022, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> 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@intel.com> > > --- > > .../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.. Stan > > > + pipe_wm.planes[plane_id].wm[level].blocks = 0; > > + pipe_wm.planes[plane_id].wm[level].lines = 0; > > + } > > + > > + pipe_wm.planes[plane_id].trans_wm.blocks = 0; > > + pipe_wm.planes[plane_id].trans_wm.lines = 0; > > + > > + for (level = 0; level <= max_level; level++) > > + skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level), > > + skl_plane_wm_level(&pipe_wm, plane_id, level)); > > + > > + skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id), > > + skl_plane_trans_wm(&pipe_wm, plane_id)); > > + > > + if (HAS_HW_SAGV_WM(dev_priv)) { > > + const struct skl_plane_wm *wm = &pipe_wm.planes[plane_id]; > > + > > + skl_write_wm_level(dev_priv, PLANE_WM_SAGV(pipe, plane_id), > > + &wm->sagv.wm0); > > + skl_write_wm_level(dev_priv, PLANE_WM_SAGV_TRANS(pipe, plane_id), > > + &wm->sagv.trans_wm); > > + } > > + > > + skl_ddb_entry_write(dev_priv, > > + PLANE_BUF_CFG(pipe, plane_id), ddb); > > + > > + if (DISPLAY_VER(dev_priv) < 11) > > + skl_ddb_entry_write(dev_priv, > > + PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y); > > +} > > + > > + > > void skl_write_cursor_wm(struct intel_plane *plane, > > const struct intel_crtc_state *crtc_state) > > { > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > > index 50604cf7398c..fb0ac4f143ab 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.h > > +++ b/drivers/gpu/drm/i915/intel_pm.h > > @@ -61,6 +61,8 @@ bool skl_wm_level_equals(const struct skl_wm_level *l1, > > bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb, > > const struct skl_ddb_entry *entries, > > int num_entries, int ignore_idx); > > +void skl_write_zero_plane_wm(struct intel_plane *plane, > > + const struct intel_crtc_state *crtc_state); > > void skl_write_plane_wm(struct intel_plane *plane, > > const struct intel_crtc_state *crtc_state); > > void skl_write_cursor_wm(struct intel_plane *plane, > > -- > Jani Nikula, Intel Open Source Graphics Center
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@intel.com> 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@intel.com> > > > --- > > > .../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/
Hi Stan Pls see some comments inline.. BR Vinod On Wed, 2022-05-18 at 13:59 +0300, Stanislav Lisovskiy 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@intel.com> > --- > .../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++) { > + pipe_wm.planes[plane_id].wm[level].blocks = 0; > + pipe_wm.planes[plane_id].wm[level].lines = 0; > + } > + > + pipe_wm.planes[plane_id].trans_wm.blocks = 0; > + pipe_wm.planes[plane_id].trans_wm.lines = 0; What about the other members of struct skl_plane_wm - uv_wm, sagv.wm, sagv.trans_wm? Or memset to 0 to the sizeof(skl_wm_level) better? > + > + for (level = 0; level <= max_level; level++) > + skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level), > + skl_plane_wm_level(&pipe_wm, plane_id, level)); > + > + skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id), > + skl_plane_trans_wm(&pipe_wm, plane_id)); > + > + if (HAS_HW_SAGV_WM(dev_priv)) { > + const struct skl_plane_wm *wm = &pipe_wm.planes[plane_id]; > + > + skl_write_wm_level(dev_priv, PLANE_WM_SAGV(pipe, plane_id), > + &wm->sagv.wm0); > + skl_write_wm_level(dev_priv, PLANE_WM_SAGV_TRANS(pipe, plane_id), > + &wm->sagv.trans_wm); > + } > + > + skl_ddb_entry_write(dev_priv, > + PLANE_BUF_CFG(pipe, plane_id), ddb); As the plane wm values are hardcode to 0 just before this line, these ddb entries might not be representing the accurate information anymore. Does that even matter? Or is it better if we ignore/skip zero-ing the wm values during the disable planes completely if it doesnt matter and just rely on disabling the plane bit in plane_ctl? > + > + if (DISPLAY_VER(dev_priv) < 11) > + skl_ddb_entry_write(dev_priv, > + PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y); > +} > + > + > void skl_write_cursor_wm(struct intel_plane *plane, > const struct intel_crtc_state *crtc_state) > { > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > index 50604cf7398c..fb0ac4f143ab 100644 > --- a/drivers/gpu/drm/i915/intel_pm.h > +++ b/drivers/gpu/drm/i915/intel_pm.h > @@ -61,6 +61,8 @@ bool skl_wm_level_equals(const struct skl_wm_level *l1, > bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb, > const struct skl_ddb_entry *entries, > int num_entries, int ignore_idx); > +void skl_write_zero_plane_wm(struct intel_plane *plane, > + const struct intel_crtc_state *crtc_state); > void skl_write_plane_wm(struct intel_plane *plane, > const struct intel_crtc_state *crtc_state); > void skl_write_cursor_wm(struct intel_plane *plane,
On Mon, May 23, 2022 at 11:06:05AM +0300, Govindapillai, Vinod wrote: > Hi Stan > > Pls see some comments inline.. > > BR > Vinod > > On Wed, 2022-05-18 at 13:59 +0300, Stanislav Lisovskiy 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@intel.com> > > --- > > .../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++) { > > + pipe_wm.planes[plane_id].wm[level].blocks = 0; > > + pipe_wm.planes[plane_id].wm[level].lines = 0; > > + } > > + > > + pipe_wm.planes[plane_id].trans_wm.blocks = 0; > > + pipe_wm.planes[plane_id].trans_wm.lines = 0; > > What about the other members of struct skl_plane_wm - uv_wm, sagv.wm, sagv.trans_wm? > Or memset to 0 to the sizeof(skl_wm_level) better? Yes, you are right the whole structure should be zeroed. However that seems to be enough in practice though..well theretically whatever we are writing here, shouldn't even matter because we would be disabling the plane soon rightaway. > > > + > > + for (level = 0; level <= max_level; level++) > > + skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level), > > + skl_plane_wm_level(&pipe_wm, plane_id, level)); > > + > > + skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id), > > + skl_plane_trans_wm(&pipe_wm, plane_id)); > > + > > + if (HAS_HW_SAGV_WM(dev_priv)) { > > + const struct skl_plane_wm *wm = &pipe_wm.planes[plane_id]; > > + > > + skl_write_wm_level(dev_priv, PLANE_WM_SAGV(pipe, plane_id), > > + &wm->sagv.wm0); > > + skl_write_wm_level(dev_priv, PLANE_WM_SAGV_TRANS(pipe, plane_id), > > + &wm->sagv.trans_wm); > > + } > > + > > + skl_ddb_entry_write(dev_priv, > > + PLANE_BUF_CFG(pipe, plane_id), ddb); > > As the plane wm values are hardcode to 0 just before this line, these ddb entries might not be > representing the accurate information anymore. Does that even matter? It shouldn't matter at all since the plane is disabled. > > Or is it better if we ignore/skip zero-ing the wm values during the disable planes completely if it > doesnt matter and just rely on disabling the plane bit in plane_ctl? I now tend to do the same. Writing zeroed struct doesn't seem to be correct option either, otherwise this should be reflected in state as well - otherwise we are getting state mismatches. I think we shouldn't even touch wms here - it doesn't make sense to update them if we are disabling the plane anyway. If it was enabled before - I think it should have those correct wms already, which we are writing here(those from the state). And once it is enabled - we should anyway write new wm values there. I will probably send a new version, where we don't write those at all, when plane is disabled. Also I have suspicion, that I can't reproduce this with intel_reg, because it might be that in the driver we simply can be having race condition here - we have now removed spinlocks, in order to get rid of another bug(atomic evasion time exceeded), so now it is more probable to get this kind of issue. That might explain, why writing wms here matters while it shouldn't really and it is also not reproducible with direct register writes with intel_reg tool. Stan > > > + > > + if (DISPLAY_VER(dev_priv) < 11) > > + skl_ddb_entry_write(dev_priv, > > + PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y); > > +} > > + > > + > > void skl_write_cursor_wm(struct intel_plane *plane, > > const struct intel_crtc_state *crtc_state) > > { > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > > index 50604cf7398c..fb0ac4f143ab 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.h > > +++ b/drivers/gpu/drm/i915/intel_pm.h > > @@ -61,6 +61,8 @@ bool skl_wm_level_equals(const struct skl_wm_level *l1, > > bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb, > > const struct skl_ddb_entry *entries, > > int num_entries, int ignore_idx); > > +void skl_write_zero_plane_wm(struct intel_plane *plane, > > + const struct intel_crtc_state *crtc_state); > > void skl_write_plane_wm(struct intel_plane *plane, > > const struct intel_crtc_state *crtc_state); > > void skl_write_cursor_wm(struct intel_plane *plane,
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++) { + pipe_wm.planes[plane_id].wm[level].blocks = 0; + pipe_wm.planes[plane_id].wm[level].lines = 0; + } + + pipe_wm.planes[plane_id].trans_wm.blocks = 0; + pipe_wm.planes[plane_id].trans_wm.lines = 0; + + for (level = 0; level <= max_level; level++) + skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level), + skl_plane_wm_level(&pipe_wm, plane_id, level)); + + skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id), + skl_plane_trans_wm(&pipe_wm, plane_id)); + + if (HAS_HW_SAGV_WM(dev_priv)) { + const struct skl_plane_wm *wm = &pipe_wm.planes[plane_id]; + + skl_write_wm_level(dev_priv, PLANE_WM_SAGV(pipe, plane_id), + &wm->sagv.wm0); + skl_write_wm_level(dev_priv, PLANE_WM_SAGV_TRANS(pipe, plane_id), + &wm->sagv.trans_wm); + } + + skl_ddb_entry_write(dev_priv, + PLANE_BUF_CFG(pipe, plane_id), ddb); + + if (DISPLAY_VER(dev_priv) < 11) + skl_ddb_entry_write(dev_priv, + PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y); +} + + void skl_write_cursor_wm(struct intel_plane *plane, const struct intel_crtc_state *crtc_state) { diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h index 50604cf7398c..fb0ac4f143ab 100644 --- a/drivers/gpu/drm/i915/intel_pm.h +++ b/drivers/gpu/drm/i915/intel_pm.h @@ -61,6 +61,8 @@ bool skl_wm_level_equals(const struct skl_wm_level *l1, bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb, const struct skl_ddb_entry *entries, int num_entries, int ignore_idx); +void skl_write_zero_plane_wm(struct intel_plane *plane, + const struct intel_crtc_state *crtc_state); void skl_write_plane_wm(struct intel_plane *plane, const struct intel_crtc_state *crtc_state); void skl_write_cursor_wm(struct intel_plane *plane,
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@intel.com> --- .../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(-)