Date: Thu, 15 Dec 2016 10:25:19 +0100
From: Daniel Vetter <daniel@ffwll.ch>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Nicholas Mc Guire <hofrat@osadl.org>,
        Daniel Vetter <daniel.vetter@intel.com>,
        David Airlie <airlied@linux.ie>, intel-gfx@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: use udelay for very short delays
Message-ID: <20161215092519.7lzcx75oo3xjcaop@phenom.ffwll.local>
Mail-Followup-To: Jani Nikula <jani.nikula@linux.intel.com>,
        Nicholas Mc Guire <hofrat@osadl.org>,
        Daniel Vetter <daniel.vetter@intel.com>,
        David Airlie <airlied@linux.ie>, intel-gfx@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
References: <1481776147-23041-1-git-send-email-hofrat@osadl.org>
 <87wpf1pnj2.fsf@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87wpf1pnj2.fsf@intel.com>
User-Agent: NeoMutt/20161126 (1.7.1)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2761
Lines: 74

On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote:
> On Thu, 15 Dec 2016, Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > Even on fast systems a 2 microsecond delay is most likely more efficient
> > as a busy-wait loop. The overhead of a hrtimer does not seem warranted -
> > change this to a udelay(2).
> 
> Similar concerns as in [1]. We don't need the accuracy of udelay() here,
> so this boils down to which is the better use of CPU. We could probably
> relax the max delay more if that was helpful. But I'm not immediately
> sold on "is most likely more efficient" which sounds like a gut feeling.
> 
> I'm sorry it's not clear in my other reply that I do appreciate
> addressing incorrect/silly use of usleep_range(); I'm just not (yet)
> convinced udelay() is the answer.

So one reason why we unconditionally use *sleep variants is the
might_sleep check. Because in the past people have used udelay and mdelay,
those delays had to be increased a lot because hw, and at the same time
someone added users of these functions to our irq helper, resulting in irq
handling times measures in multiple ms. That's not good.

So until someone can demonstrate that there's a real benefit (which let's
be honest, for modeset code, will never be the case) I very highly prefer
to use *sleep* functions. They prevent some silly things from happening by
accident. Micro-optimizing modeset code and hampering maitainability in
the process is not good.
-Daniel

> 
> BR,
> Jani.
> 
> 
> [1] http://lkml.kernel.org/r/8737hpr32a.fsf@intel.com
> 
> 
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> >
> > Problem found by coccinelle:
> >
> > Patch was compile tested with: x86_64_defconfig (implies CONFIG_DRM_I915)
> >
> > Patch is against 4.9.0 (localversion-next is next-20161214)
> >
> >  drivers/gpu/drm/i915/intel_dsi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index 5b72c50..19fe86b 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -379,7 +379,7 @@ static void bxt_dsi_device_ready(struct intel_encoder *encoder)
> >  		val &= ~ULPS_STATE_MASK;
> >  		val |= (ULPS_STATE_ENTER | DEVICE_READY);
> >  		I915_WRITE(MIPI_DEVICE_READY(port), val);
> > -		usleep_range(2, 3);
> > +		udelay(2);
> >  
> >  		/* 3. Exit ULPS */
> >  		val = I915_READ(MIPI_DEVICE_READY(port));
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch