On Mon, Nov 14, 2016 at 06:35:10PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
> actually touching the hardware, in which case we won't force a modeset
> on all the pipes, and thus won't lock any of the other pipes either.
> That means a parallel plane update on another pipe could be looking at
> a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
> plane configuration is invalid, or potentially reject a valid update.
It's a bit a bikeshed, but everywhere else in atomic those kinds of data
are called ->state or something like that. Diverging and giving it an
atomic_ prefix seems a bit funky.
Maybe we need a dev_priv->state substruct where we could put all these
global atomic bits into?
-Daniel
>
> To overcome this we must protect writes to atomic_cdclk_freq with
> all the crtc locks, and thus for reads any single crtc lock will
> be sufficient protection.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 9 +++++++-
> drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
> 2 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0f1dfc7119e..66d2950dc657 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1874,7 +1874,14 @@ struct drm_i915_private {
>
> unsigned int fsb_freq, mem_freq, is_ddr3;
> unsigned int skl_preferred_vco_freq;
> - unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> + unsigned int cdclk_freq, max_cdclk_freq;
> +
> + /*
> + * For reading holding any crtc lock is sufficient,
> + * for writing must hold all of them.
> + */
> + unsigned int atomic_cdclk_freq;
> +
> unsigned int max_dotclk_freq;
> unsigned int rawclk_freq;
> unsigned int hpll_freq;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 70f3f0b70263..d7a4bc63b05b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13946,13 +13946,32 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
> return 0;
> }
>
> +static int intel_lock_all_pipes(struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> +
> + /* Add all pipes to the state */
> + for_each_crtc(state->dev, crtc) {
> + struct drm_crtc_state *crtc_state;
> +
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> + }
> +
> + return 0;
> +}
> +
> static int intel_modeset_all_pipes(struct drm_atomic_state *state)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> int ret = 0;
>
> - /* add all active pipes to the state */
> + /*
> + * Add all pipes to the state, and force
> + * a modeset on all the active ones.
> + */
> for_each_crtc(state->dev, crtc) {
> crtc_state = drm_atomic_get_crtc_state(state, crtc);
> if (IS_ERR(crtc_state))
> @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> if (ret < 0)
> return ret;
>
> + /*
> + * Writes to dev_priv->atomic_cdclk_freq must protected by
> + * holding all the crtc locks, even if we don't end up
> + * touching the hardware
> + */
> + if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
> + ret = intel_lock_all_pipes(state);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /* All pipes must be switched off while we change the cdclk. */
> if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> - intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
> + intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) {
> ret = intel_modeset_all_pipes(state);
> -
> - if (ret < 0)
> - return ret;
> + if (ret < 0)
> + return ret;
> + }
>
> DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual %u\n",
> intel_state->cdclk, intel_state->dev_cdclk);
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
