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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx