[PATCH v2 00/13] coordinate cht i2c-pmic and i915-punit accesses

[PATCH v2 00/13] coordinate cht i2c-pmic and i915-punit accesses

[Hans de Goede]

Hi All,

Here is v2 of my cht i2c-pmic and i915-punit access coordination
patchset. New in this version are some minor spelling / style fixes
and more importantly the inclusion of 6 extra i2c-designware patches.

These 6 patches are ready for merging, they have Wolfram's (the i2c
maintainer's) ack. The problem is the 6th i2c patch which fixes the
punit semaphore support in i2c-designware-baytrail.c, which fixes
the i2c-designware not binding to the pmic (axp288) i2c bus. But with
this fixed we actually start triggering the coordination problems the
rest of this patch-set addresses.

So the plan (again with Wolfram's ack) is for all these patches
including the i2c-designware ones to go upstream through the drm-intel
tree, to avoid an intermediate state were things don't work.

As for the how and why of this patchset, let me copy and paste the
cover letter of v1 of this set which is still valid:

This series fixes an issue where the following messages are shown in dmesg:
[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 2
i2c_designware 808622C1:06: couldn't acquire bus ownership

Usually followed by a gfx or system freeze, this is happening both on my
cherrytrail tablet as well as being seen by an user commenting on:
https://bugzilla.kernel.org/show_bug.cgi?id=155241

This problem is triggered by my patch series to fix the punit i2c bus
semaphore code in drivers/i2c/busses/designware-baytrail.c .

Which actually allows i2c access to pmic-s wich need bus-access arbritration
for the first time on cherrytrail, combined with patches from me to actually
make the axp288_fuel_gauge driver load, which leads to regular pmic i2c
accesses (when userspace checks the battery level).

The fix / workaround
====================

Testing has shown that just taking the punit i2c bus semaphore on
i2c accesses to a shared pmic i2c bus is not enough to avoid problem,
e.g. besides this series to coordinate i915 access, we also need:
https://patchwork.ozlabs.org/patch/710070/

This series introduces 2 mechanisms to coordinate direct pmic accesses
vs punit requests which lead to pmic accesses. The first mechanism is
a simple mutex, which works well for the direct punit accesses the i915
code does. However testing has shown this is not enough, we also need
to avoid doing a forcewake when a driver is directly accessing the pmic
i2c bus, otherwise we get the timeout errors quoted above, the fact
that after the forcewake errors the designware-baytrail.c code also
fails to acquire the lock, suggests that doing forcewakes during the
access window causes the i2c bus to get stuck (or the punit to get
confused).

The deal with the forcewake issue a notifier is introduced which allows
drivers to get notified before any pmic i2c transfer begins (and after
it ends). This is used in the i915 driver to do a
forcewake_get(FORCEWAKE_ALL) before the access begins avoiding needing to
it while the access is in progress.

Testing has shown that this avoids the problem for both me and the user,
where as before it could be reproduced at will.

The implementation of the fix
=============================

I've decided to put the mutex and the notifier in the iosf_mbi code,
as that is somewhat of a common middle ground between the i915 driver and
the designware-baytrail.c code, with the latter already depending on the
iosf_mbi code. I'm open for moving them if someone has a suggestion for a
better place to put them.

If iosf_mbi support is not enabled then all the functions used will become
static inline NOPs and nothing changes on the i915 side.

If the code is enabled and runs on a system which does not need pmic i2c
bus arbritration, then the mutex lock / unlock calls added to the i915 code
will still be made, but there won't be any contention, so other then the
overhead of the lock / unlock there will not be any delays. These calls are
not made in any hot paths.

The biggest downside IMHO is that taking FORCEWAKE_ALL before each bus
access will cause extra GPU wakeups, and thus powerdrain. I plan to make
the impact of this minimal by modifying any drivers (axp288 code, with
which I'm quite familiar by now) to limit their i2c accesses to a bare
minimum. Specifically I plan to e.g. cache the battery level and wait
at least 15 seconds (max 4 times a minute) before getting a fresh reading
even if userspace asks for it more often, combined with some mechanism
to only do 1 access-begin and 1 access-end notification for i2c accesses
in quick succession.

Alternatives
============

It would be tempting to say that this is not worth the trouble and we
should just disallow direct i2c accesses to the pmic bus. Unfortunately
that will cause some major issues on affected devices:
-No battery monitoring
-No "AC" plugged in monitoring
-If booted with a normal USB-A -> micro-USB cable, or no cable, plugged
 in and then the user replaces the cable with an otg USB-host cable /
 adapter, the id-pin shorting will enable a 5v boost convertor, but we
 need to disable the pmic's USB-Vbus path otherwise it will start drawing
 current from the boost convertor, leading to aprox 300mA of extra
 battery drain, this is done by the axp288_charger driver, which needs
 direct i2c access to the pmic bus

Way forward
===========

I realize the workaround this patch-set adds is not the prettiest code
ever seen, but I see no otherway to get things like battery monitoring
and proper OTG support working on affected devices without breaking
drm/i915 support. So I would like to see this merged in some form, I'm
happy to make any kind of requested changes; and/or to try alternative
fixes.

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 01/13] x86/platform/intel/iosf_mbi: Add a mutex for P-Unit access

[Hans de Goede]

One some systems the P-Unit accesses the PMIC to change various voltages
through the same bus as other kernel drivers use for e.g. battery
monitoring.

If a driver sends requests to the P-Unit which require the P-Unit to access
the PMIC bus while another driver is also accessing the PMIC bus various
bad things happen.

This commit adds a mutex to protect the P-Unit against simultaneous
accesses and 2 functions to lock / unlock this mutex.

Note on these systems the i2c-bus driver will request a sempahore from the
P-Unit for exclusive access to the PMIC bus when i2c drivers are accessing
it, but this does not appear to be sufficient, we still need to avoid
making certain P-Unit requests during the access window to avoid problems.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Changes in v2:
-Rename iosf_mbi_punit_lock/_unlock to _acquire/_release rename
-Spelling: P-Unit, PMIC
---
 arch/x86/include/asm/iosf_mbi.h    | 31 +++++++++++++++++++++++++++++++
 arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index b41ee16..f6119d0 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -88,6 +88,33 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr);
  */
 int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
 
+/**
+ * iosf_mbi_punit_acquire() - Acquire access to the P-Unit
+ *
+ * One some systems the P-Unit accesses the PMIC to change various voltages
+ * through the same bus as other kernel drivers use for e.g. battery monitoring.
+ *
+ * If a driver sends requests to the P-Unit which require the P-Unit to access
+ * the PMIC bus while another driver is also accessing the PMIC bus various bad
+ * things happen.
+ *
+ * To avoid these problems this function must be called before accessing the
+ * P-Unit or the PMIC, be it through iosf_mbi* functions or through other means.
+ *
+ * Note on these systems the i2c-bus driver will request a sempahore from the
+ * P-Unit for exclusive access to the PMIC bus when i2c drivers are accessing
+ * it, but this does not appear to be sufficient, we still need to avoid making
+ * certain P-Unit requests during the access window to avoid problems.
+ *
+ * This function locks a mutex, as such it may sleep.
+ */
+void iosf_mbi_punit_acquire(void);
+
+/**
+ * iosf_mbi_punit_release() - Release access to the P-Unit
+ */
+void iosf_mbi_punit_release(void);
+
 #else /* CONFIG_IOSF_MBI is not enabled */
 static inline
 bool iosf_mbi_available(void)
@@ -115,6 +142,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
 	WARN(1, "IOSF_MBI driver not available");
 	return -EPERM;
 }
+
+static inline void iosf_mbi_punit_acquire(void) {}
+static inline void iosf_mbi_punit_release(void) {}
+
 #endif /* CONFIG_IOSF_MBI */
 
 #endif /* IOSF_MBI_SYMS_H */
diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
index edf2c54..ed24fa9f 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -34,6 +34,7 @@
 
 static struct pci_dev *mbi_pdev;
 static DEFINE_SPINLOCK(iosf_mbi_lock);
+static DEFINE_MUTEX(iosf_mbi_punit_mutex);
 
 static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
 {
@@ -190,6 +191,18 @@ bool iosf_mbi_available(void)
 }
 EXPORT_SYMBOL(iosf_mbi_available);
 
+void iosf_mbi_punit_acquire(void)
+{
+	mutex_lock(&iosf_mbi_punit_mutex);
+}
+EXPORT_SYMBOL(iosf_mbi_punit_acquire);
+
+void iosf_mbi_punit_release(void)
+{
+	mutex_unlock(&iosf_mbi_punit_mutex);
+}
+EXPORT_SYMBOL(iosf_mbi_punit_release);
+
 #ifdef CONFIG_IOSF_MBI_DEBUG
 static u32	dbg_mdr;
 static u32	dbg_mcr;
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 02/13] x86/platform/intel/iosf_mbi: Add a PMIC bus access notifier

[Hans de Goede]

Some drivers may need to acquire P-Unit managed resources from interrupt
context, where they cannot call iosf_mbi_punit_acquire().

This commit adds a notifier chain which allows a driver to get notified
(in a process context) before other drivers start accessing the PMIC bus,
so that the driver can acquire any resources, which it may need during
the window the other driver is accessing the PMIC, before hand.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Changes in v2:
-Spelling: P-Unit, PMIC
-Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
---
 arch/x86/include/asm/iosf_mbi.h    | 54 ++++++++++++++++++++++++++++++++++++++
 arch/x86/platform/intel/iosf_mbi.c | 36 +++++++++++++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index f6119d0..59e8f8b 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -47,6 +47,10 @@
 #define QRK_MBI_UNIT_MM		0x05
 #define QRK_MBI_UNIT_SOC	0x31
 
+/* Action values for the pmic_bus_access_notifier functions */
+#define MBI_PMIC_BUS_ACCESS_BEGIN	1
+#define MBI_PMIC_BUS_ACCESS_END		2
+
 #if IS_ENABLED(CONFIG_IOSF_MBI)
 
 bool iosf_mbi_available(void);
@@ -115,6 +119,38 @@ void iosf_mbi_punit_acquire(void);
  */
 void iosf_mbi_punit_release(void);
 
+/**
+ * iosf_mbi_register_pmic_bus_access_notifier - Register PMIC bus notifier
+ *
+ * This function can be used by drivers which may need to acquire P-Unit
+ * managed resources from interrupt context, where iosf_mbi_punit_acquire()
+ * can not be used.
+ *
+ * This function allows a driver to register a notifier to get notified (in a
+ * process context) before other drivers start accessing the PMIC bus.
+ *
+ * This allows the driver to acquire any resources, which it may need during
+ * the window the other driver is accessing the PMIC, before hand.
+ *
+ * @nb: notifier_block to register
+ */
+int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb);
+
+/**
+ * iosf_mbi_register_pmic_bus_access_notifier - Unregister PMIC bus notifier
+ *
+ * @nb: notifier_block to unregister
+ */
+int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
+
+/**
+ * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain
+ *
+ * @val: action to pass into listener's notifier_call function
+ * @v: data pointer to pass into listener's notifier_call function
+ */
+int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v);
+
 #else /* CONFIG_IOSF_MBI is not enabled */
 static inline
 bool iosf_mbi_available(void)
@@ -146,6 +182,24 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
 static inline void iosf_mbi_punit_acquire(void) {}
 static inline void iosf_mbi_punit_release(void) {}
 
+static inline
+int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline
+int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline
+int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
+{
+	return 0;
+}
+
 #endif /* CONFIG_IOSF_MBI */
 
 #endif /* IOSF_MBI_SYMS_H */
diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
index ed24fa9f..a952ac1 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -35,6 +35,7 @@
 static struct pci_dev *mbi_pdev;
 static DEFINE_SPINLOCK(iosf_mbi_lock);
 static DEFINE_MUTEX(iosf_mbi_punit_mutex);
+static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
 
 static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
 {
@@ -203,6 +204,41 @@ void iosf_mbi_punit_release(void)
 }
 EXPORT_SYMBOL(iosf_mbi_punit_release);
 
+int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+	int ret;
+
+	/* Wait for the bus to go inactive before registering */
+	mutex_lock(&iosf_mbi_punit_mutex);
+	ret = blocking_notifier_chain_register(
+				&iosf_mbi_pmic_bus_access_notifier, nb);
+	mutex_unlock(&iosf_mbi_punit_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier);
+
+int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+	int ret;
+
+	/* Wait for the bus to go inactive before unregistering */
+	mutex_lock(&iosf_mbi_punit_mutex);
+	ret = blocking_notifier_chain_unregister(
+				&iosf_mbi_pmic_bus_access_notifier, nb);
+	mutex_unlock(&iosf_mbi_punit_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier);
+
+int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
+{
+	return blocking_notifier_call_chain(
+				&iosf_mbi_pmic_bus_access_notifier, val, v);
+}
+EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
+
 #ifdef CONFIG_IOSF_MBI_DEBUG
 static u32	dbg_mdr;
 static u32	dbg_mcr;
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 03/13] i2c: designware: Rename accessor_flags to flags

[Hans de Goede]

