[Intel-gfx] [PATCH] drm/i915/bdw: Add 42ms delay for IPS disable

[Runyan, Arthur J]

>-----Original Message-----
>From: Ben Widawsky [mailto:benjamin.widawsky at linux.intel.com]
>Sent: Thursday, April 10, 2014 11:41 AM
>To: Chris Wilson; Widawsky, Benjamin; Intel GFX; Runyan, Arthur J
>Subject: Re: [Intel-gfx] [PATCH] drm/i915/bdw: Add 42ms delay for IPS disable
>
>On Thu, Apr 10, 2014 at 07:12:46PM +0100, Chris Wilson wrote:
>> On Thu, Apr 10, 2014 at 11:04:17AM -0700, Ben Widawsky wrote:
>> > From: Ben Widawsky <benjamin.widawsky at linux.intel.com>
>> >
>> > This is a requirement added to the spec. This patch will present
>> > persistent corruption on the display.
>
>s/present/prevent
>
>>
>> Prevent? I don't think we want to start showing corruption to the user.
>>
>> > Cc: Art Runyan <arthur.j.runyan at intel.com>
>> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 6 +++++-
>> >  1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 7f02444..ebc84ee 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -3583,7 +3583,7 @@ void hsw_disable_ips(struct intel_crtc *crtc)
>> >  		return;
>> >
>> >  	assert_plane_enabled(dev_priv, crtc->plane);
>> > -	if (IS_BROADWELL(crtc->base.dev)) {
>> > +	if (IS_BROADWELL(dev)) {
>> >  		mutex_lock(&dev_priv->rps.hw_lock);
>> >  		WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL,
>0));
>> >  		mutex_unlock(&dev_priv->rps.hw_lock);
>> > @@ -3594,6 +3594,10 @@ void hsw_disable_ips(struct intel_crtc *crtc)
>> >
>> >  	/* We need to wait for a vblank before we can disable the plane. */
>> >  	intel_wait_for_vblank(dev, crtc->pipe);
>> > +
>> > +	/* wait for pcode to finish disabling IPS, which may take up to 42ms */
>> > +	if (IS_BROADWELL(dev))
>> > +		msleep(42);

The added delay for broadwell should come before the wait for vblank.
Also, you should check for IPS_CTL bit 31 (Enable IPS) to become 0, just in case the 42ms was a lie, or something got stuck.  

>>
>> Reading too much Douglas Adams?
>> -Chris
>>
>
>It's those damn display arTchitects

Hah.... I am an Art, but not one of those lofty architects.

>
>--
>Ben Widawsky, Intel Open Source Technology Center