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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 18, 2016 at 11:26:18AM +0100, Maarten Lankhorst wrote:
> Op 17-11-16 om 16:06 schreef Ville Syrjälä:
> > On Tue, Nov 15, 2016 at 02:53:00PM +0100, Maarten Lankhorst wrote:
> >> Op 15-11-16 om 14:41 schreef Ville Syrjälä:
> >>> On Tue, Nov 15, 2016 at 11:14:29AM +0100, Maarten Lankhorst wrote:
> >>>> Op 14-11-16 om 17:35 schreef ville.syrjala@linux.intel.com:
> >>>>> From: Ville Syrjälä <ville.syrjala@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.
> >>>>>
> >>>>> 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@linux.intel.com>
> >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@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;
> >>>>> +		}
> >>>>> +
> >>>> Would it be terrible to just use intel_modeset_all_pipes here? Since this can only be different in the all crtc's disabled case
> >>>> it won't matter much.
> >>> Is there any benefit in doing that? A bit confusing IMO to force a
> >>> modeset when you don't have to.
> >>>
> >> The case where atomic cdclk changes, but dev_cdclk stays the same can only happen
> >> if you configure a crtc, but all crtc's stay !active. In all other cases dev_cdclk
> >> will change too.
> >>
> >> intel_modeset_all_pipes will only set mode_changed on active crtc's, but it will
> >> add all crtc's to the atomic state regardless to make sure the cdclk stays consistent.
> > I still don't see what the benefit is. IMO it's just confusing to say
> > that we're going to force a modeset on a disabled pipe.
> >
> if (!active || needs_modeset(crtc_state)) continue; crtc_state->mode_changed= true; It's not doing a modeset on a disabled pipe, it only makes sure it's part of the state.

But the code will still *read* if (blah) modeset_all_pipes();
which is nonsense.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx