On Thu, Jun 25, 2015 at 4:58 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>> On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselvan2@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
>>>> > > Date: Fri, May 29, 2015 at 9:45 AM
>>>> > > Subject: Re: [PATCH] drm/i915: Fix IPS related flicker
>>>> > > To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>>> > >
>>>> > >
>>>> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
>>>> > > <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>>>> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>>> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>>>> > >>> ---
>>>> > >>> 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@xxxxxxxxxxxxxxx>
>>>> > >
>>>> > > 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@xxxxxxxxxxxxxxxxxxxxx
>>>> > >> 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@xxxxxxxxxxxxxxxxxxxxx
>>>> > 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
