[PATCH] drm/i915: s/mdelay/msleep/

[PATCH] drm/i915: s/mdelay/msleep/

[Daniel Vetter]

The first in wait_pipe_off is really just a delay loop to wait for the
scanline counter to settle (which takes about a frame worth of time).
So also shrink it a bit.

The other is in the ilk dprs code. If we need _exactly_ an 1ms wait in
there, that's already not guaranteed with the current code (since
especially at start-up we're likely to get scheduled away in between).
So replace those, too.

Noticed while trying to understand why we've again broken the panel
fitter.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 drivers/gpu/drm/i915/intel_pm.c      |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b884932..908f697 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -990,7 +990,7 @@ void intel_wait_for_pipe_off(struct drm_device *dev, int pipe)
 		/* Wait for the display line to settle */
 		do {
 			last_line = I915_READ(reg) & line_mask;
-			mdelay(5);
+			msleep(1);
 		} while (((I915_READ(reg) & line_mask) != last_line) &&
 			 time_after(timeout, jiffies));
 		if (time_after(jiffies, timeout))
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 17f157a..986770e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2380,7 +2380,7 @@ static void ironlake_enable_drps(struct drm_device *dev)
 
 	if (wait_for_atomic((I915_READ(MEMSWCTL) & MEMCTL_CMD_STS) == 0, 10))
 		DRM_ERROR("stuck trying to change perf mode\n");
-	mdelay(1);
+	msleep(1);
 
 	ironlake_set_drps(dev, fstart);
 
@@ -2411,10 +2411,10 @@ static void ironlake_disable_drps(struct drm_device *dev)
 
 	/* Go back to the starting frequency */
 	ironlake_set_drps(dev, dev_priv->ips.fstart);
-	mdelay(1);
+	msleep(1);
 	rgvswctl |= MEMCTL_CMD_STS;
 	I915_WRITE(MEMSWCTL, rgvswctl);
-	mdelay(1);
+	msleep(1);
 
 	spin_unlock_irq(&mchdev_lock);
 }
-- 
1.7.10.4

Re: [PATCH] drm/i915: s/mdelay/msleep/

[Chris Wilson]

On Sun, Apr 07, 2013 at 11:13:03AM +0200, Daniel Vetter wrote:
> The first in wait_pipe_off is really just a delay loop to wait for the
> scanline counter to settle (which takes about a frame worth of time).
> So also shrink it a bit.
> 
> The other is in the ilk dprs code. If we need _exactly_ an 1ms wait in
> there, that's already not guaranteed with the current code (since
> especially at start-up we're likely to get scheduled away in between).
> So replace those, too.
> 
> Noticed while trying to understand why we've again broken the panel
> fitter.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

The only reason the mdelay(5) persisted for so long was that we were
being lazy and not verifying that path could not be called from atomic
context.

The others I just credit Jesse for.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: s/mdelay/msleep/

[Paulo Zanoni]

Hi

2013/4/8 Chris Wilson <chris@chris-wilson.co.uk>:
> On Sun, Apr 07, 2013 at 11:13:03AM +0200, Daniel Vetter wrote:
>> The first in wait_pipe_off is really just a delay loop to wait for the
>> scanline counter to settle (which takes about a frame worth of time).
>> So also shrink it a bit.
>>
>> The other is in the ilk dprs code. If we need _exactly_ an 1ms wait in
>> there, that's already not guaranteed with the current code (since
>> especially at start-up we're likely to get scheduled away in between).
>> So replace those, too.
>>
>> Noticed while trying to understand why we've again broken the panel
>> fitter.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> The only reason the mdelay(5) persisted for so long was that we were
> being lazy and not verifying that path could not be called from atomic
> context.
>
> The others I just credit Jesse for.

Documentation/timers/timers-howto.txt says that "msleep(1~20) may not
do what the caller intends, and will often sleep longer (~20 ms actual
sleep for any value given in the 1~20ms range). In many cases this is
not the desired behavior.". It suggests us to use usleep_range().

So it seems we may significantly increase those delays we're changing.
Is this a problem for this patch?



>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



--
Paulo Zanoni

Re: [PATCH] drm/i915: s/mdelay/msleep/

[Daniel Vetter]

On Mon, Apr 8, 2013 at 1:49 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/4/8 Chris Wilson <chris@chris-wilson.co.uk>:
>> On Sun, Apr 07, 2013 at 11:13:03AM +0200, Daniel Vetter wrote:
>>> The first in wait_pipe_off is really just a delay loop to wait for the
>>> scanline counter to settle (which takes about a frame worth of time).
>>> So also shrink it a bit.
>>>
>>> The other is in the ilk dprs code. If we need _exactly_ an 1ms wait in
>>> there, that's already not guaranteed with the current code (since
>>> especially at start-up we're likely to get scheduled away in between).
>>> So replace those, too.
>>>
>>> Noticed while trying to understand why we've again broken the panel
>>> fitter.
>>>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> The only reason the mdelay(5) persisted for so long was that we were
>> being lazy and not verifying that path could not be called from atomic
>> context.
>>
>> The others I just credit Jesse for.
>
> Documentation/timers/timers-howto.txt says that "msleep(1~20) may not
> do what the caller intends, and will often sleep longer (~20 ms actual
> sleep for any value given in the 1~20ms range). In many cases this is
> not the desired behavior.". It suggests us to use usleep_range().
>
> So it seems we may significantly increase those delays we're changing.
> Is this a problem for this patch?

