[Intel-gfx] [RFC 7/8] drm/i915: Allow final wm programming to be scheduled after next vblank (v2)
Daniel Vetter
daniel at ffwll.ch
Mon Jul 6 02:07:52 PDT 2015
On Wed, Jul 01, 2015 at 07:26:00PM -0700, Matt Roper wrote:
> Add a simple mechanism to trigger final watermark updates in an
> asynchronous manner once the next vblank occurs. No platform types
> actually support atomic watermark programming until a future patch, so
> there should be no functional change yet; individual platforms will be
> converted to use this mechanism one-by-one in future patches.
>
> Note that we'll probably expand this to cover other post-vblank async
> tasks (like unpinning) at some point in the future.
>
> v2: Much simpler vblank mechanism than was used in the previous series;
> no need to allocate new heap structures.
>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
> drivers/gpu/drm/i915/i915_irq.c | 9 +++++++++
> drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_drv.h | 4 ++++
> 4 files changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2774976..5ad942e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -628,6 +628,7 @@ struct drm_i915_display_funcs {
> struct drm_crtc *crtc,
> uint32_t sprite_width, uint32_t sprite_height,
> int pixel_size, bool enable, bool scaled);
> + void (*program_watermarks)(struct drm_i915_private *dev_priv);
> int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
> /* Returns the active state of the crtc, and if the crtc is active,
> @@ -2567,6 +2568,12 @@ struct drm_i915_cmd_table {
> #define HAS_L3_DPF(dev) (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
> #define NUM_L3_SLICES(dev) (IS_HSW_GT3(dev) ? 2 : HAS_L3_DPF(dev))
>
> +/*
> + * FIXME: Not all platforms have been transitioned to atomic watermark
> + * updates yet.
> + */
> +#define HAS_ATOMIC_WM(dev_priv) (dev_priv->display.program_watermarks != NULL)
HAS_FOO is generally hw features. I think especially for just this vfunc
check is clearer to inline it.
> +
> #define GT_FREQUENCY_MULTIPLIER 50
> #define GEN9_FREQ_SCALER 3
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a6fbe64..20c7260 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1452,6 +1452,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>
> static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> {
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> + if (intel_crtc->need_vblank_wm_update) {
> + queue_work(dev_priv->wq, &intel_crtc->wm_work);
> + intel_crtc->need_vblank_wm_update = false;
We need some lock or some other means of sync to be able to cancel such an
update if userspace submits the next atomic update. Otherwise this work
might overwrite the intermediate wm values.
Imo best would be if we createa a cancel_work_sync-like interface which
ensures the work will be cancelled (when it's already scheduled) or won't
ever get scheduled through our vblank handler. That would mirror the
interface I have in mind for generic vblank workers, which imo should
closely resemble the interface we have for delayed_work (for vblank work
run in process context) and timers (for vblank run from hardirq context).
> + }
> +
> if (!drm_handle_vblank(dev, pipe))
> return false;
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 46b62cc..fa4373e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4737,6 +4737,10 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
> struct drm_device *dev = crtc->base.dev;
> struct drm_plane *plane;
>
> + if (HAS_ATOMIC_WM(to_i915(dev)))
> + /* vblank handler will kick off workqueue task to update wm's */
> + crtc->need_vblank_wm_update = true;
> +
> if (atomic->wait_vblank)
> intel_wait_for_vblank(dev, crtc->pipe);
>
> @@ -4745,7 +4749,7 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
> if (atomic->disable_cxsr)
> cstate->wm.cxsr_allowed = true;
>
> - if (crtc->atomic.update_wm_post)
> + if (!HAS_ATOMIC_WM(to_i915(dev)) && crtc->atomic.update_wm_post)
> intel_update_watermarks(&crtc->base);
>
> if (atomic->update_fbc) {
> @@ -4757,9 +4761,10 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
> if (atomic->post_enable_primary)
> intel_post_enable_primary(&crtc->base);
>
> - drm_for_each_plane_mask(plane, dev, atomic->update_sprite_watermarks)
> - intel_update_sprite_watermarks(plane, &crtc->base,
> - 0, 0, 0, false, false);
> + if (!HAS_ATOMIC_WM(to_i915(dev)))
> + drm_for_each_plane_mask(plane, dev, atomic->update_sprite_watermarks)
> + intel_update_sprite_watermarks(plane, &crtc->base,
> + 0, 0, 0, false, false);
Can't we just move this loop into intel_update_watermark after all the
atomic rework and you've just having moved the lp_wm vs. scaling wa
around? One less thing to keep in mind when transitioning.
> memset(atomic, 0, sizeof(*atomic));
> }
> @@ -14070,6 +14075,21 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
> scaler_state->scaler_id = -1;
> }
>
> +/* FIXME: This may expand to cover other tasks like unpinning in the future... */
> +static void wm_work_func(struct work_struct *work)
> +{
> + struct intel_crtc *intel_crtc =
> + container_of(work, struct intel_crtc, wm_work);
> + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> +
> + if (WARN_ON(!HAS_ATOMIC_WM(dev_priv)))
> + return;
> + if (WARN_ON(!intel_crtc->base.state->active))
> + return;
> +
> + dev_priv->display.program_watermarks(dev_priv);
> +}
> +
> static void intel_crtc_init(struct drm_device *dev, int pipe)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -14142,6 +14162,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>
> + INIT_WORK(&intel_crtc->wm_work, wm_work_func);
> +
> drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>
> WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 335b24b..775e3d0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -586,6 +586,10 @@ struct intel_crtc {
> int num_scalers;
>
> struct vlv_wm_state wm_state;
> +
> + /* Do we need to program watermark values after the next vblank? */
> + bool need_vblank_wm_update;
Maybe vblank_wm_update_pending? Since "need" is platform-specific and
invariant at runtime and so a bit confusing (to me at least).
-Daniel
> + struct work_struct wm_work;
> };
>
> struct intel_plane_wm_parameters {
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list