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

[Daniel Vetter <daniel@ffwll.ch>]

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