Rename accessor_flags to flags, so that we can use the field for
other flags too. This is a preparation patch for adding cherrytrail
support to the punit semaphore code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Takashi Iwai <tiwai@suse.de>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-designware-core.c    | 14 +++++++-------
 drivers/i2c/busses/i2c-designware-core.h    |  2 +-
 drivers/i2c/busses/i2c-designware-platdrv.c |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 6d81c56..8c3ba42 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -177,13 +177,13 @@ static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
 {
 	u32 value;
 
-	if (dev->accessor_flags & ACCESS_16BIT)
+	if (dev->flags & ACCESS_16BIT)
 		value = readw_relaxed(dev->base + offset) |
 			(readw_relaxed(dev->base + offset + 2) << 16);
 	else
 		value = readl_relaxed(dev->base + offset);
 
-	if (dev->accessor_flags & ACCESS_SWAP)
+	if (dev->flags & ACCESS_SWAP)
 		return swab32(value);
 	else
 		return value;
@@ -191,10 +191,10 @@ static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
 
 static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
 {
-	if (dev->accessor_flags & ACCESS_SWAP)
+	if (dev->flags & ACCESS_SWAP)
 		b = swab32(b);
 
-	if (dev->accessor_flags & ACCESS_16BIT) {
+	if (dev->flags & ACCESS_16BIT) {
 		writew_relaxed((u16)b, dev->base + offset);
 		writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
 	} else {
@@ -339,10 +339,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	reg = dw_readl(dev, DW_IC_COMP_TYPE);
 	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
 		/* Configure register endianess access */
-		dev->accessor_flags |= ACCESS_SWAP;
+		dev->flags |= ACCESS_SWAP;
 	} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
 		/* Configure register access mode 16bit */
-		dev->accessor_flags |= ACCESS_16BIT;
+		dev->flags |= ACCESS_16BIT;
 	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
 		dev_err(dev->dev, "Unknown Synopsys component type: "
 			"0x%08x\n", reg);
@@ -926,7 +926,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 tx_aborted:
 	if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
 		complete(&dev->cmd_complete);
-	else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
+	else if (unlikely(dev->flags & ACCESS_INTR_MASK)) {
 		/* workaround to trigger pending interrupt */
 		stat = dw_readl(dev, DW_IC_INTR_MASK);
 		i2c_dw_disable_int(dev);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 26250b4..2c50571 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -103,7 +103,7 @@ struct dw_i2c_dev {
 	unsigned int		status;
 	u32			abort_source;
 	int			irq;
-	u32			accessor_flags;
+	u32			flags;
 	struct i2c_adapter	adapter;
 	u32			functionality;
 	u32			master_cfg;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 6ce4313..3eede7b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -112,7 +112,7 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 
 	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
 	if (id && id->driver_data)
-		dev->accessor_flags |= (u32)id->driver_data;
+		dev->flags |= (u32)id->driver_data;
 
 	return 0;
 }
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 04/13] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions

[Hans de Goede]

Pass dw_i2c_dev into the helper functions, this is a preparation patch
for the punit semaphore fixes done in the other patches in this set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 1590ad0..a3f581c 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -28,14 +28,14 @@
 
 static unsigned long acquired;
 
-static int get_sem(struct device *dev, u32 *sem)
+static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 {
 	u32 data;
 	int ret;
 
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data);
 	if (ret) {
-		dev_err(dev, "iosf failed to read punit semaphore\n");
+		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
 		return ret;
 	}
 
@@ -44,18 +44,18 @@ static int get_sem(struct device *dev, u32 *sem)
 	return 0;
 }
 
-static void reset_semaphore(struct device *dev)
+static void reset_semaphore(struct dw_i2c_dev *dev)
 {
 	u32 data;
 
 	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data)) {
-		dev_err(dev, "iosf failed to reset punit semaphore during read\n");
+		dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n");
 		return;
 	}
 
 	data &= ~PUNIT_SEMAPHORE_BIT;
 	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
-		dev_err(dev, "iosf failed to reset punit semaphore during write\n");
+		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
 }
 
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -83,7 +83,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	start = jiffies;
 	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
 	do {
-		ret = get_sem(dev->dev, &sem);
+		ret = get_sem(dev, &sem);
 		if (!ret && sem) {
 			acquired = jiffies;
 			dev_dbg(dev->dev, "punit semaphore acquired after %ums\n",
@@ -95,7 +95,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	} while (time_before(jiffies, end));
 
 	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
-	reset_semaphore(dev->dev);
+	reset_semaphore(dev);
 
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &sem);
 	if (ret)
@@ -116,7 +116,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev *dev)
 	if (!dev->acquire_lock)
 		return;
 
-	reset_semaphore(dev->dev);
+	reset_semaphore(dev);
 	dev_dbg(dev->dev, "punit semaphore held for %ums\n",
 		jiffies_to_msecs(jiffies - acquired));
 }
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 05/13] i2c: designware-baytrail: Only check iosf_mbi_available() for shared hosts

[Hans de Goede]

If (!shared_host) simply return 0, this avoids delaying the probe if
iosf_mbi_available() returns false when an i2c bus is not using the
punit semaphore.

Also move the if (!iosf_mbi_available()) check to above the
dev_info, so that we do not repeat the dev_info on every probe
until iosf_mbi_available() returns true.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Takashi Iwai <tiwai@suse.de>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
---
Changes in v2:
-New patch in v2 of this set
Changes in v3:
-Use if (!shared_host) return 0, to simplify the non-shared_host path
 and to avoid nested ifs
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index a3f581c..cf02222 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -138,15 +138,16 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
 	if (ACPI_FAILURE(status))
 		return 0;
 
-	if (shared_host) {
-		dev_info(dev->dev, "I2C bus managed by PUNIT\n");
-		dev->acquire_lock = baytrail_i2c_acquire;
-		dev->release_lock = baytrail_i2c_release;
-		dev->pm_runtime_disabled = true;
-	}
+	if (!shared_host)
+		return 0;
 
 	if (!iosf_mbi_available())
 		return -EPROBE_DEFER;
 
+	dev_info(dev->dev, "I2C bus managed by PUNIT\n");
+	dev->acquire_lock = baytrail_i2c_acquire;
+	dev->release_lock = baytrail_i2c_release;
+	dev->pm_runtime_disabled = true;
+
 	return 0;
 }
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 06/13] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore

[Hans de Goede]

On my cherrytrail tablet with axp288 pmic, just doing a bunch of repeated
reads from the pmic, e.g. "i2cdump -y 14 0x34" would lookup the tablet in
1 - 3 runs guaranteed.

This seems to be causes by the cpu trying to enter C6 or C7 while we hold
the punit bus semaphore, at which point everything just hangs.

Avoid this by disallowing the CPU to enter C6 or C7 before acquiring the
punit bus semaphore.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
---
Changes in v2:
-New patch in v2 of this set
Changes in v3:
-Change commit message and comment in the code from "force the CPU to C1"
 to "Disallow the CPU to enter C6 or C7", as the CPU may still be in either
 C0 or C1 with the request pm_qos
Changes in v4:
-Rename i2c_dw_eval_lock_support to i2c_dw_probe_lock_support so that we can
 add a matching i2c_dw_remove_lock_support cleanup function
-Move qm_pos removal to new i2c_dw_remove_lock_support function
-Move pm_qos_add_request to the end of i2c_dw_probe_lock_support
Changes in v5:
-Update the pm_qos for a latency of 0 *before* requesting the semaphore,
 instead of doing it while waiting for the request to be acked
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 24 ++++++++++++++++++++++--
 drivers/i2c/busses/i2c-designware-core.h     |  9 +++++++--
 drivers/i2c/busses/i2c-designware-platdrv.c  |  4 +++-
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index cf02222..650a700 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -16,6 +16,7 @@
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/pm_qos.h>
 
 #include <asm/iosf_mbi.h>
 
@@ -56,6 +57,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
 	data &= ~PUNIT_SEMAPHORE_BIT;
 	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
 		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
+
+	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
 }
 
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -72,11 +75,18 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	if (!dev->release_lock)
 		return 0;
 
+	/*
+	 * Disallow the CPU to enter C6 or C7 state, entering these states
+	 * requires the punit to talk to the pmic and if this happens while
+	 * we're holding the semaphore, the SoC hangs.
+	 */
+	pm_qos_update_request(&dev->pm_qos, 0);
+
 	/* host driver writes to side band semaphore register */
 	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, sem);
 	if (ret) {
 		dev_err(dev->dev, "iosf punit semaphore request failed\n");
-		return ret;
+		goto out;
 	}
 
 	/* host driver waits for bit 0 to be set in semaphore register */
@@ -95,6 +105,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	} while (time_before(jiffies, end));
 
 	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
+out:
 	reset_semaphore(dev);
 
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &sem);
@@ -121,7 +132,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev *dev)
 		jiffies_to_msecs(jiffies - acquired));
 }
 
-int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
+int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
 {
 	acpi_status status;
 	unsigned long long shared_host = 0;
@@ -149,5 +160,14 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
 	dev->release_lock = baytrail_i2c_release;
 	dev->pm_runtime_disabled = true;
 
+	pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
+			   PM_QOS_DEFAULT_VALUE);
+
 	return 0;
 }
+
+void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
+{
+	if (dev->acquire_lock)
+		pm_qos_remove_request(&dev->pm_qos);
+}
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 2c50571..94a5fd1 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -23,6 +23,7 @@
  */
 
 #include <linux/i2c.h>
+#include <linux/pm_qos.h>
 
 #define DW_IC_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C |			\
 					I2C_FUNC_SMBUS_BYTE |		\
@@ -75,6 +76,7 @@
  * @fp_lcnt: fast plus LCNT value
  * @hs_hcnt: high speed HCNT value
  * @hs_lcnt: high speed LCNT value
+ * @pm_qos: pm_qos_request used while holding a hardware lock on the bus
  * @acquire_lock: function to acquire a hardware lock on the bus
  * @release_lock: function to release a hardware lock on the bus
  * @pm_runtime_disabled: true if pm runtime is disabled
@@ -122,6 +124,7 @@ struct dw_i2c_dev {
 	u16			fp_lcnt;
 	u16			hs_hcnt;
 	u16			hs_lcnt;
+	struct pm_qos_request	pm_qos;
 	int			(*acquire_lock)(struct dw_i2c_dev *dev);
 	void			(*release_lock)(struct dw_i2c_dev *dev);
 	bool			pm_runtime_disabled;
@@ -139,7 +142,9 @@ extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
 extern int i2c_dw_probe(struct dw_i2c_dev *dev);
 
 #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
-extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
+extern int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev);
+extern void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev);
 #else
-static inline int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev) { return 0; }
+static inline int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) { return 0; }
+static inline void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev) {}
 #endif
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 3eede7b..d474db0 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -238,7 +238,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	r = i2c_dw_eval_lock_support(dev);
+	r = i2c_dw_probe_lock_support(dev);
 	if (r)
 		return r;
 
@@ -307,6 +307,8 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 	if (!dev->pm_runtime_disabled)
 		pm_runtime_disable(&pdev->dev);
 
+	i2c_dw_remove_lock_support(dev);
+
 	return 0;
 }
 
-- 
2.9.3

[PATCH v2 07/13] i2c: designware-baytrail: Fix race when resetting the semaphore

[Hans de Goede]

Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that
we keep the iosf_mbi_lock locked during the read-modify-write done to
reset the semaphore.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
---
Changes in v5:
-New patch in v5 of this patch-set
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 650a700..8df529c 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 
 static void reset_semaphore(struct dw_i2c_dev *dev)
 {
-	u32 data;
-
-	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data)) {
-		dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n");
-		return;
-	}
-
-	data &= ~PUNIT_SEMAPHORE_BIT;
-	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
+	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE,
+			    0, PUNIT_SEMAPHORE_BIT))
 		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
 
 	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
-- 
2.9.3

[PATCH v2 08/13] i2c: designware-baytrail: Add support for cherrytrail

[Hans de Goede]

The cherrytrail punit has the pmic i2c bus access semaphore at a
different register address.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
---
Changes in v2:
-Adjust for accessor_flags -> flags rename
-Add flags field to struct dw_pci_controller
-Add get_sem_addr() helper replacing MODEL_CHERRYTRAIL flag checking in
 PUNIT_SEMAPHORE macro
Changes in v3:
-Add a gap between ACCESS_* and MODEL_* flags as reserved space for
 future ACCESS_* flags
Changes in v4:
-s/CHV/CHT/
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 19 +++++++++++++++----
 drivers/i2c/busses/i2c-designware-core.h     |  2 ++
 drivers/i2c/busses/i2c-designware-pcidrv.c   | 26 +++++++++++++++++++-------
 drivers/i2c/busses/i2c-designware-platdrv.c  |  2 +-
 4 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 8df529c..3effc9a 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -24,17 +24,27 @@
 
 #define SEMAPHORE_TIMEOUT	100
 #define PUNIT_SEMAPHORE		0x7
+#define PUNIT_SEMAPHORE_CHT	0x10e
 #define PUNIT_SEMAPHORE_BIT	BIT(0)
 #define PUNIT_SEMAPHORE_ACQUIRE	BIT(1)
 
 static unsigned long acquired;
 
+static u32 get_sem_addr(struct dw_i2c_dev *dev)
+{
+	if (dev->flags & MODEL_CHERRYTRAIL)
+		return PUNIT_SEMAPHORE_CHT;
+	else
+		return PUNIT_SEMAPHORE;
+}
+
 static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 {
+	u32 addr = get_sem_addr(dev);
 	u32 data;
 	int ret;
 
-	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data);
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr, &data);
 	if (ret) {
 		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
 		return ret;
@@ -47,7 +57,7 @@ static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 
 static void reset_semaphore(struct dw_i2c_dev *dev)
 {
-	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE,
+	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, get_sem_addr(dev),
 			    0, PUNIT_SEMAPHORE_BIT))
 		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
 
@@ -56,6 +66,7 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
 
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 {
+	u32 addr = get_sem_addr(dev);
 	u32 sem = PUNIT_SEMAPHORE_ACQUIRE;
 	int ret;
 	unsigned long start, end;
@@ -76,7 +87,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	pm_qos_update_request(&dev->pm_qos, 0);
 
 	/* host driver writes to side band semaphore register */
-	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, sem);
+	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, addr, sem);
 	if (ret) {
 		dev_err(dev->dev, "iosf punit semaphore request failed\n");
 		goto out;
@@ -101,7 +112,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 out:
 	reset_semaphore(dev);
 
-	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &sem);
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr, &sem);
 	if (ret)
 		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
 	else
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 94a5fd1..a8e74ca 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -135,6 +135,8 @@ struct dw_i2c_dev {
 #define ACCESS_16BIT		0x00000002
 #define ACCESS_INTR_MASK	0x00000004
 
+#define MODEL_CHERRYTRAIL	0x00000100
+
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index d6423cf..ed485b6 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -45,6 +45,7 @@ enum dw_pci_ctl_id_t {
 	medfield,
 	merrifield,
 	baytrail,
+	cherrytrail,
 	haswell,
 };
 
@@ -63,6 +64,7 @@ struct dw_pci_controller {
 	u32 rx_fifo_depth;
 	u32 clk_khz;
 	u32 functionality;
+	u32 flags;
 	struct dw_scl_sda_cfg *scl_sda_cfg;
 	int (*setup)(struct pci_dev *pdev, struct dw_pci_controller *c);
 };
@@ -170,6 +172,15 @@ static struct dw_pci_controller dw_pci_controllers[] = {
 		.functionality = I2C_FUNC_10BIT_ADDR,
 		.scl_sda_cfg = &hsw_config,
 	},
+	[cherrytrail] = {
+		.bus_num = -1,
+		.bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST,
+		.tx_fifo_depth = 32,
+		.rx_fifo_depth = 32,
+		.functionality = I2C_FUNC_10BIT_ADDR,
+		.flags = MODEL_CHERRYTRAIL,
+		.scl_sda_cfg = &byt_config,
+	},
 };
 
 #ifdef CONFIG_PM
@@ -237,6 +248,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	dev->base = pcim_iomap_table(pdev)[0];
 	dev->dev = &pdev->dev;
 	dev->irq = pdev->irq;
+	dev->flags |= controller->flags;
 
 	if (controller->setup) {
 		r = controller->setup(pdev, controller);
@@ -317,13 +329,13 @@ static const struct pci_device_id i2_designware_pci_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x9c61), haswell },
 	{ PCI_VDEVICE(INTEL, 0x9c62), haswell },
 	/* Braswell / Cherrytrail */
-	{ PCI_VDEVICE(INTEL, 0x22C1), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C2), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C3), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C4), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C5), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C6), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C7), baytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C1), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C2), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C3), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C4), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C5), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C6), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C7), cherrytrail },
 	{ 0,}
 };
 MODULE_DEVICE_TABLE(pci, i2_designware_pci_ids);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d474db0..df0ff7d 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -123,7 +123,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
 	{ "INT3432", 0 },
 	{ "INT3433", 0 },
 	{ "80860F41", 0 },
-	{ "808622C1", 0 },
+	{ "808622C1", MODEL_CHERRYTRAIL },
 	{ "AMD0010", ACCESS_INTR_MASK },
 	{ "AMDI0010", ACCESS_INTR_MASK },
 	{ "AMDI0510", 0 },
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 09/13] i2c: designware-baytrail: Acquire P-Unit access on bus acquire

[Hans de Goede]

