2014-05-22 11:48 GMT-03:00 <ville.syrjala@xxxxxxxxxxxxxxx>:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
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.
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.
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.
> + 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"?
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)?
> +
> +/*
> * 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?
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.
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
