* [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-01-26 2:01 ` Stephen Boyd
0 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-01-26 2:01 UTC (permalink / raw)
To: Rob Clark
Cc: linux-kernel, linux-arm-msm, freedreno, dri-devel, Krishna Manikandan
Lockdep complains about an AA deadlock when rebooting the device.
============================================
WARNING: possible recursive locking detected
5.4.91 #1 Not tainted
--------------------------------------------
reboot/5213 is trying to acquire lock:
ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
but task is already holding lock:
ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&kms->commit_lock[i]);
lock(&kms->commit_lock[i]);
*** DEADLOCK ***
May be due to missing lock nesting notation
6 locks held by reboot/5213:
__arm64_sys_reboot+0x148/0x2a0
device_shutdown+0x10c/0x2c4
drm_atomic_helper_shutdown+0x48/0xfc
modeset_lock+0x120/0x24c
lock_crtcs+0x60/0xa4
stack backtrace:
CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
Hardware name: Google Pompom (rev1) with LTE (DT)
Call trace:
dump_backtrace+0x0/0x1dc
show_stack+0x24/0x30
dump_stack+0xfc/0x1a8
__lock_acquire+0xcd0/0x22b8
lock_acquire+0x1ec/0x240
__mutex_lock_common+0xe0/0xc84
mutex_lock_nested+0x48/0x58
lock_crtcs+0x60/0xa4
msm_atomic_commit_tail+0x348/0x570
commit_tail+0xdc/0x178
drm_atomic_helper_commit+0x160/0x168
drm_atomic_commit+0x68/0x80
This is because lockdep thinks all the locks taken in lock_crtcs() are
the same lock, when they actually aren't. That's because we call
mutex_init() in msm_kms_init() and that assigns on static key for every
lock initialized in this loop. Let's allocate a dynamic number of
lock_class_keys and assign them to each lock so that lockdep can figure
out an AA deadlock isn't possible here.
Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
Cc: Krishna Manikandan <mkrishn@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index d8151a89e163..4735251a394d 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -157,6 +157,7 @@ struct msm_kms {
* from the crtc's pending_timer close to end of the frame:
*/
struct mutex commit_lock[MAX_CRTCS];
+ struct lock_class_key commit_lock_keys[MAX_CRTCS];
unsigned pending_crtc_mask;
struct msm_pending_timer pending_timers[MAX_CRTCS];
};
@@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
{
unsigned i, ret;
- for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
- mutex_init(&kms->commit_lock[i]);
+ for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
+ lockdep_register_key(&kms->commit_lock_keys[i]);
+ __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
+ &kms->commit_lock_keys[i]);
+ }
kms->funcs = funcs;
base-commit: 19c329f6808995b142b3966301f217c831e7cf31
--
https://chromeos.dev
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-01-26 2:01 ` Stephen Boyd
0 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-01-26 2:01 UTC (permalink / raw)
To: Rob Clark
Cc: linux-arm-msm, freedreno, linux-kernel, dri-devel, Krishna Manikandan
Lockdep complains about an AA deadlock when rebooting the device.
============================================
WARNING: possible recursive locking detected
5.4.91 #1 Not tainted
--------------------------------------------
reboot/5213 is trying to acquire lock:
ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
but task is already holding lock:
ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&kms->commit_lock[i]);
lock(&kms->commit_lock[i]);
*** DEADLOCK ***
May be due to missing lock nesting notation
6 locks held by reboot/5213:
__arm64_sys_reboot+0x148/0x2a0
device_shutdown+0x10c/0x2c4
drm_atomic_helper_shutdown+0x48/0xfc
modeset_lock+0x120/0x24c
lock_crtcs+0x60/0xa4
stack backtrace:
CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
Hardware name: Google Pompom (rev1) with LTE (DT)
Call trace:
dump_backtrace+0x0/0x1dc
show_stack+0x24/0x30
dump_stack+0xfc/0x1a8
__lock_acquire+0xcd0/0x22b8
lock_acquire+0x1ec/0x240
__mutex_lock_common+0xe0/0xc84
mutex_lock_nested+0x48/0x58
lock_crtcs+0x60/0xa4
msm_atomic_commit_tail+0x348/0x570
commit_tail+0xdc/0x178
drm_atomic_helper_commit+0x160/0x168
drm_atomic_commit+0x68/0x80
This is because lockdep thinks all the locks taken in lock_crtcs() are
the same lock, when they actually aren't. That's because we call
mutex_init() in msm_kms_init() and that assigns on static key for every
lock initialized in this loop. Let's allocate a dynamic number of
lock_class_keys and assign them to each lock so that lockdep can figure
out an AA deadlock isn't possible here.
Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
Cc: Krishna Manikandan <mkrishn@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index d8151a89e163..4735251a394d 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -157,6 +157,7 @@ struct msm_kms {
* from the crtc's pending_timer close to end of the frame:
*/
struct mutex commit_lock[MAX_CRTCS];
+ struct lock_class_key commit_lock_keys[MAX_CRTCS];
unsigned pending_crtc_mask;
struct msm_pending_timer pending_timers[MAX_CRTCS];
};
@@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
{
unsigned i, ret;
- for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
- mutex_init(&kms->commit_lock[i]);
+ for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
+ lockdep_register_key(&kms->commit_lock_keys[i]);
+ __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
+ &kms->commit_lock_keys[i]);
+ }
kms->funcs = funcs;
base-commit: 19c329f6808995b142b3966301f217c831e7cf31
--
https://chromeos.dev
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv2 0/3] iio: Add a ChromeOS EC MKBP proximity driver
2021-01-26 2:01 ` Stephen Boyd
(?)
@ 2021-01-26 2:01 ` Stephen Boyd
-1 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-01-26 2:01 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel, linux-iio, Dmitry Torokhov, Benson Leung,
Guenter Roeck, Douglas Anderson, Gwendal Grignou, devicetree,
Rob Herring
This is a different approach to [1] where I tried to add this proximity
sensor logic to the input subsystem. Instead, we'll take the approach of
making a small IIO proximity driver that parses the EC switch bitmap to
find out if the front proximity sensor is detecting something or not.
This allows us to treat proximity sensors as IIO devices all the time in
userspace instead of handling this switch on the EC via the input
subsystem and then other proximity sensors via IIO.
I propose this is all merged through IIO subsystem. Please ack
the first patch so it can be merged that way.
Changes from v1:
* Driver moved location
* Put mkbp everywhere
* Fixed up DT binding to not fail and make sure is a child of cros-ec
* Simplified logic for sending a message
* Dropped CONFIG_OF usage
* Sorted includes
[1] https://lore.kernel.org/r/20201205004709.3126266-1-swboyd@chromium.org
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: <devicetree@vger.kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Stephen Boyd (3):
platform/chrome: cros_ec: Add SW_FRONT_PROXIMITY MKBP define
dt-bindings: iio: Add cros ec proximity yaml doc
iio: proximity: Add a ChromeOS EC MKBP proximity driver
.../google,cros-ec-mkbp-proximity.yaml | 38 +++
.../bindings/mfd/google,cros-ec.yaml | 3 +
drivers/iio/proximity/Kconfig | 11 +
drivers/iio/proximity/Makefile | 1 +
.../iio/proximity/cros_ec_mkbp_proximity.c | 243 ++++++++++++++++++
.../linux/platform_data/cros_ec_commands.h | 1 +
6 files changed, 297 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/proximity/google,cros-ec-mkbp-proximity.yaml
create mode 100644 drivers/iio/proximity/cros_ec_mkbp_proximity.c
base-commit: 19c329f6808995b142b3966301f217c831e7cf31
--
https://chromeos.dev
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/3] platform/chrome: cros_ec: Add SW_FRONT_PROXIMITY MKBP define
2021-01-26 2:01 ` Stephen Boyd
(?)
(?)
@ 2021-01-26 2:01 ` Stephen Boyd
-1 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-01-26 2:01 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel, linux-iio, Dmitry Torokhov, Benson Leung,
Guenter Roeck, Douglas Anderson, Gwendal Grignou
Some cros ECs support a front proximity MKBP event via
'EC_MKBP_FRONT_PROXIMITY'. Add this define so it can be used in a
future patch.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
include/linux/platform_data/cros_ec_commands.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 86376779ab31..776e0b2be0e9 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -3457,6 +3457,7 @@ struct ec_response_get_next_event_v1 {
#define EC_MKBP_LID_OPEN 0
#define EC_MKBP_TABLET_MODE 1
#define EC_MKBP_BASE_ATTACHED 2
+#define EC_MKBP_FRONT_PROXIMITY 3
/* Run keyboard factory test scanning */
#define EC_CMD_KEYBOARD_FACTORY_TEST 0x0068
--
https://chromeos.dev
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/3] dt-bindings: iio: Add cros ec proximity yaml doc
2021-01-26 2:01 ` Stephen Boyd
` (2 preceding siblings ...)
(?)
@ 2021-01-26 2:01 ` Stephen Boyd
-1 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-01-26 2:01 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel, linux-iio, Dmitry Torokhov, Benson Leung,
Guenter Roeck, Douglas Anderson, Gwendal Grignou, devicetree,
Rob Herring
Some cros ECs support a front proximity MKBP event via
'EC_MKBP_FRONT_PROXIMITY'. Add a DT binding to document this feature via
a node that is a child of the main cros_ec device node. Devices that
have this ability will describe this in firmware.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: <devicetree@vger.kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
Changes from v1:
* Added additionalProperties
* Included proximity in cros-ec yaml
.../google,cros-ec-mkbp-proximity.yaml | 38 +++++++++++++++++++
.../bindings/mfd/google,cros-ec.yaml | 3 ++
2 files changed, 41 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/proximity/google,cros-ec-mkbp-proximity.yaml
diff --git a/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-mkbp-proximity.yaml b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-mkbp-proximity.yaml
new file mode 100644
index 000000000000..c3141c2be286
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-mkbp-proximity.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/iio/proximity/google,cros-ec-mkbp-proximity.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ChromeOS EC MKBP Proximity Sensor
+
+maintainers:
+ - Stephen Boyd <swboyd@chromium.org>
+ - Benson Leung <bleung@chromium.org>
+ - Enric Balletbo i Serra <enric.balletbo@collabora.com>
+
+description: |
+ Google's ChromeOS EC sometimes has the ability to detect user proximity.
+ This is implemented on the EC as near/far logic and exposed to the OS
+ via an MKBP switch bit.
+
+properties:
+ compatible:
+ const: google,cros-ec-mkbp-proximity
+
+ label:
+ description: Name for proximity sensor
+
+required:
+ - compatible
+
+unevaluatedProperties: false
+additionalProperties: false
+
+examples:
+ - |
+ proximity {
+ compatible = "google,cros-ec-mkbp-proximity";
+ label = "proximity-wifi-lte";
+ };
diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
index 76bf16ee27ec..479a9f15de32 100644
--- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
+++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
@@ -94,6 +94,9 @@ properties:
keyboard-controller:
$ref: "/schemas/input/google,cros-ec-keyb.yaml#"
+ proximity:
+ $ref: "/schemas/iio/proximity/google,cros-ec-mkbp-proximity.yaml#"
+
codecs:
type: object
additionalProperties: false
--
https://chromeos.dev
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver
2021-01-26 2:01 ` Stephen Boyd
` (3 preceding siblings ...)
(?)
@ 2021-01-26 2:01 ` Stephen Boyd
-1 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-01-26 2:01 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel, linux-iio, Dmitry Torokhov, Benson Leung,
Guenter Roeck, Douglas Anderson, Gwendal Grignou
Add support for a ChromeOS EC proximity driver that exposes a "front"
proximity sensor via the IIO subsystem. The EC decides when front
proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to
notify the kernel of proximity. Similarly, when proximity detects
something far away it sets the switch bit to 0. For now this driver
exposes a single sensor, but it could be expanded in the future via more
MKBP bits if desired.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
Changes from v1:
* Sorted includes
* Renamed to have MKBP everywhere
* Use last_event_time for timestamp
* Dropped claim calls
* Dropped useless dev assignment
drivers/iio/proximity/Kconfig | 11 +
drivers/iio/proximity/Makefile | 1 +
.../iio/proximity/cros_ec_mkbp_proximity.c | 243 ++++++++++++++++++
3 files changed, 255 insertions(+)
create mode 100644 drivers/iio/proximity/cros_ec_mkbp_proximity.c
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 12672a0e89ed..7c7203ca3ac6 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -21,6 +21,17 @@ endmenu
menu "Proximity and distance sensors"
+config CROS_EC_MKBP_PROXIMITY
+ tristate "ChromeOS EC MKBP Proximity sensor"
+ depends on CROS_EC
+ help
+ Say Y here to enable the proximity sensor implemented via the ChromeOS EC MKBP
+ switches protocol. You must enable one bus option (CROS_EC_I2C or CROS_EC_SPI)
+ to use this.
+
+ To compile this driver as a module, choose M here: the
+ module will be called cros_ec_mkbp_proximity.
+
config ISL29501
tristate "Intersil ISL29501 Time Of Flight sensor"
depends on I2C
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index 9c1aca1a8b79..cbdac09433eb 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -5,6 +5,7 @@
# When adding new entries keep the list in alphabetical order
obj-$(CONFIG_AS3935) += as3935.o
+obj-$(CONFIG_CROS_EC_MKBP_PROXIMITY) += cros_ec_mkbp_proximity.o
obj-$(CONFIG_ISL29501) += isl29501.o
obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
obj-$(CONFIG_MB1232) += mb1232.o
diff --git a/drivers/iio/proximity/cros_ec_mkbp_proximity.c b/drivers/iio/proximity/cros_ec_mkbp_proximity.c
new file mode 100644
index 000000000000..3c85d1843e3b
--- /dev/null
+++ b/drivers/iio/proximity/cros_ec_mkbp_proximity.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for cros-ec proximity sensor exposed through MKBP switch
+ *
+ * Copyright 2021 Google LLC.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#include <asm/unaligned.h>
+
+struct cros_ec_mkbp_proximity_data {
+ struct cros_ec_device *ec;
+ struct iio_dev *indio_dev;
+ struct mutex lock;
+ struct notifier_block notifier;
+ bool enabled;
+};
+
+static const struct iio_event_spec cros_ec_mkbp_proximity_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
+static const struct iio_chan_spec cros_ec_mkbp_proximity_chan_spec[] = {
+ {
+ .type = IIO_PROXIMITY,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .event_spec = cros_ec_mkbp_proximity_events,
+ .num_event_specs = ARRAY_SIZE(cros_ec_mkbp_proximity_events),
+ },
+};
+
+static int cros_ec_mkbp_proximity_parse_state(const void *data)
+{
+ u32 switches = get_unaligned_le32(data);
+
+ return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY));
+}
+
+static int cros_ec_mkbp_proximity_query(struct cros_ec_device *ec_dev,
+ int *state)
+{
+ struct {
+ struct cros_ec_command msg;
+ union {
+ struct ec_params_mkbp_info params;
+ u32 switches;
+ };
+ } __packed buf = { };
+ struct ec_params_mkbp_info *params = &buf.params;
+ struct cros_ec_command *msg = &buf.msg;
+ u32 *switches = &buf.switches;
+ size_t insize = sizeof(*switches);
+ int ret;
+
+ msg->command = EC_CMD_MKBP_INFO;
+ msg->version = 1;
+ msg->outsize = sizeof(*params);
+ msg->insize = insize;
+
+ params->info_type = EC_MKBP_INFO_CURRENT;
+ params->event_type = EC_MKBP_EVENT_SWITCH;
+
+ ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+ if (ret < 0)
+ return ret;
+
+ if (ret != insize) {
+ dev_warn(ec_dev->dev, "wrong result size: %d != %zu\n", ret,
+ insize);
+ return -EPROTO;
+ }
+
+ *state = cros_ec_mkbp_proximity_parse_state(switches);
+ return IIO_VAL_INT;
+}
+
+static int cros_ec_mkbp_proximity_notify(struct notifier_block *nb,
+ unsigned long queued_during_suspend,
+ void *_ec)
+{
+ struct cros_ec_mkbp_proximity_data *data;
+ struct cros_ec_device *ec = _ec;
+ u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
+ void *switches = &ec->event_data.data.switches;
+ struct iio_dev *indio_dev;
+ s64 timestamp;
+ int state, dir;
+ u64 ev;
+
+ if (event_type == EC_MKBP_EVENT_SWITCH) {
+ data = container_of(nb, struct cros_ec_mkbp_proximity_data,
+ notifier);
+ indio_dev = data->indio_dev;
+
+ mutex_lock(&data->lock);
+ if (data->enabled) {
+ timestamp = ktime_to_ns(ec->last_event_time);
+ state = cros_ec_mkbp_proximity_parse_state(switches);
+ dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+
+ ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+ IIO_EV_TYPE_THRESH, dir);
+ iio_push_event(indio_dev, ev, timestamp);
+ }
+ mutex_unlock(&data->lock);
+ }
+
+ return NOTIFY_OK;
+}
+
+static int cros_ec_mkbp_proximity_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, int *val,
+ int *val2, long mask)
+{
+ struct cros_ec_mkbp_proximity_data *data = iio_priv(indio_dev);
+ struct cros_ec_device *ec = data->ec;
+
+ if (chan->type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ return cros_ec_mkbp_proximity_query(ec, val);
+ }
+
+ return -EINVAL;
+}
+
+static int cros_ec_mkbp_proximity_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct cros_ec_mkbp_proximity_data *data = iio_priv(indio_dev);
+
+ return data->enabled;
+}
+
+static int cros_ec_mkbp_proximity_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir, int state)
+{
+ struct cros_ec_mkbp_proximity_data *data = iio_priv(indio_dev);
+
+ mutex_lock(&data->lock);
+ data->enabled = state;
+ mutex_unlock(&data->lock);
+
+ return 0;
+}
+
+static const struct iio_info cros_ec_mkbp_proximity_info = {
+ .read_raw = cros_ec_mkbp_proximity_read_raw,
+ .read_event_config = cros_ec_mkbp_proximity_read_event_config,
+ .write_event_config = cros_ec_mkbp_proximity_write_event_config,
+};
+
+static int cros_ec_mkbp_proximity_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cros_ec_device *ec = dev_get_drvdata(dev->parent);
+ struct iio_dev *indio_dev;
+ struct cros_ec_mkbp_proximity_data *data;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ data->ec = ec;
+ data->indio_dev = indio_dev;
+ mutex_init(&data->lock);
+ platform_set_drvdata(pdev, data);
+
+ indio_dev->name = dev->driver->name;
+ indio_dev->info = &cros_ec_mkbp_proximity_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = cros_ec_mkbp_proximity_chan_spec;
+ indio_dev->num_channels = ARRAY_SIZE(cros_ec_mkbp_proximity_chan_spec);
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ return ret;
+
+ data->notifier.notifier_call = cros_ec_mkbp_proximity_notify;
+ ret = blocking_notifier_chain_register(&ec->event_notifier,
+ &data->notifier);
+ if (ret)
+ dev_err(dev, "cannot register notifier: %d\n", ret);
+
+ return ret;
+}
+
+static int cros_ec_mkbp_proximity_remove(struct platform_device *pdev)
+{
+ struct cros_ec_mkbp_proximity_data *data = platform_get_drvdata(pdev);
+ struct cros_ec_device *ec = data->ec;
+
+ blocking_notifier_chain_unregister(&ec->event_notifier,
+ &data->notifier);
+
+ return 0;
+}
+
+static const struct of_device_id cros_ec_mkbp_proximity_of_match[] = {
+ { .compatible = "google,cros-ec-mkbp-proximity" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, cros_ec_mkbp_proximity_of_match);
+
+static struct platform_driver cros_ec_mkbp_proximity_driver = {
+ .driver = {
+ .name = "cros-ec-mkbp-proximity",
+ .of_match_table = of_match_ptr(cros_ec_mkbp_proximity_of_match),
+ },
+ .probe = cros_ec_mkbp_proximity_probe,
+ .remove = cros_ec_mkbp_proximity_remove,
+};
+module_platform_driver(cros_ec_mkbp_proximity_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ChromeOS EC MKBP proximity sensor driver");
--
https://chromeos.dev
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
2021-02-03 22:11 ` Rob Clark
@ 2021-02-04 15:17 ` Daniel Vetter
-1 siblings, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2021-02-04 15:17 UTC (permalink / raw)
To: Rob Clark
Cc: Stephen Boyd, Krishna Manikandan, freedreno,
Linux Kernel Mailing List, dri-devel, linux-arm-msm
On Wed, Feb 03, 2021 at 02:11:09PM -0800, Rob Clark wrote:
> On Wed, Feb 3, 2021 at 1:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Rob Clark (2021-02-03 09:29:09)
> > > On Wed, Feb 3, 2021 at 2:10 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> > > > > On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > >
> > > > > > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > > > > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > > > > > the same lock, when they actually aren't. That's because we call
> > > > > > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > > > > > lock initialized in this loop. Let's allocate a dynamic number of
> > > > > > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > > > > > out an AA deadlock isn't possible here.
> > > > > > >
> > > > > > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > > > > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > > > >
> > > > > > This smells like throwing more bad after initial bad code ...
> > > > > >
> > > > > > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
> > > >
> > > > Some technical on the patch itself: I think you want
> > > > mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
> > > > classes hand-rolled. It's defacto the same, but much more obviously
> > > > correct since self-documenting.
> > >
> > > hmm, yeah, that is a bit cleaner.. but this patch is already on
> > > msm-next, maybe I'll add a patch on top to change it
> >
> > How many CRTCs are there? The subclass number tops out at 8, per
> > MAX_LOCKDEP_SUBCLASSES so if we have more than that many bits possible
> > then it will fail.
Hm good point, tbh the mutex_lock_nested annotations isn't super awesome
either, it would be kinda neat if we could put that annotation into
mutex_lock_init fairly statically (and at that point we could allos resize
the array fairly easily I think at runtime).
The nice thing with the nesting index is just that it makes it a bit more
obvious that there's a static nesting going on and why it's ok.
-Daniel
> conveniently MAX_CRTCS is 8.. realistically I don't *think* you'd ever
> see more than 2 or 3
>
> BR,
> -R
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-02-04 15:17 ` Daniel Vetter
0 siblings, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2021-02-04 15:17 UTC (permalink / raw)
To: Rob Clark
Cc: Krishna Manikandan, linux-arm-msm, Linux Kernel Mailing List,
dri-devel, Stephen Boyd, freedreno
On Wed, Feb 03, 2021 at 02:11:09PM -0800, Rob Clark wrote:
> On Wed, Feb 3, 2021 at 1:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Rob Clark (2021-02-03 09:29:09)
> > > On Wed, Feb 3, 2021 at 2:10 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> > > > > On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > >
> > > > > > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > > > > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > > > > > the same lock, when they actually aren't. That's because we call
> > > > > > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > > > > > lock initialized in this loop. Let's allocate a dynamic number of
> > > > > > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > > > > > out an AA deadlock isn't possible here.
> > > > > > >
> > > > > > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > > > > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > > > >
> > > > > > This smells like throwing more bad after initial bad code ...
> > > > > >
> > > > > > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
> > > >
> > > > Some technical on the patch itself: I think you want
> > > > mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
> > > > classes hand-rolled. It's defacto the same, but much more obviously
> > > > correct since self-documenting.
> > >
> > > hmm, yeah, that is a bit cleaner.. but this patch is already on
> > > msm-next, maybe I'll add a patch on top to change it
> >
> > How many CRTCs are there? The subclass number tops out at 8, per
> > MAX_LOCKDEP_SUBCLASSES so if we have more than that many bits possible
> > then it will fail.
Hm good point, tbh the mutex_lock_nested annotations isn't super awesome
either, it would be kinda neat if we could put that annotation into
mutex_lock_init fairly statically (and at that point we could allos resize
the array fairly easily I think at runtime).
The nice thing with the nesting index is just that it makes it a bit more
obvious that there's a static nesting going on and why it's ok.
-Daniel
> conveniently MAX_CRTCS is 8.. realistically I don't *think* you'd ever
> see more than 2 or 3
>
> BR,
> -R
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
2021-02-03 21:58 ` Stephen Boyd
@ 2021-02-03 22:11 ` Rob Clark
-1 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2021-02-03 22:11 UTC (permalink / raw)
To: Stephen Boyd
Cc: Krishna Manikandan, Linux Kernel Mailing List, dri-devel,
freedreno, linux-arm-msm
On Wed, Feb 3, 2021 at 1:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rob Clark (2021-02-03 09:29:09)
> > On Wed, Feb 3, 2021 at 2:10 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> > > > On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > > > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > > > > the same lock, when they actually aren't. That's because we call
> > > > > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > > > > lock initialized in this loop. Let's allocate a dynamic number of
> > > > > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > > > > out an AA deadlock isn't possible here.
> > > > > >
> > > > > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > > > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > > >
> > > > > This smells like throwing more bad after initial bad code ...
> > > > >
> > > > > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
> > >
> > > Some technical on the patch itself: I think you want
> > > mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
> > > classes hand-rolled. It's defacto the same, but much more obviously
> > > correct since self-documenting.
> >
> > hmm, yeah, that is a bit cleaner.. but this patch is already on
> > msm-next, maybe I'll add a patch on top to change it
>
> How many CRTCs are there? The subclass number tops out at 8, per
> MAX_LOCKDEP_SUBCLASSES so if we have more than that many bits possible
> then it will fail.
conveniently MAX_CRTCS is 8.. realistically I don't *think* you'd ever
see more than 2 or 3
BR,
-R
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-02-03 22:11 ` Rob Clark
0 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2021-02-03 22:11 UTC (permalink / raw)
To: Stephen Boyd
Cc: Krishna Manikandan, freedreno, Linux Kernel Mailing List,
dri-devel, linux-arm-msm
On Wed, Feb 3, 2021 at 1:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rob Clark (2021-02-03 09:29:09)
> > On Wed, Feb 3, 2021 at 2:10 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> > > > On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > > > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > > > > the same lock, when they actually aren't. That's because we call
> > > > > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > > > > lock initialized in this loop. Let's allocate a dynamic number of
> > > > > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > > > > out an AA deadlock isn't possible here.
> > > > > >
> > > > > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > > > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > > >
> > > > > This smells like throwing more bad after initial bad code ...
> > > > >
> > > > > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
> > >
> > > Some technical on the patch itself: I think you want
> > > mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
> > > classes hand-rolled. It's defacto the same, but much more obviously
> > > correct since self-documenting.
> >
> > hmm, yeah, that is a bit cleaner.. but this patch is already on
> > msm-next, maybe I'll add a patch on top to change it
>
> How many CRTCs are there? The subclass number tops out at 8, per
> MAX_LOCKDEP_SUBCLASSES so if we have more than that many bits possible
> then it will fail.
conveniently MAX_CRTCS is 8.. realistically I don't *think* you'd ever
see more than 2 or 3
BR,
-R
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
2021-02-03 17:29 ` Rob Clark
@ 2021-02-03 21:58 ` Stephen Boyd
-1 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-02-03 21:58 UTC (permalink / raw)
To: Krishna Manikandan, Linux Kernel Mailing List, Rob Clark,
dri-devel, freedreno, linux-arm-msm
Quoting Rob Clark (2021-02-03 09:29:09)
> On Wed, Feb 3, 2021 at 2:10 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> > > On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > > > the same lock, when they actually aren't. That's because we call
> > > > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > > > lock initialized in this loop. Let's allocate a dynamic number of
> > > > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > > > out an AA deadlock isn't possible here.
> > > > >
> > > > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > >
> > > > This smells like throwing more bad after initial bad code ...
> > > >
> > > > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
> >
> > Some technical on the patch itself: I think you want
> > mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
> > classes hand-rolled. It's defacto the same, but much more obviously
> > correct since self-documenting.
>
> hmm, yeah, that is a bit cleaner.. but this patch is already on
> msm-next, maybe I'll add a patch on top to change it
How many CRTCs are there? The subclass number tops out at 8, per
MAX_LOCKDEP_SUBCLASSES so if we have more than that many bits possible
then it will fail.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-02-03 21:58 ` Stephen Boyd
0 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-02-03 21:58 UTC (permalink / raw)
To: Krishna Manikandan, Linux Kernel Mailing List, Rob Clark,
dri-devel, freedreno, linux-arm-msm
Quoting Rob Clark (2021-02-03 09:29:09)
> On Wed, Feb 3, 2021 at 2:10 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> > > On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > > > the same lock, when they actually aren't. That's because we call
> > > > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > > > lock initialized in this loop. Let's allocate a dynamic number of
> > > > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > > > out an AA deadlock isn't possible here.
> > > > >
> > > > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > >
> > > > This smells like throwing more bad after initial bad code ...
> > > >
> > > > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
> >
> > Some technical on the patch itself: I think you want
> > mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
> > classes hand-rolled. It's defacto the same, but much more obviously
> > correct since self-documenting.
>
> hmm, yeah, that is a bit cleaner.. but this patch is already on
> msm-next, maybe I'll add a patch on top to change it
How many CRTCs are there? The subclass number tops out at 8, per
MAX_LOCKDEP_SUBCLASSES so if we have more than that many bits possible
then it will fail.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
2021-02-03 10:10 ` Daniel Vetter
@ 2021-02-03 17:29 ` Rob Clark
-1 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2021-02-03 17:29 UTC (permalink / raw)
To: Rob Clark, Stephen Boyd, linux-arm-msm, freedreno,
Linux Kernel Mailing List, dri-devel, Krishna Manikandan
On Wed, Feb 3, 2021 at 2:10 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> > On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > > Lockdep complains about an AA deadlock when rebooting the device.
> > > >
> > > > ============================================
> > > > WARNING: possible recursive locking detected
> > > > 5.4.91 #1 Not tainted
> > > > --------------------------------------------
> > > > reboot/5213 is trying to acquire lock:
> > > > ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> > > >
> > > > but task is already holding lock:
> > > > ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> > > >
> > > > other info that might help us debug this:
> > > > Possible unsafe locking scenario:
> > > >
> > > > CPU0
> > > > ----
> > > > lock(&kms->commit_lock[i]);
> > > > lock(&kms->commit_lock[i]);
> > > >
> > > > *** DEADLOCK ***
> > > >
> > > > May be due to missing lock nesting notation
> > > >
> > > > 6 locks held by reboot/5213:
> > > > __arm64_sys_reboot+0x148/0x2a0
> > > > device_shutdown+0x10c/0x2c4
> > > > drm_atomic_helper_shutdown+0x48/0xfc
> > > > modeset_lock+0x120/0x24c
> > > > lock_crtcs+0x60/0xa4
> > > >
> > > > stack backtrace:
> > > > CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> > > > Hardware name: Google Pompom (rev1) with LTE (DT)
> > > > Call trace:
> > > > dump_backtrace+0x0/0x1dc
> > > > show_stack+0x24/0x30
> > > > dump_stack+0xfc/0x1a8
> > > > __lock_acquire+0xcd0/0x22b8
> > > > lock_acquire+0x1ec/0x240
> > > > __mutex_lock_common+0xe0/0xc84
> > > > mutex_lock_nested+0x48/0x58
> > > > lock_crtcs+0x60/0xa4
> > > > msm_atomic_commit_tail+0x348/0x570
> > > > commit_tail+0xdc/0x178
> > > > drm_atomic_helper_commit+0x160/0x168
> > > > drm_atomic_commit+0x68/0x80
> > > >
> > > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > > the same lock, when they actually aren't. That's because we call
> > > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > > lock initialized in this loop. Let's allocate a dynamic number of
> > > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > > out an AA deadlock isn't possible here.
> > > >
> > > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > >
> > > This smells like throwing more bad after initial bad code ...
> > >
> > > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
>
> Some technical on the patch itself: I think you want
> mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
> classes hand-rolled. It's defacto the same, but much more obviously
> correct since self-documenting.
hmm, yeah, that is a bit cleaner.. but this patch is already on
msm-next, maybe I'll add a patch on top to change it
> > > Yes I know the locking you're doing here is correct, but that goes to the
> > > second issue: Why is this needed? atomic_async_update helpers are supposed
> > > to take care of ordering fun like this, if they're not, we need to address
> > > things there. The problem that
> >
> > Maybe a better solution would be helper awareness of hw that has
> > double-buffered state and flush bits.. ie. something that looks a bit
> > more like the internal kms fxn ptrs. Currently the locking is
> > protecting something that the atomic helpers are not aware of, ie.
> > we've already written previous cursor updates to hw and are just
> > waiting until close to vblank to write the flush bits
> >
> > But, we've been over this before. I'd tried various approaches.. the
> > current scheme replaces seanpaul's earlier attempts to do it the
> > "helper" way. The current implementation does the best job of
> > avoiding fps drops when the legacy cursor uapi is in play. (And yes,
> > legacy cursor + atomic ioctls is maybe not the greatest, but it is
> > what it is.)
>
> I didn't read enough of the context and got confused, the flush handling
> looks all reasonable and obviously needs some locks to avoid races with
> updates.
>
> It still looks a bit strange that you need multi-crtc locks for cursor
> (generally this stuff is supposed to be solved with ordering) and why the
> async helpers don't work since msm has something that's pretty close
> itself. Atomic+cursor is a bit nasty, but if every driver hacks this
> together themselves then there's not much chance of this ever really
> working well across the board. And aside from the flush bit instead of
> automatic double buffering (which you're just emulating) there's not
> really anything special with msm afaics. So pretty sure that if this
> doesn't work for msm, it doesn't work anywhere else.
I did dig out a hub/keyboard/mouse for the one mtk device I have to
see how it behaves with cursor updates.. the fps drops are pretty
bad.. roughly the same as they were with msm prior to the current
"defer the flush bits" scheme. Maybe I'm just more picky than others.
Better helper support for this sort of hw would be useful, since I
think it is not uncommon. (At least omap is similar, I didn't really
look at mtk/rockchip.) If I had a clone to handle the display side of
things, my clone would work on that ;-)
BR,
-R
> -Daniel
>
> >
> > BR,
> > -R
> >
> > >
> > > commit b3d91800d9ac35014e0349292273a6fa7938d402
> > > Author: Krishna Manikandan <mkrishn@codeaurora.org>
> > > Date: Fri Oct 16 19:40:43 2020 +0530
> > >
> > > drm/msm: Fix race condition in msm driver with async layer updates
> > >
> > > is _the_ reason we have drm_crtc_commit to track stuff, and Maxime has
> > > recently rolled out a pile of changes to vc4 to use these things
> > > correctly. Hacking some glorious hand-rolled locking for synchronization
> > > of updates really should be the exception for kms drivers, not the rule.
> > > And this one here doesn't look like an exception by far (the one legit I
> > > know of is the locking issues amdgpu has between atomic_commit_tail and
> > > gpu reset, and that one is really nasty, so not going to get fixed in
> > > helpers, ever).
> > >
> > > Cheers, Daniel
> > >
> > > > ---
> > > > drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> > > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > > > index d8151a89e163..4735251a394d 100644
> > > > --- a/drivers/gpu/drm/msm/msm_kms.h
> > > > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > > > @@ -157,6 +157,7 @@ struct msm_kms {
> > > > * from the crtc's pending_timer close to end of the frame:
> > > > */
> > > > struct mutex commit_lock[MAX_CRTCS];
> > > > + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> > > > unsigned pending_crtc_mask;
> > > > struct msm_pending_timer pending_timers[MAX_CRTCS];
> > > > };
> > > > @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> > > > {
> > > > unsigned i, ret;
> > > >
> > > > - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> > > > - mutex_init(&kms->commit_lock[i]);
> > > > + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> > > > + lockdep_register_key(&kms->commit_lock_keys[i]);
> > > > + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> > > > + &kms->commit_lock_keys[i]);
> > > > + }
> > > >
> > > > kms->funcs = funcs;
> > > >
> > > >
> > > > base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> > > > --
> > > > https://chromeos.dev
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-02-03 17:29 ` Rob Clark
0 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2021-02-03 17:29 UTC (permalink / raw)
To: Rob Clark, Stephen Boyd, linux-arm-msm, freedreno,
Linux Kernel Mailing List, dri-devel, Krishna Manikandan
On Wed, Feb 3, 2021 at 2:10 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> > On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > > Lockdep complains about an AA deadlock when rebooting the device.
> > > >
> > > > ============================================
> > > > WARNING: possible recursive locking detected
> > > > 5.4.91 #1 Not tainted
> > > > --------------------------------------------
> > > > reboot/5213 is trying to acquire lock:
> > > > ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> > > >
> > > > but task is already holding lock:
> > > > ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> > > >
> > > > other info that might help us debug this:
> > > > Possible unsafe locking scenario:
> > > >
> > > > CPU0
> > > > ----
> > > > lock(&kms->commit_lock[i]);
> > > > lock(&kms->commit_lock[i]);
> > > >
> > > > *** DEADLOCK ***
> > > >
> > > > May be due to missing lock nesting notation
> > > >
> > > > 6 locks held by reboot/5213:
> > > > __arm64_sys_reboot+0x148/0x2a0
> > > > device_shutdown+0x10c/0x2c4
> > > > drm_atomic_helper_shutdown+0x48/0xfc
> > > > modeset_lock+0x120/0x24c
> > > > lock_crtcs+0x60/0xa4
> > > >
> > > > stack backtrace:
> > > > CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> > > > Hardware name: Google Pompom (rev1) with LTE (DT)
> > > > Call trace:
> > > > dump_backtrace+0x0/0x1dc
> > > > show_stack+0x24/0x30
> > > > dump_stack+0xfc/0x1a8
> > > > __lock_acquire+0xcd0/0x22b8
> > > > lock_acquire+0x1ec/0x240
> > > > __mutex_lock_common+0xe0/0xc84
> > > > mutex_lock_nested+0x48/0x58
> > > > lock_crtcs+0x60/0xa4
> > > > msm_atomic_commit_tail+0x348/0x570
> > > > commit_tail+0xdc/0x178
> > > > drm_atomic_helper_commit+0x160/0x168
> > > > drm_atomic_commit+0x68/0x80
> > > >
> > > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > > the same lock, when they actually aren't. That's because we call
> > > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > > lock initialized in this loop. Let's allocate a dynamic number of
> > > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > > out an AA deadlock isn't possible here.
> > > >
> > > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > >
> > > This smells like throwing more bad after initial bad code ...
> > >
> > > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
>
> Some technical on the patch itself: I think you want
> mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
> classes hand-rolled. It's defacto the same, but much more obviously
> correct since self-documenting.
hmm, yeah, that is a bit cleaner.. but this patch is already on
msm-next, maybe I'll add a patch on top to change it
> > > Yes I know the locking you're doing here is correct, but that goes to the
> > > second issue: Why is this needed? atomic_async_update helpers are supposed
> > > to take care of ordering fun like this, if they're not, we need to address
> > > things there. The problem that
> >
> > Maybe a better solution would be helper awareness of hw that has
> > double-buffered state and flush bits.. ie. something that looks a bit
> > more like the internal kms fxn ptrs. Currently the locking is
> > protecting something that the atomic helpers are not aware of, ie.
> > we've already written previous cursor updates to hw and are just
> > waiting until close to vblank to write the flush bits
> >
> > But, we've been over this before. I'd tried various approaches.. the
> > current scheme replaces seanpaul's earlier attempts to do it the
> > "helper" way. The current implementation does the best job of
> > avoiding fps drops when the legacy cursor uapi is in play. (And yes,
> > legacy cursor + atomic ioctls is maybe not the greatest, but it is
> > what it is.)
>
> I didn't read enough of the context and got confused, the flush handling
> looks all reasonable and obviously needs some locks to avoid races with
> updates.
>
> It still looks a bit strange that you need multi-crtc locks for cursor
> (generally this stuff is supposed to be solved with ordering) and why the
> async helpers don't work since msm has something that's pretty close
> itself. Atomic+cursor is a bit nasty, but if every driver hacks this
> together themselves then there's not much chance of this ever really
> working well across the board. And aside from the flush bit instead of
> automatic double buffering (which you're just emulating) there's not
> really anything special with msm afaics. So pretty sure that if this
> doesn't work for msm, it doesn't work anywhere else.
I did dig out a hub/keyboard/mouse for the one mtk device I have to
see how it behaves with cursor updates.. the fps drops are pretty
bad.. roughly the same as they were with msm prior to the current
"defer the flush bits" scheme. Maybe I'm just more picky than others.
Better helper support for this sort of hw would be useful, since I
think it is not uncommon. (At least omap is similar, I didn't really
look at mtk/rockchip.) If I had a clone to handle the display side of
things, my clone would work on that ;-)
BR,
-R
> -Daniel
>
> >
> > BR,
> > -R
> >
> > >
> > > commit b3d91800d9ac35014e0349292273a6fa7938d402
> > > Author: Krishna Manikandan <mkrishn@codeaurora.org>
> > > Date: Fri Oct 16 19:40:43 2020 +0530
> > >
> > > drm/msm: Fix race condition in msm driver with async layer updates
> > >
> > > is _the_ reason we have drm_crtc_commit to track stuff, and Maxime has
> > > recently rolled out a pile of changes to vc4 to use these things
> > > correctly. Hacking some glorious hand-rolled locking for synchronization
> > > of updates really should be the exception for kms drivers, not the rule.
> > > And this one here doesn't look like an exception by far (the one legit I
> > > know of is the locking issues amdgpu has between atomic_commit_tail and
> > > gpu reset, and that one is really nasty, so not going to get fixed in
> > > helpers, ever).
> > >
> > > Cheers, Daniel
> > >
> > > > ---
> > > > drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> > > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > > > index d8151a89e163..4735251a394d 100644
> > > > --- a/drivers/gpu/drm/msm/msm_kms.h
> > > > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > > > @@ -157,6 +157,7 @@ struct msm_kms {
> > > > * from the crtc's pending_timer close to end of the frame:
> > > > */
> > > > struct mutex commit_lock[MAX_CRTCS];
> > > > + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> > > > unsigned pending_crtc_mask;
> > > > struct msm_pending_timer pending_timers[MAX_CRTCS];
> > > > };
> > > > @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> > > > {
> > > > unsigned i, ret;
> > > >
> > > > - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> > > > - mutex_init(&kms->commit_lock[i]);
> > > > + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> > > > + lockdep_register_key(&kms->commit_lock_keys[i]);
> > > > + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> > > > + &kms->commit_lock_keys[i]);
> > > > + }
> > > >
> > > > kms->funcs = funcs;
> > > >
> > > >
> > > > base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> > > > --
> > > > https://chromeos.dev
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
2021-02-02 16:51 ` Rob Clark
@ 2021-02-03 10:10 ` Daniel Vetter
-1 siblings, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2021-02-03 10:10 UTC (permalink / raw)
To: Rob Clark
Cc: Stephen Boyd, linux-arm-msm, freedreno,
Linux Kernel Mailing List, dri-devel, Krishna Manikandan
On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > Lockdep complains about an AA deadlock when rebooting the device.
> > >
> > > ============================================
> > > WARNING: possible recursive locking detected
> > > 5.4.91 #1 Not tainted
> > > --------------------------------------------
> > > reboot/5213 is trying to acquire lock:
> > > ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> > >
> > > but task is already holding lock:
> > > ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> > >
> > > other info that might help us debug this:
> > > Possible unsafe locking scenario:
> > >
> > > CPU0
> > > ----
> > > lock(&kms->commit_lock[i]);
> > > lock(&kms->commit_lock[i]);
> > >
> > > *** DEADLOCK ***
> > >
> > > May be due to missing lock nesting notation
> > >
> > > 6 locks held by reboot/5213:
> > > __arm64_sys_reboot+0x148/0x2a0
> > > device_shutdown+0x10c/0x2c4
> > > drm_atomic_helper_shutdown+0x48/0xfc
> > > modeset_lock+0x120/0x24c
> > > lock_crtcs+0x60/0xa4
> > >
> > > stack backtrace:
> > > CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> > > Hardware name: Google Pompom (rev1) with LTE (DT)
> > > Call trace:
> > > dump_backtrace+0x0/0x1dc
> > > show_stack+0x24/0x30
> > > dump_stack+0xfc/0x1a8
> > > __lock_acquire+0xcd0/0x22b8
> > > lock_acquire+0x1ec/0x240
> > > __mutex_lock_common+0xe0/0xc84
> > > mutex_lock_nested+0x48/0x58
> > > lock_crtcs+0x60/0xa4
> > > msm_atomic_commit_tail+0x348/0x570
> > > commit_tail+0xdc/0x178
> > > drm_atomic_helper_commit+0x160/0x168
> > > drm_atomic_commit+0x68/0x80
> > >
> > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > the same lock, when they actually aren't. That's because we call
> > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > lock initialized in this loop. Let's allocate a dynamic number of
> > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > out an AA deadlock isn't possible here.
> > >
> > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> >
> > This smells like throwing more bad after initial bad code ...
> >
> > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
Some technical on the patch itself: I think you want
mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
classes hand-rolled. It's defacto the same, but much more obviously
correct since self-documenting.
> > Yes I know the locking you're doing here is correct, but that goes to the
> > second issue: Why is this needed? atomic_async_update helpers are supposed
> > to take care of ordering fun like this, if they're not, we need to address
> > things there. The problem that
>
> Maybe a better solution would be helper awareness of hw that has
> double-buffered state and flush bits.. ie. something that looks a bit
> more like the internal kms fxn ptrs. Currently the locking is
> protecting something that the atomic helpers are not aware of, ie.
> we've already written previous cursor updates to hw and are just
> waiting until close to vblank to write the flush bits
>
> But, we've been over this before. I'd tried various approaches.. the
> current scheme replaces seanpaul's earlier attempts to do it the
> "helper" way. The current implementation does the best job of
> avoiding fps drops when the legacy cursor uapi is in play. (And yes,
> legacy cursor + atomic ioctls is maybe not the greatest, but it is
> what it is.)
I didn't read enough of the context and got confused, the flush handling
looks all reasonable and obviously needs some locks to avoid races with
updates.
It still looks a bit strange that you need multi-crtc locks for cursor
(generally this stuff is supposed to be solved with ordering) and why the
async helpers don't work since msm has something that's pretty close
itself. Atomic+cursor is a bit nasty, but if every driver hacks this
together themselves then there's not much chance of this ever really
working well across the board. And aside from the flush bit instead of
automatic double buffering (which you're just emulating) there's not
really anything special with msm afaics. So pretty sure that if this
doesn't work for msm, it doesn't work anywhere else.
-Daniel
>
> BR,
> -R
>
> >
> > commit b3d91800d9ac35014e0349292273a6fa7938d402
> > Author: Krishna Manikandan <mkrishn@codeaurora.org>
> > Date: Fri Oct 16 19:40:43 2020 +0530
> >
> > drm/msm: Fix race condition in msm driver with async layer updates
> >
> > is _the_ reason we have drm_crtc_commit to track stuff, and Maxime has
> > recently rolled out a pile of changes to vc4 to use these things
> > correctly. Hacking some glorious hand-rolled locking for synchronization
> > of updates really should be the exception for kms drivers, not the rule.
> > And this one here doesn't look like an exception by far (the one legit I
> > know of is the locking issues amdgpu has between atomic_commit_tail and
> > gpu reset, and that one is really nasty, so not going to get fixed in
> > helpers, ever).
> >
> > Cheers, Daniel
> >
> > > ---
> > > drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > > index d8151a89e163..4735251a394d 100644
> > > --- a/drivers/gpu/drm/msm/msm_kms.h
> > > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > > @@ -157,6 +157,7 @@ struct msm_kms {
> > > * from the crtc's pending_timer close to end of the frame:
> > > */
> > > struct mutex commit_lock[MAX_CRTCS];
> > > + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> > > unsigned pending_crtc_mask;
> > > struct msm_pending_timer pending_timers[MAX_CRTCS];
> > > };
> > > @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> > > {
> > > unsigned i, ret;
> > >
> > > - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> > > - mutex_init(&kms->commit_lock[i]);
> > > + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> > > + lockdep_register_key(&kms->commit_lock_keys[i]);
> > > + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> > > + &kms->commit_lock_keys[i]);
> > > + }
> > >
> > > kms->funcs = funcs;
> > >
> > >
> > > base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> > > --
> > > https://chromeos.dev
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-02-03 10:10 ` Daniel Vetter
0 siblings, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2021-02-03 10:10 UTC (permalink / raw)
To: Rob Clark
Cc: Krishna Manikandan, linux-arm-msm, Linux Kernel Mailing List,
dri-devel, Stephen Boyd, freedreno
On Tue, Feb 02, 2021 at 08:51:25AM -0800, Rob Clark wrote:
> On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > > Lockdep complains about an AA deadlock when rebooting the device.
> > >
> > > ============================================
> > > WARNING: possible recursive locking detected
> > > 5.4.91 #1 Not tainted
> > > --------------------------------------------
> > > reboot/5213 is trying to acquire lock:
> > > ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> > >
> > > but task is already holding lock:
> > > ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> > >
> > > other info that might help us debug this:
> > > Possible unsafe locking scenario:
> > >
> > > CPU0
> > > ----
> > > lock(&kms->commit_lock[i]);
> > > lock(&kms->commit_lock[i]);
> > >
> > > *** DEADLOCK ***
> > >
> > > May be due to missing lock nesting notation
> > >
> > > 6 locks held by reboot/5213:
> > > __arm64_sys_reboot+0x148/0x2a0
> > > device_shutdown+0x10c/0x2c4
> > > drm_atomic_helper_shutdown+0x48/0xfc
> > > modeset_lock+0x120/0x24c
> > > lock_crtcs+0x60/0xa4
> > >
> > > stack backtrace:
> > > CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> > > Hardware name: Google Pompom (rev1) with LTE (DT)
> > > Call trace:
> > > dump_backtrace+0x0/0x1dc
> > > show_stack+0x24/0x30
> > > dump_stack+0xfc/0x1a8
> > > __lock_acquire+0xcd0/0x22b8
> > > lock_acquire+0x1ec/0x240
> > > __mutex_lock_common+0xe0/0xc84
> > > mutex_lock_nested+0x48/0x58
> > > lock_crtcs+0x60/0xa4
> > > msm_atomic_commit_tail+0x348/0x570
> > > commit_tail+0xdc/0x178
> > > drm_atomic_helper_commit+0x160/0x168
> > > drm_atomic_commit+0x68/0x80
> > >
> > > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > > the same lock, when they actually aren't. That's because we call
> > > mutex_init() in msm_kms_init() and that assigns on static key for every
> > > lock initialized in this loop. Let's allocate a dynamic number of
> > > lock_class_keys and assign them to each lock so that lockdep can figure
> > > out an AA deadlock isn't possible here.
> > >
> > > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> >
> > This smells like throwing more bad after initial bad code ...
> >
> > First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
Some technical on the patch itself: I think you want
mutex_lock_nested(crtc->lock, drm_crtc_index(crtc)), not your own locking
classes hand-rolled. It's defacto the same, but much more obviously
correct since self-documenting.
> > Yes I know the locking you're doing here is correct, but that goes to the
> > second issue: Why is this needed? atomic_async_update helpers are supposed
> > to take care of ordering fun like this, if they're not, we need to address
> > things there. The problem that
>
> Maybe a better solution would be helper awareness of hw that has
> double-buffered state and flush bits.. ie. something that looks a bit
> more like the internal kms fxn ptrs. Currently the locking is
> protecting something that the atomic helpers are not aware of, ie.
> we've already written previous cursor updates to hw and are just
> waiting until close to vblank to write the flush bits
>
> But, we've been over this before. I'd tried various approaches.. the
> current scheme replaces seanpaul's earlier attempts to do it the
> "helper" way. The current implementation does the best job of
> avoiding fps drops when the legacy cursor uapi is in play. (And yes,
> legacy cursor + atomic ioctls is maybe not the greatest, but it is
> what it is.)
I didn't read enough of the context and got confused, the flush handling
looks all reasonable and obviously needs some locks to avoid races with
updates.
It still looks a bit strange that you need multi-crtc locks for cursor
(generally this stuff is supposed to be solved with ordering) and why the
async helpers don't work since msm has something that's pretty close
itself. Atomic+cursor is a bit nasty, but if every driver hacks this
together themselves then there's not much chance of this ever really
working well across the board. And aside from the flush bit instead of
automatic double buffering (which you're just emulating) there's not
really anything special with msm afaics. So pretty sure that if this
doesn't work for msm, it doesn't work anywhere else.
-Daniel
>
> BR,
> -R
>
> >
> > commit b3d91800d9ac35014e0349292273a6fa7938d402
> > Author: Krishna Manikandan <mkrishn@codeaurora.org>
> > Date: Fri Oct 16 19:40:43 2020 +0530
> >
> > drm/msm: Fix race condition in msm driver with async layer updates
> >
> > is _the_ reason we have drm_crtc_commit to track stuff, and Maxime has
> > recently rolled out a pile of changes to vc4 to use these things
> > correctly. Hacking some glorious hand-rolled locking for synchronization
> > of updates really should be the exception for kms drivers, not the rule.
> > And this one here doesn't look like an exception by far (the one legit I
> > know of is the locking issues amdgpu has between atomic_commit_tail and
> > gpu reset, and that one is really nasty, so not going to get fixed in
> > helpers, ever).
> >
> > Cheers, Daniel
> >
> > > ---
> > > drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > > index d8151a89e163..4735251a394d 100644
> > > --- a/drivers/gpu/drm/msm/msm_kms.h
> > > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > > @@ -157,6 +157,7 @@ struct msm_kms {
> > > * from the crtc's pending_timer close to end of the frame:
> > > */
> > > struct mutex commit_lock[MAX_CRTCS];
> > > + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> > > unsigned pending_crtc_mask;
> > > struct msm_pending_timer pending_timers[MAX_CRTCS];
> > > };
> > > @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> > > {
> > > unsigned i, ret;
> > >
> > > - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> > > - mutex_init(&kms->commit_lock[i]);
> > > + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> > > + lockdep_register_key(&kms->commit_lock_keys[i]);
> > > + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> > > + &kms->commit_lock_keys[i]);
> > > + }
> > >
> > > kms->funcs = funcs;
> > >
> > >
> > > base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> > > --
> > > https://chromeos.dev
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
2021-02-02 15:46 ` Daniel Vetter
@ 2021-02-02 16:51 ` Rob Clark
-1 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2021-02-02 16:51 UTC (permalink / raw)
To: Stephen Boyd, Rob Clark, linux-arm-msm, freedreno,
Linux Kernel Mailing List, dri-devel, Krishna Manikandan
On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > Lockdep complains about an AA deadlock when rebooting the device.
> >
> > ============================================
> > WARNING: possible recursive locking detected
> > 5.4.91 #1 Not tainted
> > --------------------------------------------
> > reboot/5213 is trying to acquire lock:
> > ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> >
> > but task is already holding lock:
> > ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> >
> > other info that might help us debug this:
> > Possible unsafe locking scenario:
> >
> > CPU0
> > ----
> > lock(&kms->commit_lock[i]);
> > lock(&kms->commit_lock[i]);
> >
> > *** DEADLOCK ***
> >
> > May be due to missing lock nesting notation
> >
> > 6 locks held by reboot/5213:
> > __arm64_sys_reboot+0x148/0x2a0
> > device_shutdown+0x10c/0x2c4
> > drm_atomic_helper_shutdown+0x48/0xfc
> > modeset_lock+0x120/0x24c
> > lock_crtcs+0x60/0xa4
> >
> > stack backtrace:
> > CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> > Hardware name: Google Pompom (rev1) with LTE (DT)
> > Call trace:
> > dump_backtrace+0x0/0x1dc
> > show_stack+0x24/0x30
> > dump_stack+0xfc/0x1a8
> > __lock_acquire+0xcd0/0x22b8
> > lock_acquire+0x1ec/0x240
> > __mutex_lock_common+0xe0/0xc84
> > mutex_lock_nested+0x48/0x58
> > lock_crtcs+0x60/0xa4
> > msm_atomic_commit_tail+0x348/0x570
> > commit_tail+0xdc/0x178
> > drm_atomic_helper_commit+0x160/0x168
> > drm_atomic_commit+0x68/0x80
> >
> > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > the same lock, when they actually aren't. That's because we call
> > mutex_init() in msm_kms_init() and that assigns on static key for every
> > lock initialized in this loop. Let's allocate a dynamic number of
> > lock_class_keys and assign them to each lock so that lockdep can figure
> > out an AA deadlock isn't possible here.
> >
> > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
> This smells like throwing more bad after initial bad code ...
>
> First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
>
> Yes I know the locking you're doing here is correct, but that goes to the
> second issue: Why is this needed? atomic_async_update helpers are supposed
> to take care of ordering fun like this, if they're not, we need to address
> things there. The problem that
Maybe a better solution would be helper awareness of hw that has
double-buffered state and flush bits.. ie. something that looks a bit
more like the internal kms fxn ptrs. Currently the locking is
protecting something that the atomic helpers are not aware of, ie.
we've already written previous cursor updates to hw and are just
waiting until close to vblank to write the flush bits
But, we've been over this before. I'd tried various approaches.. the
current scheme replaces seanpaul's earlier attempts to do it the
"helper" way. The current implementation does the best job of
avoiding fps drops when the legacy cursor uapi is in play. (And yes,
legacy cursor + atomic ioctls is maybe not the greatest, but it is
what it is.)
BR,
-R
>
> commit b3d91800d9ac35014e0349292273a6fa7938d402
> Author: Krishna Manikandan <mkrishn@codeaurora.org>
> Date: Fri Oct 16 19:40:43 2020 +0530
>
> drm/msm: Fix race condition in msm driver with async layer updates
>
> is _the_ reason we have drm_crtc_commit to track stuff, and Maxime has
> recently rolled out a pile of changes to vc4 to use these things
> correctly. Hacking some glorious hand-rolled locking for synchronization
> of updates really should be the exception for kms drivers, not the rule.
> And this one here doesn't look like an exception by far (the one legit I
> know of is the locking issues amdgpu has between atomic_commit_tail and
> gpu reset, and that one is really nasty, so not going to get fixed in
> helpers, ever).
>
> Cheers, Daniel
>
> > ---
> > drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > index d8151a89e163..4735251a394d 100644
> > --- a/drivers/gpu/drm/msm/msm_kms.h
> > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > @@ -157,6 +157,7 @@ struct msm_kms {
> > * from the crtc's pending_timer close to end of the frame:
> > */
> > struct mutex commit_lock[MAX_CRTCS];
> > + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> > unsigned pending_crtc_mask;
> > struct msm_pending_timer pending_timers[MAX_CRTCS];
> > };
> > @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> > {
> > unsigned i, ret;
> >
> > - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> > - mutex_init(&kms->commit_lock[i]);
> > + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> > + lockdep_register_key(&kms->commit_lock_keys[i]);
> > + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> > + &kms->commit_lock_keys[i]);
> > + }
> >
> > kms->funcs = funcs;
> >
> >
> > base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> > --
> > https://chromeos.dev
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-02-02 16:51 ` Rob Clark
0 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2021-02-02 16:51 UTC (permalink / raw)
To: Stephen Boyd, Rob Clark, linux-arm-msm, freedreno,
Linux Kernel Mailing List, dri-devel, Krishna Manikandan
On Tue, Feb 2, 2021 at 7:46 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> > Lockdep complains about an AA deadlock when rebooting the device.
> >
> > ============================================
> > WARNING: possible recursive locking detected
> > 5.4.91 #1 Not tainted
> > --------------------------------------------
> > reboot/5213 is trying to acquire lock:
> > ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> >
> > but task is already holding lock:
> > ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
> >
> > other info that might help us debug this:
> > Possible unsafe locking scenario:
> >
> > CPU0
> > ----
> > lock(&kms->commit_lock[i]);
> > lock(&kms->commit_lock[i]);
> >
> > *** DEADLOCK ***
> >
> > May be due to missing lock nesting notation
> >
> > 6 locks held by reboot/5213:
> > __arm64_sys_reboot+0x148/0x2a0
> > device_shutdown+0x10c/0x2c4
> > drm_atomic_helper_shutdown+0x48/0xfc
> > modeset_lock+0x120/0x24c
> > lock_crtcs+0x60/0xa4
> >
> > stack backtrace:
> > CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> > Hardware name: Google Pompom (rev1) with LTE (DT)
> > Call trace:
> > dump_backtrace+0x0/0x1dc
> > show_stack+0x24/0x30
> > dump_stack+0xfc/0x1a8
> > __lock_acquire+0xcd0/0x22b8
> > lock_acquire+0x1ec/0x240
> > __mutex_lock_common+0xe0/0xc84
> > mutex_lock_nested+0x48/0x58
> > lock_crtcs+0x60/0xa4
> > msm_atomic_commit_tail+0x348/0x570
> > commit_tail+0xdc/0x178
> > drm_atomic_helper_commit+0x160/0x168
> > drm_atomic_commit+0x68/0x80
> >
> > This is because lockdep thinks all the locks taken in lock_crtcs() are
> > the same lock, when they actually aren't. That's because we call
> > mutex_init() in msm_kms_init() and that assigns on static key for every
> > lock initialized in this loop. Let's allocate a dynamic number of
> > lock_class_keys and assign them to each lock so that lockdep can figure
> > out an AA deadlock isn't possible here.
> >
> > Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> > Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
> This smells like throwing more bad after initial bad code ...
>
> First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
>
> Yes I know the locking you're doing here is correct, but that goes to the
> second issue: Why is this needed? atomic_async_update helpers are supposed
> to take care of ordering fun like this, if they're not, we need to address
> things there. The problem that
Maybe a better solution would be helper awareness of hw that has
double-buffered state and flush bits.. ie. something that looks a bit
more like the internal kms fxn ptrs. Currently the locking is
protecting something that the atomic helpers are not aware of, ie.
we've already written previous cursor updates to hw and are just
waiting until close to vblank to write the flush bits
But, we've been over this before. I'd tried various approaches.. the
current scheme replaces seanpaul's earlier attempts to do it the
"helper" way. The current implementation does the best job of
avoiding fps drops when the legacy cursor uapi is in play. (And yes,
legacy cursor + atomic ioctls is maybe not the greatest, but it is
what it is.)
BR,
-R
>
> commit b3d91800d9ac35014e0349292273a6fa7938d402
> Author: Krishna Manikandan <mkrishn@codeaurora.org>
> Date: Fri Oct 16 19:40:43 2020 +0530
>
> drm/msm: Fix race condition in msm driver with async layer updates
>
> is _the_ reason we have drm_crtc_commit to track stuff, and Maxime has
> recently rolled out a pile of changes to vc4 to use these things
> correctly. Hacking some glorious hand-rolled locking for synchronization
> of updates really should be the exception for kms drivers, not the rule.
> And this one here doesn't look like an exception by far (the one legit I
> know of is the locking issues amdgpu has between atomic_commit_tail and
> gpu reset, and that one is really nasty, so not going to get fixed in
> helpers, ever).
>
> Cheers, Daniel
>
> > ---
> > drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > index d8151a89e163..4735251a394d 100644
> > --- a/drivers/gpu/drm/msm/msm_kms.h
> > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > @@ -157,6 +157,7 @@ struct msm_kms {
> > * from the crtc's pending_timer close to end of the frame:
> > */
> > struct mutex commit_lock[MAX_CRTCS];
> > + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> > unsigned pending_crtc_mask;
> > struct msm_pending_timer pending_timers[MAX_CRTCS];
> > };
> > @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> > {
> > unsigned i, ret;
> >
> > - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> > - mutex_init(&kms->commit_lock[i]);
> > + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> > + lockdep_register_key(&kms->commit_lock_keys[i]);
> > + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> > + &kms->commit_lock_keys[i]);
> > + }
> >
> > kms->funcs = funcs;
> >
> >
> > base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> > --
> > https://chromeos.dev
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
2021-01-25 23:49 ` Stephen Boyd
@ 2021-02-02 15:46 ` Daniel Vetter
-1 siblings, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2021-02-02 15:46 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rob Clark, linux-arm-msm, freedreno, linux-kernel, dri-devel,
Krishna Manikandan
On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> Lockdep complains about an AA deadlock when rebooting the device.
>
> ============================================
> WARNING: possible recursive locking detected
> 5.4.91 #1 Not tainted
> --------------------------------------------
> reboot/5213 is trying to acquire lock:
> ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
>
> but task is already holding lock:
> ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&kms->commit_lock[i]);
> lock(&kms->commit_lock[i]);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 6 locks held by reboot/5213:
> __arm64_sys_reboot+0x148/0x2a0
> device_shutdown+0x10c/0x2c4
> drm_atomic_helper_shutdown+0x48/0xfc
> modeset_lock+0x120/0x24c
> lock_crtcs+0x60/0xa4
>
> stack backtrace:
> CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> Hardware name: Google Pompom (rev1) with LTE (DT)
> Call trace:
> dump_backtrace+0x0/0x1dc
> show_stack+0x24/0x30
> dump_stack+0xfc/0x1a8
> __lock_acquire+0xcd0/0x22b8
> lock_acquire+0x1ec/0x240
> __mutex_lock_common+0xe0/0xc84
> mutex_lock_nested+0x48/0x58
> lock_crtcs+0x60/0xa4
> msm_atomic_commit_tail+0x348/0x570
> commit_tail+0xdc/0x178
> drm_atomic_helper_commit+0x160/0x168
> drm_atomic_commit+0x68/0x80
>
> This is because lockdep thinks all the locks taken in lock_crtcs() are
> the same lock, when they actually aren't. That's because we call
> mutex_init() in msm_kms_init() and that assigns on static key for every
> lock initialized in this loop. Let's allocate a dynamic number of
> lock_class_keys and assign them to each lock so that lockdep can figure
> out an AA deadlock isn't possible here.
>
> Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
This smells like throwing more bad after initial bad code ...
First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
Yes I know the locking you're doing here is correct, but that goes to the
second issue: Why is this needed? atomic_async_update helpers are supposed
to take care of ordering fun like this, if they're not, we need to address
things there. The problem that
commit b3d91800d9ac35014e0349292273a6fa7938d402
Author: Krishna Manikandan <mkrishn@codeaurora.org>
Date: Fri Oct 16 19:40:43 2020 +0530
drm/msm: Fix race condition in msm driver with async layer updates
is _the_ reason we have drm_crtc_commit to track stuff, and Maxime has
recently rolled out a pile of changes to vc4 to use these things
correctly. Hacking some glorious hand-rolled locking for synchronization
of updates really should be the exception for kms drivers, not the rule.
And this one here doesn't look like an exception by far (the one legit I
know of is the locking issues amdgpu has between atomic_commit_tail and
gpu reset, and that one is really nasty, so not going to get fixed in
helpers, ever).
Cheers, Daniel
> ---
> drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index d8151a89e163..4735251a394d 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -157,6 +157,7 @@ struct msm_kms {
> * from the crtc's pending_timer close to end of the frame:
> */
> struct mutex commit_lock[MAX_CRTCS];
> + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> unsigned pending_crtc_mask;
> struct msm_pending_timer pending_timers[MAX_CRTCS];
> };
> @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> {
> unsigned i, ret;
>
> - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> - mutex_init(&kms->commit_lock[i]);
> + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> + lockdep_register_key(&kms->commit_lock_keys[i]);
> + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> + &kms->commit_lock_keys[i]);
> + }
>
> kms->funcs = funcs;
>
>
> base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> --
> https://chromeos.dev
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-02-02 15:46 ` Daniel Vetter
0 siblings, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2021-02-02 15:46 UTC (permalink / raw)
To: Stephen Boyd
Cc: Krishna Manikandan, linux-arm-msm, linux-kernel, dri-devel, freedreno
On Mon, Jan 25, 2021 at 03:49:01PM -0800, Stephen Boyd wrote:
> Lockdep complains about an AA deadlock when rebooting the device.
>
> ============================================
> WARNING: possible recursive locking detected
> 5.4.91 #1 Not tainted
> --------------------------------------------
> reboot/5213 is trying to acquire lock:
> ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
>
> but task is already holding lock:
> ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&kms->commit_lock[i]);
> lock(&kms->commit_lock[i]);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 6 locks held by reboot/5213:
> __arm64_sys_reboot+0x148/0x2a0
> device_shutdown+0x10c/0x2c4
> drm_atomic_helper_shutdown+0x48/0xfc
> modeset_lock+0x120/0x24c
> lock_crtcs+0x60/0xa4
>
> stack backtrace:
> CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> Hardware name: Google Pompom (rev1) with LTE (DT)
> Call trace:
> dump_backtrace+0x0/0x1dc
> show_stack+0x24/0x30
> dump_stack+0xfc/0x1a8
> __lock_acquire+0xcd0/0x22b8
> lock_acquire+0x1ec/0x240
> __mutex_lock_common+0xe0/0xc84
> mutex_lock_nested+0x48/0x58
> lock_crtcs+0x60/0xa4
> msm_atomic_commit_tail+0x348/0x570
> commit_tail+0xdc/0x178
> drm_atomic_helper_commit+0x160/0x168
> drm_atomic_commit+0x68/0x80
>
> This is because lockdep thinks all the locks taken in lock_crtcs() are
> the same lock, when they actually aren't. That's because we call
> mutex_init() in msm_kms_init() and that assigns on static key for every
> lock initialized in this loop. Let's allocate a dynamic number of
> lock_class_keys and assign them to each lock so that lockdep can figure
> out an AA deadlock isn't possible here.
>
> Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
This smells like throwing more bad after initial bad code ...
First a rant: https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
Yes I know the locking you're doing here is correct, but that goes to the
second issue: Why is this needed? atomic_async_update helpers are supposed
to take care of ordering fun like this, if they're not, we need to address
things there. The problem that
commit b3d91800d9ac35014e0349292273a6fa7938d402
Author: Krishna Manikandan <mkrishn@codeaurora.org>
Date: Fri Oct 16 19:40:43 2020 +0530
drm/msm: Fix race condition in msm driver with async layer updates
is _the_ reason we have drm_crtc_commit to track stuff, and Maxime has
recently rolled out a pile of changes to vc4 to use these things
correctly. Hacking some glorious hand-rolled locking for synchronization
of updates really should be the exception for kms drivers, not the rule.
And this one here doesn't look like an exception by far (the one legit I
know of is the locking issues amdgpu has between atomic_commit_tail and
gpu reset, and that one is really nasty, so not going to get fixed in
helpers, ever).
Cheers, Daniel
> ---
> drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index d8151a89e163..4735251a394d 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -157,6 +157,7 @@ struct msm_kms {
> * from the crtc's pending_timer close to end of the frame:
> */
> struct mutex commit_lock[MAX_CRTCS];
> + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> unsigned pending_crtc_mask;
> struct msm_pending_timer pending_timers[MAX_CRTCS];
> };
> @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> {
> unsigned i, ret;
>
> - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> - mutex_init(&kms->commit_lock[i]);
> + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> + lockdep_register_key(&kms->commit_lock_keys[i]);
> + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> + &kms->commit_lock_keys[i]);
> + }
>
> kms->funcs = funcs;
>
>
> base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> --
> https://chromeos.dev
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
2021-01-25 23:49 ` Stephen Boyd
@ 2021-01-28 16:39 ` Rob Clark
-1 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2021-01-28 16:39 UTC (permalink / raw)
To: Stephen Boyd
Cc: Linux Kernel Mailing List, linux-arm-msm, freedreno, dri-devel,
Krishna Manikandan
On Mon, Jan 25, 2021 at 3:49 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Lockdep complains about an AA deadlock when rebooting the device.
>
> ============================================
> WARNING: possible recursive locking detected
> 5.4.91 #1 Not tainted
> --------------------------------------------
> reboot/5213 is trying to acquire lock:
> ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
>
> but task is already holding lock:
> ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&kms->commit_lock[i]);
> lock(&kms->commit_lock[i]);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 6 locks held by reboot/5213:
> __arm64_sys_reboot+0x148/0x2a0
> device_shutdown+0x10c/0x2c4
> drm_atomic_helper_shutdown+0x48/0xfc
> modeset_lock+0x120/0x24c
> lock_crtcs+0x60/0xa4
>
> stack backtrace:
> CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> Hardware name: Google Pompom (rev1) with LTE (DT)
> Call trace:
> dump_backtrace+0x0/0x1dc
> show_stack+0x24/0x30
> dump_stack+0xfc/0x1a8
> __lock_acquire+0xcd0/0x22b8
> lock_acquire+0x1ec/0x240
> __mutex_lock_common+0xe0/0xc84
> mutex_lock_nested+0x48/0x58
> lock_crtcs+0x60/0xa4
> msm_atomic_commit_tail+0x348/0x570
> commit_tail+0xdc/0x178
> drm_atomic_helper_commit+0x160/0x168
> drm_atomic_commit+0x68/0x80
>
> This is because lockdep thinks all the locks taken in lock_crtcs() are
> the same lock, when they actually aren't. That's because we call
> mutex_init() in msm_kms_init() and that assigns on static key for every
nit, s/on/one/ ?
BR,
-R
> lock initialized in this loop. Let's allocate a dynamic number of
> lock_class_keys and assign them to each lock so that lockdep can figure
> out an AA deadlock isn't possible here.
>
> Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index d8151a89e163..4735251a394d 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -157,6 +157,7 @@ struct msm_kms {
> * from the crtc's pending_timer close to end of the frame:
> */
> struct mutex commit_lock[MAX_CRTCS];
> + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> unsigned pending_crtc_mask;
> struct msm_pending_timer pending_timers[MAX_CRTCS];
> };
> @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> {
> unsigned i, ret;
>
> - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> - mutex_init(&kms->commit_lock[i]);
> + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> + lockdep_register_key(&kms->commit_lock_keys[i]);
> + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> + &kms->commit_lock_keys[i]);
> + }
>
> kms->funcs = funcs;
>
>
> base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> --
> https://chromeos.dev
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-01-28 16:39 ` Rob Clark
0 siblings, 0 replies; 24+ messages in thread
From: Rob Clark @ 2021-01-28 16:39 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-arm-msm, freedreno, Linux Kernel Mailing List, dri-devel,
Krishna Manikandan
On Mon, Jan 25, 2021 at 3:49 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Lockdep complains about an AA deadlock when rebooting the device.
>
> ============================================
> WARNING: possible recursive locking detected
> 5.4.91 #1 Not tainted
> --------------------------------------------
> reboot/5213 is trying to acquire lock:
> ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
>
> but task is already holding lock:
> ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&kms->commit_lock[i]);
> lock(&kms->commit_lock[i]);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 6 locks held by reboot/5213:
> __arm64_sys_reboot+0x148/0x2a0
> device_shutdown+0x10c/0x2c4
> drm_atomic_helper_shutdown+0x48/0xfc
> modeset_lock+0x120/0x24c
> lock_crtcs+0x60/0xa4
>
> stack backtrace:
> CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
> Hardware name: Google Pompom (rev1) with LTE (DT)
> Call trace:
> dump_backtrace+0x0/0x1dc
> show_stack+0x24/0x30
> dump_stack+0xfc/0x1a8
> __lock_acquire+0xcd0/0x22b8
> lock_acquire+0x1ec/0x240
> __mutex_lock_common+0xe0/0xc84
> mutex_lock_nested+0x48/0x58
> lock_crtcs+0x60/0xa4
> msm_atomic_commit_tail+0x348/0x570
> commit_tail+0xdc/0x178
> drm_atomic_helper_commit+0x160/0x168
> drm_atomic_commit+0x68/0x80
>
> This is because lockdep thinks all the locks taken in lock_crtcs() are
> the same lock, when they actually aren't. That's because we call
> mutex_init() in msm_kms_init() and that assigns on static key for every
nit, s/on/one/ ?
BR,
-R
> lock initialized in this loop. Let's allocate a dynamic number of
> lock_class_keys and assign them to each lock so that lockdep can figure
> out an AA deadlock isn't possible here.
>
> Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
> Cc: Krishna Manikandan <mkrishn@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index d8151a89e163..4735251a394d 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -157,6 +157,7 @@ struct msm_kms {
> * from the crtc's pending_timer close to end of the frame:
> */
> struct mutex commit_lock[MAX_CRTCS];
> + struct lock_class_key commit_lock_keys[MAX_CRTCS];
> unsigned pending_crtc_mask;
> struct msm_pending_timer pending_timers[MAX_CRTCS];
> };
> @@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
> {
> unsigned i, ret;
>
> - for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
> - mutex_init(&kms->commit_lock[i]);
> + for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
> + lockdep_register_key(&kms->commit_lock_keys[i]);
> + __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
> + &kms->commit_lock_keys[i]);
> + }
>
> kms->funcs = funcs;
>
>
> base-commit: 19c329f6808995b142b3966301f217c831e7cf31
> --
> https://chromeos.dev
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-01-25 23:49 ` Stephen Boyd
0 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-01-25 23:49 UTC (permalink / raw)
To: Rob Clark
Cc: linux-kernel, linux-arm-msm, freedreno, dri-devel, Krishna Manikandan
Lockdep complains about an AA deadlock when rebooting the device.
============================================
WARNING: possible recursive locking detected
5.4.91 #1 Not tainted
--------------------------------------------
reboot/5213 is trying to acquire lock:
ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
but task is already holding lock:
ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&kms->commit_lock[i]);
lock(&kms->commit_lock[i]);
*** DEADLOCK ***
May be due to missing lock nesting notation
6 locks held by reboot/5213:
__arm64_sys_reboot+0x148/0x2a0
device_shutdown+0x10c/0x2c4
drm_atomic_helper_shutdown+0x48/0xfc
modeset_lock+0x120/0x24c
lock_crtcs+0x60/0xa4
stack backtrace:
CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
Hardware name: Google Pompom (rev1) with LTE (DT)
Call trace:
dump_backtrace+0x0/0x1dc
show_stack+0x24/0x30
dump_stack+0xfc/0x1a8
__lock_acquire+0xcd0/0x22b8
lock_acquire+0x1ec/0x240
__mutex_lock_common+0xe0/0xc84
mutex_lock_nested+0x48/0x58
lock_crtcs+0x60/0xa4
msm_atomic_commit_tail+0x348/0x570
commit_tail+0xdc/0x178
drm_atomic_helper_commit+0x160/0x168
drm_atomic_commit+0x68/0x80
This is because lockdep thinks all the locks taken in lock_crtcs() are
the same lock, when they actually aren't. That's because we call
mutex_init() in msm_kms_init() and that assigns on static key for every
lock initialized in this loop. Let's allocate a dynamic number of
lock_class_keys and assign them to each lock so that lockdep can figure
out an AA deadlock isn't possible here.
Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
Cc: Krishna Manikandan <mkrishn@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index d8151a89e163..4735251a394d 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -157,6 +157,7 @@ struct msm_kms {
* from the crtc's pending_timer close to end of the frame:
*/
struct mutex commit_lock[MAX_CRTCS];
+ struct lock_class_key commit_lock_keys[MAX_CRTCS];
unsigned pending_crtc_mask;
struct msm_pending_timer pending_timers[MAX_CRTCS];
};
@@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
{
unsigned i, ret;
- for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
- mutex_init(&kms->commit_lock[i]);
+ for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
+ lockdep_register_key(&kms->commit_lock_keys[i]);
+ __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
+ &kms->commit_lock_keys[i]);
+ }
kms->funcs = funcs;
base-commit: 19c329f6808995b142b3966301f217c831e7cf31
--
https://chromeos.dev
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex
@ 2021-01-25 23:49 ` Stephen Boyd
0 siblings, 0 replies; 24+ messages in thread
From: Stephen Boyd @ 2021-01-25 23:49 UTC (permalink / raw)
To: Rob Clark
Cc: linux-arm-msm, freedreno, linux-kernel, dri-devel, Krishna Manikandan
Lockdep complains about an AA deadlock when rebooting the device.
============================================
WARNING: possible recursive locking detected
5.4.91 #1 Not tainted
--------------------------------------------
reboot/5213 is trying to acquire lock:
ffffff80d13391b0 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
but task is already holding lock:
ffffff80d1339110 (&kms->commit_lock[i]){+.+.}, at: lock_crtcs+0x60/0xa4
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&kms->commit_lock[i]);
lock(&kms->commit_lock[i]);
*** DEADLOCK ***
May be due to missing lock nesting notation
6 locks held by reboot/5213:
__arm64_sys_reboot+0x148/0x2a0
device_shutdown+0x10c/0x2c4
drm_atomic_helper_shutdown+0x48/0xfc
modeset_lock+0x120/0x24c
lock_crtcs+0x60/0xa4
stack backtrace:
CPU: 4 PID: 5213 Comm: reboot Not tainted 5.4.91 #1
Hardware name: Google Pompom (rev1) with LTE (DT)
Call trace:
dump_backtrace+0x0/0x1dc
show_stack+0x24/0x30
dump_stack+0xfc/0x1a8
__lock_acquire+0xcd0/0x22b8
lock_acquire+0x1ec/0x240
__mutex_lock_common+0xe0/0xc84
mutex_lock_nested+0x48/0x58
lock_crtcs+0x60/0xa4
msm_atomic_commit_tail+0x348/0x570
commit_tail+0xdc/0x178
drm_atomic_helper_commit+0x160/0x168
drm_atomic_commit+0x68/0x80
This is because lockdep thinks all the locks taken in lock_crtcs() are
the same lock, when they actually aren't. That's because we call
mutex_init() in msm_kms_init() and that assigns on static key for every
lock initialized in this loop. Let's allocate a dynamic number of
lock_class_keys and assign them to each lock so that lockdep can figure
out an AA deadlock isn't possible here.
Fixes: b3d91800d9ac ("drm/msm: Fix race condition in msm driver with async layer updates")
Cc: Krishna Manikandan <mkrishn@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/gpu/drm/msm/msm_kms.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index d8151a89e163..4735251a394d 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -157,6 +157,7 @@ struct msm_kms {
* from the crtc's pending_timer close to end of the frame:
*/
struct mutex commit_lock[MAX_CRTCS];
+ struct lock_class_key commit_lock_keys[MAX_CRTCS];
unsigned pending_crtc_mask;
struct msm_pending_timer pending_timers[MAX_CRTCS];
};
@@ -166,8 +167,11 @@ static inline int msm_kms_init(struct msm_kms *kms,
{
unsigned i, ret;
- for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
- mutex_init(&kms->commit_lock[i]);
+ for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++) {
+ lockdep_register_key(&kms->commit_lock_keys[i]);
+ __mutex_init(&kms->commit_lock[i], "&kms->commit_lock[i]",
+ &kms->commit_lock_keys[i]);
+ }
kms->funcs = funcs;
base-commit: 19c329f6808995b142b3966301f217c831e7cf31
--
https://chromeos.dev
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-02-04 15:19 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-26 2:01 [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex Stephen Boyd
2021-01-26 2:01 ` Stephen Boyd
2021-01-26 2:01 ` [PATCHv2 0/3] iio: Add a ChromeOS EC MKBP proximity driver Stephen Boyd
2021-01-26 2:01 ` [PATCH v2 1/3] platform/chrome: cros_ec: Add SW_FRONT_PROXIMITY MKBP define Stephen Boyd
2021-01-26 2:01 ` [PATCH v2 2/3] dt-bindings: iio: Add cros ec proximity yaml doc Stephen Boyd
2021-01-26 2:01 ` [PATCH v2 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver Stephen Boyd
-- strict thread matches above, loose matches on Subject: below --
2021-01-25 23:49 [PATCH] drm/msm/kms: Make a lock_class_key for each crtc mutex Stephen Boyd
2021-01-25 23:49 ` Stephen Boyd
2021-01-28 16:39 ` Rob Clark
2021-01-28 16:39 ` Rob Clark
2021-02-02 15:46 ` Daniel Vetter
2021-02-02 15:46 ` Daniel Vetter
2021-02-02 16:51 ` Rob Clark
2021-02-02 16:51 ` Rob Clark
2021-02-03 10:10 ` Daniel Vetter
2021-02-03 10:10 ` Daniel Vetter
2021-02-03 17:29 ` Rob Clark
2021-02-03 17:29 ` Rob Clark
2021-02-03 21:58 ` Stephen Boyd
2021-02-03 21:58 ` Stephen Boyd
2021-02-03 22:11 ` Rob Clark
2021-02-03 22:11 ` Rob Clark
2021-02-04 15:17 ` Daniel Vetter
2021-02-04 15:17 ` Daniel Vetter
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.