Acquire P-Unit access to stop others from accessing the P-Unit while the
PMIC i2c bus is in use. This is necessary because accessing the P-Unit
from the kernel may result in the P-Unit trying to access the PMIC i2c
bus, which results in a hang when it happens while we own the PMIC i2c
bus semaphore.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
---
Changes in v2:
-Spelling: P-Unit, PMIC
-Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 3effc9a..7eddc3b 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
 		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
 
 	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
+
+	iosf_mbi_punit_release();
 }
 
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	if (!dev->release_lock)
 		return 0;
 
+	iosf_mbi_punit_acquire();
+
 	/*
 	 * Disallow the CPU to enter C6 or C7 state, entering these states
 	 * requires the punit to talk to the pmic and if this happens while
-- 
2.9.3

[PATCH v2 10/13] i2c: designware-baytrail: Call pmic_bus_access_notifier_chain

[Hans de Goede]

Call the iosf_mbi pmic_bus_access_notifier_chain on bus acquire / release.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
---
Changes in v2:
-Spelling: P-Unit, PMIC
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 7eddc3b..1749a0f 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -63,6 +63,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
 
 	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
 
+	iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_END,
+						     NULL);
 	iosf_mbi_punit_release();
 }
 
@@ -82,6 +84,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 		return 0;
 
 	iosf_mbi_punit_acquire();
+	iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_BEGIN,
+						     NULL);
 
 	/*
 	 * Disallow the CPU to enter C6 or C7 state, entering these states
-- 
2.9.3

[PATCH v2 11/13] drm/i915: Add intel_uncore_suspend / resume functions

[Hans de Goede]

Rename intel_uncore_early_sanitize to intel_uncore_resume, dropping the
(always true) restore_forcewake argument and add a new intel_uncore_resume
function to replace the intel_uncore_forcewake_reset(dev_priv, false)
calls done from the suspend / runtime_suspend functions and make
intel_uncore_forcewake_reset private.

This is a preparation patch for adding PMIC bus access notifier support.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
Changes in v2:
-Spelling: P-Unit, PMIC
---
 drivers/gpu/drm/i915/i915_drv.c     |  6 +++---
 drivers/gpu/drm/i915/i915_drv.h     |  6 ++----
 drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++++-------
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index aefab9a..5a62d7a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1445,7 +1445,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 	opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
 	intel_opregion_notify_adapter(dev_priv, opregion_target_state);
 
-	intel_uncore_forcewake_reset(dev_priv, false);
+	intel_uncore_suspend(dev_priv);
 	intel_opregion_unregister(dev_priv);
 
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
@@ -1690,7 +1690,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
 		DRM_ERROR("Resume prepare failed: %d, continuing anyway\n",
 			  ret);
 
-	intel_uncore_early_sanitize(dev_priv, true);
+	intel_uncore_resume(dev_priv);
 
 	if (IS_GEN9_LP(dev_priv)) {
 		if (!dev_priv->suspended_to_idle)
@@ -2344,7 +2344,7 @@ static int intel_runtime_suspend(struct device *kdev)
 		return ret;
 	}
 
-	intel_uncore_forcewake_reset(dev_priv, false);
+	intel_uncore_suspend(dev_priv);
 
 	enable_rpm_wakeref_asserts(dev_priv);
 	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e9b4ece..c717329 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2976,14 +2976,12 @@ int intel_irq_install(struct drm_i915_private *dev_priv);
 void intel_irq_uninstall(struct drm_i915_private *dev_priv);
 
 extern void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
-extern void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
-					bool restore_forcewake);
 extern void intel_uncore_init(struct drm_i915_private *dev_priv);
 extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
 extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
 extern void intel_uncore_fini(struct drm_i915_private *dev_priv);
-extern void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
-					 bool restore);
+extern void intel_uncore_suspend(struct drm_i915_private *dev_priv);
+extern void intel_uncore_resume(struct drm_i915_private *dev_priv);
 const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
 void intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
 				enum forcewake_domains domains);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index abe0888..3767307 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -250,7 +250,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
 	return HRTIMER_NORESTART;
 }
 
-void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
+static void __intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
 				  bool restore)
 {
 	unsigned long irqflags;
@@ -424,13 +424,17 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
 	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B_LAST))
 		info->has_decoupled_mmio = false;
 
-	intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
+	__intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
 }
 
-void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
-				 bool restore_forcewake)
+void intel_uncore_suspend(struct drm_i915_private *dev_priv)
 {
-	__intel_uncore_early_sanitize(dev_priv, restore_forcewake);
+	__intel_uncore_forcewake_reset(dev_priv, false);
+}
+
+void intel_uncore_resume(struct drm_i915_private *dev_priv)
+{
+	__intel_uncore_early_sanitize(dev_priv, true);
 	i915_check_and_clear_faults(dev_priv);
 }
 
@@ -1463,7 +1467,7 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv)
 {
 	/* Paranoia: make sure we have disabled everything before we exit. */
 	intel_uncore_sanitize(dev_priv);
-	intel_uncore_forcewake_reset(dev_priv, false);
+	__intel_uncore_forcewake_reset(dev_priv, false);
 }
 
 #define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1)
@@ -1679,7 +1683,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
 
 	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
 
-	intel_uncore_forcewake_reset(dev_priv, true);
+	__intel_uncore_forcewake_reset(dev_priv, true);
 
 	return ret;
 }
-- 
2.9.3

[PATCH v2 12/13] drm/i915: Listen for PMIC bus access notifications

[Hans de Goede]

Listen for PMIC bus access notifications and get FORCEWAKE_ALL while
the bus is accessed to avoid needing to do any forcewakes, which need
PMIC bus access, while the PMIC bus is busy:

This fixes errors like these showing up in dmesg, usually followed
by a gfx or system freeze:

[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 2
i2c_designware 808622C1:06: couldn't acquire bus ownership

Downside of this approach is that it causes wakeups whenever the PMIC
bus is accessed. Unfortunately we cannot simply wait for the PMIC bus
to go idle when we hit a race, as forcewakes may be done from interrupt
handlers where we cannot sleep to wait for the i2c PMIC bus access to
finish.

Note that the notifications and thus the wakeups will only happen on
baytrail / cherrytrail devices using PMICs with a shared i2c bus for
P-Unit and host PMIC access (i2c busses with a _SEM method in their
APCI node), e.g. an axp288 PMIC.

I plan to write some patches for drivers accessing the PMIC bus to
limit their bus accesses to a bare minimum (e.g. cache registers, do not
update battery level more often then 4 times a minute), to limit the
amount of wakeups.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
Changes in v2:
-Spelling: P-Unit, PMIC
---
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_uncore.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c717329..52f7dde 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -721,6 +721,7 @@ struct intel_uncore {
 	const struct intel_forcewake_range *fw_domains_table;
 	unsigned int fw_domains_table_entries;
 
+	struct notifier_block pmic_bus_access_nb;
 	struct intel_uncore_funcs funcs;
 
 	unsigned fifo_count;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 3767307..175fe02 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -25,6 +25,7 @@
 #include "intel_drv.h"
 #include "i915_vgpu.h"
 
+#include <asm/iosf_mbi.h>
 #include <linux/pm_runtime.h>
 
 #define FORCEWAKE_ACK_TIMEOUT_MS 50
@@ -429,12 +430,16 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
 
 void intel_uncore_suspend(struct drm_i915_private *dev_priv)
 {
+	iosf_mbi_unregister_pmic_bus_access_notifier(
+		&dev_priv->uncore.pmic_bus_access_nb);
 	__intel_uncore_forcewake_reset(dev_priv, false);
 }
 
 void intel_uncore_resume(struct drm_i915_private *dev_priv)
 {
 	__intel_uncore_early_sanitize(dev_priv, true);
+	iosf_mbi_register_pmic_bus_access_notifier(
+		&dev_priv->uncore.pmic_bus_access_nb);
 	i915_check_and_clear_faults(dev_priv);
 }
 
@@ -1390,6 +1395,28 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
 	dev_priv->uncore.fw_domains_table_entries = ARRAY_SIZE((d)); \
 }
 
+static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
+					 unsigned long action, void *data)
+{
+	struct drm_i915_private *dev_priv = container_of(nb,
+			struct drm_i915_private, uncore.pmic_bus_access_nb);
+
+	switch (action) {
+	case MBI_PMIC_BUS_ACCESS_BEGIN:
+		/*
+		 * forcewake all to make sure that we don't need to forcewake
+		 * any power-planes while the pmic bus is busy.
+		 */
+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+		break;
+	case MBI_PMIC_BUS_ACCESS_END:
+		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
 void intel_uncore_init(struct drm_i915_private *dev_priv)
 {
 	i915_check_vgpu(dev_priv);
@@ -1399,6 +1426,8 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
 	__intel_uncore_early_sanitize(dev_priv, false);
 
 	dev_priv->uncore.unclaimed_mmio_check = 1;
+	dev_priv->uncore.pmic_bus_access_nb.notifier_call =
+		i915_pmic_bus_access_notifier;
 
 	switch (INTEL_INFO(dev_priv)->gen) {
 	default:
@@ -1458,6 +1487,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
 		ASSIGN_READ_MMIO_VFUNCS(vgpu);
 	}
 
+	iosf_mbi_register_pmic_bus_access_notifier(
+		&dev_priv->uncore.pmic_bus_access_nb);
+
 	i915_check_and_clear_faults(dev_priv);
 }
 #undef ASSIGN_WRITE_MMIO_VFUNCS
@@ -1465,6 +1497,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
 
 void intel_uncore_fini(struct drm_i915_private *dev_priv)
 {
+	iosf_mbi_unregister_pmic_bus_access_notifier(
+		&dev_priv->uncore.pmic_bus_access_nb);
+
 	/* Paranoia: make sure we have disabled everything before we exit. */
 	intel_uncore_sanitize(dev_priv);
 	__intel_uncore_forcewake_reset(dev_priv, false);
-- 
2.9.3

[PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Hans de Goede]

Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
around P-Unit write accesses.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
Changes in v2:
-Spelling: P-Unit, PMIC
-Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
---
 drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
 drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
 3 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5604701..13e5152 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -47,6 +47,7 @@
 #include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 #include <linux/reservation.h>
+#include <asm/iosf_mbi.h>
 
 static bool is_mmio_work(struct intel_flip_work *work)
 {
@@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 		cmd = 0;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_acquire();
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	val &= ~DSPFREQGUAR_MASK;
 	val |= (cmd << DSPFREQGUAR_SHIFT);
@@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 		     50)) {
 		DRM_ERROR("timed out waiting for CDclk change\n");
 	}
+	iosf_mbi_punit_release();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	mutex_lock(&dev_priv->sb_lock);
@@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_acquire();
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	val &= ~DSPFREQGUAR_MASK_CHV;
 	val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
@@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 		     50)) {
 		DRM_ERROR("timed out waiting for CDclk change\n");
 	}
+	iosf_mbi_punit_release();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	intel_update_cdclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 249623d..adff84a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -32,6 +32,7 @@
 #include "../../../platform/x86/intel_ips.h"
 #include <linux/module.h>
 #include <drm/drm_atomic_helper.h>
