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

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

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.

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