On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote: > We cannot let IPS enabled with no plane on the pipe: > > BSpec: "IPS cannot be enabled until after at least one plane has > been enabled for at least one vertical blank." and "IPS must be > disabled while there is still at least one plane enabled on the > same pipe as IPS." This restriction apply to HSW and BDW. > > However a shortcut path on update primary plane function > to make primary plane invisible by setting DSPCTRL to 0 > was leting IPS enabled while there was no > other plane enabled on the pipe causing flickerings that we were > believing that it was caused by that other restriction where > ips cannot be used when pixel rate is greater than 95% of cdclok. > > v2: Don't mess with Atomic path as pointed out by Ville. > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583 > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4e3f302..5a6b17b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane, > intel_crtc->atomic.wait_vblank = true; > } > > + /* > + * FIXME: Actually if we will still have any other plane enabled > + * on the pipe we could let IPS enabled still, but for > + * now lets consider that when we make primary invisible > + * by setting DSPCNTR to 0 on update_primary_plane function > + * IPS needs to be disable. > + */ > + if (!state->visible || !fb) > + intel_crtc->atomic.disable_ips = true; > + How could it be visible without an fb? > intel_crtc->atomic.fb_bits |= > INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); > > @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc) > if (intel_crtc->atomic.disable_fbc) > intel_fbc_disable(dev); > > + if (intel_crtc->atomic.disable_ips) > + hsw_disable_ips(intel_crtc); > + > if (intel_crtc->atomic.pre_disable_primary) > intel_pre_disable_primary(crtc); intel_pre_disable_primary() would already disable IPS. Except no one sets .pre_disable_primary=true. OTOH that thing mostly seems to do stuff that has nothing to do with the primary plane (cxsr disable, fifo underrun reporting disable on gen2), so I don't think we want to use that. In any case we should really have the IPS state as part of the crtc state. These global disable_foo things should just be killed IMO. Hmm, except to do this properly we'd then need to track the hw IPS state separately somewhere. I guess we can just go with this for now. At least it's not really making things worse, so (maybe with the !fb check dropped): Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 2afb31a..1059283 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit { > /* Sleepable operations to perform before commit */ > bool wait_for_flips; > bool disable_fbc; > + bool disable_ips; > bool pre_disable_primary; > bool update_wm; > unsigned disabled_planes; > -- > 2.1.0 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