+#include <asm/iosf_mbi.h>
 
 /**
  * DOC: RC6
@@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
 	u32 val;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_acquire();
 
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
 	if (enable)
@@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
 		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
 		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
 
+	iosf_mbi_punit_release();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
@@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
 	u32 val;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_acquire();
 
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	if (enable)
@@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
 		val &= ~DSP_MAXFIFO_PM5_ENABLE;
 	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
 
+	iosf_mbi_punit_release();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
@@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 
 	if (IS_CHERRYVIEW(dev_priv)) {
 		mutex_lock(&dev_priv->rps.hw_lock);
+		iosf_mbi_punit_acquire();
 
 		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 		if (val & DSP_MAXFIFO_PM5_ENABLE)
@@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 				wm->level = VLV_WM_LEVEL_DDR_DVFS;
 		}
 
+		iosf_mbi_punit_release();
 		mutex_unlock(&dev_priv->rps.hw_lock);
 	}
 
@@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
 	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
 
 	if (val != dev_priv->rps.cur_freq) {
+		iosf_mbi_punit_acquire();
 		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
+		iosf_mbi_punit_release();
 		if (!IS_CHERRYVIEW(dev_priv))
 			gen6_set_rps_thresholds(dev_priv, val);
 	}
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index c0b7e95..e66bcc8 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -28,6 +28,7 @@
 
 #include <linux/pm_runtime.h>
 #include <linux/vgaarb.h>
+#include <asm/iosf_mbi.h>
 
 #include "i915_drv.h"
 #include "intel_drv.h"
@@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
 	if (COND)
 		goto out;
 
+	iosf_mbi_punit_acquire();
+
 	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
 	ctrl &= ~mask;
 	ctrl |= state;
@@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
 			  state,
 			  vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
 
+	iosf_mbi_punit_release();
+
 #undef COND
 
 out:
@@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
 	if (COND)
 		goto out;
 
+	iosf_mbi_punit_acquire();
+
 	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	ctrl &= ~DP_SSC_MASK(pipe);
 	ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
@@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
 			  state,
 			  vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
 
+	iosf_mbi_punit_release();
+
 #undef COND
 
 out:
-- 
2.9.3

Re: [PATCH v2 06/13] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore

[Andy Shevchenko]

On Mon, 2017-01-23 at 22:09 +0100, Hans de Goede wrote:
> On my cherrytrail tablet with axp288 pmic, just doing a bunch of
> repeated
> reads from the pmic, e.g. "i2cdump -y 14 0x34" would lookup the tablet
> in
> 1 - 3 runs guaranteed.
> 
> This seems to be causes by the cpu trying to enter C6 or C7 while we
> hold
> the punit bus semaphore, at which point everything just hangs.
> 
> Avoid this by disallowing the CPU to enter C6 or C7 before acquiring
> the
> punit bus semaphore.


> Changes in v5:
> -Update the pm_qos for a latency of 0 *before* requesting the
> semaphore,
>  instead of doing it while waiting for the request to be acked

So, that's why you put to reset_semaphore() instead of
baytrail_i2c_release(), right?

I perhaps missed your answer to my comment about that.

> > @@ -56,6 +57,8 @@ static void reset_semaphore(struct dw_i2c_dev
> > *dev)
>  	data &= ~PUNIT_SEMAPHORE_BIT;
>  	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> PUNIT_SEMAPHORE, data))
>  		dev_err(dev->dev, "iosf failed to reset punit
> semaphore during write\n");
> +
> +	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 06/13] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore

[Hans de Goede]

Hi,

On 01/24/2017 10:51 AM, Andy Shevchenko wrote:
> On Mon, 2017-01-23 at 22:09 +0100, Hans de Goede wrote:
>> On my cherrytrail tablet with axp288 pmic, just doing a bunch of
>> repeated
>> reads from the pmic, e.g. "i2cdump -y 14 0x34" would lookup the tablet
>> in
>> 1 - 3 runs guaranteed.
>>
>> This seems to be causes by the cpu trying to enter C6 or C7 while we
>> hold
>> the punit bus semaphore, at which point everything just hangs.
>>
>> Avoid this by disallowing the CPU to enter C6 or C7 before acquiring
>> the
>> punit bus semaphore.
>
>
>> Changes in v5:
>> -Update the pm_qos for a latency of 0 *before* requesting the
>> semaphore,
>>  instead of doing it while waiting for the request to be acked
>
> So, that's why you put to reset_semaphore() instead of
> baytrail_i2c_release(), right?

That is why the goto out gets introduced, baytrail_i2c_release()
and reset_semaphore() are functionally equivalent,
baytrail_i2c_release() does a bunch of argument error checking
and then calls reset_semaphore().

Regards,

Hans


> I perhaps missed your answer to my comment about that.
>
>>> @@ -56,6 +57,8 @@ static void reset_semaphore(struct dw_i2c_dev
>>> *dev)
>>  	data &= ~PUNIT_SEMAPHORE_BIT;
>>  	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
>> PUNIT_SEMAPHORE, data))
>>  		dev_err(dev->dev, "iosf failed to reset punit
>> semaphore during write\n");
>> +
>> +	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
>>  }
>

Re: [PATCH v2 00/13] coordinate cht i2c-pmic and i915-punit accesses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]


> So the plan (again with Wolfram's ack) is for all these patches
> including the i2c-designware ones to go upstream through the drm-intel
> tree, to avoid an intermediate state were things don't work.

Yes. I'd just like to pull in an immutable branch into my i2c/for-next
once this series is accepted.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 09/13] i2c: designware-baytrail: Acquire P-Unit access on bus acquire

[Jarkko Nikula]

On 01/23/2017 11:09 PM, Hans de Goede wrote:
> Acquire P-Unit access to stop others from accessing the P-Unit while the
> PMIC i2c bus is in use. This is necessary because accessing the P-Unit
> from the kernel may result in the P-Unit trying to access the PMIC i2c
> bus, which results in a hang when it happens while we own the PMIC i2c
> bus semaphore.
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
> ---
> Changes in v2:
> -Spelling: P-Unit, PMIC
> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
>  1 file changed, 4 insertions(+)
>

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 10/13] i2c: designware-baytrail: Call pmic_bus_access_notifier_chain

[Jarkko Nikula]

On 01/23/2017 11:09 PM, Hans de Goede wrote:
> Call the iosf_mbi pmic_bus_access_notifier_chain on bus acquire / release.
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
> ---
> Changes in v2:
> -Spelling: P-Unit, PMIC
> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
>  1 file changed, 4 insertions(+)
>

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/13] drm/i915: Add intel_uncore_suspend / resume functions

[Ville Syrjälä]

On Mon, Jan 23, 2017 at 10:09:56PM +0100, Hans de Goede wrote:
> Rename intel_uncore_early_sanitize to intel_uncore_resume, dropping the
> (always true) restore_forcewake argument and add a new intel_uncore_resume
> function to replace the intel_uncore_forcewake_reset(dev_priv, false)
> calls done from the suspend / runtime_suspend functions and make
> intel_uncore_forcewake_reset private.
> 
> This is a preparation patch for adding PMIC bus access notifier support.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
> Changes in v2:
> -Spelling: P-Unit, PMIC
> ---
>  drivers/gpu/drm/i915/i915_drv.c     |  6 +++---
>  drivers/gpu/drm/i915/i915_drv.h     |  6 ++----
>  drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++++-------
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index aefab9a..5a62d7a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1445,7 +1445,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
>  	intel_opregion_notify_adapter(dev_priv, opregion_target_state);
>  
> -	intel_uncore_forcewake_reset(dev_priv, false);
> +	intel_uncore_suspend(dev_priv);
>  	intel_opregion_unregister(dev_priv);
>  
>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
> @@ -1690,7 +1690,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  		DRM_ERROR("Resume prepare failed: %d, continuing anyway\n",
>  			  ret);
>  
> -	intel_uncore_early_sanitize(dev_priv, true);
> +	intel_uncore_resume(dev_priv);
>  
>  	if (IS_GEN9_LP(dev_priv)) {
>  		if (!dev_priv->suspended_to_idle)
> @@ -2344,7 +2344,7 @@ static int intel_runtime_suspend(struct device *kdev)
>  		return ret;
>  	}
>  
> -	intel_uncore_forcewake_reset(dev_priv, false);
> +	intel_uncore_suspend(dev_priv);

Doing one from early_resume and the other from the normal suspend makes
my brain hurt a little. If we do that I think we should at least name
the functions appropriately.

>  
>  	enable_rpm_wakeref_asserts(dev_priv);
>  	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e9b4ece..c717329 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2976,14 +2976,12 @@ int intel_irq_install(struct drm_i915_private *dev_priv);
>  void intel_irq_uninstall(struct drm_i915_private *dev_priv);
>  
>  extern void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
> -extern void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
> -					bool restore_forcewake);
>  extern void intel_uncore_init(struct drm_i915_private *dev_priv);
>  extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
>  extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
>  extern void intel_uncore_fini(struct drm_i915_private *dev_priv);
> -extern void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
> -					 bool restore);
> +extern void intel_uncore_suspend(struct drm_i915_private *dev_priv);
> +extern void intel_uncore_resume(struct drm_i915_private *dev_priv);
>  const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
>  void intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
>  				enum forcewake_domains domains);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index abe0888..3767307 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -250,7 +250,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>  	return HRTIMER_NORESTART;
>  }
>  
> -void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
> +static void __intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  				  bool restore)

Maybe leave out this rename to keep the diff a little easier to parse.

>  {
>  	unsigned long irqflags;
> @@ -424,13 +424,17 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
>  	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B_LAST))
>  		info->has_decoupled_mmio = false;
>  
> -	intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
> +	__intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
>  }
>  
> -void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
> -				 bool restore_forcewake)
> +void intel_uncore_suspend(struct drm_i915_private *dev_priv)
>  {
> -	__intel_uncore_early_sanitize(dev_priv, restore_forcewake);
> +	__intel_uncore_forcewake_reset(dev_priv, false);
> +}
> +
> +void intel_uncore_resume(struct drm_i915_private *dev_priv)
> +{
> +	__intel_uncore_early_sanitize(dev_priv, true);
>  	i915_check_and_clear_faults(dev_priv);
>  }
>  
> @@ -1463,7 +1467,7 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv)
>  {
>  	/* Paranoia: make sure we have disabled everything before we exit. */
>  	intel_uncore_sanitize(dev_priv);
> -	intel_uncore_forcewake_reset(dev_priv, false);
> +	__intel_uncore_forcewake_reset(dev_priv, false);
>  }
>  
>  #define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1)
> @@ -1679,7 +1683,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>  
>  	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
>  
> -	intel_uncore_forcewake_reset(dev_priv, true);
> +	__intel_uncore_forcewake_reset(dev_priv, true);
>  
>  	return ret;
>  }
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Ville Syrjälä]

On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> around P-Unit write accesses.

Can't we just stuff the calls into the actual punit write function
rather than sprinkling them all over the place?

+ a comment would be nice why it's there.

Do we need a kconfig select/depends on the iosf_mbi thing? Or some
ifdefs?

> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
> Changes in v2:
> -Spelling: P-Unit, PMIC
> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
> ---
>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5604701..13e5152 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -47,6 +47,7 @@
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
>  #include <linux/reservation.h>
> +#include <asm/iosf_mbi.h>
>  
>  static bool is_mmio_work(struct intel_flip_work *work)
>  {
> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>  		cmd = 0;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_acquire();
> +
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	val &= ~DSPFREQGUAR_MASK;
>  	val |= (cmd << DSPFREQGUAR_SHIFT);
> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>  		     50)) {
>  		DRM_ERROR("timed out waiting for CDclk change\n");
>  	}
> +	iosf_mbi_punit_release();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	mutex_lock(&dev_priv->sb_lock);
> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>  	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_acquire();
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	val &= ~DSPFREQGUAR_MASK_CHV;
>  	val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>  		     50)) {
>  		DRM_ERROR("timed out waiting for CDclk change\n");
>  	}
> +	iosf_mbi_punit_release();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	intel_update_cdclk(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 249623d..adff84a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -32,6 +32,7 @@
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <asm/iosf_mbi.h>
>  
>  /**
>   * DOC: RC6
> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>  	u32 val;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_acquire();
>  
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>  	if (enable)
> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>  		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>  		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>  
> +	iosf_mbi_punit_release();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>  	u32 val;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_acquire();
>  
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	if (enable)
> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>  		val &= ~DSP_MAXFIFO_PM5_ENABLE;
>  	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>  
> +	iosf_mbi_punit_release();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		mutex_lock(&dev_priv->rps.hw_lock);
> +		iosf_mbi_punit_acquire();
>  
>  		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  		if (val & DSP_MAXFIFO_PM5_ENABLE)
> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  				wm->level = VLV_WM_LEVEL_DDR_DVFS;
>  		}
>  
> +		iosf_mbi_punit_release();
>  		mutex_unlock(&dev_priv->rps.hw_lock);
>  	}
>  
> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>  
>  	if (val != dev_priv->rps.cur_freq) {
> +		iosf_mbi_punit_acquire();
>  		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> +		iosf_mbi_punit_release();
>  		if (!IS_CHERRYVIEW(dev_priv))
>  			gen6_set_rps_thresholds(dev_priv, val);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index c0b7e95..e66bcc8 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -28,6 +28,7 @@
>  
>  #include <linux/pm_runtime.h>
>  #include <linux/vgaarb.h>
> +#include <asm/iosf_mbi.h>
>  
>  #include "i915_drv.h"
>  #include "intel_drv.h"
> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>  	if (COND)
>  		goto out;
>  
> +	iosf_mbi_punit_acquire();
> +
>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>  	ctrl &= ~mask;
>  	ctrl |= state;
> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>  			  state,
>  			  vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>  
> +	iosf_mbi_punit_release();
> +
>  #undef COND
>  
>  out:
> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>  	if (COND)
>  		goto out;
>  
> +	iosf_mbi_punit_acquire();
> +
>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	ctrl &= ~DP_SSC_MASK(pipe);
>  	ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>  			  state,
>  			  vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>  
> +	iosf_mbi_punit_release();
> +
>  #undef COND
>  
>  out:
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 12/13] drm/i915: Listen for PMIC bus access notifications

[Ville Syrjälä]

On Mon, Jan 23, 2017 at 10:09:57PM +0100, Hans de Goede wrote:
> Listen for PMIC bus access notifications and get FORCEWAKE_ALL while
> the bus is accessed to avoid needing to do any forcewakes, which need
> PMIC bus access, while the PMIC bus is busy:
> 
> This fixes errors like these showing up in dmesg, usually followed
> by a gfx or system freeze:
> 
> [drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
> [drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
> i2c_designware 808622C1:06: punit semaphore timed out, resetting
> i2c_designware 808622C1:06: PUNIT SEM: 2
> i2c_designware 808622C1:06: couldn't acquire bus ownership
> 
> Downside of this approach is that it causes wakeups whenever the PMIC
> bus is accessed. Unfortunately we cannot simply wait for the PMIC bus
> to go idle when we hit a race, as forcewakes may be done from interrupt
> handlers where we cannot sleep to wait for the i2c PMIC bus access to
> finish.
> 
> Note that the notifications and thus the wakeups will only happen on
> baytrail / cherrytrail devices using PMICs with a shared i2c bus for
> P-Unit and host PMIC access (i2c busses with a _SEM method in their
> APCI node), e.g. an axp288 PMIC.
> 
> I plan to write some patches for drivers accessing the PMIC bus to
> limit their bus accesses to a bare minimum (e.g. cache registers, do not
> update battery level more often then 4 times a minute), to limit the
> amount of wakeups.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
> Changes in v2:
> -Spelling: P-Unit, PMIC
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_uncore.c | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c717329..52f7dde 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -721,6 +721,7 @@ struct intel_uncore {
>  	const struct intel_forcewake_range *fw_domains_table;
>  	unsigned int fw_domains_table_entries;
>  
> +	struct notifier_block pmic_bus_access_nb;
>  	struct intel_uncore_funcs funcs;
>  
>  	unsigned fifo_count;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 3767307..175fe02 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -25,6 +25,7 @@
>  #include "intel_drv.h"
>  #include "i915_vgpu.h"
>  
> +#include <asm/iosf_mbi.h>
>  #include <linux/pm_runtime.h>
>  
>  #define FORCEWAKE_ACK_TIMEOUT_MS 50
> @@ -429,12 +430,16 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
>  
>  void intel_uncore_suspend(struct drm_i915_private *dev_priv)
>  {
> +	iosf_mbi_unregister_pmic_bus_access_notifier(
> +		&dev_priv->uncore.pmic_bus_access_nb);
>  	__intel_uncore_forcewake_reset(dev_priv, false);
>  }
>  
>  void intel_uncore_resume(struct drm_i915_private *dev_priv)
>  {
>  	__intel_uncore_early_sanitize(dev_priv, true);
> +	iosf_mbi_register_pmic_bus_access_notifier(
> +		&dev_priv->uncore.pmic_bus_access_nb);
>  	i915_check_and_clear_faults(dev_priv);
>  }

The early/normal/late suspend/resume ordering starts to bother me a
little more now. I wonder if we're totally safe wrt. the suspend/resume
order of the devices now.

> @@ -1390,6 +1395,28 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>  	dev_priv->uncore.fw_domains_table_entries = ARRAY_SIZE((d)); \
>  }
>  
> +static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
> +					 unsigned long action, void *data)
> +{
> +	struct drm_i915_private *dev_priv = container_of(nb,
> +			struct drm_i915_private, uncore.pmic_bus_access_nb);
> +
> +	switch (action) {
> +	case MBI_PMIC_BUS_ACCESS_BEGIN:
> +		/*
> +		 * forcewake all to make sure that we don't need to forcewake
> +		 * any power-planes while the pmic bus is busy.
> +		 */
> +		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);

I must say I don't really like this stuff at all. But if it helps I gues
we should go for it. I'd like to see the comment elaborate a bit more on
why we think it's is needed.

> +		break;
> +	case MBI_PMIC_BUS_ACCESS_END:
> +		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>  void intel_uncore_init(struct drm_i915_private *dev_priv)
>  {
>  	i915_check_vgpu(dev_priv);
> @@ -1399,6 +1426,8 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>  	__intel_uncore_early_sanitize(dev_priv, false);
>  
>  	dev_priv->uncore.unclaimed_mmio_check = 1;
> +	dev_priv->uncore.pmic_bus_access_nb.notifier_call =
> +		i915_pmic_bus_access_notifier;
>  
>  	switch (INTEL_INFO(dev_priv)->gen) {
>  	default:
> @@ -1458,6 +1487,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>  		ASSIGN_READ_MMIO_VFUNCS(vgpu);
>  	}
>  
> +	iosf_mbi_register_pmic_bus_access_notifier(
> +		&dev_priv->uncore.pmic_bus_access_nb);
> +
>  	i915_check_and_clear_faults(dev_priv);
>  }
>  #undef ASSIGN_WRITE_MMIO_VFUNCS
> @@ -1465,6 +1497,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>  
>  void intel_uncore_fini(struct drm_i915_private *dev_priv)
>  {
> +	iosf_mbi_unregister_pmic_bus_access_notifier(
> +		&dev_priv->uncore.pmic_bus_access_nb);
> +
>  	/* Paranoia: make sure we have disabled everything before we exit. */
>  	intel_uncore_sanitize(dev_priv);
>  	__intel_uncore_forcewake_reset(dev_priv, false);
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/13] drm/i915: Add intel_uncore_suspend / resume functions

