<div dir="ltr"> <div class="gmail_quote"><div dir="ltr">On Thu, Jun 25, 2015 at 4:58 AM Jani Nikula <<a href="mailto:jani.nikula@linux.intel.com">jani.nikula@linux.intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, 18 Jun 2015, Jani Nikula <<a href="mailto:jani.nikula@linux.intel.com" target="_blank">jani.nikula@linux.intel.com</a>> wrote: > On Thu, 18 Jun 2015, Jani Nikula <<a href="mailto:jani.nikula@linux.intel.com" target="_blank">jani.nikula@linux.intel.com</a>> wrote: >> On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <<a href="mailto:conselvan2@gmail.com" target="_blank">conselvan2@gmail.com</a>> 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 <<a href="mailto:rodrigo.vivi@gmail.com" target="_blank">rodrigo.vivi@gmail.com</a>> 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. And keeps conflicting too badly for me to figure out what needs to be done. Is this still needed in dinq? </blockquote><div> </div><div>To be honest, on drm-intel-nightly: 2015y-06m-17d-12h-44m-01s</div><div>with ips enabled I'm not facing the bad flicker anymore.</div><div> </div><div>However since we still have the following 2 lines on update_primary_plane_function:</div><div> </div><div>if (!visible || !fb) {</div><div> I915_WRITE(reg, 0);</div><div> </div><div>I believe we need this protection. Let me refactor it..</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> BR, Jani. > > BR, > Jani. > > > >> >> BR, >> Jani. >> >> >>> >>> Thanks, >>> Ander >>> >>>> >>>> > >>>> > BR, >>>> > Jani. >>>> > >>>> > >>>> > > >>>> > > Thanks, >>>> > > Rodrigo. >>>> > > >>>> > > >>>> > > ---------- Forwarded message ---------- >>>> > > From: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@gmail.com" target="_blank">rodrigo.vivi@gmail.com</a>> >>>> > > Date: Fri, May 29, 2015 at 9:45 AM >>>> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker >>>> > > To: Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a>> >>>> > > >>>> > > >>>> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä >>>> > > <<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a>> 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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=85583" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=85583</a> >>>> > >>> Cc: Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a>> >>>> > >>> Cc: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com" target="_blank">paulo.r.zanoni@intel.com</a>> >>>> > >>> Signed-off-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com" target="_blank">rodrigo.vivi@intel.com</a>> >>>> > >>> --- >>>> > >>> 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ä <<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a>> >>>> > > >>>> > > 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 >>>> > >> <a href="mailto:Intel-gfx@lists.freedesktop.org" target="_blank">Intel-gfx@lists.freedesktop.org</a> >>>> > >> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> >>>> > > >>>> > > >>>> > > >>>> > > -- >>>> > > Rodrigo Vivi >>>> > > Blog: <a href="http://blog.vivi.eng.br" rel="noreferrer" target="_blank">http://blog.vivi.eng.br</a> >>>> > > >>>> > > >>>> > > -- >>>> > > Rodrigo Vivi >>>> > > Blog: <a href="http://blog.vivi.eng.br" rel="noreferrer" target="_blank">http://blog.vivi.eng.br</a> >>>> > >>>> > -- >>>> > Jani Nikula, Intel Open Source Technology Center >>>> > _______________________________________________ >>>> > Intel-gfx mailing list >>>> > <a href="mailto:Intel-gfx@lists.freedesktop.org" target="_blank">Intel-gfx@lists.freedesktop.org</a> >>>> > <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> >>>> >>> >>> >> >> -- >> Jani Nikula, Intel Open Source Technology Center > > -- > Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list <a href="mailto:Intel-gfx@lists.freedesktop.org" target="_blank">Intel-gfx@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </blockquote></div></div>