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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselvan2@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@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?

BR,
Jani.


>
> BR,
> Jani.
>
>
>
>>
>> BR,
>> Jani.
>>
>>
>>>
>>> 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
>>>> 
>>>
>>>
>>
>> -- 
>> 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