[Hans de Goede]

Hi,

On 01/27/2017 02:45 PM, Ville Syrjälä wrote:
> On Mon, Jan 23, 2017 at 10:09:56PM +0100, Hans de Goede wrote:
>> Rename intel_uncore_early_sanitize to intel_uncore_resume, dropping the
>> (always true) restore_forcewake argument and add a new intel_uncore_resume
>> function to replace the intel_uncore_forcewake_reset(dev_priv, false)
>> calls done from the suspend / runtime_suspend functions and make
>> intel_uncore_forcewake_reset private.
>>
>> This is a preparation patch for adding PMIC bus access notifier support.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>> Changes in v2:
>> -Spelling: P-Unit, PMIC
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c     |  6 +++---
>>  drivers/gpu/drm/i915/i915_drv.h     |  6 ++----
>>  drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++++-------
>>  3 files changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index aefab9a..5a62d7a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1445,7 +1445,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>>  	opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
>>  	intel_opregion_notify_adapter(dev_priv, opregion_target_state);
>>
>> -	intel_uncore_forcewake_reset(dev_priv, false);
>> +	intel_uncore_suspend(dev_priv);
>>  	intel_opregion_unregister(dev_priv);
>>
>>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
>> @@ -1690,7 +1690,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
>>  		DRM_ERROR("Resume prepare failed: %d, continuing anyway\n",
>>  			  ret);
>>
>> -	intel_uncore_early_sanitize(dev_priv, true);
>> +	intel_uncore_resume(dev_priv);
>>
>>  	if (IS_GEN9_LP(dev_priv)) {
>>  		if (!dev_priv->suspended_to_idle)
>> @@ -2344,7 +2344,7 @@ static int intel_runtime_suspend(struct device *kdev)
>>  		return ret;
>>  	}
>>
>> -	intel_uncore_forcewake_reset(dev_priv, false);
>> +	intel_uncore_suspend(dev_priv);
>
> Doing one from early_resume and the other from the normal suspend makes
> my brain hurt a little.

Ack, note that this patch does not introduce this, it just renames things
a bit, the sequence of things is not changed.

> If we do that I think we should at least name
> the functions appropriately.

Ok, so you want me to rename intel_uncore_resume to intel_uncore_resume_early
I assume ?

>>
>>  	enable_rpm_wakeref_asserts(dev_priv);
>>  	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index e9b4ece..c717329 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2976,14 +2976,12 @@ int intel_irq_install(struct drm_i915_private *dev_priv);
>>  void intel_irq_uninstall(struct drm_i915_private *dev_priv);
>>
>>  extern void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
>> -extern void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
>> -					bool restore_forcewake);
>>  extern void intel_uncore_init(struct drm_i915_private *dev_priv);
>>  extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
>>  extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
>>  extern void intel_uncore_fini(struct drm_i915_private *dev_priv);
>> -extern void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>> -					 bool restore);
>> +extern void intel_uncore_suspend(struct drm_i915_private *dev_priv);
>> +extern void intel_uncore_resume(struct drm_i915_private *dev_priv);
>>  const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
>>  void intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
>>  				enum forcewake_domains domains);
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index abe0888..3767307 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -250,7 +250,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>>  	return HRTIMER_NORESTART;
>>  }
>>
>> -void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>> +static void __intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>>  				  bool restore)
>
> Maybe leave out this rename to keep the diff a little easier to parse.

Ack will do for v3.

>
>>  {
>>  	unsigned long irqflags;
>> @@ -424,13 +424,17 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
>>  	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B_LAST))
>>  		info->has_decoupled_mmio = false;
>>
>> -	intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
>> +	__intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
>>  }
>>
>> -void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
>> -				 bool restore_forcewake)
>> +void intel_uncore_suspend(struct drm_i915_private *dev_priv)
>>  {
>> -	__intel_uncore_early_sanitize(dev_priv, restore_forcewake);
>> +	__intel_uncore_forcewake_reset(dev_priv, false);
>> +}
>> +
>> +void intel_uncore_resume(struct drm_i915_private *dev_priv)
>> +{
>> +	__intel_uncore_early_sanitize(dev_priv, true);
>>  	i915_check_and_clear_faults(dev_priv);
>>  }
>>
>> @@ -1463,7 +1467,7 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv)
>>  {
>>  	/* Paranoia: make sure we have disabled everything before we exit. */
>>  	intel_uncore_sanitize(dev_priv);
>> -	intel_uncore_forcewake_reset(dev_priv, false);
>> +	__intel_uncore_forcewake_reset(dev_priv, false);
>>  }
>>
>>  #define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1)
>> @@ -1679,7 +1683,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>>
>>  	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
>>
>> -	intel_uncore_forcewake_reset(dev_priv, true);
>> +	__intel_uncore_forcewake_reset(dev_priv, true);
>>
>>  	return ret;
>>  }
>> --
>> 2.9.3
>

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Hans de Goede]

Hi,

On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>> around P-Unit write accesses.
>
> Can't we just stuff the calls into the actual punit write function
> rather than sprinkling them all over the place?

punit access is acquired across sections like this:

         iosf_mbi_punit_acquire();

         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
         val &= ~DSPFREQGUAR_MASK;
         val |= (cmd << DSPFREQGUAR_SHIFT);
         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
                      50)) {
                 DRM_ERROR("timed out waiting for CDclk change\n");
         }
         iosf_mbi_punit_release();

Where we want to wait for the requested change to have taken
effect before releasing the punit.

>
> + a comment would be nice why it's there.

I will add comments to the acquire calls.

> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> ifdefs?

No, the iosf_mbi header defines empty inline versions of
iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
this does mean that iosf_mbi must be builtin if the i915
driver is. I'll add:

	depends on DRM_I915=IOSF_MBI || IOSF_MBI=y

To the i915 Kconfig to enforce this.

Regards,

Hans


>
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>> Changes in v2:
>> -Spelling: P-Unit, PMIC
>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>> ---
>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 5604701..13e5152 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -47,6 +47,7 @@
>>  #include <drm/drm_rect.h>
>>  #include <linux/dma_remapping.h>
>>  #include <linux/reservation.h>
>> +#include <asm/iosf_mbi.h>
>>
>>  static bool is_mmio_work(struct intel_flip_work *work)
>>  {
>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>  		cmd = 0;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_acquire();
>> +
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	val &= ~DSPFREQGUAR_MASK;
>>  	val |= (cmd << DSPFREQGUAR_SHIFT);
>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>  		     50)) {
>>  		DRM_ERROR("timed out waiting for CDclk change\n");
>>  	}
>> +	iosf_mbi_punit_release();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>
>>  	mutex_lock(&dev_priv->sb_lock);
>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>  	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_acquire();
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	val &= ~DSPFREQGUAR_MASK_CHV;
>>  	val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>  		     50)) {
>>  		DRM_ERROR("timed out waiting for CDclk change\n");
>>  	}
>> +	iosf_mbi_punit_release();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>
>>  	intel_update_cdclk(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 249623d..adff84a 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -32,6 +32,7 @@
>>  #include "../../../platform/x86/intel_ips.h"
>>  #include <linux/module.h>
>>  #include <drm/drm_atomic_helper.h>
>> +#include <asm/iosf_mbi.h>
>>
>>  /**
>>   * DOC: RC6
>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>  	u32 val;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_acquire();
>>
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>>  	if (enable)
>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>  		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>>  		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>>
>> +	iosf_mbi_punit_release();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>  }
>>
>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>  	u32 val;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_acquire();
>>
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	if (enable)
>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>  		val &= ~DSP_MAXFIFO_PM5_ENABLE;
>>  	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>
>> +	iosf_mbi_punit_release();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>  }
>>
>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>
>>  	if (IS_CHERRYVIEW(dev_priv)) {
>>  		mutex_lock(&dev_priv->rps.hw_lock);
>> +		iosf_mbi_punit_acquire();
>>
>>  		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  		if (val & DSP_MAXFIFO_PM5_ENABLE)
>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>  				wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>  		}
>>
>> +		iosf_mbi_punit_release();
>>  		mutex_unlock(&dev_priv->rps.hw_lock);
>>  	}
>>
>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>  	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>
>>  	if (val != dev_priv->rps.cur_freq) {
>> +		iosf_mbi_punit_acquire();
>>  		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>> +		iosf_mbi_punit_release();
>>  		if (!IS_CHERRYVIEW(dev_priv))
>>  			gen6_set_rps_thresholds(dev_priv, val);
>>  	}
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index c0b7e95..e66bcc8 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -28,6 +28,7 @@
>>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/vgaarb.h>
>> +#include <asm/iosf_mbi.h>
>>
>>  #include "i915_drv.h"
>>  #include "intel_drv.h"
>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>  	if (COND)
>>  		goto out;
>>
>> +	iosf_mbi_punit_acquire();
>> +
>>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>>  	ctrl &= ~mask;
>>  	ctrl |= state;
>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>  			  state,
>>  			  vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>>
>> +	iosf_mbi_punit_release();
>> +
>>  #undef COND
>>
>>  out:
>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>  	if (COND)
>>  		goto out;
>>
>> +	iosf_mbi_punit_acquire();
>> +
>>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	ctrl &= ~DP_SSC_MASK(pipe);
>>  	ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>  			  state,
>>  			  vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>>
>> +	iosf_mbi_punit_release();
>> +
>>  #undef COND
>>
>>  out:
>> --
>> 2.9.3
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Hans de Goede]

Hi,

On 01/28/2017 05:25 PM, Hans de Goede wrote:
> Hi,
>
> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>> around P-Unit write accesses.
>>
>> Can't we just stuff the calls into the actual punit write function
>> rather than sprinkling them all over the place?
>
> punit access is acquired across sections like this:
>
>         iosf_mbi_punit_acquire();
>
>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>         val &= ~DSPFREQGUAR_MASK;
>         val |= (cmd << DSPFREQGUAR_SHIFT);
>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>                      50)) {
>                 DRM_ERROR("timed out waiting for CDclk change\n");
>         }
>         iosf_mbi_punit_release();
>
> Where we want to wait for the requested change to have taken
> effect before releasing the punit.
>
>>
>> + a comment would be nice why it's there.
>
> I will add comments to the acquire calls.
>
>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>> ifdefs?
>
> No, the iosf_mbi header defines empty inline versions of
> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> this does mean that iosf_mbi must be builtin if the i915
> driver is. I'll add:
>
>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>
> To the i915 Kconfig to enforce this.

Hmm, ok so that does not work (long cyclic dependency through the
selection of ACPI_VIDEO).

So I've now added this instead:

	# iosf_mbi needs to be builtin if we are builtin
	select IOSF_MBI if DRM_I915=y

Regards,

Hans


>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>>> ---
>>> Changes in v2:
>>> -Spelling: P-Unit, PMIC
>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>>>  3 files changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 5604701..13e5152 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -47,6 +47,7 @@
>>>  #include <drm/drm_rect.h>
>>>  #include <linux/dma_remapping.h>
>>>  #include <linux/reservation.h>
>>> +#include <asm/iosf_mbi.h>
>>>
>>>  static bool is_mmio_work(struct intel_flip_work *work)
>>>  {
>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>          cmd = 0;
>>>
>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>> +    iosf_mbi_punit_acquire();
>>> +
>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>      val &= ~DSPFREQGUAR_MASK;
>>>      val |= (cmd << DSPFREQGUAR_SHIFT);
>>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>               50)) {
>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>      }
>>> +    iosf_mbi_punit_release();
>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>
>>>      mutex_lock(&dev_priv->sb_lock);
>>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>>>
>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>> +    iosf_mbi_punit_acquire();
>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>      val &= ~DSPFREQGUAR_MASK_CHV;
>>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>               50)) {
>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>      }
>>> +    iosf_mbi_punit_release();
>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>
>>>      intel_update_cdclk(dev_priv);
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 249623d..adff84a 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -32,6 +32,7 @@
>>>  #include "../../../platform/x86/intel_ips.h"
>>>  #include <linux/module.h>
>>>  #include <drm/drm_atomic_helper.h>
>>> +#include <asm/iosf_mbi.h>
>>>
>>>  /**
>>>   * DOC: RC6
>>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>      u32 val;
>>>
>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>> +    iosf_mbi_punit_acquire();
>>>
>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>>>      if (enable)
>>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>>>
>>> +    iosf_mbi_punit_release();
>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>  }
>>>
>>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>      u32 val;
>>>
>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>> +    iosf_mbi_punit_acquire();
>>>
>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>      if (enable)
>>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
>>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>
>>> +    iosf_mbi_punit_release();
>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>  }
>>>
>>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>
>>>      if (IS_CHERRYVIEW(dev_priv)) {
>>>          mutex_lock(&dev_priv->rps.hw_lock);
>>> +        iosf_mbi_punit_acquire();
>>>
>>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>>          }
>>>
>>> +        iosf_mbi_punit_release();
>>>          mutex_unlock(&dev_priv->rps.hw_lock);
>>>      }
>>>
>>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>>
>>>      if (val != dev_priv->rps.cur_freq) {
>>> +        iosf_mbi_punit_acquire();
>>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>>> +        iosf_mbi_punit_release();
>>>          if (!IS_CHERRYVIEW(dev_priv))
>>>              gen6_set_rps_thresholds(dev_priv, val);
>>>      }
>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> index c0b7e95..e66bcc8 100644
>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> @@ -28,6 +28,7 @@
>>>
>>>  #include <linux/pm_runtime.h>
>>>  #include <linux/vgaarb.h>
>>> +#include <asm/iosf_mbi.h>
>>>
>>>  #include "i915_drv.h"
>>>  #include "intel_drv.h"
>>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>      if (COND)
>>>          goto out;
>>>
>>> +    iosf_mbi_punit_acquire();
>>> +
>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>>>      ctrl &= ~mask;
>>>      ctrl |= state;
>>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>                state,
>>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>>>
>>> +    iosf_mbi_punit_release();
>>> +
>>>  #undef COND
>>>
>>>  out:
>>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>      if (COND)
>>>          goto out;
>>>
>>> +    iosf_mbi_punit_acquire();
>>> +
>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>      ctrl &= ~DP_SSC_MASK(pipe);
>>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
>>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>                state,
>>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>>>
>>> +    iosf_mbi_punit_release();
>>> +
>>>  #undef COND
>>>
>>>  out:
>>> --
>>> 2.9.3
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 12/13] drm/i915: Listen for PMIC bus access notifications

[Hans de Goede]

Hi,

On 01/27/2017 02:52 PM, Ville Syrjälä wrote:
> On Mon, Jan 23, 2017 at 10:09:57PM +0100, Hans de Goede wrote:
>> Listen for PMIC bus access notifications and get FORCEWAKE_ALL while
>> the bus is accessed to avoid needing to do any forcewakes, which need
>> PMIC bus access, while the PMIC bus is busy:
>>
>> This fixes errors like these showing up in dmesg, usually followed
>> by a gfx or system freeze:
>>
>> [drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
>> [drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
>> i2c_designware 808622C1:06: punit semaphore timed out, resetting
>> i2c_designware 808622C1:06: PUNIT SEM: 2
>> i2c_designware 808622C1:06: couldn't acquire bus ownership
>>
>> Downside of this approach is that it causes wakeups whenever the PMIC
>> bus is accessed. Unfortunately we cannot simply wait for the PMIC bus
>> to go idle when we hit a race, as forcewakes may be done from interrupt
>> handlers where we cannot sleep to wait for the i2c PMIC bus access to
>> finish.
>>
>> Note that the notifications and thus the wakeups will only happen on
>> baytrail / cherrytrail devices using PMICs with a shared i2c bus for
>> P-Unit and host PMIC access (i2c busses with a _SEM method in their
>> APCI node), e.g. an axp288 PMIC.
>>
>> I plan to write some patches for drivers accessing the PMIC bus to
>> limit their bus accesses to a bare minimum (e.g. cache registers, do not
>> update battery level more often then 4 times a minute), to limit the
>> amount of wakeups.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>> Changes in v2:
>> -Spelling: P-Unit, PMIC
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>>  drivers/gpu/drm/i915/intel_uncore.c | 35 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index c717329..52f7dde 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -721,6 +721,7 @@ struct intel_uncore {
>>  	const struct intel_forcewake_range *fw_domains_table;
>>  	unsigned int fw_domains_table_entries;
>>
>> +	struct notifier_block pmic_bus_access_nb;
>>  	struct intel_uncore_funcs funcs;
>>
>>  	unsigned fifo_count;
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 3767307..175fe02 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -25,6 +25,7 @@
>>  #include "intel_drv.h"
>>  #include "i915_vgpu.h"
>>
>> +#include <asm/iosf_mbi.h>
>>  #include <linux/pm_runtime.h>
>>
>>  #define FORCEWAKE_ACK_TIMEOUT_MS 50
>> @@ -429,12 +430,16 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
>>
>>  void intel_uncore_suspend(struct drm_i915_private *dev_priv)
>>  {
>> +	iosf_mbi_unregister_pmic_bus_access_notifier(
>> +		&dev_priv->uncore.pmic_bus_access_nb);
>>  	__intel_uncore_forcewake_reset(dev_priv, false);
>>  }
>>
>>  void intel_uncore_resume(struct drm_i915_private *dev_priv)
>>  {
>>  	__intel_uncore_early_sanitize(dev_priv, true);
>> +	iosf_mbi_register_pmic_bus_access_notifier(
>> +		&dev_priv->uncore.pmic_bus_access_nb);
>>  	i915_check_and_clear_faults(dev_priv);
>>  }
>
> The early/normal/late suspend/resume ordering starts to bother me a
> little more now. I wonder if we're totally safe wrt. the suspend/resume
> order of the devices now.

As I mentioned before I'm not touching the existing ordering. As for the
newly added calls the unregister must be done before calling
intel_uncore_forcewake_reset() to make sure that the
i915_pmic_bus_access_notifier() has put any forcewakes it may hold. Note
this is ensured to happen on unregister as iosf_mbi_unregister_pmic_bus_access_notifier
waits for the bus to be idle before unregistering the notifier
(in a race free manner of course).

Likewise he iosf_mbi_register_pmic_bus_access_notifier() call is
done at the only possible spot, it must be done afer
__intel_uncore_early_sanitize and I added a WARN_ON to get a traceback
and the "timed out waiting for forcewake ack request" errors this patch
fixes are triggered (on suspend/resume) from i915_check_and_clear_faults().

Note tagorereddy was seeing these under different circumstances,
this is just the code-path I hit in my tests.

So we cannot just do a iosf_mbi_punit_acquire around the
i915_check_and_clear_faults() call, there are many other places
where a forcewake is done and we could potentially race.

>
>> @@ -1390,6 +1395,28 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>>  	dev_priv->uncore.fw_domains_table_entries = ARRAY_SIZE((d)); \
>>  }
>>
>> +static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
>> +					 unsigned long action, void *data)
>> +{
>> +	struct drm_i915_private *dev_priv = container_of(nb,
>> +			struct drm_i915_private, uncore.pmic_bus_access_nb);
>> +
>> +	switch (action) {
>> +	case MBI_PMIC_BUS_ACCESS_BEGIN:
>> +		/*
>> +		 * forcewake all to make sure that we don't need to forcewake
>> +		 * any power-planes while the pmic bus is busy.
>> +		 */
>> +		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>
> I must say I don't really like this stuff at all. But if it helps I gues
> we should go for it. I'd like to see the comment elaborate a bit more on
> why we think it's is needed.

OK, how about:

                 /*
                  * forcewake all now to make sure that we don't need to do a
                  * forcewake later which on systems where this notifier gets
                  * called requires the punit to access to the shared pmic i2c
                  * bus, which will be busy after this notification, leading to:
                  * "render: timed out waiting for forcewake ack request."
                  * errors.
                  */

?

>
>> +		break;
>> +	case MBI_PMIC_BUS_ACCESS_END:
>> +		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>> +		break;
>> +	}
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>>  void intel_uncore_init(struct drm_i915_private *dev_priv)
>>  {
>>  	i915_check_vgpu(dev_priv);
>> @@ -1399,6 +1426,8 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>>  	__intel_uncore_early_sanitize(dev_priv, false);
>>
>>  	dev_priv->uncore.unclaimed_mmio_check = 1;
>> +	dev_priv->uncore.pmic_bus_access_nb.notifier_call =
>> +		i915_pmic_bus_access_notifier;
>>
>>  	switch (INTEL_INFO(dev_priv)->gen) {
>>  	default:
>> @@ -1458,6 +1487,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>>  		ASSIGN_READ_MMIO_VFUNCS(vgpu);
>>  	}
>>
>> +	iosf_mbi_register_pmic_bus_access_notifier(
>> +		&dev_priv->uncore.pmic_bus_access_nb);
>> +
>>  	i915_check_and_clear_faults(dev_priv);
>>  }
>>  #undef ASSIGN_WRITE_MMIO_VFUNCS
>> @@ -1465,6 +1497,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>>
>>  void intel_uncore_fini(struct drm_i915_private *dev_priv)
>>  {
>> +	iosf_mbi_unregister_pmic_bus_access_notifier(
>> +		&dev_priv->uncore.pmic_bus_access_nb);
>> +
>>  	/* Paranoia: make sure we have disabled everything before we exit. */
>>  	intel_uncore_sanitize(dev_priv);
>>  	__intel_uncore_forcewake_reset(dev_priv, false);
>> --
>> 2.9.3
>


Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Ville Syrjälä]

