On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselvan2@xxxxxxxxx> wrote: >> On Fri, 2015-06-05 at 12:11 +0300, Ville Syrjälä wrote: >>> On Fri, Jun 05, 2015 at 11:51:42AM +0300, Jani Nikula wrote: >>> > 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. >>> >>> It's good enough for me. This part of the driver is a quite a mess >>> anyway currently, so doesn't matter too much what we stick in there. >> >> Ping. Seems like this still isn't merged. Does it need more work or did >> it just fall through the cracks? > > It fell between the cracks. I know the world isn't black and white, but > it doesn't help the maintainers when review is some shade of grey. > > I've pushed this to drm-intel-next-fixes for now, but it has missed the > train for both the v4.1 release and the main drm-next feature pull > request for the v4.2 merge window. I expect this to land upstream in > v4.2-rc2, unless there's an additional drm-next pull request during the > merge window. I've added cc: stable. > > Thanks for the patch, and I guess the review was, uh, "good enough for > me" now... :p Argh, I'll take that back. This conflicts with dinq, and while doing so also confuses git rerere enough to uncover a previous much bigger conflict that I have no intention of resolving again before the weekend. I'll return to it next week. Sorry. BR, Jani. > > BR, > Jani. > > >> >> Thanks, >> Ander >> >>> >>> > >>> > 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 >>> >> >> > > -- > Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
