[Intel-gfx] [PATCH v2 09/16] drm/i915: Actually perform the watermark update in two phases
Ville Syrjälä
ville.syrjala at linux.intel.com
Mon Jun 9 20:28:08 CEST 2014
On Tue, Jun 03, 2014 at 07:47:11PM -0300, Paulo Zanoni wrote:
> 2014-05-22 11:48 GMT-03:00 <ville.syrjala at linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > Switch the code over to using the two phase watermark update. The steps
> > generally follow this pattern:
> >
> > 1. Calculate new plane parameters for changed planes
> > 2. Calculate new target and intermediate watermarks
> > 3. Check that both the target and intermediate watermarks are valid
> > 4. Program the hardware with the intermediate watermarks
> > 5. Program the plane registers
> > 6. Arm the vblank watermark update machinery for the next vblank
> > 7. Program the hardware with the target watermarks (after vblank)
> >
> > v2: Rebased, pass intel_crtc around, s/intel_crtc/crtc/
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> This patch is huge. If we bisect any regression to it, we will be
> completely lost and hopeless. Also, my tiny little brain doesn't have
> enough power to do a proper review of this patch with so many changes
> happening all at the same place. I tried, but I gave up in the middle.
>
> Please try to convert this patch into many smaller patches. I don't
> know if the following idea is actually possible, but it is what I
> could extract from what I read of your patch:
> - I'd start with the way you update watermarks parameters. Start by
> patching ilk_compute_wm_parameters() and making it directly call your
> new update_xxx_params() functions. You can even do separate patches
> for _pri, _cur and _spr patches since it seems the _spr code is
> different for most of your patch due to the old
> intel_update_sprite_watermarks() function.
> - Then you change the way you use to set your parameters (there's a
> comment below mentioning the specific line which I'm talking about
> here)
> - Then you can patch intel_display.c so you will only update the WM
> params when you actually change the HW (the _hw_plane and
> update_cursor functions). Again, you can even split _pri, _cur and
> _spr on separate patches.
> - Then you can introduce the update_cursor_wm() and
> update_primary_wm() functions, but still make them call the old-style
> intel_update_watermarks() or something similar.
> - You can also add the functions to deal with intermediate watermarks,
> but without using them. Or you could, for example, make the
> intermediate watermarks just be the same as the "old" ones, so the
> only real update will be the final one (which, I believe, will mean
> the code still behaves as it does today, no regressions).
> - Then you can add the final piece of the code that uses all the new
> infrastructure to actually generate and use the intermediate
> watermarks. And this would be a much smaller patch.
I guess I can try to chunk it up more. But in that case I'll have to
make all the intermediate stages as impotent as possible since no one
will be testing them. So I fear it's going to be more patches that
essentially do nothing.
>
> There are still some comments below:
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 14 ++-
> > drivers/gpu/drm/i915/intel_display.c | 58 ++++++++-
> > drivers/gpu/drm/i915/intel_drv.h | 35 ++++--
> > drivers/gpu/drm/i915/intel_pm.c | 229 +++++++++++++++++++++++++++--------
> > drivers/gpu/drm/i915/intel_sprite.c | 119 ++++++++++++------
> > 5 files changed, 347 insertions(+), 108 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index d4f8ae8..5b1404e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -405,9 +405,12 @@ struct intel_connector;
> > struct intel_crtc_config;
> > struct intel_plane_config;
> > struct intel_crtc;
> > +struct intel_plane;
> > struct intel_limit;
> > struct dpll;
> > struct intel_crtc_wm_config;
> > +struct intel_plane_wm_parameters;
> > +struct intel_crtc_wm_config;
> >
> > struct drm_i915_display_funcs {
> > bool (*fbc_enabled)(struct drm_device *dev);
> > @@ -434,10 +437,13 @@ struct drm_i915_display_funcs {
> > struct dpll *match_clock,
> > struct dpll *best_clock);
> > void (*update_wm)(struct drm_crtc *crtc);
> > - void (*update_sprite_wm)(struct drm_plane *plane,
> > - struct drm_crtc *crtc,
> > - uint32_t sprite_width, int pixel_size,
> > - bool enable, bool scaled);
> > + int (*update_primary_wm)(struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config);
> > + int (*update_cursor_wm)(struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config);
> > + int (*update_sprite_wm)(struct intel_plane *plane,
> > + struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config);
> > void (*program_wm_pre)(struct intel_crtc *crtc,
> > const struct intel_crtc_wm_config *config);
> > void (*program_wm_post)(struct intel_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 408b238..5bf1633 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2078,6 +2078,19 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
> > POSTING_READ(reg);
> > }
> >
> > +static void update_pri_params(struct intel_crtc *crtc,
> > + struct intel_plane_wm_parameters *params,
> > + bool primary_enabled)
> > +{
> > + if (!crtc->active || !primary_enabled)
> > + return;
> > +
> > + params->horiz_pixels = crtc->config.pipe_src_w;
> > + params->bytes_per_pixel =
> > + drm_format_plane_cpp(crtc->base.primary->fb->pixel_format, 0);
> > + params->enabled = true;
> > +}
>
> You add two copies of this function, one here and one at the very end.
Yeah seems I was lazy and copy pasted the thing to the sprite code.
I'll need to give it a proper name and make it non static.
>
> Also, the params->bytes_per_pixel used here is different from the one
> originally used at ilk_compute_wm_parameters(). This should be on a
> separate patch with a nice explanation of the differences between
> both.
>
> > +
> > /**
> > * intel_enable_primary_hw_plane - enable the primary plane on a given pipe
> > * @dev_priv: i915 private structure
> > @@ -2091,6 +2104,8 @@ static void intel_enable_primary_hw_plane(struct drm_i915_private *dev_priv,
> > {
> > struct intel_crtc *intel_crtc =
> > to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > + struct intel_crtc_wm_config config = {};
>
> These temporary structs that get created an assigned really bother me,
> especially in these cases where we are just interested in a sub-field
> of the struct. This probably suggests there is something wrong with
> our abstractions. After all this work is merged, we should probably
> try to get rid of these things.
I think many of the temp structs are an artifact of not having a
plane_config struct or nuclear flip. Once we get those the mess
should hopefully clear a bit.
>
>
> > + int ret;
> > int reg;
> > u32 val;
> >
> > @@ -2100,14 +2115,24 @@ static void intel_enable_primary_hw_plane(struct drm_i915_private *dev_priv,
> > if (intel_crtc->primary_enabled)
> > return;
> >
> > + update_pri_params(intel_crtc, &config.pri, true);
> > +
> > + ret = intel_update_primary_watermarks(intel_crtc, &config);
> > + WARN(ret, "primary watermarks invalid\n");
> > +
> > intel_crtc->primary_enabled = true;
> >
> > reg = DSPCNTR(plane);
> > val = I915_READ(reg);
> > WARN_ON(val & DISPLAY_PLANE_ENABLE);
> >
> > + intel_crtc->pri_wm = config.pri;
> > + intel_program_watermarks_pre(intel_crtc, &config);
> > +
> > I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
> > intel_flush_primary_plane(dev_priv, plane);
> > +
> > + intel_program_watermarks_post(intel_crtc, &config);
> > }
> >
> > /**
> > @@ -2123,20 +2148,32 @@ static void intel_disable_primary_hw_plane(struct drm_i915_private *dev_priv,
> > {
> > struct intel_crtc *intel_crtc =
> > to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > + struct intel_crtc_wm_config config = {};
> > + int ret;
> > int reg;
> > u32 val;
> >
> > if (!intel_crtc->primary_enabled)
> > return;
> >
> > + update_pri_params(intel_crtc, &config.pri, false);
> > +
> > + ret = intel_update_primary_watermarks(intel_crtc, &config);
> > + WARN(ret, "primary watermarks invalid\n");
> > +
> > intel_crtc->primary_enabled = false;
> >
> > reg = DSPCNTR(plane);
> > val = I915_READ(reg);
> > WARN_ON((val & DISPLAY_PLANE_ENABLE) == 0);
> >
> > + intel_crtc->pri_wm = config.pri;
> > + intel_program_watermarks_pre(intel_crtc, &config);
> > +
> > I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
> > intel_flush_primary_plane(dev_priv, plane);
> > +
> > + intel_program_watermarks_post(intel_crtc, &config);
> > }
> >
> > static bool need_vtd_wa(struct drm_device *dev)
> > @@ -3989,7 +4026,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> > */
> > intel_crtc_load_lut(crtc);
> >
> > - intel_update_watermarks(crtc);
> > intel_enable_pipe(intel_crtc);
> >
> > if (intel_crtc->config.has_pch_encoder)
> > @@ -4100,7 +4136,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> > intel_ddi_set_pipe_settings(crtc);
> > intel_ddi_enable_transcoder_func(crtc);
> >
> > - intel_update_watermarks(crtc);
> > intel_enable_pipe(intel_crtc);
> >
> > if (intel_crtc->config.has_pch_encoder)
> > @@ -4188,7 +4223,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> > }
> >
> > intel_crtc->active = false;
> > - intel_update_watermarks(crtc);
> >
> > mutex_lock(&dev->struct_mutex);
> > intel_update_fbc(dev);
> > @@ -4236,7 +4270,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> > }
> >
> > intel_crtc->active = false;
> > - intel_update_watermarks(crtc);
> >
> > mutex_lock(&dev->struct_mutex);
> > intel_update_fbc(dev);
> > @@ -7985,11 +8018,13 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> > struct drm_device *dev = crtc->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > + struct intel_crtc_wm_config config = {};
> > int pipe = intel_crtc->pipe;
> > int x = intel_crtc->cursor_x;
> > int y = intel_crtc->cursor_y;
> > u32 base = 0, pos = 0;
> > bool visible;
> > + int ret;
> >
> > if (on)
> > base = intel_crtc->cursor_addr;
> > @@ -8022,6 +8057,19 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> > if (!visible && !intel_crtc->cursor_visible)
> > return;
> >
> > + if (visible) {
> > + /* FIXME should we use the clipped width? */
> > + config.cur.horiz_pixels = intel_crtc->cursor_width;
> > + config.cur.bytes_per_pixel = 4;
> > + config.cur.enabled = true;
> > + }
> > +
> > + ret = intel_update_cursor_watermarks(intel_crtc, &config);
> > + WARN(ret, "cursor watermarks invalid\n");
> > +
> > + intel_crtc->cur_wm = config.cur;
> > + intel_program_watermarks_pre(intel_crtc, &config);
> > +
> > I915_WRITE(CURPOS(pipe), pos);
> >
> > if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
> > @@ -8030,6 +8078,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> > i845_update_cursor(crtc, base);
> > else
> > i9xx_update_cursor(crtc, base);
> > +
> > + intel_program_watermarks_post(intel_crtc, &config);
> > }
> >
> > static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 4b59be3..1ec4379 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -358,7 +358,15 @@ struct intel_pipe_wm {
> > bool sprites_scaled;
> > };
> >
> > +struct intel_plane_wm_parameters {
> > + uint32_t horiz_pixels;
> > + uint8_t bytes_per_pixel;
> > + bool enabled;
> > + bool scaled;
> > +};
> > +
> > struct intel_crtc_wm_config {
> > + struct intel_plane_wm_parameters pri, spr, cur;
> > /* target watermarks for the pipe */
> > struct intel_pipe_wm target;
> > /* intermediate watermarks for pending/active->target transition */
> > @@ -444,18 +452,14 @@ struct intel_crtc {
> > spinlock_t lock;
> > } wm;
> >
> > + struct intel_plane_wm_parameters pri_wm;
> > + struct intel_plane_wm_parameters cur_wm;
>
> Why isn't this inside "struct wm"?
Eventually these should migrate to plane_config, so I like them sticking
out a bit in the meantime.
>
> Also please add _params to the name. Every new patch adds a different
> WM struct with a similar name, I am confused.
>
> > +
> > wait_queue_head_t vbl_wait;
> >
> > int scanline_offset;
> > };
> >
> > -struct intel_plane_wm_parameters {
> > - uint32_t horiz_pixels;
> > - uint8_t bytes_per_pixel;
> > - bool enabled;
> > - bool scaled;
> > -};
> > -
> > struct intel_plane {
> > struct drm_plane base;
> > int plane;
> > @@ -483,9 +487,11 @@ struct intel_plane {
> > int crtc_x, int crtc_y,
> > unsigned int crtc_w, unsigned int crtc_h,
> > uint32_t x, uint32_t y,
> > - uint32_t src_w, uint32_t src_h);
> > + uint32_t src_w, uint32_t src_h,
> > + const struct intel_crtc_wm_config *config);
> > void (*disable_plane)(struct drm_plane *plane,
> > - struct drm_crtc *crtc);
> > + struct drm_crtc *crtc,
> > + const struct intel_crtc_wm_config *config);
> > int (*update_colorkey)(struct drm_plane *plane,
> > struct drm_intel_sprite_colorkey *key);
> > void (*get_colorkey)(struct drm_plane *plane,
> > @@ -969,10 +975,13 @@ void intel_init_clock_gating(struct drm_device *dev);
> > void intel_suspend_hw(struct drm_device *dev);
> > int ilk_wm_max_level(const struct drm_device *dev);
> > void intel_update_watermarks(struct drm_crtc *crtc);
> > -void intel_update_sprite_watermarks(struct drm_plane *plane,
> > - struct drm_crtc *crtc,
> > - uint32_t sprite_width, int pixel_size,
> > - bool enabled, bool scaled);
> > +int intel_update_cursor_watermarks(struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config);
> > +int intel_update_primary_watermarks(struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config);
> > +int intel_update_sprite_watermarks(struct intel_plane *plane,
> > + struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config);
> > void intel_init_pm(struct drm_device *dev);
> > void intel_pm_setup(struct drm_device *dev);
> > bool intel_fbc_enabled(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ccf920a..13366b7 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2186,13 +2186,9 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
> > p->active = true;
> > p->pipe_htotal = intel_crtc->config.adjusted_mode.crtc_htotal;
> > p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> > - p->pri.bytes_per_pixel = crtc->primary->fb->bits_per_pixel / 8;
> > - p->cur.bytes_per_pixel = 4;
> > - p->pri.horiz_pixels = intel_crtc->config.pipe_src_w;
> > - p->cur.horiz_pixels = intel_crtc->cursor_width;
> > - /* TODO: for now, assume primary and cursor planes are always enabled. */
> > - p->pri.enabled = true;
> > - p->cur.enabled = true;
> > +
> > + p->pri = intel_crtc->pri_wm;
> > + p->cur = intel_crtc->cur_wm;
> >
> > drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
> > struct intel_plane *intel_plane = to_intel_plane(plane);
> > @@ -2292,6 +2288,35 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > }
> >
> > /*
> > + * Merge two pipe watermark sets.
> > + * Used for computing intermediate watermark levels
> > + * for transitioning between two different configurations.
> > + */
> > +static void ilk_wm_merge_intermediate(struct drm_device *dev,
> > + struct intel_pipe_wm *a,
> > + const struct intel_pipe_wm *b)
> > +{
> > + int level, max_level = ilk_wm_max_level(dev);
> > +
> > + a->pipe_enabled |= b->pipe_enabled;
> > + a->sprites_enabled |= b->sprites_enabled;
> > + a->sprites_scaled |= b->sprites_scaled;
> > +
> > + /* Merge _all_ levels including 0 */
> > + for (level = 0; level <= max_level; level++) {
> > + struct intel_wm_level *a_wm = &a->wm[level];
> > + const struct intel_wm_level *b_wm = &b->wm[level];
> > +
> > + a_wm->enable &= b_wm->enable;
> > +
> > + a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val);
> > + a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val);
> > + a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val);
> > + a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val);
> > + }
> > +}
>
> Maybe this question don't make much sense, but:
> - What if A was WM calculated with one type of paritioning and/or FBC
> enabled, while B was calculated with the other partitioning and/or FBC
> disabled? Can't we reach a case where the watermarks generated by this
> function are insufficient (due to fbc/partitioning/etc) or don't even
> make sense (values we would never generate with the current BSpec
> algorithms)?
This just merges the target/pending/active watermarks for a single pipe,
which don't yet have any notion of fbc or FIFO splits (apart from
primary vs. sprite). The only things we know about those watermarks is
that the values fit in the registers, and that the first level has been
deemed valid.
>
>
> > +
> > +/*
> > * Merge the watermarks from all active pipes for a specific level.
> > */
> > static void ilk_merge_wm_level(struct drm_device *dev,
> > @@ -2844,31 +2869,6 @@ static void ilk_program_watermarks(struct drm_device *dev)
> > ilk_write_wm_values(dev_priv, &results);
> > }
> >
> > -static void ilk_update_wm(struct drm_crtc *crtc)
> > -{
> > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > - struct drm_device *dev = crtc->dev;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct ilk_pipe_wm_parameters params = {};
> > - struct intel_pipe_wm pipe_wm = {};
> > -
> > - ilk_compute_wm_parameters(crtc, ¶ms);
> > -
> > - intel_compute_pipe_wm(crtc, ¶ms, &pipe_wm);
> > -
> > - mutex_lock(&dev_priv->wm.mutex);
> > -
> > - if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
> > - goto unlock;
> > -
> > - intel_crtc->wm.active = pipe_wm;
> > -
> > - ilk_program_watermarks(dev);
> > -
> > - unlock:
> > - mutex_unlock(&dev_priv->wm.mutex);
> > -}
> > -
> > static void ilk_update_watermarks(struct drm_device *dev)
> > {
> > bool changed;
> > @@ -2923,6 +2923,71 @@ static void ilk_setup_pending_watermarks(struct intel_crtc *crtc,
> > spin_unlock_irq(&crtc->wm.lock);
> > }
> >
> > +static int ilk_pipe_compute_watermarks(struct intel_crtc *crtc,
> > + struct intel_pipe_wm *target,
> > + struct intel_pipe_wm *intm)
> > +{
> > + struct drm_device *dev = crtc->base.dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_pipe_wm intm_pending;
> > + bool dirty;
> > +
> > + /* are the target watermarks valid at all? */
> > + if (!ilk_validate_pipe_wm(dev, target))
> > + return -EINVAL;
> > +
> > + /*
> > + * We need to come up with intermediate watermark levels
> > + * that will support both the old and new plane configuration
> > + * since we can't flip over to the final watermarks until
> > + * the plane configuration has been latched at some future vblank.
> > + *
> > + * Additionally if there's already an update pending, we can't
> > + * yet be sure which plane configuration will be active at the
> > + * time we apply the intermediate watermarks, so we must account
> > + * for both possibilities.
> > + */
> > + mutex_lock(&dev_priv->wm.mutex);
> > +
> > + intm_pending = crtc->wm.pending;
> > + *intm = crtc->wm.active;
> > +
> > + spin_lock_irq(&crtc->wm.lock);
> > + dirty = crtc->wm.dirty;
> > + spin_unlock_irq(&crtc->wm.lock);
> > +
> > + mutex_unlock(&dev_priv->wm.mutex);
> > +
> > + /*
> > + * If the intermediate watermarks aren't valid, we must tell the user to
> > + * try something a bit different. There are two cases to be considered.
> > + * 1) there is no pending update:
> > + * If the intermediate watermarks for transitioning from the currently
> > + * active configuration to the new configuration aren't valid, the
> > + * user must choose another configuration as there is no safe way to
> > + * transition from the currently active config to the new config.
>
> Why/how would this happen?
Eg. if there's currently just the primary plane enabled which requires
more than 50% of the LP0 FIFO space available for one pipe, and then
the user wants enable a sprite and disable the primary. Or vice versa.
active: fifo=128 pri=80 spr=0
target: fifo=128 pri=0 spr=40
intermediate: fifo=128 pri=80 spr=40 -> no can do (80>64)
Hmm. Although now I think about it again I guess it shouldn't actually
matter since the FIFO split is automagically changed by the hardware
when the sprite gets enabled. So oversubscribing the intermediate
watermarks like that should be fine. The hardware itself won't be
doing anything wrong as long as the planes go on+off at the same
vblank. But I'd need to revisit the watermark validation code to accept
such configurations, and would need to think a bit more more in case
there is some other scenario that could hit such issues.
Also there is definitely something fishy happening with primary<->sprite
changes. The screen tends to flash black without underruns getting
reported. But IIRC it might be somehow related to the colorkey as well.
I should poke at the hardware again a bit at some point and try to get
to the bottom of this.
>
> In the worst case, the intermediate watermarks would just disable all
> the LP WMs, go back to 1/2 partitioning, disable FBC, and use the
> maximum register values for level 0. Anything that needs more than
> that is an unsupported mode. By the way, we could get completely rid
> of these intermediate WM calculations if we just used these maximum
> values as the intermediate ones: of course, they would be less
> power-efficient than your current code, but maybe the difference
> wouldn't be noticeable.
Computing the intermediate values is the easy part here, so I don't see
much point in not doing it. Saving power is good.
>
> Thanks,
> Paulo
>
> > + * 2) there is a pending update:
> > + * If the intermediate watermarks for transitioning from the ccurrently
> > + * pending configuration to the new configuration are valid, we can
> > + * simply tell the user to try again after a while.
> > + */
> > + if (dirty) {
> > + ilk_wm_merge_intermediate(dev, &intm_pending, target);
> > + if (!ilk_validate_pipe_wm(dev, &intm_pending))
> > + return -EINVAL;
> > +
> > + ilk_wm_merge_intermediate(dev, intm, &intm_pending);
> > + if (!ilk_validate_pipe_wm(dev, intm))
> > + return -EAGAIN;
> > + } else {
> > + ilk_wm_merge_intermediate(dev, intm, target);
> > + if (!ilk_validate_pipe_wm(dev, intm))
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static void ilk_watermark_work(struct work_struct *work)
> > {
> > struct drm_i915_private *dev_priv =
> > @@ -2964,18 +3029,46 @@ static void ilk_wm_cancel(struct intel_crtc *crtc)
> > }
> > }
> >
> > -static void ilk_update_sprite_wm(struct drm_plane *plane,
> > - struct drm_crtc *crtc,
> > - uint32_t sprite_width, int pixel_size,
> > - bool enabled, bool scaled)
> > +static int ilk_update_primary_wm(struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config)
> > +{
> > + struct ilk_pipe_wm_parameters params = {};
> > +
> > + ilk_compute_wm_parameters(&crtc->base, ¶ms);
> > +
> > + params.pri = config->pri;
> > +
> > + intel_compute_pipe_wm(&crtc->base, ¶ms, &config->target);
> > +
> > + return ilk_pipe_compute_watermarks(crtc,
> > + &config->target,
> > + &config->intm);
> > +}
> > +
> > +static int ilk_update_cursor_wm(struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config)
> > +{
> > + struct ilk_pipe_wm_parameters params = {};
> > +
> > + ilk_compute_wm_parameters(&crtc->base, ¶ms);
> > +
> > + params.cur = config->cur;
> > +
> > + intel_compute_pipe_wm(&crtc->base, ¶ms, &config->target);
> > +
> > + return ilk_pipe_compute_watermarks(crtc,
> > + &config->target,
> > + &config->intm);
> > +}
> > +
> > +static int ilk_update_sprite_wm(struct intel_plane *plane,
> > + struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config)
> > {
> > - struct drm_device *dev = plane->dev;
> > - struct intel_plane *intel_plane = to_intel_plane(plane);
> > + struct drm_device *dev = crtc->base.dev;
> > + struct ilk_pipe_wm_parameters params = {};
> >
> > - intel_plane->wm.enabled = enabled;
> > - intel_plane->wm.scaled = scaled;
> > - intel_plane->wm.horiz_pixels = sprite_width;
> > - intel_plane->wm.bytes_per_pixel = pixel_size;
> > + ilk_compute_wm_parameters(&crtc->base, ¶ms);
> >
> > /*
> > * IVB workaround: must disable low power watermarks for at least
> > @@ -2984,10 +3077,17 @@ static void ilk_update_sprite_wm(struct drm_plane *plane,
> > *
> > * WaCxSRDisabledForSpriteScaling:ivb
> > */
> > - if (IS_IVYBRIDGE(dev) && scaled && ilk_disable_lp_wm(dev))
> > - intel_wait_for_vblank(dev, intel_plane->pipe);
> > + if (IS_IVYBRIDGE(dev) && config->spr.scaled && ilk_disable_lp_wm(dev))
> > + intel_wait_for_vblank(dev, plane->pipe);
> > +
> > + params.pri = config->pri;
> > + params.spr = config->spr;
> > +
> > + intel_compute_pipe_wm(&crtc->base, ¶ms, &config->target);
> >
> > - ilk_update_wm(crtc);
> > + return ilk_pipe_compute_watermarks(crtc,
> > + &config->target,
> > + &config->intm);
> > }
> >
> > static void _ilk_pipe_wm_hw_to_sw(struct drm_crtc *crtc)
> > @@ -3086,16 +3186,38 @@ void intel_update_watermarks(struct drm_crtc *crtc)
> > dev_priv->display.update_wm(crtc);
> > }
> >
> > -void intel_update_sprite_watermarks(struct drm_plane *plane,
> > - struct drm_crtc *crtc,
> > - uint32_t sprite_width, int pixel_size,
> > - bool enabled, bool scaled)
> > +int intel_update_primary_watermarks(struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config)
> > {
> > - struct drm_i915_private *dev_priv = plane->dev->dev_private;
> > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +
> > + if (dev_priv->display.update_primary_wm)
> > + return dev_priv->display.update_primary_wm(crtc, config);
> > +
> > + return 0;
> > +}
> > +
> > +int intel_update_cursor_watermarks(struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config)
> > +{
> > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > +
> > + if (dev_priv->display.update_cursor_wm)
> > + return dev_priv->display.update_cursor_wm(crtc, config);
> > +
> > + return 0;
> > +}
> > +
> > +int intel_update_sprite_watermarks(struct intel_plane *plane,
> > + struct intel_crtc *crtc,
> > + struct intel_crtc_wm_config *config)
> > +{
> > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >
> > if (dev_priv->display.update_sprite_wm)
> > - dev_priv->display.update_sprite_wm(plane, crtc, sprite_width,
> > - pixel_size, enabled, scaled);
> > + return dev_priv->display.update_sprite_wm(plane, crtc, config);
> > +
> > + return 0;
> > }
> >
> > void intel_program_watermarks_pre(struct intel_crtc *crtc,
> > @@ -6653,7 +6775,8 @@ void intel_init_pm(struct drm_device *dev)
> > dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) ||
> > (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] &&
> > dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
> > - dev_priv->display.update_wm = ilk_update_wm;
> > + dev_priv->display.update_primary_wm = ilk_update_primary_wm;
> > + dev_priv->display.update_cursor_wm = ilk_update_cursor_wm;
> > dev_priv->display.update_sprite_wm = ilk_update_sprite_wm;
> > dev_priv->display.program_wm_pre = ilk_program_wm_pre;
> > dev_priv->display.program_wm_post = ilk_program_wm_post;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index d6acd6b..c9b1750 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -131,7 +131,7 @@ static void intel_update_primary_plane(struct intel_crtc *crtc)
> > struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > int reg = DSPCNTR(crtc->plane);
> >
> > - if (crtc->primary_enabled)
> > + if (crtc->pri_wm.enabled)
> > I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
> > else
> > I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
> > @@ -143,7 +143,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > struct drm_i915_gem_object *obj, int crtc_x, int crtc_y,
> > unsigned int crtc_w, unsigned int crtc_h,
> > uint32_t x, uint32_t y,
> > - uint32_t src_w, uint32_t src_h)
> > + uint32_t src_w, uint32_t src_h,
> > + const struct intel_crtc_wm_config *config)
> > {
> > struct drm_device *dev = dplane->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -218,9 +219,6 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >
> > sprctl |= SP_ENABLE;
> >
> > - intel_update_sprite_watermarks(dplane, crtc, src_w, pixel_size, true,
> > - src_w != crtc_w || src_h != crtc_h);
> > -
> > /* Sizes are 0 based */
> > src_w--;
> > src_h--;
> > @@ -234,6 +232,10 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > fb->pitches[0]);
> > linear_offset -= sprsurf_offset;
> >
> > + intel_crtc->pri_wm = config->pri;
> > + intel_plane->wm = config->spr;
> > + intel_program_watermarks_pre(intel_crtc, config);
> > +
> > atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> >
> > intel_update_primary_plane(intel_crtc);
> > @@ -255,10 +257,13 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> >
> > if (atomic_update)
> > intel_pipe_update_end(intel_crtc, start_vbl_count);
> > +
> > + intel_program_watermarks_post(intel_crtc, config);
> > }
> >
> > static void
> > -vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > +vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> > + const struct intel_crtc_wm_config *config)
> > {
> > struct drm_device *dev = dplane->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -269,6 +274,10 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > u32 start_vbl_count;
> > bool atomic_update;
> >
> > + intel_crtc->pri_wm = config->pri;
> > + intel_plane->wm = config->spr;
> > + intel_program_watermarks_pre(intel_crtc, config);
> > +
> > atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> >
> > intel_update_primary_plane(intel_crtc);
> > @@ -283,7 +292,7 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> > if (atomic_update)
> > intel_pipe_update_end(intel_crtc, start_vbl_count);
> >
> > - intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
> > + intel_program_watermarks_post(intel_crtc, config);
> > }
> >
> > static int
> > @@ -343,7 +352,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > struct drm_i915_gem_object *obj, int crtc_x, int crtc_y,
> > unsigned int crtc_w, unsigned int crtc_h,
> > uint32_t x, uint32_t y,
> > - uint32_t src_w, uint32_t src_h)
> > + uint32_t src_w, uint32_t src_h,
> > + const struct intel_crtc_wm_config *config)
> > {
> > struct drm_device *dev = plane->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -406,9 +416,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > sprctl |= SPRITE_PIPE_CSC_ENABLE;
> >
> > - intel_update_sprite_watermarks(plane, crtc, src_w, pixel_size, true,
> > - src_w != crtc_w || src_h != crtc_h);
> > -
> > /* Sizes are 0 based */
> > src_w--;
> > src_h--;
> > @@ -424,6 +431,10 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > pixel_size, fb->pitches[0]);
> > linear_offset -= sprsurf_offset;
> >
> > + intel_crtc->pri_wm = config->pri;
> > + intel_plane->wm = config->spr;
> > + intel_program_watermarks_pre(intel_crtc, config);
> > +
> > atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> >
> > intel_update_primary_plane(intel_crtc);
> > @@ -451,10 +462,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >
> > if (atomic_update)
> > intel_pipe_update_end(intel_crtc, start_vbl_count);
> > +
> > + intel_program_watermarks_post(intel_crtc, config);
> > }
> >
> > static void
> > -ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > +ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > + const struct intel_crtc_wm_config *config)
> > {
> > struct drm_device *dev = plane->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -464,6 +478,10 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > u32 start_vbl_count;
> > bool atomic_update;
> >
> > + intel_crtc->pri_wm = config->pri;
> > + intel_plane->wm = config->spr;
> > + intel_program_watermarks_pre(intel_crtc, config);
> > +
> > atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> >
> > intel_update_primary_plane(intel_crtc);
> > @@ -480,13 +498,7 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > if (atomic_update)
> > intel_pipe_update_end(intel_crtc, start_vbl_count);
> >
> > - /*
> > - * Avoid underruns when disabling the sprite.
> > - * FIXME remove once watermark updates are done properly.
> > - */
> > - intel_wait_for_vblank(dev, pipe);
> > -
> > - intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
> > + intel_program_watermarks_post(intel_crtc, config);
> > }
> >
> > static int
> > @@ -549,7 +561,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > struct drm_i915_gem_object *obj, int crtc_x, int crtc_y,
> > unsigned int crtc_w, unsigned int crtc_h,
> > uint32_t x, uint32_t y,
> > - uint32_t src_w, uint32_t src_h)
> > + uint32_t src_w, uint32_t src_h,
> > + const struct intel_crtc_wm_config *config)
> > {
> > struct drm_device *dev = plane->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -606,9 +619,6 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
> > dvscntr |= DVS_ENABLE;
> >
> > - intel_update_sprite_watermarks(plane, crtc, src_w, pixel_size, true,
> > - src_w != crtc_w || src_h != crtc_h);
> > -
> > /* Sizes are 0 based */
> > src_w--;
> > src_h--;
> > @@ -625,6 +635,10 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > pixel_size, fb->pitches[0]);
> > linear_offset -= dvssurf_offset;
> >
> > + intel_crtc->pri_wm = config->pri;
> > + intel_plane->wm = config->spr;
> > + intel_program_watermarks_pre(intel_crtc, config);
> > +
> > atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> >
> > intel_update_primary_plane(intel_crtc);
> > @@ -647,10 +661,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >
> > if (atomic_update)
> > intel_pipe_update_end(intel_crtc, start_vbl_count);
> > +
> > + intel_program_watermarks_post(intel_crtc, config);
> > }
> >
> > static void
> > -ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > +ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > + const struct intel_crtc_wm_config *config)
> > {
> > struct drm_device *dev = plane->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -660,6 +677,10 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > u32 start_vbl_count;
> > bool atomic_update;
> >
> > + intel_crtc->pri_wm = config->pri;
> > + intel_plane->wm = config->spr;
> > + intel_program_watermarks_pre(intel_crtc, config);
> > +
> > atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> >
> > intel_update_primary_plane(intel_crtc);
> > @@ -675,13 +696,7 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> > if (atomic_update)
> > intel_pipe_update_end(intel_crtc, start_vbl_count);
> >
> > - /*
> > - * Avoid underruns when disabling the sprite.
> > - * FIXME remove once watermark updates are done properly.
> > - */
> > - intel_wait_for_vblank(dev, pipe);
> > -
> > - intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
> > + intel_program_watermarks_post(intel_crtc, config);
> > }
> >
> > static void
> > @@ -801,6 +816,19 @@ static bool colorkey_enabled(struct intel_plane *intel_plane)
> > return key.flags != I915_SET_COLORKEY_NONE;
> > }
> >
> > +static void update_pri_params(struct intel_crtc *crtc,
> > + struct intel_plane_wm_parameters *params,
> > + bool primary_enabled)
> > +{
> > + if (!crtc->active || !primary_enabled)
> > + return;
> > +
> > + params->horiz_pixels = crtc->config.pipe_src_w;
> > + params->bytes_per_pixel =
> > + drm_format_plane_cpp(crtc->base.primary->fb->pixel_format, 0);
> > + params->enabled = true;
> > +}
> > +
> > static int
> > intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> > @@ -852,6 +880,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > .src_w = src_w,
> > .src_h = src_h,
> > };
> > + struct intel_crtc_wm_config config = {};
> >
> > /* Don't modify another pipe's plane */
> > if (intel_plane->pipe != intel_crtc->pipe) {
> > @@ -989,6 +1018,19 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > primary_enabled = !drm_rect_equals(&dst, &clip) || colorkey_enabled(intel_plane);
> > WARN_ON(!primary_enabled && !visible && intel_crtc->active);
> >
> > + if (visible) {
> > + config.spr.horiz_pixels = src_w;
> > + config.spr.bytes_per_pixel = pixel_size;
> > + config.spr.enabled = true;
> > + config.spr.scaled = src_w != crtc_w || src_h != crtc_h;
> > + }
> > +
> > + update_pri_params(intel_crtc, &config.pri, primary_enabled);
> > +
> > + ret = intel_update_sprite_watermarks(intel_plane, intel_crtc, &config);
> > + if (ret)
> > + return ret;
> > +
> > mutex_lock(&dev->struct_mutex);
> >
> > /* Note that this will apply the VT-d workaround for scanouts,
> > @@ -1027,9 +1069,10 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > if (visible)
> > intel_plane->update_plane(plane, crtc, fb, obj,
> > crtc_x, crtc_y, crtc_w, crtc_h,
> > - src_x, src_y, src_w, src_h);
> > + src_x, src_y, src_w, src_h,
> > + &config);
> > else
> > - intel_plane->disable_plane(plane, crtc);
> > + intel_plane->disable_plane(plane, crtc, &config);
> >
> > if (!primary_was_enabled && primary_enabled)
> > intel_post_enable_primary(crtc);
> > @@ -1060,6 +1103,8 @@ intel_disable_plane(struct drm_plane *plane)
> > struct drm_device *dev = plane->dev;
> > struct intel_plane *intel_plane = to_intel_plane(plane);
> > struct intel_crtc *intel_crtc;
> > + struct intel_crtc_wm_config config = {};
> > + int ret;
> >
> > if (!plane->fb)
> > return 0;
> > @@ -1069,12 +1114,18 @@ intel_disable_plane(struct drm_plane *plane)
> >
> > intel_crtc = to_intel_crtc(plane->crtc);
> >
> > + update_pri_params(intel_crtc, &config.pri, true);
> > +
> > + ret = intel_update_sprite_watermarks(intel_plane, intel_crtc, &config);
> > + if (ret)
> > + return ret;
> > +
> > if (intel_crtc->active) {
> > bool primary_was_enabled = intel_crtc->primary_enabled;
> >
> > intel_crtc->primary_enabled = true;
> >
> > - intel_plane->disable_plane(plane, plane->crtc);
> > + intel_plane->disable_plane(plane, plane->crtc, &config);
> >
> > if (!primary_was_enabled && intel_crtc->primary_enabled)
> > intel_post_enable_primary(plane->crtc);
> > --
> > 1.8.5.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list