From patchwork Wed Jul 26 13:26:47 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 9864795 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 7BDE1602B1 for ; Wed, 26 Jul 2017 13:26:58 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 87B672873D for ; Wed, 26 Jul 2017 13:26:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 798D628758; Wed, 26 Jul 2017 13:26:58 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 846482873D for ; Wed, 26 Jul 2017 13:26:57 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id F2E456E776; Wed, 26 Jul 2017 13:26:56 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com [IPv6:2a00:1450:400c:c09::242]) by gabe.freedesktop.org (Postfix) with ESMTPS id 379E36E776 for ; Wed, 26 Jul 2017 13:26:55 +0000 (UTC) Received: by mail-wm0-x242.google.com with SMTP id r123so10622453wmb.5 for ; Wed, 26 Jul 2017 06:26:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id; bh=IiDOBWnJ38jL8t4e230Fk0NYM9RS/NocgDZZTtpmmm8=; b=ItMRsREAvgUSoxWMZoH0jyry4pZ4vx5xTqDJjTnYFd2EHqiMiVwKM4MfxpL/EHgB2V f4G4MU/7On7EWH4lCLjxBoxRnbMoatpI9ITR5ReWGaQ8Qhqmg5cwm7k/lQ/Kg80eeokY JDgbwRtIKHZmAf54weVJPgEYheSIsxhLgk2Yw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=IiDOBWnJ38jL8t4e230Fk0NYM9RS/NocgDZZTtpmmm8=; b=Lh6r2WOsjZ3//ESOLzRqJDaGEV6dblFaUSrlLNzIkU6bovrWQxEMKjwtr0GVdTpjQE tq+yEGaehGHmKJrFqi5IFIoucaG/bkwhwa0OZes86DjdmAZ732EHICRj3YLiQ18vzGdR ypFKcv054hUPxIStOaJlMCLyAP4rMksfMr7tK3RrqL2sO7IuraGefKhAJsHmkOYDVzCB 0faNdNUCR2Y/ANJipzFhGak+zZAC7OPMlPeyW6WdR09VqMOyTx8ILlc7c0/WcACWMCf6 dRhidYZF+xTZx7WVTbf/ae+JW13oSOOYdQSdI0MhjcRbkaQ2B3pGFeYjhVhX0PtpRoO6 zVlQ== X-Gm-Message-State: AIVw110WKQoM3Jqpgve1gydFXPtsYyCpU/Xq+fXSXlt214wCtWoVYcCL JFBUQjWBE8JLltb13F8= X-Received: by 10.80.224.200 with SMTP id j8mr843037edl.230.1501075613487; Wed, 26 Jul 2017 06:26:53 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:5640:0:960b:2678:e223:c1c6]) by smtp.gmail.com with ESMTPSA id k8sm7919775edb.12.2017.07.26.06.26.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 26 Jul 2017 06:26:52 -0700 (PDT) From: Daniel Vetter To: Intel Graphics Development Date: Wed, 26 Jul 2017 15:26:47 +0200 Message-Id: <20170726132647.31833-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.13.3 Cc: Daniel Vetter , Daniel Vetter Subject: [Intel-gfx] [PATCH] drm/i915: Rework sdvo proxy i2c locking X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP lockdep complaints about a locking recursion for the i2c bus lock because both the sdvo ddc proxy bus and the gmbus nested within use the same locking class. It's not really a deadlock since we never nest the other way round, but it's annoying. Fix it by pulling the gmbus locking into the i2c lock_ops for both i2c_adapater and making sure that the ddc_proxy_xfer function is entirely lockless. Re-layouting the extracted function resulted in some whitespace cleanups, I figured we might as well keep them. v2: Review from Chris: - s/locked/unlocked/ since I got the naming backwards - Use the vfuncs of the proxied adatper instead of re-rolling copies. That's more consistent with the other proxying we're doing. Cc: Chris Wilson Reviewed-by: Chris Wilson Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_i2c.c | 36 +++++++++++++++++++++-- drivers/gpu/drm/i915/intel_sdvo.c | 62 +++++++++++++++++++++++++++++++-------- 2 files changed, 84 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 3c9e00d4ba5a..6698826954e1 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -592,7 +592,6 @@ gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) int ret; intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - mutex_lock(&dev_priv->gmbus_mutex); if (bus->force_bit) { ret = i2c_bit_algo.master_xfer(adapter, msgs, num); @@ -604,7 +603,6 @@ gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) bus->force_bit |= GMBUS_FORCE_BIT_RETRY; } - mutex_unlock(&dev_priv->gmbus_mutex); intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); return ret; @@ -624,6 +622,39 @@ static const struct i2c_algorithm gmbus_algorithm = { .functionality = gmbus_func }; +static void gmbus_lock_bus(struct i2c_adapter *adapter, + unsigned int flags) +{ + struct intel_gmbus *bus = to_intel_gmbus(adapter); + struct drm_i915_private *dev_priv = bus->dev_priv; + + mutex_lock(&dev_priv->gmbus_mutex); +} + +static int gmbus_trylock_bus(struct i2c_adapter *adapter, + unsigned int flags) +{ + struct intel_gmbus *bus = to_intel_gmbus(adapter); + struct drm_i915_private *dev_priv = bus->dev_priv; + + return mutex_trylock(&dev_priv->gmbus_mutex); +} + +static void gmbus_unlock_bus(struct i2c_adapter *adapter, + unsigned int flags) +{ + struct intel_gmbus *bus = to_intel_gmbus(adapter); + struct drm_i915_private *dev_priv = bus->dev_priv; + + mutex_unlock(&dev_priv->gmbus_mutex); +} + +const struct i2c_lock_operations gmbus_lock_ops = { + .lock_bus = gmbus_lock_bus, + .trylock_bus = gmbus_trylock_bus, + .unlock_bus = gmbus_unlock_bus, +}; + /** * intel_gmbus_setup - instantiate all Intel i2c GMBuses * @dev_priv: i915 device private @@ -665,6 +696,7 @@ int intel_setup_gmbus(struct drm_i915_private *dev_priv) bus->dev_priv = dev_priv; bus->adapter.algo = &gmbus_algorithm; + bus->adapter.lock_ops = &gmbus_lock_ops; /* * We wish to retry with bit banging diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index bea8152ae859..f09221dcd4c0 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -451,23 +451,24 @@ static const char * const cmd_status_names[] = { "Scaling not supported" }; -static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd, - const void *args, int args_len) +static bool __intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd, + const void *args, int args_len, + bool unlocked) { u8 *buf, status; struct i2c_msg *msgs; int i, ret = true; - /* Would be simpler to allocate both in one go ? */ + /* Would be simpler to allocate both in one go ? */ buf = kzalloc(args_len * 2 + 2, GFP_KERNEL); if (!buf) return false; msgs = kcalloc(args_len + 3, sizeof(*msgs), GFP_KERNEL); if (!msgs) { - kfree(buf); + kfree(buf); return false; - } + } intel_sdvo_debug_write(intel_sdvo, cmd, args, args_len); @@ -498,7 +499,10 @@ static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd, msgs[i+2].len = 1; msgs[i+2].buf = &status; - ret = i2c_transfer(intel_sdvo->i2c, msgs, i+3); + if (unlocked) + ret = i2c_transfer(intel_sdvo->i2c, msgs, i+3); + else + ret = __i2c_transfer(intel_sdvo->i2c, msgs, i+3); if (ret < 0) { DRM_DEBUG_KMS("I2c transfer returned %d\n", ret); ret = false; @@ -516,6 +520,12 @@ static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd, return ret; } +static bool intel_sdvo_write_cmd(struct intel_sdvo *intel_sdvo, u8 cmd, + const void *args, int args_len) +{ + return __intel_sdvo_write_cmd(intel_sdvo, cmd, args, args_len, true); +} + static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, void *response, int response_len) { @@ -602,13 +612,13 @@ static int intel_sdvo_get_pixel_multiplier(const struct drm_display_mode *adjust return 4; } -static bool intel_sdvo_set_control_bus_switch(struct intel_sdvo *intel_sdvo, - u8 ddc_bus) +static bool __intel_sdvo_set_control_bus_switch(struct intel_sdvo *intel_sdvo, + u8 ddc_bus) { /* This must be the immediately preceding write before the i2c xfer */ - return intel_sdvo_write_cmd(intel_sdvo, - SDVO_CMD_SET_CONTROL_BUS_SWITCH, - &ddc_bus, 1); + return __intel_sdvo_write_cmd(intel_sdvo, + SDVO_CMD_SET_CONTROL_BUS_SWITCH, + &ddc_bus, 1, false); } static bool intel_sdvo_set_value(struct intel_sdvo *intel_sdvo, u8 cmd, const void *data, int len) @@ -2924,7 +2934,7 @@ static int intel_sdvo_ddc_proxy_xfer(struct i2c_adapter *adapter, { struct intel_sdvo *sdvo = adapter->algo_data; - if (!intel_sdvo_set_control_bus_switch(sdvo, sdvo->ddc_bus)) + if (!__intel_sdvo_set_control_bus_switch(sdvo, sdvo->ddc_bus)) return -EIO; return sdvo->i2c->algo->master_xfer(sdvo->i2c, msgs, num); @@ -2941,6 +2951,33 @@ static const struct i2c_algorithm intel_sdvo_ddc_proxy = { .functionality = intel_sdvo_ddc_proxy_func }; +static void proxy_lock_bus(struct i2c_adapter *adapter, + unsigned int flags) +{ + struct intel_sdvo *sdvo = adapter->algo_data; + sdvo->i2c->lock_ops->lock_bus(sdvo->i2c, flags); +} + +static int proxy_trylock_bus(struct i2c_adapter *adapter, + unsigned int flags) +{ + struct intel_sdvo *sdvo = adapter->algo_data; + return sdvo->i2c->lock_ops->trylock_bus(sdvo->i2c, flags); +} + +static void proxy_unlock_bus(struct i2c_adapter *adapter, + unsigned int flags) +{ + struct intel_sdvo *sdvo = adapter->algo_data; + sdvo->i2c->lock_ops->unlock_bus(sdvo->i2c, flags); +} + +const struct i2c_lock_operations proxy_lock_ops = { + .lock_bus = proxy_lock_bus, + .trylock_bus = proxy_trylock_bus, + .unlock_bus = proxy_unlock_bus, +}; + static bool intel_sdvo_init_ddc_proxy(struct intel_sdvo *sdvo, struct drm_i915_private *dev_priv) @@ -2953,6 +2990,7 @@ intel_sdvo_init_ddc_proxy(struct intel_sdvo *sdvo, sdvo->ddc.dev.parent = &pdev->dev; sdvo->ddc.algo_data = sdvo; sdvo->ddc.algo = &intel_sdvo_ddc_proxy; + sdvo->ddc.lock_ops = &proxy_lock_ops; return i2c_add_adapter(&sdvo->ddc) == 0; }