On Thu, 04 Jun 2015, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote: > 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? Ville's question, I'd like Ville's ack on it. BR, Jani. > > 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 -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
