[Intel-gfx] [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Daniel Vetter]

On Tue, Nov 15, 2016 at 10:08:10AM +0100, Daniel Vetter wrote:
> On Mon, Nov 14, 2016 at 06:35:10PM +0200, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > 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?

Even more bikesheds in this area: Having different names for the same
stuff in intel_state and dev_priv is imo also bad.

Another one: We unconditionally put the values from intel_state into
dev_priv when intel_state->modeset is set. I think going more full-on with
state structs (intel_global_modeset_state) and pointers would streamline
the code a lot. With a full state structure and a real pointer in
intel_state we could also use the full get_state/destroy_state pattern,
and hide the locking in there. Everyone who just needs read access could
then either look at dev_priv->modeset_state->cdclk_freq, or we could have
a copy in intel_pipe_config.

Oh well, I guess cleaning that up is something for another day. Patch
itself looks correct.

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

> -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 at linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch