On Tue, Nov 20, 2018 at 04:48:39PM -0800, Matt Roper wrote:
> On Wed, Nov 14, 2018 at 11:07:26PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > On SKL+ the plane WM/BUF_CFG registers are a proper part of each
> > plane's register set. That means accessing them will cancel any
> > pending plane update, and we would need a PLANE_SURF register write
> > to arm the wm/ddb change as well.
> >
> > To avoid all the problems with that let's just move the wm/ddb
> > programming into the plane update/disable hooks. Now all plane
> > registers get written in one (hopefully atomic) operation.
> >
> > To make that feasible we'll move the plane ddb tracking into
> > the crtc state. Watermarks were already tracked there.
> >
> > v2: Rebase due to input CSC
> > v3: Split out a bunch of junk (Matt)
> >
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 21 +-
> > drivers/gpu/drm/i915/i915_drv.h | 3 -
> > drivers/gpu/drm/i915/intel_display.c | 16 +-
> > drivers/gpu/drm/i915/intel_display.h | 11 +-
> > drivers/gpu/drm/i915/intel_drv.h | 9 +
> > drivers/gpu/drm/i915/intel_pm.c | 317 ++++++++++++---------------
> > drivers/gpu/drm/i915/intel_sprite.c | 4 +
> > 7 files changed, 181 insertions(+), 200 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 670db5073d70..f8b2200947cf 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -3437,31 +3437,32 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
> > {
> > struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > struct drm_device *dev = &dev_priv->drm;
> > - struct skl_ddb_allocation *ddb;
> > struct skl_ddb_entry *entry;
> > - enum pipe pipe;
> > - int plane;
> > + struct intel_crtc *crtc;
> >
> > if (INTEL_GEN(dev_priv) < 9)
> > return -ENODEV;
> >
> > drm_modeset_lock_all(dev);
> >
> > - ddb = &dev_priv->wm.skl_hw.ddb;
> > -
> > seq_printf(m, "%-15s%8s%8s%8s\n", "", "Start", "End", "Size");
> >
> > - for_each_pipe(dev_priv, pipe) {
> > + for_each_intel_crtc(&dev_priv->drm, crtc) {
> > + struct intel_crtc_state *crtc_state =
> > + to_intel_crtc_state(crtc->base.state);
> > + enum pipe pipe = crtc->pipe;
> > + enum plane_id plane_id;
> > +
> > seq_printf(m, "Pipe %c\n", pipe_name(pipe));
> >
> > - for_each_universal_plane(dev_priv, pipe, plane) {
> > - entry = &ddb->plane[pipe][plane];
> > - seq_printf(m, " Plane%-8d%8u%8u%8u\n", plane + 1,
> > + for_each_plane_id_on_crtc(crtc, plane_id) {
> > + entry = &crtc_state->wm.skl.plane_ddb_y[plane_id];
> > + seq_printf(m, " Plane%-8d%8u%8u%8u\n", plane_id + 1,
> > entry->start, entry->end,
> > skl_ddb_entry_size(entry));
> > }
> >
> > - entry = &ddb->plane[pipe][PLANE_CURSOR];
> > + entry = &crtc_state->wm.skl.plane_ddb_y[PLANE_CURSOR];
> > seq_printf(m, " %-13s%8u%8u%8u\n", "Cursor", entry->start,
> > entry->end, skl_ddb_entry_size(entry));
> > }
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 5d686b585a95..89af64fe90a5 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1241,9 +1241,6 @@ static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
> > }
> >
> > struct skl_ddb_allocation {
> > - /* packed/y */
> > - struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES];
> > - struct skl_ddb_entry uv_plane[I915_MAX_PIPES][I915_MAX_PLANES];
> > u8 enabled_slices; /* GEN11 has configurable 2 slices */
> > };
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0caba7258fee..2981cea3704a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10083,6 +10083,10 @@ static void i9xx_update_cursor(struct intel_plane *plane,
> > * except when the plane is getting enabled at which time
> > * the CURCNTR write arms the update.
> > */
> > +
> > + if (INTEL_GEN(dev_priv) >= 9)
> > + skl_write_cursor_wm(plane, crtc_state);
> > +
> > if (plane->cursor.base != base ||
> > plane->cursor.size != fbc_ctl ||
> > plane->cursor.cntl != cntl) {
> > @@ -11872,6 +11876,8 @@ static void verify_wm_state(struct drm_crtc *crtc,
> > struct skl_pipe_wm hw_wm, *sw_wm;
> > struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> > struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
> > + struct skl_ddb_entry hw_ddb_y[I915_MAX_PLANES];
> > + struct skl_ddb_entry hw_ddb_uv[I915_MAX_PLANES];
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > const enum pipe pipe = intel_crtc->pipe;
> > int plane, level, max_level = ilk_wm_max_level(dev_priv);
> > @@ -11882,6 +11888,8 @@ static void verify_wm_state(struct drm_crtc *crtc,
> > skl_pipe_wm_get_hw_state(crtc, &hw_wm);
> > sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
> >
> > + skl_pipe_ddb_get_hw_state(intel_crtc, hw_ddb_y, hw_ddb_uv);
> > +
> > skl_ddb_get_hw_state(dev_priv, &hw_ddb);
> > sw_ddb = &dev_priv->wm.skl_hw.ddb;
> >
> > @@ -11924,8 +11932,8 @@ static void verify_wm_state(struct drm_crtc *crtc,
> > }
> >
> > /* DDB */
> > - hw_ddb_entry = &hw_ddb.plane[pipe][plane];
> > - sw_ddb_entry = &sw_ddb->plane[pipe][plane];
> > + hw_ddb_entry = &hw_ddb_y[plane];
> > + sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[plane];
> >
> > if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> > DRM_ERROR("mismatch in DDB state pipe %c plane %d (expected (%u,%u), found (%u,%u))\n",
> > @@ -11974,8 +11982,8 @@ static void verify_wm_state(struct drm_crtc *crtc,
> > }
> >
> > /* DDB */
> > - hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> > - sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> > + hw_ddb_entry = &hw_ddb_y[PLANE_CURSOR];
> > + sw_ddb_entry = &to_intel_crtc_state(new_state)->wm.skl.plane_ddb_y[PLANE_CURSOR];
> >
> > if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
> > DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index df9e6ebb27de..078406dc65e5 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -319,7 +319,7 @@ struct intel_link_m_n {
> > &(dev)->mode_config.plane_list, \
> > base.head) \
> > for_each_if((plane_mask) & \
> > - drm_plane_mask(&intel_plane->base)))
> > + drm_plane_mask(&intel_plane->base))
> >
> > #define for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) \
> > list_for_each_entry(intel_plane, \
> > @@ -415,6 +415,15 @@ struct intel_link_m_n {
> > (__i)++) \
> > for_each_if(plane)
> >
> > +#define for_each_oldnew_intel_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \
> > + for ((__i) = 0; \
> > + (__i) < (__state)->base.dev->mode_config.num_crtc && \
> > + ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
> > + (old_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].old_state), \
> > + (new_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].new_state), 1); \
> > + (__i)++) \
> > + for_each_if(crtc)
> > +
> > void intel_link_compute_m_n(int bpp, int nlanes,
> > int pixel_clock, int link_clock,
> > struct intel_link_m_n *m_n,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 23b33970db17..4a9af09c483a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -705,6 +705,8 @@ struct intel_crtc_wm_state {
> > /* gen9+ only needs 1-step wm programming */
> > struct skl_pipe_wm optimal;
> > struct skl_ddb_entry ddb;
> > + struct skl_ddb_entry plane_ddb_y[I915_MAX_PLANES];
> > + struct skl_ddb_entry plane_ddb_uv[I915_MAX_PLANES];
>
> I feel like naming these _y and _uv is going to cause confusion. The Y
> and UV are backwards half the time (based on the swap we have to do
> farther down), and doesn't have meaning at all for RGB. Maybe name
> these something like "plane_ddb_main" and "plane_ddb_extra" to somewhat
> disassociate it with the actual type of data it's supposed to represent?
>
> If you want to keep the _y and _uv (which are nice and short), I think
> we at least need some warning comments here explaining the behavior.
Yeah, I was also thinking we might want to just start following the
hardware convention with these and get rid of the swap()s. My initial
idea was just something like ddb[] and ddb_nv12[] to match the
register names.
But let's do that as a followup maybe?
>
> > } skl;
> >
> > struct {
> > @@ -2183,6 +2185,9 @@ void g4x_wm_get_hw_state(struct drm_device *dev);
> > void vlv_wm_get_hw_state(struct drm_device *dev);
> > void ilk_wm_get_hw_state(struct drm_device *dev);
> > void skl_wm_get_hw_state(struct drm_device *dev);
> > +void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
> > + struct skl_ddb_entry *ddb_y,
> > + struct skl_ddb_entry *ddb_uv);
> > void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> > struct skl_ddb_allocation *ddb /* out */);
> > void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
> > @@ -2197,6 +2202,10 @@ 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_plane_wm(struct intel_plane *plane,
> > + const struct intel_crtc_state *crtc_state);
> > +void skl_write_cursor_wm(struct intel_plane *plane,
> > + const struct intel_crtc_state *crtc_state);
> > bool ilk_disable_lp_wm(struct drm_device *dev);
> > int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
> > struct intel_crtc_state *cstate);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index a21654c974ba..1b337004054a 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3912,68 +3912,70 @@ static void
> > skl_ddb_get_hw_plane_state(struct drm_i915_private *dev_priv,
> > const enum pipe pipe,
> > const enum plane_id plane_id,
> > - struct skl_ddb_allocation *ddb /* out */)
> > + struct skl_ddb_entry *ddb_y,
> > + struct skl_ddb_entry *ddb_uv)
> > {
> > - u32 val, val2 = 0;
> > - int fourcc, pixel_format;
> > + u32 val, val2;
> > + u32 fourcc = 0;
> >
> > /* Cursor doesn't support NV12/planar, so no extra calculation needed */
> > if (plane_id == PLANE_CURSOR) {
> > val = I915_READ(CUR_BUF_CFG(pipe));
> > - skl_ddb_entry_init_from_hw(dev_priv,
> > - &ddb->plane[pipe][plane_id], val);
> > + skl_ddb_entry_init_from_hw(dev_priv, ddb_y, val);
> > return;
> > }
> >
> > val = I915_READ(PLANE_CTL(pipe, plane_id));
> >
> > /* No DDB allocated for disabled planes */
> > - if (!(val & PLANE_CTL_ENABLE))
> > - return;
>
> I think we should mention the readout behavior change in the commit
> message (i.e., we now read out DDB even if we think the plane is
> disabled). Either that or move the behavior change to a separate patch.
>
> We should also drop the comment line since I don't think it really makes
> sense anymore.
Sure.
>
> > + if (val & PLANE_CTL_ENABLE)
> > + fourcc = skl_format_to_fourcc(val & PLANE_CTL_FORMAT_MASK,
> > + val & PLANE_CTL_ORDER_RGBX,
> > + val & PLANE_CTL_ALPHA_MASK);
> >
> > - pixel_format = val & PLANE_CTL_FORMAT_MASK;
> > - fourcc = skl_format_to_fourcc(pixel_format,
> > - val & PLANE_CTL_ORDER_RGBX,
> > - val & PLANE_CTL_ALPHA_MASK);
> > -
> > - val = I915_READ(PLANE_BUF_CFG(pipe, plane_id));
> > - if (fourcc == DRM_FORMAT_NV12 && INTEL_GEN(dev_priv) < 11) {
> > + if (INTEL_GEN(dev_priv) >= 11) {
> > + val = I915_READ(PLANE_BUF_CFG(pipe, plane_id));
> > + skl_ddb_entry_init_from_hw(dev_priv, ddb_y, val);
> > + } else {
> > + val = I915_READ(PLANE_BUF_CFG(pipe, plane_id));
> > val2 = I915_READ(PLANE_NV12_BUF_CFG(pipe, plane_id));
> >
> > - skl_ddb_entry_init_from_hw(dev_priv,
> > - &ddb->plane[pipe][plane_id], val2);
> > - skl_ddb_entry_init_from_hw(dev_priv,
> > - &ddb->uv_plane[pipe][plane_id], val);
> > - } else {
> > - skl_ddb_entry_init_from_hw(dev_priv,
> > - &ddb->plane[pipe][plane_id], val);
> > + if (fourcc == DRM_FORMAT_NV12)
> > + swap(val, val2);
> > +
> > + skl_ddb_entry_init_from_hw(dev_priv, ddb_y, val);
> > + skl_ddb_entry_init_from_hw(dev_priv, ddb_uv, val2);
> > }
> > }
> >
> > +void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
> > + struct skl_ddb_entry *ddb_y,
> > + struct skl_ddb_entry *ddb_uv)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > + enum intel_display_power_domain power_domain;
> > + enum pipe pipe = crtc->pipe;
> > + enum plane_id plane_id;
> > +
> > + power_domain = POWER_DOMAIN_PIPE(pipe);
> > + if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
> > + return;
> > +
> > + for_each_plane_id_on_crtc(crtc, plane_id)
> > + skl_ddb_get_hw_plane_state(dev_priv, pipe,
> > + plane_id,
> > + &ddb_y[plane_id],
> > + &ddb_uv[plane_id]);
> > +
> > + intel_display_power_put(dev_priv, power_domain);
> > +}
> > +
> > void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> > struct skl_ddb_allocation *ddb /* out */)
> > {
> > - struct intel_crtc *crtc;
> > -
> > memset(ddb, 0, sizeof(*ddb));
>
> We can drop the memset. The entire structure is only 1 byte now, and we
> set its only remaining field on the next line.
Ack.
>
> >
> > ddb->enabled_slices = intel_enabled_dbuf_slices_num(dev_priv);
> > -
> > - for_each_intel_crtc(&dev_priv->drm, crtc) {
> > - enum intel_display_power_domain power_domain;
> > - enum plane_id plane_id;
> > - enum pipe pipe = crtc->pipe;
> > -
> > - power_domain = POWER_DOMAIN_PIPE(pipe);
> > - if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
> > - continue;
> > -
> > - for_each_plane_id_on_crtc(crtc, plane_id)
> > - skl_ddb_get_hw_plane_state(dev_priv, pipe,
> > - plane_id, ddb);
> > -
> > - intel_display_power_put(dev_priv, power_domain);
> > - }
> > }
> >
> > /*
> > @@ -4371,7 +4373,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > struct drm_crtc *crtc = cstate->base.crtc;
> > struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > - enum pipe pipe = intel_crtc->pipe;
> > struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> > uint16_t alloc_size, start;
> > uint16_t minimum[I915_MAX_PLANES] = {};
> > @@ -4384,8 +4385,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > uint16_t total_min_blocks = 0;
> >
> > /* Clear the partitioning for disabled planes. */
> > - memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> > - memset(ddb->uv_plane[pipe], 0, sizeof(ddb->uv_plane[pipe]));
> > + memset(cstate->wm.skl.plane_ddb_y, 0, sizeof(cstate->wm.skl.plane_ddb_y));
> > + memset(cstate->wm.skl.plane_ddb_uv, 0, sizeof(cstate->wm.skl.plane_ddb_uv));
> >
> > if (WARN_ON(!state))
> > return 0;
> > @@ -4432,8 +4433,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > }
> >
> > alloc_size -= total_min_blocks;
> > - ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
> > - ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> > + cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
> > + cstate->wm.skl.plane_ddb_y[PLANE_CURSOR].end = alloc->end;
> >
> > /*
> > * 2. Distribute the remaining space in proportion to the amount of
> > @@ -4464,8 +4465,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> >
> > /* Leave disabled planes at (0,0) */
> > if (data_rate) {
> > - ddb->plane[pipe][plane_id].start = start;
> > - ddb->plane[pipe][plane_id].end = start + plane_blocks;
> > + cstate->wm.skl.plane_ddb_y[plane_id].start = start;
> > + cstate->wm.skl.plane_ddb_y[plane_id].end = start + plane_blocks;
> > }
> >
> > start += plane_blocks;
> > @@ -4480,8 +4481,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_plane_blocks);
> >
> > if (uv_data_rate) {
> > - ddb->uv_plane[pipe][plane_id].start = start;
> > - ddb->uv_plane[pipe][plane_id].end =
> > + cstate->wm.skl.plane_ddb_uv[plane_id].start = start;
> > + cstate->wm.skl.plane_ddb_uv[plane_id].end =
> > start + uv_plane_blocks;
> > }
> >
> > @@ -4939,16 +4940,13 @@ static void skl_compute_transition_wm(const struct intel_crtc_state *cstate,
> > }
> > }
> >
> > -static int skl_build_plane_wm_single(struct skl_ddb_allocation *ddb,
> > - struct intel_crtc_state *crtc_state,
> > +static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *plane_state,
> > enum plane_id plane_id, int color_plane)
> > {
> > - struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> > + u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_y[plane_id]);
> > struct skl_wm_params wm_params;
> > - enum pipe pipe = plane->pipe;
> > - uint16_t ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
> > int ret;
> >
> > ret = skl_compute_plane_wm_params(crtc_state, plane_state,
> > @@ -4966,16 +4964,13 @@ static int skl_build_plane_wm_single(struct skl_ddb_allocation *ddb,
> > return 0;
> > }
> >
> > -static int skl_build_plane_wm_uv(struct skl_ddb_allocation *ddb,
> > - struct intel_crtc_state *crtc_state,
> > +static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *plane_state,
> > enum plane_id plane_id)
> > {
> > - struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> > + u16 ddb_blocks = skl_ddb_entry_size(&crtc_state->wm.skl.plane_ddb_uv[plane_id]);
> > struct skl_wm_params wm_params;
> > - enum pipe pipe = plane->pipe;
> > - uint16_t ddb_blocks = skl_ddb_entry_size(&ddb->uv_plane[pipe][plane_id]);
> > int ret;
> >
> > wm->is_planar = true;
> > @@ -4994,8 +4989,7 @@ static int skl_build_plane_wm_uv(struct skl_ddb_allocation *ddb,
> > return 0;
> > }
> >
> > -static int skl_build_plane_wm(struct skl_ddb_allocation *ddb,
> > - struct skl_pipe_wm *pipe_wm,
> > +static int skl_build_plane_wm(struct skl_pipe_wm *pipe_wm,
> > struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *plane_state)
> > {
> > @@ -5007,13 +5001,13 @@ static int skl_build_plane_wm(struct skl_ddb_allocation *ddb,
> > if (!intel_wm_plane_visible(crtc_state, plane_state))
> > return 0;
> >
> > - ret = skl_build_plane_wm_single(ddb, crtc_state, plane_state,
> > + ret = skl_build_plane_wm_single(crtc_state, plane_state,
> > plane_id, 0);
> > if (ret)
> > return ret;
> >
> > if (fb->format->is_yuv && fb->format->num_planes > 1) {
> > - ret = skl_build_plane_wm_uv(ddb, crtc_state, plane_state,
> > + ret = skl_build_plane_wm_uv(crtc_state, plane_state,
> > plane_id);
> > if (ret)
> > return ret;
> > @@ -5022,8 +5016,7 @@ static int skl_build_plane_wm(struct skl_ddb_allocation *ddb,
> > return 0;
> > }
> >
> > -static int icl_build_plane_wm(struct skl_ddb_allocation *ddb,
> > - struct skl_pipe_wm *pipe_wm,
> > +static int icl_build_plane_wm(struct skl_pipe_wm *pipe_wm,
> > struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *plane_state)
> > {
> > @@ -5041,17 +5034,17 @@ static int icl_build_plane_wm(struct skl_ddb_allocation *ddb,
> > WARN_ON(!fb->format->is_yuv ||
> > fb->format->num_planes == 1);
> >
> > - ret = skl_build_plane_wm_single(ddb, crtc_state, plane_state,
> > + ret = skl_build_plane_wm_single(crtc_state, plane_state,
> > y_plane_id, 0);
> > if (ret)
> > return ret;
> >
> > - ret = skl_build_plane_wm_single(ddb, crtc_state, plane_state,
> > + ret = skl_build_plane_wm_single(crtc_state, plane_state,
> > plane_id, 1);
> > if (ret)
> > return ret;
> > } else if (intel_wm_plane_visible(crtc_state, plane_state)) {
> > - ret = skl_build_plane_wm_single(ddb, crtc_state, plane_state,
> > + ret = skl_build_plane_wm_single(crtc_state, plane_state,
> > plane_id, 0);
> > if (ret)
> > return ret;
> > @@ -5061,7 +5054,6 @@ static int icl_build_plane_wm(struct skl_ddb_allocation *ddb,
> > }
> >
> > static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> > - struct skl_ddb_allocation *ddb,
> > struct skl_pipe_wm *pipe_wm)
> > {
> > struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> > @@ -5081,10 +5073,10 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> > to_intel_plane_state(pstate);
> >
> > if (INTEL_GEN(dev_priv) >= 11)
> > - ret = icl_build_plane_wm(ddb, pipe_wm,
> > + ret = icl_build_plane_wm(pipe_wm,
> > cstate, intel_pstate);
> > else
> > - ret = skl_build_plane_wm(ddb, pipe_wm,
> > + ret = skl_build_plane_wm(pipe_wm,
> > cstate, intel_pstate);
> > if (ret)
> > return ret;
> > @@ -5100,9 +5092,9 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
> > const struct skl_ddb_entry *entry)
> > {
> > if (entry->end)
> > - I915_WRITE(reg, (entry->end - 1) << 16 | entry->start);
> > + I915_WRITE_FW(reg, (entry->end - 1) << 16 | entry->start);
> > else
> > - I915_WRITE(reg, 0);
> > + I915_WRITE_FW(reg, 0);
> > }
> >
> > static void skl_write_wm_level(struct drm_i915_private *dev_priv,
> > @@ -5117,19 +5109,22 @@ static void skl_write_wm_level(struct drm_i915_private *dev_priv,
> > val |= level->plane_res_l << PLANE_WM_LINES_SHIFT;
> > }
> >
> > - I915_WRITE(reg, val);
> > + I915_WRITE_FW(reg, val);
> > }
> >
> > -static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> > - const struct skl_plane_wm *wm,
> > - const struct skl_ddb_allocation *ddb,
> > - enum plane_id plane_id)
> > +void skl_write_plane_wm(struct intel_plane *plane,
> > + const struct intel_crtc_state *crtc_state)
> > {
> > - struct drm_crtc *crtc = &intel_crtc->base;
> > - struct drm_device *dev = crtc->dev;
> > - struct drm_i915_private *dev_priv = to_i915(dev);
> > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > int level, max_level = ilk_wm_max_level(dev_priv);
> > - enum pipe pipe = intel_crtc->pipe;
> > + enum plane_id plane_id = plane->id;
> > + enum pipe pipe = plane->pipe;
> > + const struct skl_plane_wm *wm =
> > + &crtc_state->wm.skl.optimal.planes[plane_id];
> > + const struct skl_ddb_entry *ddb_y =
> > + &crtc_state->wm.skl.plane_ddb_y[plane_id];
> > + const struct skl_ddb_entry *ddb_uv =
> > + &crtc_state->wm.skl.plane_ddb_uv[plane_id];
> >
> > for (level = 0; level <= max_level; level++) {
> > skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level),
> > @@ -5138,29 +5133,32 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> > skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
> > &wm->trans_wm);
> >
> > - if (wm->is_planar && INTEL_GEN(dev_priv) < 11) {
> > - skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> > - &ddb->uv_plane[pipe][plane_id]);
> > + if (INTEL_GEN(dev_priv) >= 11) {
> > skl_ddb_entry_write(dev_priv,
> > - PLANE_NV12_BUF_CFG(pipe, plane_id),
> > - &ddb->plane[pipe][plane_id]);
> > - } else {
> > - skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> > - &ddb->plane[pipe][plane_id]);
> > - if (INTEL_GEN(dev_priv) < 11)
> > - I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
> > + PLANE_BUF_CFG(pipe, plane_id), ddb_y);
> > + return;
> > }
> > +
> > + if (wm->is_planar)
> > + swap(ddb_y, ddb_uv);
>
> As noted above, I understand why this is, but it sure looks confusing at
> a casual glance...
>
>
> Matt
>
> > +
> > + skl_ddb_entry_write(dev_priv,
> > + PLANE_BUF_CFG(pipe, plane_id), ddb_y);
> > + skl_ddb_entry_write(dev_priv,
> > + PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_uv);
> > }
> >
> > -static void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > - const struct skl_plane_wm *wm,
> > - const struct skl_ddb_allocation *ddb)
> > +void skl_write_cursor_wm(struct intel_plane *plane,
> > + const struct intel_crtc_state *crtc_state)
> > {
> > - struct drm_crtc *crtc = &intel_crtc->base;
> > - struct drm_device *dev = crtc->dev;
> > - struct drm_i915_private *dev_priv = to_i915(dev);
> > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > int level, max_level = ilk_wm_max_level(dev_priv);
> > - enum pipe pipe = intel_crtc->pipe;
> > + enum plane_id plane_id = plane->id;
> > + enum pipe pipe = plane->pipe;
> > + const struct skl_plane_wm *wm =
> > + &crtc_state->wm.skl.optimal.planes[plane_id];
> > + const struct skl_ddb_entry *ddb =
> > + &crtc_state->wm.skl.plane_ddb_y[plane_id];
> >
> > for (level = 0; level <= max_level; level++) {
> > skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
> > @@ -5168,8 +5166,7 @@ static void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > }
> > skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm->trans_wm);
> >
> > - skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> > - &ddb->plane[pipe][PLANE_CURSOR]);
> > + skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe), ddb);
> > }
> >
> > bool skl_wm_level_equals(const struct skl_wm_level *l1,
> > @@ -5210,13 +5207,12 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb,
> > static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
> > const struct skl_pipe_wm *old_pipe_wm,
> > struct skl_pipe_wm *pipe_wm, /* out */
> > - struct skl_ddb_allocation *ddb, /* out */
> > bool *changed /* out */)
> > {
> > struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
> > int ret;
> >
> > - ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
> > + ret = skl_build_pipe_wm(intel_cstate, pipe_wm);
> > if (ret)
> > return ret;
> >
> > @@ -5242,42 +5238,29 @@ pipes_modified(struct drm_atomic_state *state)
> > }
> >
> > static int
> > -skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
> > +skl_ddb_add_affected_planes(const struct intel_crtc_state *old_crtc_state,
> > + struct intel_crtc_state *new_crtc_state)
> > {
> > - struct drm_atomic_state *state = cstate->base.state;
> > - struct drm_device *dev = state->dev;
> > - struct drm_crtc *crtc = cstate->base.crtc;
> > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > - struct drm_i915_private *dev_priv = to_i915(dev);
> > - struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > - struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
> > - struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> > - struct drm_plane *plane;
> > - enum pipe pipe = intel_crtc->pipe;
> > + struct intel_atomic_state *state = to_intel_atomic_state(new_crtc_state->base.state);
> > + struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > + struct intel_plane *plane;
> >
> > - drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
> > - struct drm_plane_state *plane_state;
> > - struct intel_plane *linked;
> > - enum plane_id plane_id = to_intel_plane(plane)->id;
> > + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> > + struct intel_plane_state *plane_state;
> > + enum plane_id plane_id = plane->id;
> >
> > - if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id],
> > - &new_ddb->plane[pipe][plane_id]) &&
> > - skl_ddb_entry_equal(&cur_ddb->uv_plane[pipe][plane_id],
> > - &new_ddb->uv_plane[pipe][plane_id]))
> > + if (skl_ddb_entry_equal(&old_crtc_state->wm.skl.plane_ddb_y[plane_id],
> > + &new_crtc_state->wm.skl.plane_ddb_y[plane_id]) &&
> > + skl_ddb_entry_equal(&old_crtc_state->wm.skl.plane_ddb_uv[plane_id],
> > + &new_crtc_state->wm.skl.plane_ddb_uv[plane_id]))
> > continue;
> >
> > - plane_state = drm_atomic_get_plane_state(state, plane);
> > + plane_state = intel_atomic_get_plane_state(state, plane);
> > if (IS_ERR(plane_state))
> > return PTR_ERR(plane_state);
> >
> > - /* Make sure linked plane is updated too */
> > - linked = to_intel_plane_state(plane_state)->linked_plane;
> > - if (!linked)
> > - continue;
> > -
> > - plane_state = drm_atomic_get_plane_state(state, &linked->base);
> > - if (IS_ERR(plane_state))
> > - return PTR_ERR(plane_state);
> > + new_crtc_state->update_planes |= BIT(plane_id);
> > }
> >
> > return 0;
> > @@ -5289,18 +5272,21 @@ skl_compute_ddb(struct drm_atomic_state *state)
> > const struct drm_i915_private *dev_priv = to_i915(state->dev);
> > struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
> > + struct intel_crtc_state *old_crtc_state;
> > + struct intel_crtc_state *new_crtc_state;
> > struct intel_crtc *crtc;
> > - struct intel_crtc_state *cstate;
> > int ret, i;
> >
> > memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb));
> >
> > - for_each_new_intel_crtc_in_state(intel_state, crtc, cstate, i) {
> > - ret = skl_allocate_pipe_ddb(cstate, ddb);
> > + for_each_oldnew_intel_crtc_in_state(intel_state, crtc, old_crtc_state,
> > + new_crtc_state, i) {
> > + ret = skl_allocate_pipe_ddb(new_crtc_state, ddb);
> > if (ret)
> > return ret;
> >
> > - ret = skl_ddb_add_affected_planes(cstate);
> > + ret = skl_ddb_add_affected_planes(old_crtc_state,
> > + new_crtc_state);
> > if (ret)
> > return ret;
> > }
> > @@ -5309,36 +5295,29 @@ skl_compute_ddb(struct drm_atomic_state *state)
> > }
> >
> > static void
> > -skl_print_wm_changes(const struct drm_atomic_state *state)
> > +skl_print_wm_changes(struct intel_atomic_state *state)
> > {
> > - const struct drm_device *dev = state->dev;
> > - const struct drm_i915_private *dev_priv = to_i915(dev);
> > - const struct intel_atomic_state *intel_state =
> > - to_intel_atomic_state(state);
> > - const struct drm_crtc *crtc;
> > - const struct drm_crtc_state *cstate;
> > - const struct intel_plane *intel_plane;
> > - const struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb;
> > - const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > + const struct intel_crtc_state *old_crtc_state;
> > + const struct intel_crtc_state *new_crtc_state;
> > + struct intel_plane *plane;
> > + struct intel_crtc *crtc;
> > int i;
> >
> > - for_each_new_crtc_in_state(state, crtc, cstate, i) {
> > - const struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > - enum pipe pipe = intel_crtc->pipe;
> > -
> > - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> > - enum plane_id plane_id = intel_plane->id;
> > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > + new_crtc_state, i) {
> > + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> > + enum plane_id plane_id = plane->id;
> > const struct skl_ddb_entry *old, *new;
> >
> > - old = &old_ddb->plane[pipe][plane_id];
> > - new = &new_ddb->plane[pipe][plane_id];
> > + old = &old_crtc_state->wm.skl.plane_ddb_y[plane_id];
> > + new = &new_crtc_state->wm.skl.plane_ddb_y[plane_id];
> >
> > if (skl_ddb_entry_equal(old, new))
> > continue;
> >
> > DRM_DEBUG_KMS("[PLANE:%d:%s] ddb (%d - %d) -> (%d - %d)\n",
> > - intel_plane->base.base.id,
> > - intel_plane->base.name,
> > + plane->base.base.id, plane->base.name,
> > old->start, old->end,
> > new->start, new->end);
> > }
> > @@ -5474,8 +5453,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> > &to_intel_crtc_state(crtc->state)->wm.skl.optimal;
> >
> > pipe_wm = &intel_cstate->wm.skl.optimal;
> > - ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
> > - &results->ddb, &changed);
> > + ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm, &changed);
> > if (ret)
> > return ret;
> >
> > @@ -5489,7 +5467,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> > intel_cstate->update_wm_pre = true;
> > }
> >
> > - skl_print_wm_changes(state);
> > + skl_print_wm_changes(intel_state);
> >
> > return 0;
> > }
> > @@ -5500,23 +5478,12 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
> > struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> > - const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
> > enum pipe pipe = crtc->pipe;
> > - enum plane_id plane_id;
> >
> > if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
> > return;
> >
> > I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> > -
> > - for_each_plane_id_on_crtc(crtc, plane_id) {
> > - if (plane_id != PLANE_CURSOR)
> > - skl_write_plane_wm(crtc, &pipe_wm->planes[plane_id],
> > - ddb, plane_id);
> > - else
> > - skl_write_cursor_wm(crtc, &pipe_wm->planes[plane_id],
> > - ddb);
> > - }
> > }
> >
> > static void skl_initial_wm(struct intel_atomic_state *state,
> > @@ -5526,8 +5493,6 @@ static void skl_initial_wm(struct intel_atomic_state *state,
> > struct drm_device *dev = intel_crtc->base.dev;
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > struct skl_ddb_values *results = &state->wm_results;
> > - struct skl_ddb_values *hw_vals = &dev_priv->wm.skl_hw;
> > - enum pipe pipe = intel_crtc->pipe;
> >
> > if ((results->dirty_pipes & drm_crtc_mask(&intel_crtc->base)) == 0)
> > return;
> > @@ -5537,11 +5502,6 @@ static void skl_initial_wm(struct intel_atomic_state *state,
> > if (cstate->base.active_changed)
> > skl_atomic_update_crtc_wm(state, cstate);
> >
> > - memcpy(hw_vals->ddb.uv_plane[pipe], results->ddb.uv_plane[pipe],
> > - sizeof(hw_vals->ddb.uv_plane[pipe]));
> > - memcpy(hw_vals->ddb.plane[pipe], results->ddb.plane[pipe],
> > - sizeof(hw_vals->ddb.plane[pipe]));
> > -
> > mutex_unlock(&dev_priv->wm.wm_mutex);
> > }
> >
> > @@ -5692,13 +5652,6 @@ void skl_wm_get_hw_state(struct drm_device *dev)
> > if (dev_priv->active_crtcs) {
> > /* Fully recompute DDB on first atomic commit */
> > dev_priv->wm.distrust_bios_wm = true;
> > - } else {
> > - /*
> > - * Easy/common case; just sanitize DDB now if everything off
> > - * Keep dbuf slice info intact
> > - */
> > - memset(ddb->plane, 0, sizeof(ddb->plane));
> > - memset(ddb->uv_plane, 0, sizeof(ddb->uv_plane));
> > }
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 74d904a49bf9..0262159e7084 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -542,6 +542,8 @@ skl_program_plane(struct intel_plane *plane,
> > if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> > icl_program_input_csc_coeff(crtc_state, plane_state);
> >
> > + skl_write_plane_wm(plane, crtc_state);
> > +
> > I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> > I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
> > I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
> > @@ -604,6 +606,8 @@ skl_disable_plane(struct intel_plane *plane,
> >
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > + skl_write_plane_wm(plane, crtc_state);
> > +
> > I915_WRITE_FW(PLANE_CTL(pipe, plane_id), 0);
> > I915_WRITE_FW(PLANE_SURF(pipe, plane_id), 0);
> >
> > --
> > 2.18.1
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
