[Intel-gfx] Fwd: [PATCH] drm/i915: Fix IPS related flicker

Fri Jun 5 02:11:18 PDT 2015

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.

> 
> 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

-- 
Ville Syrjälä
Intel OTC