drm/i915: Fix IPS related flicker

Submitted by Rodrigo Vivi on May 28, 2015, 6:07 p.m.

Details

Message ID 1432836431-2994-1-git-send-email-rodrigo.vivi@intel.com
State Accepted, archived
Commit ac88cd738425e04dbed3706621cf613a00708834
Headers show

Commit Message

Rodrigo Vivi May 28, 2015, 6:07 p.m.
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(+)

Patch hide | download patch | download mbox

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;
+
 		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);
 
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;

Comments

Ville Syrjälä May 29, 2015, 8:47 a.m.
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?

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

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>

>  
> 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
shuang.he@intel.com May 31, 2015, 2:36 p.m.
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6501
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                 -1              315/315              314/315
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                 -1              320/320              319/320
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
*BDW  igt@gem_flink_race@flink_name      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_display.c:#assert_plane[i915]()@WARNING:.* at .* assert_plane
assertion_failure@assertion failure
WARNING:at_drivers/gpu/drm/drm_irq.c:#drm_wait_one_vblank[drm]()@WARNING:.* at .* drm_wait_one_vblank+0x
Note: You need to pay more attention to line start with '*'
Rodrigo Vivi June 4, 2015, 6:55 p.m.
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?

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
Jani Nikula June 5, 2015, 8:51 a.m.
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
Ville Syrjälä June 5, 2015, 9:11 a.m.
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.

> 
> 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
Ander Conselvan de Oliveira June 18, 2015, 10:58 a.m.
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
>
Jani Nikula June 18, 2015, 11:53 a.m.
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

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 June 18, 2015, 11:58 a.m.
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.

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 June 25, 2015, 12:01 p.m.
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
Rodrigo Vivi June 25, 2015, 4:21 p.m.
On Thu, Jun 25, 2015 at 4:58 AM 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, 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?
>

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@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
>
Jani Nikula June 25, 2015, 4:49 p.m.
On Thu, 25 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Thu, Jun 25, 2015 at 4:58 AM 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, 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?
>>
>
> 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..

I think we need to apply this patch to drm-intel-next-fixes (for v4.2
and cc: stable), and the refactored version on drm-intel-next-queued
(for v4.3).

BR,
Jani.

>
>
>>
>> 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
>>
Jani Nikula June 26, 2015, 9:11 a.m.
On Thu, 28 May 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> 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>

Pushed to drm-intel-next-fixes, thanks for the patch and review. I
resolved the conflicts -nightly in favor of drm-intel-next-queued, so
your other patch is needed for that.

BR,
Jani.

> ---
>  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;
> +
>  		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);
>  
> 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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx