[Intel-gfx] Fwd: [PATCH] drm/i915: Fix IPS related flicker
Rodrigo Vivi
rodrigo.vivi at gmail.com
Thu Jun 25 09:21:36 PDT 2015
On Thu, Jun 25, 2015 at 4:58 AM Jani Nikula <jani.nikula at linux.intel.com>
wrote:
> On Thu, 18 Jun 2015, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> > On Thu, 18 Jun 2015, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> >> On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselvan2 at 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.vivi at 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..
>
> BR,
> Jani.
>
>
> >
> > BR,
> > Jani.
> >
> >
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >>>
> >>> Thanks,
> >>> Ander
> >>>
> >>>>
> >>>> >
> >>>> > BR,
> >>>> > Jani.
> >>>> >
> >>>> >
> >>>> > >
> >>>> > > Thanks,
> >>>> > > Rodrigo.
> >>>> > >
> >>>> > >
> >>>> > > ---------- Forwarded message ----------
> >>>> > > From: Rodrigo Vivi <rodrigo.vivi at 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.syrjala at linux.intel.com>
> >>>> > >
> >>>> > >
> >>>> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
> >>>> > > <ville.syrjala at 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.syrjala at linux.intel.com>
> >>>> > >>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >>>> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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.syrjala at 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 at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20150625/cafcfaab/attachment.html>
More information about the Intel-gfx
mailing list