On Tue, Nov 15, 2016 at 11:14:29AM +0100, Maarten Lankhorst wrote:
> Op 14-11-16 om 17:35 schreef ville.syrjala@xxxxxxxxxxxxxxx:
> > 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.
> >
> > 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;
> > + }
> > +
> 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.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
