Re: Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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

Thanks,
Ander

> 
> > 
> > BR,
> > Jani.
> > 
> > 
> > >
> > > Thanks,
> > > Rodrigo.
> > >
> > >
> > > ---------- Forwarded message ----------
> > > From: Rodrigo Vivi <rodrigo.vivi@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@linux.intel.com>
> > >
> > >
> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
> > > <ville.syrjala@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@linux.intel.com>
> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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@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
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx