[PATCH] drm/i915: Treat resetting of the current framebuffer as a no-op

[PATCH] drm/i915: Treat resetting of the current framebuffer as a no-op

[Chris Wilson]

If none of the CRTC parameters change along with the framebuffer, we can
forgo rewriting the register and waiting for a vblank. There are a few
calls made by the display managers as they start up which tend to end up
performing no-ops on the current CRTC settings.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 981549c..f4450f1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2286,6 +2286,11 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return -EINVAL;
 	}
 
+	if (fb == crtc->fb && crtc->x == x && crtc->y == y) {
+		DRM_DEBUG_KMS("skipping reset of current fb");
+		return 0;
+	}
+
 	mutex_lock(&dev->struct_mutex);
 	ret = intel_pin_and_fence_fb_obj(dev,
 					 to_intel_framebuffer(fb)->obj,
-- 
1.7.10.4

Re: [PATCH] drm/i915: Treat resetting of the current framebuffer as a no-op

[Daniel Vetter]

On Thu, May 23, 2013 at 01:57:17PM +0100, Chris Wilson wrote:
> If none of the CRTC parameters change along with the framebuffer, we can
> forgo rewriting the register and waiting for a vblank. There are a few
> calls made by the display managers as they start up which tend to end up
> performing no-ops on the current CRTC settings.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Makes sense. Queued for -next, thanks for the patch. Now the only things
left (besides beating fastboot into good shape) is to cache the edids a
bit and we've (hopefully) killed all kms stalls at startup ...
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 981549c..f4450f1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2286,6 +2286,11 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  		return -EINVAL;
>  	}
>  
> +	if (fb == crtc->fb && crtc->x == x && crtc->y == y) {
> +		DRM_DEBUG_KMS("skipping reset of current fb");
> +		return 0;
> +	}
> +
>  	mutex_lock(&dev->struct_mutex);
>  	ret = intel_pin_and_fence_fb_obj(dev,
>  					 to_intel_framebuffer(fb)->obj,
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Treat resetting of the current framebuffer as a no-op

[Paulo Zanoni]

2013/5/23 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, May 23, 2013 at 01:57:17PM +0100, Chris Wilson wrote:
>> If none of the CRTC parameters change along with the framebuffer, we can
>> forgo rewriting the register and waiting for a vblank. There are a few
>> calls made by the display managers as they start up which tend to end up
>> performing no-ops on the current CRTC settings.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Makes sense. Queued for -next, thanks for the patch. Now the only things
> left (besides beating fastboot into good shape) is to cache the edids a
> bit and we've (hopefully) killed all kms stalls at startup ...

This commit introduced a regression.

- Boot with both eDP and DP plugged
- When I boot like this, eDP1 has 1920x1080 and DP1 has 1920x1080i.
- Run "xrandr --output DP1 --mode 0x55" (that's 1024x768@60Hz here)
- See the black screen on DP output, dmesg has the "skipping reset of
current fb" message.
- After we get the black screen, if we run "xrandr --output DP1 --off;
xrandr --output DP1 --mode 0x55" the mode will work.

If I diff the "bad state" with the "good state" we'll see the cause is
the DSPCNTR register. When we do the early return in
intel_pipe_set_base we don't call the update_plane function. For me
what changes is the pixel format and the trickle feed bits.


> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 981549c..f4450f1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2286,6 +2286,11 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>>               return -EINVAL;
>>       }
>>
>> +     if (fb == crtc->fb && crtc->x == x && crtc->y == y) {
>> +             DRM_DEBUG_KMS("skipping reset of current fb");
>> +             return 0;
>> +     }
>> +
>>       mutex_lock(&dev->struct_mutex);
>>       ret = intel_pin_and_fence_fb_obj(dev,
>>                                        to_intel_framebuffer(fb)->obj,
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Treat resetting of the current framebuffer as a no-op

[Daniel Vetter]

On Fri, May 31, 2013 at 4:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/5/23 Daniel Vetter <daniel@ffwll.ch>:
>> On Thu, May 23, 2013 at 01:57:17PM +0100, Chris Wilson wrote:
>>> If none of the CRTC parameters change along with the framebuffer, we can
>>> forgo rewriting the register and waiting for a vblank. There are a few
>>> calls made by the display managers as they start up which tend to end up
>>> performing no-ops on the current CRTC settings.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> Makes sense. Queued for -next, thanks for the patch. Now the only things
>> left (besides beating fastboot into good shape) is to cache the edids a
>> bit and we've (hopefully) killed all kms stalls at startup ...
>
> This commit introduced a regression.
>
> - Boot with both eDP and DP plugged
> - When I boot like this, eDP1 has 1920x1080 and DP1 has 1920x1080i.
> - Run "xrandr --output DP1 --mode 0x55" (that's 1024x768@60Hz here)
> - See the black screen on DP output, dmesg has the "skipping reset of
> current fb" message.
> - After we get the black screen, if we run "xrandr --output DP1 --off;
> xrandr --output DP1 --mode 0x55" the mode will work.
>
> If I diff the "bad state" with the "good state" we'll see the cause is
> the DSPCNTR register. When we do the early return in
> intel_pipe_set_base we don't call the update_plane function. For me
> what changes is the pixel format and the trickle feed bits.

Oh, in the modeset case we can't optimize the update_fb away, even
when both fbs are the same ...

Just to check that our level of paranoia is still high enough: Has the
modeset state checker complained or not?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Treat resetting of the current framebuffer as a no-op

[Paulo Zanoni]

2013/5/31 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, May 31, 2013 at 4:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> 2013/5/23 Daniel Vetter <daniel@ffwll.ch>:
>>> On Thu, May 23, 2013 at 01:57:17PM +0100, Chris Wilson wrote:
>>>> If none of the CRTC parameters change along with the framebuffer, we can
>>>> forgo rewriting the register and waiting for a vblank. There are a few
>>>> calls made by the display managers as they start up which tend to end up
>>>> performing no-ops on the current CRTC settings.
>>>>
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> Makes sense. Queued for -next, thanks for the patch. Now the only things
>>> left (besides beating fastboot into good shape) is to cache the edids a
>>> bit and we've (hopefully) killed all kms stalls at startup ...
>>
>> This commit introduced a regression.
>>
>> - Boot with both eDP and DP plugged
>> - When I boot like this, eDP1 has 1920x1080 and DP1 has 1920x1080i.
>> - Run "xrandr --output DP1 --mode 0x55" (that's 1024x768@60Hz here)
>> - See the black screen on DP output, dmesg has the "skipping reset of
>> current fb" message.
>> - After we get the black screen, if we run "xrandr --output DP1 --off;
>> xrandr --output DP1 --mode 0x55" the mode will work.
>>
>> If I diff the "bad state" with the "good state" we'll see the cause is
>> the DSPCNTR register. When we do the early return in
>> intel_pipe_set_base we don't call the update_plane function. For me
>> what changes is the pixel format and the trickle feed bits.
>
> Oh, in the modeset case we can't optimize the update_fb away, even
> when both fbs are the same ...
>
> Just to check that our level of paranoia is still high enough: Has the
> modeset state checker complained or not?

No. The register diff is really only a few bits of the DSPCNTR register.


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Treat resetting of the current framebuffer as a no-op

[Ville Syrjälä]

On Fri, May 31, 2013 at 05:15:10PM +0200, Daniel Vetter wrote:
> On Fri, May 31, 2013 at 4:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > 2013/5/23 Daniel Vetter <daniel@ffwll.ch>:
> >> On Thu, May 23, 2013 at 01:57:17PM +0100, Chris Wilson wrote:
> >>> If none of the CRTC parameters change along with the framebuffer, we can
> >>> forgo rewriting the register and waiting for a vblank. There are a few
> >>> calls made by the display managers as they start up which tend to end up
> >>> performing no-ops on the current CRTC settings.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>
> >> Makes sense. Queued for -next, thanks for the patch. Now the only things
> >> left (besides beating fastboot into good shape) is to cache the edids a
> >> bit and we've (hopefully) killed all kms stalls at startup ...
> >
> > This commit introduced a regression.
> >
> > - Boot with both eDP and DP plugged
> > - When I boot like this, eDP1 has 1920x1080 and DP1 has 1920x1080i.
> > - Run "xrandr --output DP1 --mode 0x55" (that's 1024x768@60Hz here)
> > - See the black screen on DP output, dmesg has the "skipping reset of
> > current fb" message.
> > - After we get the black screen, if we run "xrandr --output DP1 --off;
> > xrandr --output DP1 --mode 0x55" the mode will work.
> >
> > If I diff the "bad state" with the "good state" we'll see the cause is
> > the DSPCNTR register. When we do the early return in
> > intel_pipe_set_base we don't call the update_plane function. For me
> > what changes is the pixel format and the trickle feed bits.
> 
> Oh, in the modeset case we can't optimize the update_fb away, even
> when both fbs are the same ...

I think there are two problems currently:

1) .crtc_mode_set will clear DSPCNTR, so .update_plane() is needed to
   re-populate the relevant bits

2) We no longer do a tiling_mode check on the obj. That's done in
   intel_pin_and_fence_fb_obj() which is now skipped too.

   I've been pondering whether we should just prevent tiling changes
   for any obj with fbs...

> 
> Just to check that our level of paranoia is still high enough: Has the
> modeset state checker complained or not?
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Treat resetting of the current framebuffer as a no-op

[Chris Wilson]

On Fri, May 31, 2013 at 05:15:10PM +0200, Daniel Vetter wrote:
> On Fri, May 31, 2013 at 4:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > 2013/5/23 Daniel Vetter <daniel@ffwll.ch>:
> >> On Thu, May 23, 2013 at 01:57:17PM +0100, Chris Wilson wrote:
> >>> If none of the CRTC parameters change along with the framebuffer, we can
> >>> forgo rewriting the register and waiting for a vblank. There are a few
> >>> calls made by the display managers as they start up which tend to end up
> >>> performing no-ops on the current CRTC settings.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>
> >> Makes sense. Queued for -next, thanks for the patch. Now the only things
> >> left (besides beating fastboot into good shape) is to cache the edids a
> >> bit and we've (hopefully) killed all kms stalls at startup ...
> >
> > This commit introduced a regression.
> >
> > - Boot with both eDP and DP plugged
> > - When I boot like this, eDP1 has 1920x1080 and DP1 has 1920x1080i.
> > - Run "xrandr --output DP1 --mode 0x55" (that's 1024x768@60Hz here)
> > - See the black screen on DP output, dmesg has the "skipping reset of
> > current fb" message.
> > - After we get the black screen, if we run "xrandr --output DP1 --off;
> > xrandr --output DP1 --mode 0x55" the mode will work.
> >
> > If I diff the "bad state" with the "good state" we'll see the cause is
> > the DSPCNTR register. When we do the early return in
> > intel_pipe_set_base we don't call the update_plane function. For me
> > what changes is the pixel format and the trickle feed bits.
> 
> Oh, in the modeset case we can't optimize the update_fb away, even
> when both fbs are the same ...

Hopefully we can push the check higher to the fb fast path then? The
skip is much safer in the kernel as it has correct knowledge of the
current state (as opposed to trying to track it the ddx sharing control
of the machine).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Treat resetting of the current framebuffer as a no-op

[Daniel Vetter]

On Fri, May 31, 2013 at 7:50 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, May 31, 2013 at 05:15:10PM +0200, Daniel Vetter wrote:
>> On Fri, May 31, 2013 at 4:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> > 2013/5/23 Daniel Vetter <daniel@ffwll.ch>:
>> >> On Thu, May 23, 2013 at 01:57:17PM +0100, Chris Wilson wrote:
>> >>> If none of the CRTC parameters change along with the framebuffer, we can
>> >>> forgo rewriting the register and waiting for a vblank. There are a few
>> >>> calls made by the display managers as they start up which tend to end up
>> >>> performing no-ops on the current CRTC settings.
>> >>>
>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >>
>> >> Makes sense. Queued for -next, thanks for the patch. Now the only things
>> >> left (besides beating fastboot into good shape) is to cache the edids a
>> >> bit and we've (hopefully) killed all kms stalls at startup ...
>> >
>> > This commit introduced a regression.
>> >
>> > - Boot with both eDP and DP plugged
>> > - When I boot like this, eDP1 has 1920x1080 and DP1 has 1920x1080i.
>> > - Run "xrandr --output DP1 --mode 0x55" (that's 1024x768@60Hz here)
>> > - See the black screen on DP output, dmesg has the "skipping reset of
>> > current fb" message.
>> > - After we get the black screen, if we run "xrandr --output DP1 --off;
>> > xrandr --output DP1 --mode 0x55" the mode will work.
>> >
>> > If I diff the "bad state" with the "good state" we'll see the cause is
>> > the DSPCNTR register. When we do the early return in
>> > intel_pipe_set_base we don't call the update_plane function. For me
>> > what changes is the pixel format and the trickle feed bits.
>>
>> Oh, in the modeset case we can't optimize the update_fb away, even
>> when both fbs are the same ...
>
> Hopefully we can push the check higher to the fb fast path then? The
> skip is much safer in the kernel as it has correct knowledge of the
> current state (as opposed to trying to track it the ddx sharing control
> of the machine).

Yeah, I think we could move it out into the setcrtc logic. Dropped the
patch in it's current version for now.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Always disable trickle feed on Broadwater/Crestline

[Chris Wilson]

The programming notes for Broadwater and Crestline mandate that the
trickle feed disable bit is asserted.

See section 1.16.2 MI_ARB_STATE.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_pm.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4126fb1..3dfb5be 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4930,6 +4930,9 @@ static void crestline_init_clock_gating(struct drm_device *dev)
 	I915_WRITE(DSPCLK_GATE_D, 0);
 	I915_WRITE(RAMCLK_GATE_D, 0);
 	I915_WRITE16(DEUC, 0);
+
+	I915_WRITE(MI_ARB_STATE,
+		   _MASKED_BIT_ENABLE(MI_ARB_DISPLAY_TRICKLE_FEED_DISABLE));
 }
 
 static void broadwater_init_clock_gating(struct drm_device *dev)
@@ -4942,6 +4945,9 @@ static void broadwater_init_clock_gating(struct drm_device *dev)
 		   I965_ISC_CLOCK_GATE_DISABLE |
 		   I965_FBC_CLOCK_GATE_DISABLE);
 	I915_WRITE(RENCLK_GATE_D2, 0);
+
+	I915_WRITE(MI_ARB_STATE,
+		   _MASKED_BIT_ENABLE(MI_ARB_DISPLAY_TRICKLE_FEED_DISABLE));
 }
 
 static void gen3_init_clock_gating(struct drm_device *dev)
-- 
1.7.10.4