I just noticed that I had forgotten to reply-all... Jani, would you consider merge this fix with the explanation above related to Ville's question? or do you want/need any action here? Thanks, Rodrigo. ---------- Forwarded message ---------- From: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> Date: Fri, May 29, 2015 at 9:45 AM Subject: Re: [PATCH] drm/i915: Fix IPS related flicker To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > 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? I don't like this !fb here as well, but I just tried to keep exactly same if statement that makes I915_WRITE(DSPCNTRL, 0) on update primary plane func... > >> 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. agree. > > 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> Thanks > >> >> 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 -- Rodrigo Vivi Blog: http://blog.vivi.eng.br -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