On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 01/28/2017 05:25 PM, Hans de Goede wrote:
> > Hi,
> >
> > On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> >> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> >>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> >>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> >>> around P-Unit write accesses.
> >>
> >> Can't we just stuff the calls into the actual punit write function
> >> rather than sprinkling them all over the place?
> >
> > punit access is acquired across sections like this:
> >
> >         iosf_mbi_punit_acquire();
> >
> >         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >         val &= ~DSPFREQGUAR_MASK;
> >         val |= (cmd << DSPFREQGUAR_SHIFT);
> >         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> >                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> >                      50)) {
> >                 DRM_ERROR("timed out waiting for CDclk change\n");
> >         }
> >         iosf_mbi_punit_release();
> >
> > Where we want to wait for the requested change to have taken
> > effect before releasing the punit.

Hmm. That's somewhat unfortunate. It also highlights a problem with the
patch wrt. RPS. We don't wait for the GPU to actually change frequencies
in set_rps() because that would slow things down too much. So I have to
wonder how much luck is needed to make this workaround really effective.

> >
> >>
> >> + a comment would be nice why it's there.
> >
> > I will add comments to the acquire calls.
> >
> >> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >> ifdefs?
> >
> > No, the iosf_mbi header defines empty inline versions of
> > iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> > this does mean that iosf_mbi must be builtin if the i915
> > driver is. I'll add:
> >
> >     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >
> > To the i915 Kconfig to enforce this.
> 
> Hmm, ok so that does not work (long cyclic dependency through the
> selection of ACPI_VIDEO).
> 
> So I've now added this instead:
> 
> 	# iosf_mbi needs to be builtin if we are builtin
> 	select IOSF_MBI if DRM_I915=y

That's probably not going to help anyone since i915 is usually a module.

> 
> Regards,
> 
> Hans
> 
> 
> >>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> >>> ---
> >>> Changes in v2:
> >>> -Spelling: P-Unit, PMIC
> >>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
> >>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
> >>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
> >>>  3 files changed, 24 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 5604701..13e5152 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -47,6 +47,7 @@
> >>>  #include <drm/drm_rect.h>
> >>>  #include <linux/dma_remapping.h>
> >>>  #include <linux/reservation.h>
> >>> +#include <asm/iosf_mbi.h>
> >>>
> >>>  static bool is_mmio_work(struct intel_flip_work *work)
> >>>  {
> >>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>          cmd = 0;
> >>>
> >>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>> +    iosf_mbi_punit_acquire();
> >>> +
> >>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>      val &= ~DSPFREQGUAR_MASK;
> >>>      val |= (cmd << DSPFREQGUAR_SHIFT);
> >>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>               50)) {
> >>>          DRM_ERROR("timed out waiting for CDclk change\n");
> >>>      }
> >>> +    iosf_mbi_punit_release();
> >>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>
> >>>      mutex_lock(&dev_priv->sb_lock);
> >>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
> >>>
> >>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>> +    iosf_mbi_punit_acquire();
> >>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>      val &= ~DSPFREQGUAR_MASK_CHV;
> >>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
> >>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>               50)) {
> >>>          DRM_ERROR("timed out waiting for CDclk change\n");
> >>>      }
> >>> +    iosf_mbi_punit_release();
> >>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>
> >>>      intel_update_cdclk(dev_priv);
> >>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>> index 249623d..adff84a 100644
> >>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>> @@ -32,6 +32,7 @@
> >>>  #include "../../../platform/x86/intel_ips.h"
> >>>  #include <linux/module.h>
> >>>  #include <drm/drm_atomic_helper.h>
> >>> +#include <asm/iosf_mbi.h>
> >>>
> >>>  /**
> >>>   * DOC: RC6
> >>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> >>>      u32 val;
> >>>
> >>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>> +    iosf_mbi_punit_acquire();
> >>>
> >>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
> >>>      if (enable)
> >>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> >>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
> >>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
> >>>
> >>> +    iosf_mbi_punit_release();
> >>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>  }
> >>>
> >>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >>>      u32 val;
> >>>
> >>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>> +    iosf_mbi_punit_acquire();
> >>>
> >>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>      if (enable)
> >>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
> >>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>>
> >>> +    iosf_mbi_punit_release();
> >>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>  }
> >>>
> >>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>>
> >>>      if (IS_CHERRYVIEW(dev_priv)) {
> >>>          mutex_lock(&dev_priv->rps.hw_lock);
> >>> +        iosf_mbi_punit_acquire();
> >>>
> >>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
> >>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
> >>>          }
> >>>
> >>> +        iosf_mbi_punit_release();
> >>>          mutex_unlock(&dev_priv->rps.hw_lock);
> >>>      }
> >>>
> >>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
> >>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
> >>>
> >>>      if (val != dev_priv->rps.cur_freq) {
> >>> +        iosf_mbi_punit_acquire();
> >>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> >>> +        iosf_mbi_punit_release();
> >>>          if (!IS_CHERRYVIEW(dev_priv))
> >>>              gen6_set_rps_thresholds(dev_priv, val);
> >>>      }
> >>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> index c0b7e95..e66bcc8 100644
> >>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> @@ -28,6 +28,7 @@
> >>>
> >>>  #include <linux/pm_runtime.h>
> >>>  #include <linux/vgaarb.h>
> >>> +#include <asm/iosf_mbi.h>
> >>>
> >>>  #include "i915_drv.h"
> >>>  #include "intel_drv.h"
> >>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> >>>      if (COND)
> >>>          goto out;
> >>>
> >>> +    iosf_mbi_punit_acquire();
> >>> +
> >>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
> >>>      ctrl &= ~mask;
> >>>      ctrl |= state;
> >>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> >>>                state,
> >>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
> >>>
> >>> +    iosf_mbi_punit_release();
> >>> +
> >>>  #undef COND
> >>>
> >>>  out:
> >>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
> >>>      if (COND)
> >>>          goto out;
> >>>
> >>> +    iosf_mbi_punit_acquire();
> >>> +
> >>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>      ctrl &= ~DP_SSC_MASK(pipe);
> >>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
> >>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
> >>>                state,
> >>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
> >>>
> >>> +    iosf_mbi_punit_release();
> >>> +
> >>>  #undef COND
> >>>
> >>>  out:
> >>> --
> >>> 2.9.3
> >>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Hans de Goede]

Hi,

On 30-01-17 14:10, Ville Syrjälä wrote:
> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>> around P-Unit write accesses.
>>>>
>>>> Can't we just stuff the calls into the actual punit write function
>>>> rather than sprinkling them all over the place?
>>>
>>> punit access is acquired across sections like this:
>>>
>>>         iosf_mbi_punit_acquire();
>>>
>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>         val &= ~DSPFREQGUAR_MASK;
>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>                      50)) {
>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
>>>         }
>>>         iosf_mbi_punit_release();
>>>
>>> Where we want to wait for the requested change to have taken
>>> effect before releasing the punit.
>
> Hmm. That's somewhat unfortunate. It also highlights a problem with the
> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
> in set_rps() because that would slow things down too much. So I have to
> wonder how much luck is needed to make this workaround really effective.

So the history of this patch-set is that I wrote this patch before
writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
active (patch 12/13). Since a lot of testing was done with this
patch included in the patch-set and since it seemed a good idea
regardless (given my experience with accessing the punit vs
pmic bus accesses) I decided to leave it in.

Possibly just the patch to get FORCEWAKE_ALL is enough, that one
actually fixed things for me. That is also why I made this the
last patch in the set. I asked tagorereddy to test his system
without this patch, but he did not get around to that.

After all we do tell the punit to not touch the bus by acquiring
the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
for RPS freq changes it honors that and properly waits. Maybe it
honors that for all punit requests i915 does and the only real
problem is the forcewake stuff ?

I can try to drop this patch from my queue and run without it
for a while and see if things don't regress. And also ask
tagorereddy again to test his system that way.

Does that (dropping this patch for now) sound like a good idea?

>>>> + a comment would be nice why it's there.
>>>
>>> I will add comments to the acquire calls.
>>>
>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>>>> ifdefs?
>>>
>>> No, the iosf_mbi header defines empty inline versions of
>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
>>> this does mean that iosf_mbi must be builtin if the i915
>>> driver is. I'll add:
>>>
>>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>>>
>>> To the i915 Kconfig to enforce this.
>>
>> Hmm, ok so that does not work (long cyclic dependency through the
>> selection of ACPI_VIDEO).
>>
>> So I've now added this instead:
>>
>> 	# iosf_mbi needs to be builtin if we are builtin
>> 	select IOSF_MBI if DRM_I915=y
>
> That's probably not going to help anyone since i915 is usually a module.

Right, that is fine, then either the IOSF_MBI symbols are available,
or IOSF_MBI is disabled and we get the inline nops from the header.

The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
realistic IMHO, but will get triggered by the random-config testing
several contributors do and lead to an unresolved symbol error there.

Hmm, thinking about this, this hunk actually belongs in 12/13 as that
is the first patch to use iosf_mbi functions.

Regards,

Hans



>>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> -Spelling: P-Unit, PMIC
>>>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>>>>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>>>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>>>>>  3 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 5604701..13e5152 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -47,6 +47,7 @@
>>>>>  #include <drm/drm_rect.h>
>>>>>  #include <linux/dma_remapping.h>
>>>>>  #include <linux/reservation.h>
>>>>> +#include <asm/iosf_mbi.h>
>>>>>
>>>>>  static bool is_mmio_work(struct intel_flip_work *work)
>>>>>  {
>>>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>          cmd = 0;
>>>>>
>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +    iosf_mbi_punit_acquire();
>>>>> +
>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>      val &= ~DSPFREQGUAR_MASK;
>>>>>      val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>               50)) {
>>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>      }
>>>>> +    iosf_mbi_punit_release();
>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>
>>>>>      mutex_lock(&dev_priv->sb_lock);
>>>>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>>>>>
>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +    iosf_mbi_punit_acquire();
>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>      val &= ~DSPFREQGUAR_MASK_CHV;
>>>>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>>>>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>               50)) {
>>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>      }
>>>>> +    iosf_mbi_punit_release();
>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>
>>>>>      intel_update_cdclk(dev_priv);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>> index 249623d..adff84a 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>> @@ -32,6 +32,7 @@
>>>>>  #include "../../../platform/x86/intel_ips.h"
>>>>>  #include <linux/module.h>
>>>>>  #include <drm/drm_atomic_helper.h>
>>>>> +#include <asm/iosf_mbi.h>
>>>>>
>>>>>  /**
>>>>>   * DOC: RC6
>>>>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>>>      u32 val;
>>>>>
>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +    iosf_mbi_punit_acquire();
>>>>>
>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>>>>>      if (enable)
>>>>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>>>>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>>>>>
>>>>> +    iosf_mbi_punit_release();
>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>  }
>>>>>
>>>>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>>>      u32 val;
>>>>>
>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +    iosf_mbi_punit_acquire();
>>>>>
>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>      if (enable)
>>>>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
>>>>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>
>>>>> +    iosf_mbi_punit_release();
>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>  }
>>>>>
>>>>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>>
>>>>>      if (IS_CHERRYVIEW(dev_priv)) {
>>>>>          mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +        iosf_mbi_punit_acquire();
>>>>>
>>>>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
>>>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>>>>          }
>>>>>
>>>>> +        iosf_mbi_punit_release();
>>>>>          mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>      }
>>>>>
>>>>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>>>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>>>>
>>>>>      if (val != dev_priv->rps.cur_freq) {
>>>>> +        iosf_mbi_punit_acquire();
>>>>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>>>>> +        iosf_mbi_punit_release();
>>>>>          if (!IS_CHERRYVIEW(dev_priv))
>>>>>              gen6_set_rps_thresholds(dev_priv, val);
>>>>>      }
>>>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> index c0b7e95..e66bcc8 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> @@ -28,6 +28,7 @@
>>>>>
>>>>>  #include <linux/pm_runtime.h>
>>>>>  #include <linux/vgaarb.h>
>>>>> +#include <asm/iosf_mbi.h>
>>>>>
>>>>>  #include "i915_drv.h"
>>>>>  #include "intel_drv.h"
>>>>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>>>      if (COND)
>>>>>          goto out;
>>>>>
>>>>> +    iosf_mbi_punit_acquire();
>>>>> +
>>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>>>>>      ctrl &= ~mask;
>>>>>      ctrl |= state;
>>>>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>>>                state,
>>>>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>>>>>
>>>>> +    iosf_mbi_punit_release();
>>>>> +
>>>>>  #undef COND
>>>>>
>>>>>  out:
>>>>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>>>      if (COND)
>>>>>          goto out;
>>>>>
>>>>> +    iosf_mbi_punit_acquire();
>>>>> +
>>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>      ctrl &= ~DP_SSC_MASK(pipe);
>>>>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
>>>>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>>>                state,
>>>>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>>>>>
>>>>> +    iosf_mbi_punit_release();
>>>>> +
>>>>>  #undef COND
>>>>>
>>>>>  out:
>>>>> --
>>>>> 2.9.3
>>>>
>

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Ville Syrjälä]

On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 30-01-17 14:10, Ville Syrjälä wrote:
> > On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 01/28/2017 05:25 PM, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> >>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> >>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> >>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> >>>>> around P-Unit write accesses.
> >>>>
> >>>> Can't we just stuff the calls into the actual punit write function
> >>>> rather than sprinkling them all over the place?
> >>>
> >>> punit access is acquired across sections like this:
> >>>
> >>>         iosf_mbi_punit_acquire();
> >>>
> >>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>         val &= ~DSPFREQGUAR_MASK;
> >>>         val |= (cmd << DSPFREQGUAR_SHIFT);
> >>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> >>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> >>>                      50)) {
> >>>                 DRM_ERROR("timed out waiting for CDclk change\n");
> >>>         }
> >>>         iosf_mbi_punit_release();
> >>>
> >>> Where we want to wait for the requested change to have taken
> >>> effect before releasing the punit.
> >
> > Hmm. That's somewhat unfortunate. It also highlights a problem with the
> > patch wrt. RPS. We don't wait for the GPU to actually change frequencies
> > in set_rps() because that would slow things down too much. So I have to
> > wonder how much luck is needed to make this workaround really effective.
> 
> So the history of this patch-set is that I wrote this patch before
> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
> active (patch 12/13). Since a lot of testing was done with this
> patch included in the patch-set and since it seemed a good idea
> regardless (given my experience with accessing the punit vs
> pmic bus accesses) I decided to leave it in.
> 
> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
> actually fixed things for me. That is also why I made this the
> last patch in the set. I asked tagorereddy to test his system
> without this patch, but he did not get around to that.
> 
> After all we do tell the punit to not touch the bus by acquiring
> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
> for RPS freq changes it honors that and properly waits. Maybe it
> honors that for all punit requests i915 does and the only real
> problem is the forcewake stuff ?
> 
> I can try to drop this patch from my queue and run without it
> for a while and see if things don't regress. And also ask
> tagorereddy again to test his system that way.
> 
> Does that (dropping this patch for now) sound like a good idea?

More test results couldn't hurt at least. It also makes me wonder if
just bumping the timeouts to some ridiculously high value would fix
the problem as well.

> 
> >>>> + a comment would be nice why it's there.
> >>>
> >>> I will add comments to the acquire calls.
> >>>
> >>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >>>> ifdefs?
> >>>
> >>> No, the iosf_mbi header defines empty inline versions of
> >>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> >>> this does mean that iosf_mbi must be builtin if the i915
> >>> driver is. I'll add:
> >>>
> >>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >>>
> >>> To the i915 Kconfig to enforce this.
> >>
> >> Hmm, ok so that does not work (long cyclic dependency through the
> >> selection of ACPI_VIDEO).
> >>
> >> So I've now added this instead:
> >>
> >> 	# iosf_mbi needs to be builtin if we are builtin
> >> 	select IOSF_MBI if DRM_I915=y
> >
> > That's probably not going to help anyone since i915 is usually a module.
> 
> Right, that is fine, then either the IOSF_MBI symbols are available,
> or IOSF_MBI is disabled and we get the inline nops from the header.
> 
> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
> realistic IMHO, but will get triggered by the random-config testing
> several contributors do and lead to an unresolved symbol error there.

Well, from the user POV anything with IOSF_MBI==n can be a problem.
So I'm not sure if we should allow that.

> 
> Hmm, thinking about this, this hunk actually belongs in 12/13 as that
> is the first patch to use iosf_mbi functions.
> 
> Regards,
> 
> Hans
> 
> 
> 
> >>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> >>>>> ---
> >>>>> Changes in v2:
> >>>>> -Spelling: P-Unit, PMIC
> >>>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
> >>>>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
> >>>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
> >>>>>  3 files changed, 24 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index 5604701..13e5152 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -47,6 +47,7 @@
> >>>>>  #include <drm/drm_rect.h>
> >>>>>  #include <linux/dma_remapping.h>
> >>>>>  #include <linux/reservation.h>
> >>>>> +#include <asm/iosf_mbi.h>
> >>>>>
> >>>>>  static bool is_mmio_work(struct intel_flip_work *work)
> >>>>>  {
> >>>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>>>          cmd = 0;
> >>>>>
> >>>>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>> +
> >>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>      val &= ~DSPFREQGUAR_MASK;
> >>>>>      val |= (cmd << DSPFREQGUAR_SHIFT);
> >>>>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>>>               50)) {
> >>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
> >>>>>      }
> >>>>> +    iosf_mbi_punit_release();
> >>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>
> >>>>>      mutex_lock(&dev_priv->sb_lock);
> >>>>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
> >>>>>
> >>>>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>      val &= ~DSPFREQGUAR_MASK_CHV;
> >>>>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
> >>>>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>>>               50)) {
> >>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
> >>>>>      }
> >>>>> +    iosf_mbi_punit_release();
> >>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>
> >>>>>      intel_update_cdclk(dev_priv);
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>>> index 249623d..adff84a 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>>> @@ -32,6 +32,7 @@
> >>>>>  #include "../../../platform/x86/intel_ips.h"
> >>>>>  #include <linux/module.h>
> >>>>>  #include <drm/drm_atomic_helper.h>
> >>>>> +#include <asm/iosf_mbi.h>
> >>>>>
> >>>>>  /**
> >>>>>   * DOC: RC6
> >>>>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> >>>>>      u32 val;
> >>>>>
> >>>>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>>
> >>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
> >>>>>      if (enable)
> >>>>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> >>>>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
> >>>>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
> >>>>>
> >>>>> +    iosf_mbi_punit_release();
> >>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>  }
> >>>>>
> >>>>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >>>>>      u32 val;
> >>>>>
> >>>>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>>
> >>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>      if (enable)
> >>>>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >>>>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
> >>>>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>>>>
> >>>>> +    iosf_mbi_punit_release();
> >>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>  }
> >>>>>
> >>>>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>>>>
> >>>>>      if (IS_CHERRYVIEW(dev_priv)) {
> >>>>>          mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +        iosf_mbi_punit_acquire();
> >>>>>
> >>>>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
> >>>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>>>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
> >>>>>          }
> >>>>>
> >>>>> +        iosf_mbi_punit_release();
> >>>>>          mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>      }
> >>>>>
> >>>>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
> >>>>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
> >>>>>
> >>>>>      if (val != dev_priv->rps.cur_freq) {
> >>>>> +        iosf_mbi_punit_acquire();
> >>>>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> >>>>> +        iosf_mbi_punit_release();
> >>>>>          if (!IS_CHERRYVIEW(dev_priv))
> >>>>>              gen6_set_rps_thresholds(dev_priv, val);
> >>>>>      }
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>>>> index c0b7e95..e66bcc8 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>>>> @@ -28,6 +28,7 @@
> >>>>>
> >>>>>  #include <linux/pm_runtime.h>
> >>>>>  #include <linux/vgaarb.h>
> >>>>> +#include <asm/iosf_mbi.h>
> >>>>>
> >>>>>  #include "i915_drv.h"
> >>>>>  #include "intel_drv.h"
> >>>>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> >>>>>      if (COND)
> >>>>>          goto out;
> >>>>>
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>> +
> >>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
> >>>>>      ctrl &= ~mask;
> >>>>>      ctrl |= state;
> >>>>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> >>>>>                state,
> >>>>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
> >>>>>
> >>>>> +    iosf_mbi_punit_release();
> >>>>> +
> >>>>>  #undef COND
> >>>>>
> >>>>>  out:
> >>>>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
> >>>>>      if (COND)
> >>>>>          goto out;
> >>>>>
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>> +
> >>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>      ctrl &= ~DP_SSC_MASK(pipe);
> >>>>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
> >>>>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
> >>>>>                state,
> >>>>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
> >>>>>
> >>>>> +    iosf_mbi_punit_release();
> >>>>> +
> >>>>>  #undef COND
> >>>>>
> >>>>>  out:
> >>>>> --
> >>>>> 2.9.3
> >>>>
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Hans de Goede]

Hi,

On 30-01-17 16:11, Ville Syrjälä wrote:
> On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-01-17 14:10, Ville Syrjälä wrote:
>>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>>>> around P-Unit write accesses.
>>>>>>
>>>>>> Can't we just stuff the calls into the actual punit write function
>>>>>> rather than sprinkling them all over the place?
>>>>>
>>>>> punit access is acquired across sections like this:
>>>>>
>>>>>         iosf_mbi_punit_acquire();
>>>>>
>>>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>         val &= ~DSPFREQGUAR_MASK;
>>>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>>>                      50)) {
>>>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>         }
>>>>>         iosf_mbi_punit_release();
>>>>>
>>>>> Where we want to wait for the requested change to have taken
>>>>> effect before releasing the punit.
>>>
>>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
>>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
>>> in set_rps() because that would slow things down too much. So I have to
>>> wonder how much luck is needed to make this workaround really effective.
>>
>> So the history of this patch-set is that I wrote this patch before
>> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
>> active (patch 12/13). Since a lot of testing was done with this
>> patch included in the patch-set and since it seemed a good idea
>> regardless (given my experience with accessing the punit vs
>> pmic bus accesses) I decided to leave it in.
>>
>> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
>> actually fixed things for me. That is also why I made this the
>> last patch in the set. I asked tagorereddy to test his system
>> without this patch, but he did not get around to that.
>>
>> After all we do tell the punit to not touch the bus by acquiring
>> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
>> for RPS freq changes it honors that and properly waits. Maybe it
>> honors that for all punit requests i915 does and the only real
>> problem is the forcewake stuff ?
>>
>> I can try to drop this patch from my queue and run without it
>> for a while and see if things don't regress. And also ask
>> tagorereddy again to test his system that way.
>>
>> Does that (dropping this patch for now) sound like a good idea?
>
> More test results couldn't hurt at least. It also makes me wonder if
> just bumping the timeouts to some ridiculously high value would fix
> the problem as well.

I've already tried bumping the forcewake timeout from 50 to 250ms,
before writing the patch to just get forcewake_all before the pmic
bus access begins, that does not fix things, and since we busy wait
for this timeout from non-sleeping context 250ms already is way too
high.

>>>>>> + a comment would be nice why it's there.
>>>>>
>>>>> I will add comments to the acquire calls.
>>>>>
>>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>>>>>> ifdefs?
>>>>>
>>>>> No, the iosf_mbi header defines empty inline versions of
>>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
>>>>> this does mean that iosf_mbi must be builtin if the i915
>>>>> driver is. I'll add:
>>>>>
>>>>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>>>>>
>>>>> To the i915 Kconfig to enforce this.
>>>>
>>>> Hmm, ok so that does not work (long cyclic dependency through the
>>>> selection of ACPI_VIDEO).
>>>>
>>>> So I've now added this instead:
>>>>
>>>> 	# iosf_mbi needs to be builtin if we are builtin
>>>> 	select IOSF_MBI if DRM_I915=y
>>>
>>> That's probably not going to help anyone since i915 is usually a module.
>>
>> Right, that is fine, then either the IOSF_MBI symbols are available,
>> or IOSF_MBI is disabled and we get the inline nops from the header.
>>
>> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
>> realistic IMHO, but will get triggered by the random-config testing
>> several contributors do and lead to an unresolved symbol error there.
>
> Well, from the user POV anything with IOSF_MBI==n can be a problem.
> So I'm not sure if we should allow that.

So you're suggesting we just add an unconditional "select IOSF_MBI"
to the i915 Kconfig entry?

Regards,

Hans



>>>>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> -Spelling: P-Unit, PMIC
>>>>>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>>>>>>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>>>>>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>>>>>>>  3 files changed, 24 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>>> index 5604701..13e5152 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>>> @@ -47,6 +47,7 @@
>>>>>>>  #include <drm/drm_rect.h>
>>>>>>>  #include <linux/dma_remapping.h>
>>>>>>>  #include <linux/reservation.h>
>>>>>>> +#include <asm/iosf_mbi.h>
>>>>>>>
>>>>>>>  static bool is_mmio_work(struct intel_flip_work *work)
>>>>>>>  {
>>>>>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>>>          cmd = 0;
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>> +
>>>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>      val &= ~DSPFREQGUAR_MASK;
>>>>>>>      val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>>>               50)) {
>>>>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>>>      }
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->sb_lock);
>>>>>>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>      val &= ~DSPFREQGUAR_MASK_CHV;
>>>>>>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>>>>>>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>>>               50)) {
>>>>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>>>      }
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>
>>>>>>>      intel_update_cdclk(dev_priv);
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> index 249623d..adff84a 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> @@ -32,6 +32,7 @@
>>>>>>>  #include "../../../platform/x86/intel_ips.h"
>>>>>>>  #include <linux/module.h>
>>>>>>>  #include <drm/drm_atomic_helper.h>
>>>>>>> +#include <asm/iosf_mbi.h>
>>>>>>>
>>>>>>>  /**
>>>>>>>   * DOC: RC6
>>>>>>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>>>>>      u32 val;
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>>
>>>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>>>>>>>      if (enable)
>>>>>>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>>>>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>>>>>>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>>>>>>>
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>  }
>>>>>>>
>>>>>>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>>>>>      u32 val;
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>>
>>>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>      if (enable)
>>>>>>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>>>>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
>>>>>>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>>>
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>  }
>>>>>>>
>>>>>>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>>>>
>>>>>>>      if (IS_CHERRYVIEW(dev_priv)) {
>>>>>>>          mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +        iosf_mbi_punit_acquire();
>>>>>>>
>>>>>>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
>>>>>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>>>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>>>>>>          }
>>>>>>>
>>>>>>> +        iosf_mbi_punit_release();
>>>>>>>          mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>      }
>>>>>>>
>>>>>>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>>>>>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>>>>>>
>>>>>>>      if (val != dev_priv->rps.cur_freq) {
>>>>>>> +        iosf_mbi_punit_acquire();
>>>>>>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>>>>>>> +        iosf_mbi_punit_release();
>>>>>>>          if (!IS_CHERRYVIEW(dev_priv))
>>>>>>>              gen6_set_rps_thresholds(dev_priv, val);
>>>>>>>      }
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>>>> index c0b7e95..e66bcc8 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>>>> @@ -28,6 +28,7 @@
>>>>>>>
>>>>>>>  #include <linux/pm_runtime.h>
>>>>>>>  #include <linux/vgaarb.h>
>>>>>>> +#include <asm/iosf_mbi.h>
>>>>>>>
>>>>>>>  #include "i915_drv.h"
>>>>>>>  #include "intel_drv.h"
>>>>>>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>>>>>      if (COND)
>>>>>>>          goto out;
>>>>>>>
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>> +
>>>>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>>>>>>>      ctrl &= ~mask;
>>>>>>>      ctrl |= state;
>>>>>>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>>>>>                state,
>>>>>>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>>>>>>>
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>> +
>>>>>>>  #undef COND
>>>>>>>
>>>>>>>  out:
>>>>>>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>>>>>      if (COND)
>>>>>>>          goto out;
>>>>>>>
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>> +
>>>>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>      ctrl &= ~DP_SSC_MASK(pipe);
>>>>>>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
>>>>>>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>>>>>                state,
>>>>>>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>>>>>>>
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>> +
>>>>>>>  #undef COND
>>>>>>>
>>>>>>>  out:
>>>>>>> --
>>>>>>> 2.9.3
>>>>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Ville Syrjälä]

On Mon, Jan 30, 2017 at 04:27:58PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 30-01-17 16:11, Ville Syrjälä wrote:
> > On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 30-01-17 14:10, Ville Syrjälä wrote:
> >>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> >>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> >>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> >>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> >>>>>>> around P-Unit write accesses.
> >>>>>>
> >>>>>> Can't we just stuff the calls into the actual punit write function
> >>>>>> rather than sprinkling them all over the place?
> >>>>>
> >>>>> punit access is acquired across sections like this:
> >>>>>
> >>>>>         iosf_mbi_punit_acquire();
> >>>>>
> >>>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>         val &= ~DSPFREQGUAR_MASK;
> >>>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
> >>>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> >>>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> >>>>>                      50)) {
> >>>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
> >>>>>         }
> >>>>>         iosf_mbi_punit_release();
> >>>>>
> >>>>> Where we want to wait for the requested change to have taken
> >>>>> effect before releasing the punit.
> >>>
> >>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
> >>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
> >>> in set_rps() because that would slow things down too much. So I have to
> >>> wonder how much luck is needed to make this workaround really effective.
> >>
> >> So the history of this patch-set is that I wrote this patch before
> >> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
> >> active (patch 12/13). Since a lot of testing was done with this
> >> patch included in the patch-set and since it seemed a good idea
> >> regardless (given my experience with accessing the punit vs
> >> pmic bus accesses) I decided to leave it in.
> >>
> >> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
> >> actually fixed things for me. That is also why I made this the
> >> last patch in the set. I asked tagorereddy to test his system
> >> without this patch, but he did not get around to that.
> >>
> >> After all we do tell the punit to not touch the bus by acquiring
> >> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
> >> for RPS freq changes it honors that and properly waits. Maybe it
> >> honors that for all punit requests i915 does and the only real
> >> problem is the forcewake stuff ?
> >>
> >> I can try to drop this patch from my queue and run without it
> >> for a while and see if things don't regress. And also ask
> >> tagorereddy again to test his system that way.
> >>
> >> Does that (dropping this patch for now) sound like a good idea?
> >
> > More test results couldn't hurt at least. It also makes me wonder if
> > just bumping the timeouts to some ridiculously high value would fix
> > the problem as well.
> 
> I've already tried bumping the forcewake timeout from 50 to 250ms,
> before writing the patch to just get forcewake_all before the pmic
> bus access begins, that does not fix things,

And you bumped the i2c mutex timeout as well? Or does that fail somehow
gracefully if it can't get the mutex?

> and since we busy wait
> for this timeout from non-sleeping context 250ms already is way too
> high.

Sure, but I'm just trying to understand if the problem is simply caused
by proceeding with some hardware access without getting the i2c mutex.

> 
> >>>>>> + a comment would be nice why it's there.
> >>>>>
> >>>>> I will add comments to the acquire calls.
> >>>>>
> >>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >>>>>> ifdefs?
> >>>>>
> >>>>> No, the iosf_mbi header defines empty inline versions of
> >>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> >>>>> this does mean that iosf_mbi must be builtin if the i915
> >>>>> driver is. I'll add:
> >>>>>
> >>>>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >>>>>
> >>>>> To the i915 Kconfig to enforce this.
> >>>>
> >>>> Hmm, ok so that does not work (long cyclic dependency through the
> >>>> selection of ACPI_VIDEO).
> >>>>
> >>>> So I've now added this instead:
> >>>>
> >>>> 	# iosf_mbi needs to be builtin if we are builtin
> >>>> 	select IOSF_MBI if DRM_I915=y
> >>>
> >>> That's probably not going to help anyone since i915 is usually a module.
> >>
> >> Right, that is fine, then either the IOSF_MBI symbols are available,
> >> or IOSF_MBI is disabled and we get the inline nops from the header.
> >>
> >> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
> >> realistic IMHO, but will get triggered by the random-config testing
> >> several contributors do and lead to an unresolved symbol error there.
> >
> > Well, from the user POV anything with IOSF_MBI==n can be a problem.
> > So I'm not sure if we should allow that.
> 
> So you're suggesting we just add an unconditional "select IOSF_MBI"
> to the i915 Kconfig entry?

Yeah, that should at least cut down the number of people accidentally
misconfiguring their kernels and hitting this problem in the future.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Hans de Goede]

Hi,

On 30-01-17 16:38, Ville Syrjälä wrote:
> On Mon, Jan 30, 2017 at 04:27:58PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-01-17 16:11, Ville Syrjälä wrote:
>>> On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 30-01-17 14:10, Ville Syrjälä wrote:
>>>>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>>>>>> around P-Unit write accesses.
>>>>>>>>
>>>>>>>> Can't we just stuff the calls into the actual punit write function
>>>>>>>> rather than sprinkling them all over the place?
>>>>>>>
>>>>>>> punit access is acquired across sections like this:
>>>>>>>
>>>>>>>         iosf_mbi_punit_acquire();
>>>>>>>
>>>>>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>         val &= ~DSPFREQGUAR_MASK;
>>>>>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>>>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>>>>>                      50)) {
>>>>>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>>>         }
>>>>>>>         iosf_mbi_punit_release();
>>>>>>>
>>>>>>> Where we want to wait for the requested change to have taken
>>>>>>> effect before releasing the punit.
>>>>>
>>>>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
>>>>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
>>>>> in set_rps() because that would slow things down too much. So I have to
>>>>> wonder how much luck is needed to make this workaround really effective.
>>>>
>>>> So the history of this patch-set is that I wrote this patch before
>>>> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
>>>> active (patch 12/13). Since a lot of testing was done with this
>>>> patch included in the patch-set and since it seemed a good idea
>>>> regardless (given my experience with accessing the punit vs
>>>> pmic bus accesses) I decided to leave it in.
>>>>
>>>> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
>>>> actually fixed things for me. That is also why I made this the
>>>> last patch in the set. I asked tagorereddy to test his system
>>>> without this patch, but he did not get around to that.
>>>>
>>>> After all we do tell the punit to not touch the bus by acquiring
>>>> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
>>>> for RPS freq changes it honors that and properly waits. Maybe it
>>>> honors that for all punit requests i915 does and the only real
>>>> problem is the forcewake stuff ?
>>>>
>>>> I can try to drop this patch from my queue and run without it
>>>> for a while and see if things don't regress. And also ask
>>>> tagorereddy again to test his system that way.
>>>>
>>>> Does that (dropping this patch for now) sound like a good idea?
>>>
>>> More test results couldn't hurt at least. It also makes me wonder if
>>> just bumping the timeouts to some ridiculously high value would fix
>>> the problem as well.
>>
>> I've already tried bumping the forcewake timeout from 50 to 250ms,
>> before writing the patch to just get forcewake_all before the pmic
>> bus access begins, that does not fix things,
>
> And you bumped the i2c mutex timeout as well? Or does that fail somehow
> gracefully if it can't get the mutex?

It will fail the i2c transfer with -ETIMEOUT, which will make the driver
report an error instead of e.g. the battery level, but it should not
affect the forcewake calls and those still failed with the large
timeout. So yes basically the i2c mutex fails gracefully.

>
>> and since we busy wait
>> for this timeout from non-sleeping context 250ms already is way too
>> high.
>
> Sure, but I'm just trying to understand if the problem is simply caused
> by proceeding with some hardware access without getting the i2c mutex.

Understood.

>>>>>>>> + a comment would be nice why it's there.
>>>>>>>
>>>>>>> I will add comments to the acquire calls.
>>>>>>>
>>>>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>>>>>>>> ifdefs?
>>>>>>>
>>>>>>> No, the iosf_mbi header defines empty inline versions of
>>>>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
>>>>>>> this does mean that iosf_mbi must be builtin if the i915
>>>>>>> driver is. I'll add:
>>>>>>>
>>>>>>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>>>>>>>
>>>>>>> To the i915 Kconfig to enforce this.
>>>>>>
>>>>>> Hmm, ok so that does not work (long cyclic dependency through the
>>>>>> selection of ACPI_VIDEO).
>>>>>>
>>>>>> So I've now added this instead:
>>>>>>
>>>>>> 	# iosf_mbi needs to be builtin if we are builtin
>>>>>> 	select IOSF_MBI if DRM_I915=y
>>>>>
>>>>> That's probably not going to help anyone since i915 is usually a module.
>>>>
>>>> Right, that is fine, then either the IOSF_MBI symbols are available,
>>>> or IOSF_MBI is disabled and we get the inline nops from the header.
>>>>
>>>> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
>>>> realistic IMHO, but will get triggered by the random-config testing
>>>> several contributors do and lead to an unresolved symbol error there.
>>>
>>> Well, from the user POV anything with IOSF_MBI==n can be a problem.
>>> So I'm not sure if we should allow that.
>>
>> So you're suggesting we just add an unconditional "select IOSF_MBI"
>> to the i915 Kconfig entry?
>
> Yeah, that should at least cut down the number of people accidentally
> misconfiguring their kernels and hitting this problem in the future.

Ok.

Regards,

Hans

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Hans de Goede]

Hi,

On 30-01-17 16:11, Ville Syrjälä wrote:
> On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-01-17 14:10, Ville Syrjälä wrote:
>>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>>>> around P-Unit write accesses.
>>>>>>
>>>>>> Can't we just stuff the calls into the actual punit write function
>>>>>> rather than sprinkling them all over the place?
>>>>>
>>>>> punit access is acquired across sections like this:
>>>>>
>>>>>         iosf_mbi_punit_acquire();
>>>>>
>>>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>         val &= ~DSPFREQGUAR_MASK;
>>>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>>>                      50)) {
>>>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>         }
>>>>>         iosf_mbi_punit_release();
>>>>>
>>>>> Where we want to wait for the requested change to have taken
>>>>> effect before releasing the punit.
>>>
>>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
>>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
>>> in set_rps() because that would slow things down too much. So I have to
>>> wonder how much luck is needed to make this workaround really effective.
>>
>> So the history of this patch-set is that I wrote this patch before
>> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
>> active (patch 12/13). Since a lot of testing was done with this
>> patch included in the patch-set and since it seemed a good idea
>> regardless (given my experience with accessing the punit vs
>> pmic bus accesses) I decided to leave it in.
>>
>> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
>> actually fixed things for me. That is also why I made this the
>> last patch in the set. I asked tagorereddy to test his system
>> without this patch, but he did not get around to that.
>>
>> After all we do tell the punit to not touch the bus by acquiring
>> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
>> for RPS freq changes it honors that and properly waits. Maybe it
>> honors that for all punit requests i915 does and the only real
>> problem is the forcewake stuff ?
>>
>> I can try to drop this patch from my queue and run without it
>> for a while and see if things don't regress. And also ask
>> tagorereddy again to test his system that way.
>>
>> Does that (dropping this patch for now) sound like a good idea?
>
> More test results couldn't hurt at least.

Ok, I've done a whole bunch of suspend + resume cycles on my cht
tablet with this patch dropped and things still work fine
(where as without the first 12 patches of this patch-set that
  was a guarenteed way to get a forcewake timeout followed by
  a lockup).

So it indeed seems this test is not necessary. I'll send a v3
with that patch dropped, as well as your comments for
patches 11 and 12 being addressed.

Regards,

Hans

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx