On Thu, 25 Jun 2015, Rodrigo Vivi <rodrigo.v...@gmail.com> wrote:
> On Thu, Jun 25, 2015 at 4:58 AM Jani Nikula <jani.nik...@linux.intel.com>
> wrote:
>
>> On Thu, 18 Jun 2015, Jani Nikula <jani.nik...@linux.intel.com> wrote:
>> > On Thu, 18 Jun 2015, Jani Nikula <jani.nik...@linux.intel.com> wrote:
>> >> On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselv...@gmail.com>
>> 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.v...@gmail.com> 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?
>>
>
> To be honest, on drm-intel-nightly: 2015y-06m-17d-12h-44m-01s
> with ips enabled I'm not facing the bad flicker anymore.
>
> However since we still have the following 2 lines on
> update_primary_plane_function:
>
> if (!visible || !fb) {
> I915_WRITE(reg, 0);
>
> I believe we need this protection. Let me refactor it..I think we need to apply this patch to drm-intel-next-fixes (for v4.2 and cc: stable), and the refactored version on drm-intel-next-queued (for v4.3). BR, Jani. > > >> >> BR, >> Jani. >> >> >> > >> > BR, >> > Jani. >> > >> > >> > >> >> >> >> BR, >> >> Jani. >> >> >> >> >> >>> >> >>> Thanks, >> >>> Ander >> >>> >> >>>> >> >>>> > >> >>>> > BR, >> >>>> > Jani. >> >>>> > >> >>>> > >> >>>> > > >> >>>> > > Thanks, >> >>>> > > Rodrigo. >> >>>> > > >> >>>> > > >> >>>> > > ---------- Forwarded message ---------- >> >>>> > > From: Rodrigo Vivi <rodrigo.v...@gmail.com> >> >>>> > > Date: Fri, May 29, 2015 at 9:45 AM >> >>>> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker >> >>>> > > To: Ville Syrjälä <ville.syrj...@linux.intel.com> >> >>>> > > >> >>>> > > >> >>>> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä >> >>>> > > <ville.syrj...@linux.intel.com> 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.syrj...@linux.intel.com> >> >>>> > >>> Cc: Paulo Zanoni <paulo.r.zan...@intel.com> >> >>>> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com> >> >>>> > >>> --- >> >>>> > >>> 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.syrj...@linux.intel.com> >> >>>> > > >> >>>> > > 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@lists.freedesktop.org >> >>>> > >> 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@lists.freedesktop.org >> >>>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >>>> >> >>> >> >>> >> >> >> >> -- >> >> 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 >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
