From: Jesse Barnes <jbarnes@virtuousgeek.org>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Nouveau Dev <nouveau@lists.freedesktop.org>,
Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
Radeon Dev <xorg-driver-ati@lists.x.org>,
DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 04/37] drm/i915: rework locking for intel_dpio|sbi_read|write
Date: Wed, 12 Dec 2012 12:54:47 -0800 [thread overview]
Message-ID: <20121212125447.728e791a@jbarnes-desktop> (raw)
In-Reply-To: <1355317637-16742-5-git-send-email-daniel.vetter@ffwll.ch>
On Wed, 12 Dec 2012 14:06:44 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Spinning for up to 200 us with interrupts locked out is not good. So
> let's just spin (and even that seems to be excessive).
>
> And we don't call these functions from interrupt context, so this is
> not required. Besides that doing anything in interrupt contexts which
> might take a few hundred us is a no-go. So just convert the entire
> thing to a mutex. Also move the mutex-grabbing out of the read/write
> functions (add a WARN_ON(!is_locked)) instead) since all callers are
> nicely grouped together.
>
> Finally the real motivation for this change: Dont grab the modeset
> mutex in the dpio debugfs file, we don't need that consistency. And
> correctness of the dpio interface is ensured with the dpio_lock.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 4 +--
> drivers/gpu/drm/i915/i915_dma.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++--------------------
> 4 files changed, 25 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 58e6676..35d2ace 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1553,7 +1553,7 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> return 0;
> }
>
> - ret = mutex_lock_interruptible(&dev->mode_config.mutex);
> + ret = mutex_lock_interruptible(&dev_priv->dpio_lock);
> if (ret)
> return ret;
>
> @@ -1582,7 +1582,7 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> seq_printf(m, "DPIO_FASTCLK_DISABLE: 0x%08x\n",
> intel_dpio_read(dev_priv, DPIO_FASTCLK_DISABLE));
>
> - mutex_unlock(&dev->mode_config.mutex);
> + mutex_unlock(&dev_priv->dpio_lock);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 2635ee6..ad488f6 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1579,7 +1579,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> spin_lock_init(&dev_priv->irq_lock);
> spin_lock_init(&dev_priv->error_lock);
> spin_lock_init(&dev_priv->rps.lock);
> - spin_lock_init(&dev_priv->dpio_lock);
> + mutex_init(&dev_priv->dpio_lock);
>
> mutex_init(&dev_priv->rps.hw_lock);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e2944e9..6fa0c00 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -655,7 +655,7 @@ typedef struct drm_i915_private {
> spinlock_t irq_lock;
>
> /* DPIO indirect register protection */
> - spinlock_t dpio_lock;
> + struct mutex dpio_lock;
>
> /** Cached value of IMR to avoid reads in updating the bitfield */
> u32 pipestat[2];
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d303f2a..a0d8869 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -416,13 +416,11 @@ static const intel_limit_t intel_limits_vlv_dp = {
>
> u32 intel_dpio_read(struct drm_i915_private *dev_priv, int reg)
> {
> - unsigned long flags;
> - u32 val = 0;
> + WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
>
> - spin_lock_irqsave(&dev_priv->dpio_lock, flags);
> if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
> DRM_ERROR("DPIO idle wait timed out\n");
> - goto out_unlock;
> + return 0;
> }
>
> I915_WRITE(DPIO_REG, reg);
> @@ -430,24 +428,20 @@ u32 intel_dpio_read(struct drm_i915_private *dev_priv, int reg)
> DPIO_BYTE);
> if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
> DRM_ERROR("DPIO read wait timed out\n");
> - goto out_unlock;
> + return 0;
> }
> - val = I915_READ(DPIO_DATA);
>
> -out_unlock:
> - spin_unlock_irqrestore(&dev_priv->dpio_lock, flags);
> - return val;
> + return I915_READ(DPIO_DATA);
> }
>
> static void intel_dpio_write(struct drm_i915_private *dev_priv, int reg,
> u32 val)
> {
> - unsigned long flags;
> + WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
>
> - spin_lock_irqsave(&dev_priv->dpio_lock, flags);
> if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100)) {
> DRM_ERROR("DPIO idle wait timed out\n");
> - goto out_unlock;
> + return;
> }
>
> I915_WRITE(DPIO_DATA, val);
> @@ -456,9 +450,6 @@ static void intel_dpio_write(struct drm_i915_private *dev_priv, int reg,
> DPIO_BYTE);
> if (wait_for_atomic_us((I915_READ(DPIO_PKT) & DPIO_BUSY) == 0, 100))
> DRM_ERROR("DPIO write wait timed out\n");
> -
> -out_unlock:
> - spin_unlock_irqrestore(&dev_priv->dpio_lock, flags);
> }
>
> static void vlv_init_dpio(struct drm_device *dev)
> @@ -1455,13 +1446,12 @@ static void intel_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> static void
> intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value)
> {
> - unsigned long flags;
> + WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
>
> - spin_lock_irqsave(&dev_priv->dpio_lock, flags);
> if (wait_for((I915_READ(SBI_CTL_STAT) & SBI_BUSY) == 0,
> 100)) {
> DRM_ERROR("timeout waiting for SBI to become ready\n");
> - goto out_unlock;
> + return;
> }
>
> I915_WRITE(SBI_ADDR,
> @@ -1475,24 +1465,19 @@ intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value)
> if (wait_for((I915_READ(SBI_CTL_STAT) & (SBI_BUSY | SBI_RESPONSE_FAIL)) == 0,
> 100)) {
> DRM_ERROR("timeout waiting for SBI to complete write transaction\n");
> - goto out_unlock;
> + return;
> }
> -
> -out_unlock:
> - spin_unlock_irqrestore(&dev_priv->dpio_lock, flags);
> }
>
> static u32
> intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg)
> {
> - unsigned long flags;
> - u32 value = 0;
> + WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
>
> - spin_lock_irqsave(&dev_priv->dpio_lock, flags);
> if (wait_for((I915_READ(SBI_CTL_STAT) & SBI_BUSY) == 0,
> 100)) {
> DRM_ERROR("timeout waiting for SBI to become ready\n");
> - goto out_unlock;
> + return 0;
> }
>
> I915_WRITE(SBI_ADDR,
> @@ -1504,14 +1489,10 @@ intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg)
> if (wait_for((I915_READ(SBI_CTL_STAT) & (SBI_BUSY | SBI_RESPONSE_FAIL)) == 0,
> 100)) {
> DRM_ERROR("timeout waiting for SBI to complete read transaction\n");
> - goto out_unlock;
> + return 0;
> }
>
> - value = I915_READ(SBI_DATA);
> -
> -out_unlock:
> - spin_unlock_irqrestore(&dev_priv->dpio_lock, flags);
> - return value;
> + return I915_READ(SBI_DATA);
> }
>
> /**
> @@ -2960,6 +2941,8 @@ static void lpt_program_iclkip(struct drm_crtc *crtc)
> u32 divsel, phaseinc, auxdiv, phasedir = 0;
> u32 temp;
>
> + mutex_lock(&dev_priv->dpio_lock);
> +
> /* It is necessary to ungate the pixclk gate prior to programming
> * the divisors, and gate it back when it is done.
> */
> @@ -3041,6 +3024,8 @@ static void lpt_program_iclkip(struct drm_crtc *crtc)
> udelay(24);
>
> I915_WRITE(PIXCLK_GATE, PIXCLK_GATE_UNGATE);
> +
> + mutex_unlock(&dev_priv->dpio_lock);
> }
>
> /*
> @@ -4268,6 +4253,8 @@ static void vlv_update_pll(struct drm_crtc *crtc,
> bool is_sdvo;
> u32 temp;
>
> + mutex_lock(&dev_priv->dpio_lock);
> +
> is_sdvo = intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ||
> intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI);
>
> @@ -4351,6 +4338,8 @@ static void vlv_update_pll(struct drm_crtc *crtc,
> temp |= (1 << 21);
> intel_dpio_write(dev_priv, DPIO_DATA_CHANNEL2, temp);
> }
> +
> + mutex_unlock(&dev_priv->dpio_lock);
> }
>
> static void i9xx_update_pll(struct drm_crtc *crtc,
Looks fine, I guess you could convert the wait_for_atomic_us to the
non-atomic variant now that you have a mutex. Either way:
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
next prev parent reply other threads:[~2012-12-12 20:54 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-12 13:06 [PATCH 00/37] [RFC] revamped modeset locking Daniel Vetter
2012-12-12 13:06 ` [PATCH 01/37] drm: review locking rules in drm_crtc.c Daniel Vetter
2012-12-12 13:06 ` [PATCH 05/37] drm/i915: use drm_modeset_lock_all Daniel Vetter
2012-12-12 13:06 ` [PATCH 08/37] drm/shmobile: " Daniel Vetter
2012-12-12 13:06 ` [PATCH 10/37] drm: add per-crtc locks Daniel Vetter
2012-12-13 11:38 ` [Intel-gfx] " Ville Syrjälä
2012-12-13 11:54 ` Daniel Vetter
2012-12-13 14:25 ` Ville Syrjälä
2012-12-12 13:06 ` [PATCH 11/37] drm/radeon: add W|RREG32_IDX for MM_INDEX|DATA based mmio accesss Daniel Vetter
2012-12-12 13:07 ` [PATCH 25/37] drm: don't take modeset locks in getfb ioctl Daniel Vetter
2012-12-12 13:07 ` [PATCH 26/37] drm: fb refcounting for dirtyfb_ioctl Daniel Vetter
2012-12-12 13:07 ` [PATCH 31/37] drm/vmwgfx: add proper framebuffer refcounting Daniel Vetter
2012-12-12 13:07 ` [PATCH 33/37] drm/nouveau: try to protect nbo->pin_refcount Daniel Vetter
[not found] ` <1355317637-16742-1-git-send-email-daniel.vetter-/w4YWyX8dFk@public.gmane.org>
2012-12-12 13:06 ` [PATCH 02/37] drm/doc: integrate drm_crtc.c kerneldoc Daniel Vetter
2012-12-12 13:06 ` [PATCH 03/37] drm: add drm_modeset_lock|unlock_all Daniel Vetter
2012-12-12 13:06 ` [PATCH 04/37] drm/i915: rework locking for intel_dpio|sbi_read|write Daniel Vetter
2012-12-12 20:54 ` Jesse Barnes [this message]
2012-12-12 22:00 ` Daniel Vetter
2012-12-12 22:05 ` Jesse Barnes
2012-12-12 13:06 ` [PATCH 06/37] drm/gma500: use drm_modeset_lock_all Daniel Vetter
2012-12-12 13:06 ` [PATCH 07/37] drm/ast: " Daniel Vetter
2012-12-12 13:06 ` [PATCH 09/37] drm/vmgfx: " Daniel Vetter
2012-12-12 13:06 ` [PATCH 12/37] drm/radeon: make indirect register access concurrency-safe Daniel Vetter
2012-12-12 13:06 ` [PATCH 13/37] drm/nouveau: protect evo_wait/evo_kick sections with a channel mutex Daniel Vetter
2012-12-12 13:06 ` [PATCH 14/37] drm: only take the crtc lock for ->cursor_set Daniel Vetter
2012-12-12 13:06 ` [PATCH 15/37] drm: only take the crtc lock for ->cursor_move Daniel Vetter
2012-12-13 11:03 ` [PATCH] " Daniel Vetter
2012-12-12 13:06 ` [PATCH 16/37] drm/<drivers>: reorder framebuffer init sequence Daniel Vetter
2012-12-13 11:05 ` [PATCH 1/2] " Daniel Vetter
[not found] ` <1355396719-25286-1-git-send-email-daniel.vetter-/w4YWyX8dFk@public.gmane.org>
2012-12-13 11:05 ` [PATCH 2/2] drm/exynos: " Daniel Vetter
2012-12-13 12:26 ` Inki Dae
2012-12-13 12:43 ` Daniel Vetter
2012-12-13 15:16 ` Inki Dae
2012-12-13 16:38 ` Daniel Vetter
2012-12-14 4:57 ` Inki Dae
2012-12-14 8:40 ` Daniel Vetter
2012-12-12 13:06 ` [PATCH 17/37] drm: revamp locking around fb creation/destruction Daniel Vetter
2012-12-12 13:06 ` [PATCH 18/37] drm: create drm_framebuffer_lookup Daniel Vetter
2012-12-12 13:06 ` [PATCH 19/37] drm/gma500: move fbcon restore to lastclose Daniel Vetter
2012-12-12 13:07 ` [PATCH 20/37] drm: revamp framebuffer cleanup interfaces Daniel Vetter
2012-12-12 13:07 ` [PATCH 21/37] drm: reference framebuffers which are on the idr Daniel Vetter
2012-12-12 13:07 ` [PATCH 22/37] drm: nest modeset locks within fpriv->fbs_lock Daniel Vetter
2012-12-12 13:07 ` [PATCH 23/37] drm/i915: fixup overlay stolen memory leak Daniel Vetter
2012-12-12 13:07 ` [PATCH 24/37] drm: push modeset_lock_all into ->fb_create driver callbacks Daniel Vetter
2012-12-12 13:07 ` [PATCH 27/37] drm: refcounting for sprite framebuffers Daniel Vetter
2012-12-12 13:07 ` [PATCH 28/37] drm: encapsulate crtc->set_config calls Daniel Vetter
2012-12-12 13:07 ` [PATCH 29/37] drm: refcounting for crtc framebuffers Daniel Vetter
2012-12-12 13:07 ` [PATCH 30/37] drm/i915: dump refcount into framebuffer debugfs file Daniel Vetter
2012-12-12 13:07 ` [PATCH 32/37] drm: optimize drm_framebuffer_remove Daniel Vetter
2012-12-12 13:07 ` [PATCH 34/37] drm/ttm: fix fence locking in ttm_buffer_object_transfer Daniel Vetter
2012-12-12 14:48 ` Jerome Glisse
[not found] ` <CAH3drwbL34+omj2S3ArOPgoYRQ3uzPEiRPFLn7YkP3cOMnPiZg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-12-12 15:42 ` Daniel Vetter
2012-12-13 11:06 ` [PATCH] allow shmob+imx drm drivers to be compiled Daniel Vetter
2012-12-13 11:18 ` Daniel Vetter
2012-12-13 11:26 ` [PATCH] drm/ttm: fix fence locking in ttm_buffer_object_transfer Daniel Vetter
2012-12-12 13:07 ` [PATCH 35/37] drm/radeon: fix fence locking in the pageflip callback Daniel Vetter
2012-12-12 13:07 ` [PATCH 36/37] drm: only grab the crtc lock for pageflips Daniel Vetter
2012-12-12 13:07 ` [PATCH 37/37] drm: don't hold crtc mutexes for connector ->detect callbacks Daniel Vetter
2012-12-12 14:18 ` [PATCH 00/37] [RFC] revamped modeset locking Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20121212125447.728e791a@jbarnes-desktop \
--to=jbarnes@virtuousgeek.org \
--cc=daniel.vetter@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=nouveau@lists.freedesktop.org \
--cc=xorg-driver-ati@lists.x.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.