The hand-waving in the commit message is supposed to specifically
address this concern. For the wait_pipe_off case we certainly don't
care, and the mdelays in the ilk code run at boot-up when there's a
pretty big chance that the scheduler will take away our cpu anyway
(and so extend the wait).

If the hw really needs an exact wait, we're screwed and would need to
use tricks like stop_machine to make sure no one else can interrupt
us.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: s/mdelay/msleep/

[Jesse Barnes]

On Sun,  7 Apr 2013 11:13:03 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> The first in wait_pipe_off is really just a delay loop to wait for the
> scanline counter to settle (which takes about a frame worth of time).
> So also shrink it a bit.
> 
> The other is in the ilk dprs code. If we need _exactly_ an 1ms wait in
> there, that's already not guaranteed with the current code (since
> especially at start-up we're likely to get scheduled away in between).
> So replace those, too.
> 
> Noticed while trying to understand why we've again broken the panel
> fitter.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  drivers/gpu/drm/i915/intel_pm.c      |    6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b884932..908f697 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -990,7 +990,7 @@ void intel_wait_for_pipe_off(struct drm_device *dev, int pipe)
>  		/* Wait for the display line to settle */
>  		do {
>  			last_line = I915_READ(reg) & line_mask;
> -			mdelay(5);
> +			msleep(1);
>  		} while (((I915_READ(reg) & line_mask) != last_line) &&
>  			 time_after(timeout, jiffies));
>  		if (time_after(jiffies, timeout))
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 17f157a..986770e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2380,7 +2380,7 @@ static void ironlake_enable_drps(struct drm_device *dev)
>  
>  	if (wait_for_atomic((I915_READ(MEMSWCTL) & MEMCTL_CMD_STS) == 0, 10))
>  		DRM_ERROR("stuck trying to change perf mode\n");
> -	mdelay(1);
> +	msleep(1);
>  
>  	ironlake_set_drps(dev, fstart);
>  
> @@ -2411,10 +2411,10 @@ static void ironlake_disable_drps(struct drm_device *dev)
>  
>  	/* Go back to the starting frequency */
>  	ironlake_set_drps(dev, dev_priv->ips.fstart);
> -	mdelay(1);
> +	msleep(1);
>  	rgvswctl |= MEMCTL_CMD_STS;
>  	I915_WRITE(MEMSWCTL, rgvswctl);
> -	mdelay(1);
> +	msleep(1);
>  
>  	spin_unlock_irq(&mchdev_lock);
>  }

Fine with me.  Chris is right that I probably added these, back when I
thought a few ms at startup or mode set time wasn't so long.  Now of
course it's an eternity.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm/i915: s/mdelay/msleep/

[Daniel Vetter]

Burning cpu cycles isn't awesome, so use sleeps instead.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56316c1e945c..87be3f307e40 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1026,7 +1026,7 @@ static bool pipe_dsl_stopped(struct drm_device *dev, enum pipe pipe)
 		line_mask = DSL_LINEMASK_GEN3;
 
 	line1 = I915_READ(reg) & line_mask;
-	mdelay(5);
+	msleep(5);
 	line2 = I915_READ(reg) & line_mask;
 
 	return line1 == line2;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6eb5d76e6912..1efac89cb738 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4255,7 +4255,7 @@ static void ironlake_enable_drps(struct drm_device *dev)
 
 	if (wait_for_atomic((I915_READ(MEMSWCTL) & MEMCTL_CMD_STS) == 0, 10))
 		DRM_ERROR("stuck trying to change perf mode\n");
-	mdelay(1);
+	msleep(1);
 
 	ironlake_set_drps(dev, fstart);
 
@@ -4286,10 +4286,10 @@ static void ironlake_disable_drps(struct drm_device *dev)
 
 	/* Go back to the starting frequency */
 	ironlake_set_drps(dev, dev_priv->ips.fstart);
-	mdelay(1);
+	msleep(1);
 	rgvswctl |= MEMCTL_CMD_STS;
 	I915_WRITE(MEMSWCTL, rgvswctl);
-	mdelay(1);
+	msleep(1);
 
 	spin_unlock_irq(&mchdev_lock);
 }
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: s/mdelay/msleep/

[Ville Syrjälä]

On Tue, Jul 07, 2015 at 09:10:40AM +0200, Daniel Vetter wrote:
> Burning cpu cycles isn't awesome, so use sleeps instead.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 56316c1e945c..87be3f307e40 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1026,7 +1026,7 @@ static bool pipe_dsl_stopped(struct drm_device *dev, enum pipe pipe)
>  		line_mask = DSL_LINEMASK_GEN3;
>  
>  	line1 = I915_READ(reg) & line_mask;
> -	mdelay(5);
> +	msleep(5);

msleep() is quite inaccurate, but the accuracy of the delay shouldn't matter
too much here. The only functional worry would be if it manages to sleep for
exactly one frame and then we'd think DSL had stopped. But since we've already
told the pipe to shut down, I don't think it should ever end up scanning out
more than one frame here, so I suppose that problem should never happen.
So yeah, this seems safe.

It might make the modeset a bit slower if it ends up sleeping for a long
time, but I suppose it shouldn't be too bad. If we want to optimize for
that we could use usleep_range() and change the sleep duration to be
just a few scanlines.

>  	line2 = I915_READ(reg) & line_mask;
>  
>  	return line1 == line2;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6eb5d76e6912..1efac89cb738 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4255,7 +4255,7 @@ static void ironlake_enable_drps(struct drm_device *dev)
>  
>  	if (wait_for_atomic((I915_READ(MEMSWCTL) & MEMCTL_CMD_STS) == 0, 10))
>  		DRM_ERROR("stuck trying to change perf mode\n");
> -	mdelay(1);
> +	msleep(1);
>  
>  	ironlake_set_drps(dev, fstart);
>  
> @@ -4286,10 +4286,10 @@ static void ironlake_disable_drps(struct drm_device *dev)
>  
>  	/* Go back to the starting frequency */
>  	ironlake_set_drps(dev, dev_priv->ips.fstart);
> -	mdelay(1);
> +	msleep(1);
>  	rgvswctl |= MEMCTL_CMD_STS;
>  	I915_WRITE(MEMSWCTL, rgvswctl);
> -	mdelay(1);
> +	msleep(1);

Init/resume stuff. Doesn't seem too performance critical so msleep()
ought to be fine.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	spin_unlock_irq(&mchdev_lock);
>  }
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: s/mdelay/msleep/

[Daniel Vetter]

On Tue, Jul 07, 2015 at 11:03:10AM +0300, Ville Syrjälä wrote:
> On Tue, Jul 07, 2015 at 09:10:40AM +0200, Daniel Vetter wrote:
> > Burning cpu cycles isn't awesome, so use sleeps instead.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  drivers/gpu/drm/i915/intel_pm.c      | 6 +++---
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 56316c1e945c..87be3f307e40 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1026,7 +1026,7 @@ static bool pipe_dsl_stopped(struct drm_device *dev, enum pipe pipe)
> >  		line_mask = DSL_LINEMASK_GEN3;
> >  
> >  	line1 = I915_READ(reg) & line_mask;
> > -	mdelay(5);
> > +	msleep(5);
> 
> msleep() is quite inaccurate, but the accuracy of the delay shouldn't matter
> too much here. The only functional worry would be if it manages to sleep for
> exactly one frame and then we'd think DSL had stopped. But since we've already
> told the pipe to shut down, I don't think it should ever end up scanning out
> more than one frame here, so I suppose that problem should never happen.
> So yeah, this seems safe.
> 
> It might make the modeset a bit slower if it ends up sleeping for a long
> time, but I suppose it shouldn't be too bad. If we want to optimize for
> that we could use usleep_range() and change the sleep duration to be
> just a few scanlines.

What might be worth a shot is testing whether we get one last vblank when
the pipe goes off. Then we could augment the wait_for loop with a
wait_event_timeout based loop instead of just msleep. Still means we need
to sleep for a bit on gen2/3 to observe that the dsl really stopped, but
on gen4+ we might be able to proceed without delays.
> 
> >  	line2 = I915_READ(reg) & line_mask;
> >  
> >  	return line1 == line2;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 6eb5d76e6912..1efac89cb738 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4255,7 +4255,7 @@ static void ironlake_enable_drps(struct drm_device *dev)
> >  
> >  	if (wait_for_atomic((I915_READ(MEMSWCTL) & MEMCTL_CMD_STS) == 0, 10))
> >  		DRM_ERROR("stuck trying to change perf mode\n");
> > -	mdelay(1);
> > +	msleep(1);
> >  
> >  	ironlake_set_drps(dev, fstart);
> >  
> > @@ -4286,10 +4286,10 @@ static void ironlake_disable_drps(struct drm_device *dev)
> >  
> >  	/* Go back to the starting frequency */
> >  	ironlake_set_drps(dev, dev_priv->ips.fstart);
> > -	mdelay(1);
> > +	msleep(1);
> >  	rgvswctl |= MEMCTL_CMD_STS;
> >  	I915_WRITE(MEMSWCTL, rgvswctl);
> > -	mdelay(1);
> > +	msleep(1);
> 
> Init/resume stuff. Doesn't seem too performance critical so msleep()
> ought to be fine.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: s/mdelay/msleep/

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6738
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -3              287/287              284/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx