Add GSC support for XeHP SDV and DG2 platforms.
The series includes changes for the mei driver:
- add ability to use polling instead of interrupts
- add ability to use extended timeouts
- setup extended operational memory for GSC
The series includes changes for the i915 driver:
- allocate extended operational memory for GSC
- GSC on XeHP SDV offsets and definitions
This patch set should be merged via gfx tree as
the auxiliary device belongs there.
Greg, your ACK is required for the drives/misc/mei code base,
please review the patches.
V2: rebase over merged DG1 series and DG2 enablement patch,
fix commit messages
V3: rebase over latest tip
V4: add missed changelog in pxp dbugfs patch
V5: rebase over latest tip
fix changelog in pxp dbugfs patch
put HAX patch last to the ease of merging
reorder patches in the series
V6: change prefix from 'drm/i915/gsc:' to 'mei' in patch:
mei: add slow_fw flag to the mei auxiliary device
Address following checkpatch warnings:
CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u32' over 'uint32_t'
FILE: drivers/misc/mei/mkhi.h:54:
+ uint32_t flags;
-:51: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'cldev->bus->pxp_mode != MEI_DEV_PXP_INIT'
#51: FILE: drivers/misc/mei/bus-fixup.c:257:
+ if (!cldev->bus->fw_f_fw_ver_supported &&
+ (cldev->bus->pxp_mode != MEI_DEV_PXP_INIT)
Remove some spurious code formatting changes in:
drm/i915/gsc: allocate extended operational memory in LMEM
V7: Add new patch to add kdoc for mei_aux_device structure.
Rename slow_fw to slow_firmware flag.
Use drm_dbg/err() functions instead of dev_dbg/err() in i195
codebase.
Alexander Usyskin (4):
drm/i915/gsc: add slow_firmware flag to the gsc device definition
drm/i915/gsc: add GSC XeHP SDV platform definition
mei: gsc: wait for reset thread on stop
mei: extend timeouts on slow devices.
Daniele Ceraolo Spurio (1):
HAX: drm/i915: force INTEL_MEI_GSC on for CI
Tomas Winkler (7):
mei: add kdoc for struct mei_aux_device
mei: add slow_firmware flag to the mei auxiliary device
mei: gsc: use polling instead of interrupts
mei: mkhi: add memory ready command
mei: gsc: setup gsc extended operational memory
mei: debugfs: add pxp mode to devstate in debugfs
drm/i915/gsc: allocate extended operational memory in LMEM
Vitaly Lubart (3):
drm/i915/gsc: skip irq initialization if using polling
mei: bus: export common mkhi definitions into a separate header
mei: gsc: add transition to PXP mode in resume flow
drivers/gpu/drm/i915/Kconfig.debug | 1 +
drivers/gpu/drm/i915/gt/intel_gsc.c | 118 +++++++++++++++++++++++++---
drivers/gpu/drm/i915/gt/intel_gsc.h | 3 +
drivers/misc/mei/bus-fixup.c | 104 ++++++++++++++++--------
drivers/misc/mei/client.c | 14 ++--
drivers/misc/mei/debugfs.c | 17 ++++
drivers/misc/mei/gsc-me.c | 77 +++++++++++++++---
drivers/misc/mei/hbm.c | 12 +--
drivers/misc/mei/hw-me-regs.h | 7 ++
drivers/misc/mei/hw-me.c | 116 ++++++++++++++++++++++-----
drivers/misc/mei/hw-me.h | 14 +++-
drivers/misc/mei/hw-txe.c | 2 +-
drivers/misc/mei/hw.h | 5 ++
drivers/misc/mei/init.c | 21 ++++-
drivers/misc/mei/main.c | 2 +-
drivers/misc/mei/mei_dev.h | 26 ++++++
drivers/misc/mei/mkhi.h | 57 ++++++++++++++
drivers/misc/mei/pci-me.c | 2 +-
include/linux/mei_aux.h | 12 +++
19 files changed, 519 insertions(+), 91 deletions(-)
create mode 100644 drivers/misc/mei/mkhi.h
--
2.37.1
From: Alexander Usyskin <[email protected]>
Wait for reset work to complete before initiating
stop reset flow sequence.
Signed-off-by: Alexander Usyskin <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
drivers/misc/mei/init.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/misc/mei/init.c b/drivers/misc/mei/init.c
index eb052005ca86..5bb6ba662cc0 100644
--- a/drivers/misc/mei/init.c
+++ b/drivers/misc/mei/init.c
@@ -320,6 +320,8 @@ void mei_stop(struct mei_device *dev)
mei_clear_interrupts(dev);
mei_synchronize_irq(dev);
+ /* to catch HW-initiated reset */
+ mei_cancel_work(dev);
mutex_lock(&dev->device_lock);
--
2.37.1
From: Daniele Ceraolo Spurio <[email protected]>
After the new config option is merged we'll enable it by default in the
CI config, but for now just force it on via the i915 Kconfig so we can
get pre-merge CI results for it.
Signed-off-by: Daniele Ceraolo Spurio <[email protected]>
Signed-off-by: Alexander Usyskin <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
drivers/gpu/drm/i915/Kconfig.debug | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index e7fd3e76f8a2..be4ef485d6c1 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -48,6 +48,7 @@ config DRM_I915_DEBUG
select DRM_I915_DEBUG_RUNTIME_PM
select DRM_I915_SW_FENCE_DEBUG_OBJECTS
select DRM_I915_SELFTEST
+ select INTEL_MEI_GSC
select BROKEN # for prototype uAPI
default n
help
--
2.37.1
A work-around for a HW issue in XEHPSDV that manifests itself when SW reads
a gsc register when gsc is sending an interrupt. The work-around is
to disable interrupts and to use polling instead.
Cc: James Ausmus <[email protected]>
Signed-off-by: Vitaly Lubart <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Alexander Usyskin <[email protected]>
---
drivers/misc/mei/gsc-me.c | 48 ++++++++++++++++++++++++++------
drivers/misc/mei/hw-me.c | 58 ++++++++++++++++++++++++++++++++++++---
drivers/misc/mei/hw-me.h | 12 ++++++++
3 files changed, 105 insertions(+), 13 deletions(-)
diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
index c8145e9b62b6..2caba3a9ac35 100644
--- a/drivers/misc/mei/gsc-me.c
+++ b/drivers/misc/mei/gsc-me.c
@@ -13,6 +13,7 @@
#include <linux/ktime.h>
#include <linux/delay.h>
#include <linux/pm_runtime.h>
+#include <linux/kthread.h>
#include "mei_dev.h"
#include "hw-me.h"
@@ -66,13 +67,28 @@ static int mei_gsc_probe(struct auxiliary_device *aux_dev,
dev_set_drvdata(device, dev);
- ret = devm_request_threaded_irq(device, hw->irq,
- mei_me_irq_quick_handler,
- mei_me_irq_thread_handler,
- IRQF_ONESHOT, KBUILD_MODNAME, dev);
- if (ret) {
- dev_err(device, "irq register failed %d\n", ret);
- goto err;
+ /* use polling */
+ if (mei_me_hw_use_polling(hw)) {
+ mei_disable_interrupts(dev);
+ mei_clear_interrupts(dev);
+ init_waitqueue_head(&hw->wait_active);
+ hw->is_active = true; /* start in active mode for initialization */
+ hw->polling_thread = kthread_run(mei_me_polling_thread, dev,
+ "kmegscirqd/%s", dev_name(device));
+ if (IS_ERR(hw->polling_thread)) {
+ ret = PTR_ERR(hw->polling_thread);
+ dev_err(device, "unable to create kernel thread: %d\n", ret);
+ goto err;
+ }
+ } else {
+ ret = devm_request_threaded_irq(device, hw->irq,
+ mei_me_irq_quick_handler,
+ mei_me_irq_thread_handler,
+ IRQF_ONESHOT, KBUILD_MODNAME, dev);
+ if (ret) {
+ dev_err(device, "irq register failed %d\n", ret);
+ goto err;
+ }
}
pm_runtime_get_noresume(device);
@@ -98,7 +114,8 @@ static int mei_gsc_probe(struct auxiliary_device *aux_dev,
register_err:
mei_stop(dev);
- devm_free_irq(device, hw->irq, dev);
+ if (!mei_me_hw_use_polling(hw))
+ devm_free_irq(device, hw->irq, dev);
err:
dev_err(device, "probe failed: %d\n", ret);
@@ -119,12 +136,17 @@ static void mei_gsc_remove(struct auxiliary_device *aux_dev)
mei_stop(dev);
+ hw = to_me_hw(dev);
+ if (mei_me_hw_use_polling(hw))
+ kthread_stop(hw->polling_thread);
+
mei_deregister(dev);
pm_runtime_disable(&aux_dev->dev);
mei_disable_interrupts(dev);
- devm_free_irq(&aux_dev->dev, hw->irq, dev);
+ if (!mei_me_hw_use_polling(hw))
+ devm_free_irq(&aux_dev->dev, hw->irq, dev);
}
static int __maybe_unused mei_gsc_pm_suspend(struct device *device)
@@ -185,6 +207,9 @@ static int __maybe_unused mei_gsc_pm_runtime_suspend(struct device *device)
if (mei_write_is_idle(dev)) {
hw = to_me_hw(dev);
hw->pg_state = MEI_PG_ON;
+
+ if (mei_me_hw_use_polling(hw))
+ hw->is_active = false;
ret = 0;
} else {
ret = -EAGAIN;
@@ -209,6 +234,11 @@ static int __maybe_unused mei_gsc_pm_runtime_resume(struct device *device)
hw = to_me_hw(dev);
hw->pg_state = MEI_PG_OFF;
+ if (mei_me_hw_use_polling(hw)) {
+ hw->is_active = true;
+ wake_up(&hw->wait_active);
+ }
+
mutex_unlock(&dev->device_lock);
irq_ret = mei_me_irq_thread_handler(1, dev);
diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
index befa491e3344..46559517a902 100644
--- a/drivers/misc/mei/hw-me.c
+++ b/drivers/misc/mei/hw-me.c
@@ -10,6 +10,7 @@
#include <linux/interrupt.h>
#include <linux/pm_runtime.h>
#include <linux/sizes.h>
+#include <linux/delay.h>
#include "mei_dev.h"
#include "hbm.h"
@@ -327,9 +328,12 @@ static void mei_me_intr_clear(struct mei_device *dev)
*/
static void mei_me_intr_enable(struct mei_device *dev)
{
- u32 hcsr = mei_hcsr_read(dev);
+ u32 hcsr;
+
+ if (mei_me_hw_use_polling(to_me_hw(dev)))
+ return;
- hcsr |= H_CSR_IE_MASK;
+ hcsr = mei_hcsr_read(dev) | H_CSR_IE_MASK;
mei_hcsr_set(dev, hcsr);
}
@@ -354,6 +358,9 @@ static void mei_me_synchronize_irq(struct mei_device *dev)
{
struct mei_me_hw *hw = to_me_hw(dev);
+ if (mei_me_hw_use_polling(hw))
+ return;
+
synchronize_irq(hw->irq);
}
@@ -380,7 +387,10 @@ static void mei_me_host_set_ready(struct mei_device *dev)
{
u32 hcsr = mei_hcsr_read(dev);
- hcsr |= H_CSR_IE_MASK | H_IG | H_RDY;
+ if (!mei_me_hw_use_polling(to_me_hw(dev)))
+ hcsr |= H_CSR_IE_MASK;
+
+ hcsr |= H_IG | H_RDY;
mei_hcsr_set(dev, hcsr);
}
@@ -1176,7 +1186,7 @@ static int mei_me_hw_reset(struct mei_device *dev, bool intr_enable)
hcsr |= H_RST | H_IG | H_CSR_IS_MASK;
- if (!intr_enable)
+ if (!intr_enable || mei_me_hw_use_polling(to_me_hw(dev)))
hcsr &= ~H_CSR_IE_MASK;
dev->recvd_hw_ready = false;
@@ -1331,6 +1341,46 @@ irqreturn_t mei_me_irq_thread_handler(int irq, void *dev_id)
}
EXPORT_SYMBOL_GPL(mei_me_irq_thread_handler);
+#define MEI_POLLING_TIMEOUT_ACTIVE 100
+#define MEI_POLLING_TIMEOUT_IDLE 500
+
+int mei_me_polling_thread(void *_dev)
+{
+ struct mei_device *dev = _dev;
+ irqreturn_t irq_ret;
+ long polling_timeout = MEI_POLLING_TIMEOUT_ACTIVE;
+
+ dev_dbg(dev->dev, "kernel thread is running\n");
+ while (!kthread_should_stop()) {
+ struct mei_me_hw *hw = to_me_hw(dev);
+ u32 hcsr;
+
+ wait_event_timeout(hw->wait_active,
+ hw->is_active || kthread_should_stop(),
+ msecs_to_jiffies(MEI_POLLING_TIMEOUT_IDLE));
+
+ if (kthread_should_stop())
+ break;
+
+ hcsr = mei_hcsr_read(dev);
+ if (me_intr_src(hcsr)) {
+ polling_timeout = MEI_POLLING_TIMEOUT_ACTIVE;
+ irq_ret = mei_me_irq_thread_handler(1, dev);
+ if (irq_ret != IRQ_HANDLED)
+ dev_err(dev->dev, "irq_ret %d\n", irq_ret);
+ } else {
+ polling_timeout = clamp_val(polling_timeout + MEI_POLLING_TIMEOUT_ACTIVE,
+ MEI_POLLING_TIMEOUT_ACTIVE,
+ MEI_POLLING_TIMEOUT_IDLE);
+ }
+
+ schedule_timeout_interruptible(msecs_to_jiffies(polling_timeout));
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mei_me_polling_thread);
+
static const struct mei_hw_ops mei_me_hw_ops = {
.trc_status = mei_me_trc_status,
diff --git a/drivers/misc/mei/hw-me.h b/drivers/misc/mei/hw-me.h
index a071c645e905..ca09274ac299 100644
--- a/drivers/misc/mei/hw-me.h
+++ b/drivers/misc/mei/hw-me.h
@@ -51,6 +51,8 @@ struct mei_cfg {
* @d0i3_supported: di03 support
* @hbuf_depth: depth of hardware host/write buffer in slots
* @read_fws: read FW status register handler
+ * @wait_active: the polling thread activity wait queue
+ * @is_active: the device is active
*/
struct mei_me_hw {
const struct mei_cfg *cfg;
@@ -60,10 +62,19 @@ struct mei_me_hw {
bool d0i3_supported;
u8 hbuf_depth;
int (*read_fws)(const struct mei_device *dev, int where, u32 *val);
+ /* polling */
+ struct task_struct *polling_thread;
+ wait_queue_head_t wait_active;
+ bool is_active;
};
#define to_me_hw(dev) (struct mei_me_hw *)((dev)->hw)
+static inline bool mei_me_hw_use_polling(const struct mei_me_hw *hw)
+{
+ return hw->irq < 0;
+}
+
/**
* enum mei_cfg_idx - indices to platform specific configurations.
*
@@ -127,5 +138,6 @@ int mei_me_pg_exit_sync(struct mei_device *dev);
irqreturn_t mei_me_irq_quick_handler(int irq, void *dev_id);
irqreturn_t mei_me_irq_thread_handler(int irq, void *dev_id);
+int mei_me_polling_thread(void *_dev);
#endif /* _MEI_INTERFACE_H_ */
--
2.37.1
Add GSC memory ready command.
The command indicates to the firmware that extend operation
memory was setup and the firmware may enter PXP mode.
CC: Daniele Ceraolo Spurio <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Alexander Usyskin <[email protected]>
---
drivers/misc/mei/mkhi.h | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/mei/mkhi.h b/drivers/misc/mei/mkhi.h
index 27a9b476904e..056b76e73d40 100644
--- a/drivers/misc/mei/mkhi.h
+++ b/drivers/misc/mei/mkhi.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
- * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
+ * Copyright (c) 2003-2021, Intel Corporation. All rights reserved.
* Intel Management Engine Interface (Intel MEI) Linux driver
*/
@@ -18,6 +18,13 @@
#define MCHI_GROUP_ID 0xA
+#define MKHI_GROUP_ID_GFX 0x30
+#define MKHI_GFX_RESET_WARN_CMD_REQ 0x0
+#define MKHI_GFX_MEMORY_READY_CMD_REQ 0x1
+
+/* Allow transition to PXP mode without approval */
+#define MKHI_GFX_MEM_READY_PXP_ALLOWED 0x1
+
struct mkhi_rule_id {
__le16 rule_type;
u8 feature_id;
@@ -42,4 +49,9 @@ struct mkhi_msg {
u8 data[];
} __packed;
+struct mkhi_gfx_mem_ready {
+ struct mkhi_msg_hdr hdr;
+ u32 flags;
+} __packed;
+
#endif /* _MEI_MKHI_H_ */
--
2.37.1
From: Alexander Usyskin <[email protected]>
Define GSC on XeHP SDV (Intel(R) dGPU without display)
XeHP SDV uses the same hardware settings as DG1, but uses polling
instead of interrupts and runs the firmware in slow pace due to
hardware limitations.
Signed-off-by: Vitaly Lubart <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Alexander Usyskin <[email protected]>
---
drivers/gpu/drm/i915/gt/intel_gsc.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
index 73498c2574c8..e1040c8f2fd3 100644
--- a/drivers/gpu/drm/i915/gt/intel_gsc.c
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -56,6 +56,19 @@ static const struct gsc_def gsc_def_dg1[] = {
}
};
+static const struct gsc_def gsc_def_xehpsdv[] = {
+ {
+ /* HECI1 not enabled on the device. */
+ },
+ {
+ .name = "mei-gscfi",
+ .bar = DG1_GSC_HECI2_BASE,
+ .bar_size = GSC_BAR_LENGTH,
+ .use_polling = true,
+ .slow_firmware = true,
+ }
+};
+
static const struct gsc_def gsc_def_dg2[] = {
{
.name = "mei-gsc",
@@ -107,6 +120,8 @@ static void gsc_init_one(struct drm_i915_private *i915,
if (IS_DG1(i915)) {
def = &gsc_def_dg1[intf_id];
+ } else if (IS_XEHPSDV(i915)) {
+ def = &gsc_def_xehpsdv[intf_id];
} else if (IS_DG2(i915)) {
def = &gsc_def_dg2[intf_id];
} else {
--
2.37.1
Add pxp mode devstate to debugfs to monitor pxp state machine progress.
This is useful to debug issues in scenarios in which the pxp state
needs to be re-initialized, like during power transitions such as
suspend/resume. With this debugfs the state could be monitored
to ensure that pxp is in the ready state.
CC: Vitaly Lubart <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Alexander Usyskin <[email protected]>
---
drivers/misc/mei/debugfs.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c
index 1ce61e9e24fc..4074fec866a6 100644
--- a/drivers/misc/mei/debugfs.c
+++ b/drivers/misc/mei/debugfs.c
@@ -86,6 +86,20 @@ static int mei_dbgfs_active_show(struct seq_file *m, void *unused)
}
DEFINE_SHOW_ATTRIBUTE(mei_dbgfs_active);
+static const char *mei_dev_pxp_mode_str(enum mei_dev_pxp_mode state)
+{
+#define MEI_PXP_MODE(state) case MEI_DEV_PXP_##state: return #state
+ switch (state) {
+ MEI_PXP_MODE(DEFAULT);
+ MEI_PXP_MODE(INIT);
+ MEI_PXP_MODE(SETUP);
+ MEI_PXP_MODE(READY);
+ default:
+ return "unknown";
+ }
+#undef MEI_PXP_MODE
+}
+
static int mei_dbgfs_devstate_show(struct seq_file *m, void *unused)
{
struct mei_device *dev = m->private;
@@ -112,6 +126,9 @@ static int mei_dbgfs_devstate_show(struct seq_file *m, void *unused)
seq_printf(m, "pg: %s, %s\n",
mei_pg_is_enabled(dev) ? "ENABLED" : "DISABLED",
mei_pg_state_str(mei_pg_state(dev)));
+
+ seq_printf(m, "pxp: %s\n", mei_dev_pxp_mode_str(dev->pxp_mode));
+
return 0;
}
DEFINE_SHOW_ATTRIBUTE(mei_dbgfs_devstate);
--
2.37.1
struct mei_aux_device is an interface structure
requires proper documenation.
Signed-off-by: Tomas Winkler <[email protected]>
---
include/linux/mei_aux.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/linux/mei_aux.h b/include/linux/mei_aux.h
index 587f25128848..a0cb587006d5 100644
--- a/include/linux/mei_aux.h
+++ b/include/linux/mei_aux.h
@@ -7,6 +7,12 @@
#include <linux/auxiliary_bus.h>
+/**
+ * struct mei_aux_device - mei auxiliary device
+ * @aux_dev: - auxiliary device object
+ * @irq: interrupt driving the mei auxiliary device
+ * @bar: mmio resource bar reserved to mei auxiliary device
+ */
struct mei_aux_device {
struct auxiliary_device aux_dev;
int irq;
--
2.37.1
From: Alexander Usyskin <[email protected]>
Add slow_firmware flag to the gsc device definition
and pass it to mei auxiliary device, this instructs
the driver to use longer operation timeouts.
Signed-off-by: Alexander Usyskin <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Reviewed-by: Daniele Ceraolo Spurio <[email protected]>
---
drivers/gpu/drm/i915/gt/intel_gsc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
index e0236ff1d072..73498c2574c8 100644
--- a/drivers/gpu/drm/i915/gt/intel_gsc.c
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -41,6 +41,7 @@ struct gsc_def {
unsigned long bar;
size_t bar_size;
bool use_polling;
+ bool slow_firmware;
};
/* gsc resources and definitions (HECI1 and HECI2) */
@@ -145,6 +146,7 @@ static void gsc_init_one(struct drm_i915_private *i915,
adev->bar.end = adev->bar.start + def->bar_size - 1;
adev->bar.flags = IORESOURCE_MEM;
adev->bar.desc = IORES_DESC_NONE;
+ adev->slow_firmware = def->slow_firmware;
aux_dev = &adev->aux_dev;
aux_dev->name = def->name;
--
2.37.1
From: Vitaly Lubart <[email protected]>
Added transition to PXP mode in resume flow.
CC: Daniele Ceraolo Spurio <[email protected]>
Signed-off-by: Vitaly Lubart <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Alexander Usyskin <[email protected]>
---
drivers/misc/mei/gsc-me.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
index 6b22726aed55..75765e4df4ed 100644
--- a/drivers/misc/mei/gsc-me.c
+++ b/drivers/misc/mei/gsc-me.c
@@ -182,11 +182,22 @@ static int __maybe_unused mei_gsc_pm_suspend(struct device *device)
static int __maybe_unused mei_gsc_pm_resume(struct device *device)
{
struct mei_device *dev = dev_get_drvdata(device);
+ struct auxiliary_device *aux_dev;
+ struct mei_aux_device *adev;
int err;
+ struct mei_me_hw *hw;
if (!dev)
return -ENODEV;
+ hw = to_me_hw(dev);
+ aux_dev = to_auxiliary_dev(device);
+ adev = auxiliary_dev_to_mei_aux_dev(aux_dev);
+ if (adev->ext_op_mem.start) {
+ mei_gsc_set_ext_op_mem(hw, &adev->ext_op_mem);
+ dev->pxp_mode = MEI_DEV_PXP_INIT;
+ }
+
err = mei_restart(dev);
if (err)
return err;
--
2.37.1
On 8/6/2022 5:26 AM, Tomas Winkler wrote:
> From: Alexander Usyskin <[email protected]>
>
> Define GSC on XeHP SDV (Intel(R) dGPU without display)
>
> XeHP SDV uses the same hardware settings as DG1, but uses polling
> instead of interrupts and runs the firmware in slow pace due to
> hardware limitations.
>
> Signed-off-by: Vitaly Lubart <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> Signed-off-by: Alexander Usyskin <[email protected]>
Reviewed-by: Daniele Ceraolo Spurio <[email protected]>
Daniele
> ---
> drivers/gpu/drm/i915/gt/intel_gsc.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
> index 73498c2574c8..e1040c8f2fd3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> @@ -56,6 +56,19 @@ static const struct gsc_def gsc_def_dg1[] = {
> }
> };
>
> +static const struct gsc_def gsc_def_xehpsdv[] = {
> + {
> + /* HECI1 not enabled on the device. */
> + },
> + {
> + .name = "mei-gscfi",
> + .bar = DG1_GSC_HECI2_BASE,
> + .bar_size = GSC_BAR_LENGTH,
> + .use_polling = true,
> + .slow_firmware = true,
> + }
> +};
> +
> static const struct gsc_def gsc_def_dg2[] = {
> {
> .name = "mei-gsc",
> @@ -107,6 +120,8 @@ static void gsc_init_one(struct drm_i915_private *i915,
>
> if (IS_DG1(i915)) {
> def = &gsc_def_dg1[intf_id];
> + } else if (IS_XEHPSDV(i915)) {
> + def = &gsc_def_xehpsdv[intf_id];
> } else if (IS_DG2(i915)) {
> def = &gsc_def_dg2[intf_id];
> } else {
On Sat, Aug 06, 2022 at 03:26:31PM +0300, Tomas Winkler wrote:
> Add GSC memory ready command.
> The command indicates to the firmware that extend operation
> memory was setup and the firmware may enter PXP mode.
>
> CC: Daniele Ceraolo Spurio <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> Signed-off-by: Alexander Usyskin <[email protected]>
> ---
> drivers/misc/mei/mkhi.h | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/mei/mkhi.h b/drivers/misc/mei/mkhi.h
> index 27a9b476904e..056b76e73d40 100644
> --- a/drivers/misc/mei/mkhi.h
> +++ b/drivers/misc/mei/mkhi.h
> @@ -1,6 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
> - * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
> + * Copyright (c) 2003-2021, Intel Corporation. All rights reserved.
It is 2022 :(
On Sat, Aug 06, 2022 at 03:26:21PM +0300, Tomas Winkler wrote:
> Add GSC support for XeHP SDV and DG2 platforms.
>
> The series includes changes for the mei driver:
> - add ability to use polling instead of interrupts
> - add ability to use extended timeouts
> - setup extended operational memory for GSC
>
> The series includes changes for the i915 driver:
> - allocate extended operational memory for GSC
> - GSC on XeHP SDV offsets and definitions
>
> This patch set should be merged via gfx tree as
> the auxiliary device belongs there.
> Greg, your ACK is required for the drives/misc/mei code base,
> please review the patches.
With the exception that you all don't know what year it is:
Acked-by: Greg Kroah-Hartman <[email protected]>
On 8/6/2022 5:26 AM, Tomas Winkler wrote:
> struct mei_aux_device is an interface structure
> requires proper documenation.
>
> Signed-off-by: Tomas Winkler <[email protected]>
Reviewed-by: Daniele Ceraolo Spurio <[email protected]>
Daniele
> ---
> include/linux/mei_aux.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/mei_aux.h b/include/linux/mei_aux.h
> index 587f25128848..a0cb587006d5 100644
> --- a/include/linux/mei_aux.h
> +++ b/include/linux/mei_aux.h
> @@ -7,6 +7,12 @@
>
> #include <linux/auxiliary_bus.h>
>
> +/**
> + * struct mei_aux_device - mei auxiliary device
> + * @aux_dev: - auxiliary device object
> + * @irq: interrupt driving the mei auxiliary device
> + * @bar: mmio resource bar reserved to mei auxiliary device
> + */
> struct mei_aux_device {
> struct auxiliary_device aux_dev;
> int irq;
On 8/6/2022 5:26 AM, Tomas Winkler wrote:
> From: Alexander Usyskin <[email protected]>
>
> Wait for reset work to complete before initiating
> stop reset flow sequence.
>
> Signed-off-by: Alexander Usyskin <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
Reviewed-by: Daniele Ceraolo Spurio <[email protected]>
Daniele
> ---
> drivers/misc/mei/init.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/misc/mei/init.c b/drivers/misc/mei/init.c
> index eb052005ca86..5bb6ba662cc0 100644
> --- a/drivers/misc/mei/init.c
> +++ b/drivers/misc/mei/init.c
> @@ -320,6 +320,8 @@ void mei_stop(struct mei_device *dev)
>
> mei_clear_interrupts(dev);
> mei_synchronize_irq(dev);
> + /* to catch HW-initiated reset */
> + mei_cancel_work(dev);
>
> mutex_lock(&dev->device_lock);
>
On 8/6/2022 5:26 AM, Tomas Winkler wrote:
> A work-around for a HW issue in XEHPSDV that manifests itself when SW reads
> a gsc register when gsc is sending an interrupt. The work-around is
> to disable interrupts and to use polling instead.
>
> Cc: James Ausmus <[email protected]>
> Signed-off-by: Vitaly Lubart <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> Signed-off-by: Alexander Usyskin <[email protected]>
> ---
> drivers/misc/mei/gsc-me.c | 48 ++++++++++++++++++++++++++------
> drivers/misc/mei/hw-me.c | 58 ++++++++++++++++++++++++++++++++++++---
> drivers/misc/mei/hw-me.h | 12 ++++++++
> 3 files changed, 105 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
> index c8145e9b62b6..2caba3a9ac35 100644
> --- a/drivers/misc/mei/gsc-me.c
> +++ b/drivers/misc/mei/gsc-me.c
> @@ -13,6 +13,7 @@
> #include <linux/ktime.h>
> #include <linux/delay.h>
> #include <linux/pm_runtime.h>
> +#include <linux/kthread.h>
>
> #include "mei_dev.h"
> #include "hw-me.h"
> @@ -66,13 +67,28 @@ static int mei_gsc_probe(struct auxiliary_device *aux_dev,
>
> dev_set_drvdata(device, dev);
>
> - ret = devm_request_threaded_irq(device, hw->irq,
> - mei_me_irq_quick_handler,
> - mei_me_irq_thread_handler,
> - IRQF_ONESHOT, KBUILD_MODNAME, dev);
> - if (ret) {
> - dev_err(device, "irq register failed %d\n", ret);
> - goto err;
> + /* use polling */
> + if (mei_me_hw_use_polling(hw)) {
> + mei_disable_interrupts(dev);
> + mei_clear_interrupts(dev);
> + init_waitqueue_head(&hw->wait_active);
> + hw->is_active = true; /* start in active mode for initialization */
> + hw->polling_thread = kthread_run(mei_me_polling_thread, dev,
> + "kmegscirqd/%s", dev_name(device));
> + if (IS_ERR(hw->polling_thread)) {
> + ret = PTR_ERR(hw->polling_thread);
> + dev_err(device, "unable to create kernel thread: %d\n", ret);
> + goto err;
> + }
> + } else {
> + ret = devm_request_threaded_irq(device, hw->irq,
> + mei_me_irq_quick_handler,
> + mei_me_irq_thread_handler,
> + IRQF_ONESHOT, KBUILD_MODNAME, dev);
> + if (ret) {
> + dev_err(device, "irq register failed %d\n", ret);
> + goto err;
> + }
> }
>
> pm_runtime_get_noresume(device);
> @@ -98,7 +114,8 @@ static int mei_gsc_probe(struct auxiliary_device *aux_dev,
>
> register_err:
> mei_stop(dev);
> - devm_free_irq(device, hw->irq, dev);
> + if (!mei_me_hw_use_polling(hw))
> + devm_free_irq(device, hw->irq, dev);
>
> err:
> dev_err(device, "probe failed: %d\n", ret);
> @@ -119,12 +136,17 @@ static void mei_gsc_remove(struct auxiliary_device *aux_dev)
>
> mei_stop(dev);
>
> + hw = to_me_hw(dev);
> + if (mei_me_hw_use_polling(hw))
> + kthread_stop(hw->polling_thread);
> +
> mei_deregister(dev);
>
> pm_runtime_disable(&aux_dev->dev);
>
> mei_disable_interrupts(dev);
> - devm_free_irq(&aux_dev->dev, hw->irq, dev);
> + if (!mei_me_hw_use_polling(hw))
> + devm_free_irq(&aux_dev->dev, hw->irq, dev);
> }
>
> static int __maybe_unused mei_gsc_pm_suspend(struct device *device)
> @@ -185,6 +207,9 @@ static int __maybe_unused mei_gsc_pm_runtime_suspend(struct device *device)
> if (mei_write_is_idle(dev)) {
> hw = to_me_hw(dev);
> hw->pg_state = MEI_PG_ON;
> +
> + if (mei_me_hw_use_polling(hw))
> + hw->is_active = false;
> ret = 0;
> } else {
> ret = -EAGAIN;
> @@ -209,6 +234,11 @@ static int __maybe_unused mei_gsc_pm_runtime_resume(struct device *device)
> hw = to_me_hw(dev);
> hw->pg_state = MEI_PG_OFF;
>
> + if (mei_me_hw_use_polling(hw)) {
> + hw->is_active = true;
> + wake_up(&hw->wait_active);
> + }
> +
> mutex_unlock(&dev->device_lock);
>
> irq_ret = mei_me_irq_thread_handler(1, dev);
> diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
> index befa491e3344..46559517a902 100644
> --- a/drivers/misc/mei/hw-me.c
> +++ b/drivers/misc/mei/hw-me.c
> @@ -10,6 +10,7 @@
> #include <linux/interrupt.h>
> #include <linux/pm_runtime.h>
> #include <linux/sizes.h>
> +#include <linux/delay.h>
>
> #include "mei_dev.h"
> #include "hbm.h"
> @@ -327,9 +328,12 @@ static void mei_me_intr_clear(struct mei_device *dev)
> */
> static void mei_me_intr_enable(struct mei_device *dev)
> {
> - u32 hcsr = mei_hcsr_read(dev);
> + u32 hcsr;
> +
> + if (mei_me_hw_use_polling(to_me_hw(dev)))
> + return;
>
> - hcsr |= H_CSR_IE_MASK;
> + hcsr = mei_hcsr_read(dev) | H_CSR_IE_MASK;
> mei_hcsr_set(dev, hcsr);
> }
>
> @@ -354,6 +358,9 @@ static void mei_me_synchronize_irq(struct mei_device *dev)
> {
> struct mei_me_hw *hw = to_me_hw(dev);
>
> + if (mei_me_hw_use_polling(hw))
> + return;
> +
> synchronize_irq(hw->irq);
> }
>
> @@ -380,7 +387,10 @@ static void mei_me_host_set_ready(struct mei_device *dev)
> {
> u32 hcsr = mei_hcsr_read(dev);
>
> - hcsr |= H_CSR_IE_MASK | H_IG | H_RDY;
> + if (!mei_me_hw_use_polling(to_me_hw(dev)))
> + hcsr |= H_CSR_IE_MASK;
> +
> + hcsr |= H_IG | H_RDY;
> mei_hcsr_set(dev, hcsr);
> }
>
> @@ -1176,7 +1186,7 @@ static int mei_me_hw_reset(struct mei_device *dev, bool intr_enable)
>
> hcsr |= H_RST | H_IG | H_CSR_IS_MASK;
>
> - if (!intr_enable)
> + if (!intr_enable || mei_me_hw_use_polling(to_me_hw(dev)))
> hcsr &= ~H_CSR_IE_MASK;
>
> dev->recvd_hw_ready = false;
> @@ -1331,6 +1341,46 @@ irqreturn_t mei_me_irq_thread_handler(int irq, void *dev_id)
> }
> EXPORT_SYMBOL_GPL(mei_me_irq_thread_handler);
>
> +#define MEI_POLLING_TIMEOUT_ACTIVE 100
> +#define MEI_POLLING_TIMEOUT_IDLE 500
> +
> +int mei_me_polling_thread(void *_dev)
> +{
> + struct mei_device *dev = _dev;
> + irqreturn_t irq_ret;
> + long polling_timeout = MEI_POLLING_TIMEOUT_ACTIVE;
> +
> + dev_dbg(dev->dev, "kernel thread is running\n");
> + while (!kthread_should_stop()) {
> + struct mei_me_hw *hw = to_me_hw(dev);
> + u32 hcsr;
> +
> + wait_event_timeout(hw->wait_active,
> + hw->is_active || kthread_should_stop(),
> + msecs_to_jiffies(MEI_POLLING_TIMEOUT_IDLE));
> +
> + if (kthread_should_stop())
> + break;
> +
> + hcsr = mei_hcsr_read(dev);
> + if (me_intr_src(hcsr)) {
> + polling_timeout = MEI_POLLING_TIMEOUT_ACTIVE;
> + irq_ret = mei_me_irq_thread_handler(1, dev);
> + if (irq_ret != IRQ_HANDLED)
> + dev_err(dev->dev, "irq_ret %d\n", irq_ret);
> + } else {
> + polling_timeout = clamp_val(polling_timeout + MEI_POLLING_TIMEOUT_ACTIVE,
> + MEI_POLLING_TIMEOUT_ACTIVE,
> + MEI_POLLING_TIMEOUT_IDLE);
> + }
> +
> + schedule_timeout_interruptible(msecs_to_jiffies(polling_timeout));
> + }
IMO this loop could have used a couple of comments to make it easier to
understand what's going on with the various waits and timeouts. Not a
blocker.
Reviewed-by: Daniele Ceraolo Spurio <[email protected]>
Daniele
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mei_me_polling_thread);
> +
> static const struct mei_hw_ops mei_me_hw_ops = {
>
> .trc_status = mei_me_trc_status,
> diff --git a/drivers/misc/mei/hw-me.h b/drivers/misc/mei/hw-me.h
> index a071c645e905..ca09274ac299 100644
> --- a/drivers/misc/mei/hw-me.h
> +++ b/drivers/misc/mei/hw-me.h
> @@ -51,6 +51,8 @@ struct mei_cfg {
> * @d0i3_supported: di03 support
> * @hbuf_depth: depth of hardware host/write buffer in slots
> * @read_fws: read FW status register handler
> + * @wait_active: the polling thread activity wait queue
> + * @is_active: the device is active
> */
> struct mei_me_hw {
> const struct mei_cfg *cfg;
> @@ -60,10 +62,19 @@ struct mei_me_hw {
> bool d0i3_supported;
> u8 hbuf_depth;
> int (*read_fws)(const struct mei_device *dev, int where, u32 *val);
> + /* polling */
> + struct task_struct *polling_thread;
> + wait_queue_head_t wait_active;
> + bool is_active;
> };
>
> #define to_me_hw(dev) (struct mei_me_hw *)((dev)->hw)
>
> +static inline bool mei_me_hw_use_polling(const struct mei_me_hw *hw)
> +{
> + return hw->irq < 0;
> +}
> +
> /**
> * enum mei_cfg_idx - indices to platform specific configurations.
> *
> @@ -127,5 +138,6 @@ int mei_me_pg_exit_sync(struct mei_device *dev);
>
> irqreturn_t mei_me_irq_quick_handler(int irq, void *dev_id);
> irqreturn_t mei_me_irq_thread_handler(int irq, void *dev_id);
> +int mei_me_polling_thread(void *_dev);
>
> #endif /* _MEI_INTERFACE_H_ */
On 8/6/2022 5:26 AM, Tomas Winkler wrote:
> Add GSC memory ready command.
> The command indicates to the firmware that extend operation
> memory was setup and the firmware may enter PXP mode.
>
> CC: Daniele Ceraolo Spurio <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> Signed-off-by: Alexander Usyskin <[email protected]>
Copyright date aside, this is:
Reviewed-by: Daniele Ceraolo Spurio <[email protected]>
Daniele
> ---
> drivers/misc/mei/mkhi.h | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/mei/mkhi.h b/drivers/misc/mei/mkhi.h
> index 27a9b476904e..056b76e73d40 100644
> --- a/drivers/misc/mei/mkhi.h
> +++ b/drivers/misc/mei/mkhi.h
> @@ -1,6 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
> - * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
> + * Copyright (c) 2003-2021, Intel Corporation. All rights reserved.
> * Intel Management Engine Interface (Intel MEI) Linux driver
> */
>
> @@ -18,6 +18,13 @@
>
> #define MCHI_GROUP_ID 0xA
>
> +#define MKHI_GROUP_ID_GFX 0x30
> +#define MKHI_GFX_RESET_WARN_CMD_REQ 0x0
> +#define MKHI_GFX_MEMORY_READY_CMD_REQ 0x1
> +
> +/* Allow transition to PXP mode without approval */
> +#define MKHI_GFX_MEM_READY_PXP_ALLOWED 0x1
> +
> struct mkhi_rule_id {
> __le16 rule_type;
> u8 feature_id;
> @@ -42,4 +49,9 @@ struct mkhi_msg {
> u8 data[];
> } __packed;
>
> +struct mkhi_gfx_mem_ready {
> + struct mkhi_msg_hdr hdr;
> + u32 flags;
> +} __packed;
> +
> #endif /* _MEI_MKHI_H_ */
On 8/6/2022 5:26 AM, Tomas Winkler wrote:
> From: Vitaly Lubart <[email protected]>
>
> Added transition to PXP mode in resume flow.
>
> CC: Daniele Ceraolo Spurio <[email protected]>
> Signed-off-by: Vitaly Lubart <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> Signed-off-by: Alexander Usyskin <[email protected]>
Reviewed-by: Daniele Ceraolo Spurio <[email protected]>
Daniele
> ---
> drivers/misc/mei/gsc-me.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
> index 6b22726aed55..75765e4df4ed 100644
> --- a/drivers/misc/mei/gsc-me.c
> +++ b/drivers/misc/mei/gsc-me.c
> @@ -182,11 +182,22 @@ static int __maybe_unused mei_gsc_pm_suspend(struct device *device)
> static int __maybe_unused mei_gsc_pm_resume(struct device *device)
> {
> struct mei_device *dev = dev_get_drvdata(device);
> + struct auxiliary_device *aux_dev;
> + struct mei_aux_device *adev;
> int err;
> + struct mei_me_hw *hw;
>
> if (!dev)
> return -ENODEV;
>
> + hw = to_me_hw(dev);
> + aux_dev = to_auxiliary_dev(device);
> + adev = auxiliary_dev_to_mei_aux_dev(aux_dev);
> + if (adev->ext_op_mem.start) {
> + mei_gsc_set_ext_op_mem(hw, &adev->ext_op_mem);
> + dev->pxp_mode = MEI_DEV_PXP_INIT;
> + }
> +
> err = mei_restart(dev);
> if (err)
> return err;
On 8/6/2022 5:26 AM, Tomas Winkler wrote:
> Add pxp mode devstate to debugfs to monitor pxp state machine progress.
> This is useful to debug issues in scenarios in which the pxp state
> needs to be re-initialized, like during power transitions such as
> suspend/resume. With this debugfs the state could be monitored
> to ensure that pxp is in the ready state.
>
> CC: Vitaly Lubart <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> Signed-off-by: Alexander Usyskin <[email protected]>
Reviewed-by: Daniele Ceraolo Spurio <[email protected]>
Daniele
> ---
> drivers/misc/mei/debugfs.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c
> index 1ce61e9e24fc..4074fec866a6 100644
> --- a/drivers/misc/mei/debugfs.c
> +++ b/drivers/misc/mei/debugfs.c
> @@ -86,6 +86,20 @@ static int mei_dbgfs_active_show(struct seq_file *m, void *unused)
> }
> DEFINE_SHOW_ATTRIBUTE(mei_dbgfs_active);
>
> +static const char *mei_dev_pxp_mode_str(enum mei_dev_pxp_mode state)
> +{
> +#define MEI_PXP_MODE(state) case MEI_DEV_PXP_##state: return #state
> + switch (state) {
> + MEI_PXP_MODE(DEFAULT);
> + MEI_PXP_MODE(INIT);
> + MEI_PXP_MODE(SETUP);
> + MEI_PXP_MODE(READY);
> + default:
> + return "unknown";
> + }
> +#undef MEI_PXP_MODE
> +}
> +
> static int mei_dbgfs_devstate_show(struct seq_file *m, void *unused)
> {
> struct mei_device *dev = m->private;
> @@ -112,6 +126,9 @@ static int mei_dbgfs_devstate_show(struct seq_file *m, void *unused)
> seq_printf(m, "pg: %s, %s\n",
> mei_pg_is_enabled(dev) ? "ENABLED" : "DISABLED",
> mei_pg_state_str(mei_pg_state(dev)));
> +
> + seq_printf(m, "pxp: %s\n", mei_dev_pxp_mode_str(dev->pxp_mode));
> +
> return 0;
> }
> DEFINE_SHOW_ATTRIBUTE(mei_dbgfs_devstate);
Dave, do you have a preference how to deal with the mishap here, shall I do a
force-push to drm-intel-gt-next to correctly record the Acked-by or revert and
re-push? Or just leave it as is?
Quoting Greg Kroah-Hartman (2022-09-01 18:09:09)
> On Sat, Aug 06, 2022 at 03:26:21PM +0300, Tomas Winkler wrote:
> > Add GSC support for XeHP SDV and DG2 platforms.
> >
> > The series includes changes for the mei driver:
> > - add ability to use polling instead of interrupts
> > - add ability to use extended timeouts
> > - setup extended operational memory for GSC
> >
> > The series includes changes for the i915 driver:
> > - allocate extended operational memory for GSC
> > - GSC on XeHP SDV offsets and definitions
> >
> > This patch set should be merged via gfx tree as
> > the auxiliary device belongs there.
> > Greg, your ACK is required for the drives/misc/mei code base,
> > please review the patches.
>
> With the exception that you all don't know what year it is:
>
> Acked-by: Greg Kroah-Hartman <[email protected]>
Daniele, why were the patches applied without this A-b?
I'm just preparing the drm-intel-gt-next pull request and now it appears
like we're pushing a lot of commits outside of drm without any Acks.
Please reach out to the maintainers *before* pushing code for other
subsystems. Unless you get an explicit ack to do so, do not push such
code.
Quoting from the committer guidelines[1] the first rule is:
"Only push patches changing drivers/gpu/drm/i915."
In those cases, please ping a maintainer and don't rush things.
Regards, Joonas
[1] https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-intel.html#high-level-guidelines
On 9/9/2022 3:24 AM, Joonas Lahtinen wrote:
> Dave, do you have a preference how to deal with the mishap here, shall I do a
> force-push to drm-intel-gt-next to correctly record the Acked-by or revert and
> re-push? Or just leave it as is?
>
> Quoting Greg Kroah-Hartman (2022-09-01 18:09:09)
>> On Sat, Aug 06, 2022 at 03:26:21PM +0300, Tomas Winkler wrote:
>>> Add GSC support for XeHP SDV and DG2 platforms.
>>>
>>> The series includes changes for the mei driver:
>>> - add ability to use polling instead of interrupts
>>> - add ability to use extended timeouts
>>> - setup extended operational memory for GSC
>>>
>>> The series includes changes for the i915 driver:
>>> - allocate extended operational memory for GSC
>>> - GSC on XeHP SDV offsets and definitions
>>>
>>> This patch set should be merged via gfx tree as
>>> the auxiliary device belongs there.
>>> Greg, your ACK is required for the drives/misc/mei code base,
>>> please review the patches.
>> With the exception that you all don't know what year it is:
>>
>> Acked-by: Greg Kroah-Hartman <[email protected]>
> Daniele, why were the patches applied without this A-b?
Apologies, I usually rely on dim to pick up all the correct r-bs and
acks from the ML and to warn me if something is missing, and I didn't
realize that it hadn't automagically picked up the ack.
>
> I'm just preparing the drm-intel-gt-next pull request and now it appears
> like we're pushing a lot of commits outside of drm without any Acks.
>
> Please reach out to the maintainers *before* pushing code for other
> subsystems. Unless you get an explicit ack to do so, do not push such
> code.
I'm assuming you mean the i915 maintainers here, given that there is an
ack from Greg in this email? Rodrigo was in the loop of us needing to
merge this via drm, so I thought I was good on that side. I'll make sure
to have an explicit ack on the ML next time (which is coming relatively
soon, because there are some more mei patches in the DG2 HuC series).
>
> Quoting from the committer guidelines[1] the first rule is:
> "Only push patches changing drivers/gpu/drm/i915."
>
> In those cases, please ping a maintainer and don't rush things.
Will do. And apologies again for the mistake.
Daniele
> Regards, Joonas
>
> [1] https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-intel.html#high-level-guidelines
>
On Fri, 2022-09-09 at 08:17 -0700, Ceraolo Spurio, Daniele wrote:
>
>
> On 9/9/2022 3:24 AM, Joonas Lahtinen wrote:
> > Dave, do you have a preference how to deal with the mishap here,
> > shall I do a
> > force-push to drm-intel-gt-next to correctly record the Acked-by or
> > revert and
> > re-push? Or just leave it as is?
Dave and Daniel, this question is still pertinent.
> >
> > Quoting Greg Kroah-Hartman (2022-09-01 18:09:09)
> > > On Sat, Aug 06, 2022 at 03:26:21PM +0300, Tomas Winkler wrote:
> > > > Add GSC support for XeHP SDV and DG2 platforms.
> > > >
> > > > The series includes changes for the mei driver:
> > > > - add ability to use polling instead of interrupts
> > > > - add ability to use extended timeouts
> > > > - setup extended operational memory for GSC
> > > >
> > > > The series includes changes for the i915 driver:
> > > > - allocate extended operational memory for GSC
> > > > - GSC on XeHP SDV offsets and definitions
> > > >
> > > > This patch set should be merged via gfx tree as
> > > > the auxiliary device belongs there.
> > > > Greg, your ACK is required for the drives/misc/mei code base,
> > > > please review the patches.
> > > With the exception that you all don't know what year it is:
> > >
> > > Acked-by: Greg Kroah-Hartman <[email protected]>
> > Daniele, why were the patches applied without this A-b?
>
> Apologies, I usually rely on dim to pick up all the correct r-bs and
> acks from the ML and to warn me if something is missing, and I didn't
> realize that it hadn't automagically picked up the ack.
I understand the feeling. Recently I merged a patch from Vinay relying
on patchwork to get the reviewed-by and I forgot to double check.
dim picks up the "Link:", but I don't believe it picks any ack or rv-b
from the mailing list. Patchwork does if you use pwclient or something
like that.
Anyway, lesson to both of us to always double-check, regardless the
tool used.
>
> >
> > I'm just preparing the drm-intel-gt-next pull request and now it
> > appears
> > like we're pushing a lot of commits outside of drm without any
> > Acks.
> >
> > Please reach out to the maintainers *before* pushing code for other
> > subsystems. Unless you get an explicit ack to do so, do not push
> > such
> > code.
>
> I'm assuming you mean the i915 maintainers here, given that there is
> an
> ack from Greg in this email? Rodrigo was in the loop of us needing to
> merge this via drm, so I thought I was good on that side. I'll make
> sure
> to have an explicit ack on the ML next time (which is coming
> relatively
> soon, because there are some more mei patches in the DG2 HuC series).
That's my fault indeed. I was following the movement, but I failed
to step up right after I saw Greg's ack.
Although I also noticed some re-send and reviews in progress even
after the ack, I should had been more active there.
Sorry,
Rodrigo.
>
> >
> > Quoting from the committer guidelines[1] the first rule is:
> > "Only push patches changing drivers/gpu/drm/i915."
> >
> > In those cases, please ping a maintainer and don't rush things.
>
> Will do. And apologies again for the mistake.
>
> Daniele
>
> > Regards, Joonas
> >
> > [1] https://drm.pages.freedesktop.org/maintainer-tools/committer-
> > drm-intel.html#high-level-guidelines
> >
>
On Fri, Sep 09, 2022 at 04:33:45PM +0000, Rodrigo Vivi wrote:
>On Fri, 2022-09-09 at 08:17 -0700, Ceraolo Spurio, Daniele wrote:
>>
>>
>> On 9/9/2022 3:24 AM, Joonas Lahtinen wrote:
>> > Dave, do you have a preference how to deal with the mishap here,
>> > shall I do a
>> > force-push to drm-intel-gt-next to correctly record the Acked-by or
>> > revert and
>> > re-push? Or just leave it as is?
>
>Dave and Daniel, this question is still pertinent.
>
>> >
>> > Quoting Greg Kroah-Hartman (2022-09-01 18:09:09)
>> > > On Sat, Aug 06, 2022 at 03:26:21PM +0300, Tomas Winkler wrote:
>> > > > Add GSC support for XeHP SDV and DG2 platforms.
>> > > >
>> > > > The series includes changes for the mei driver:
>> > > > - add ability to use polling instead of interrupts
>> > > > - add ability to use extended timeouts
>> > > > - setup extended operational memory for GSC
>> > > >
>> > > > The series includes changes for the i915 driver:
>> > > > - allocate extended operational memory for GSC
>> > > > - GSC on XeHP SDV offsets and definitions
>> > > >
>> > > > This patch set should be merged via gfx tree as
>> > > > the auxiliary device belongs there.
>> > > > Greg, your ACK is required for the drives/misc/mei code base,
>> > > > please review the patches.
>> > > With the exception that you all don't know what year it is:
>> > >
>> > > Acked-by: Greg Kroah-Hartman <[email protected]>
>> > Daniele, why were the patches applied without this A-b?
>>
>> Apologies, I usually rely on dim to pick up all the correct r-bs and
>> acks from the ML and to warn me if something is missing, and I didn't
>> realize that it hadn't automagically picked up the ack.
>
>I understand the feeling. Recently I merged a patch from Vinay relying
>on patchwork to get the reviewed-by and I forgot to double check.
>
>dim picks up the "Link:", but I don't believe it picks any ack or rv-b
>from the mailing list. Patchwork does if you use pwclient or something
>like that.
When you download the patch from patchwork, it will include the
r-b/a-b for the patches, not the cover letter.
$ curl https://patchwork.freedesktop.org/api/1.0/series/106638/revisions/5/mbox/ | \
grep Acked-by
$
Patchwork simply ignores the cover and does nothing.
b4 has an option to propagate the a-b on cover to the patches (-t):
$ b4 am -o - -t [email protected] | grep Acked
...
Acked-by: Greg Kroah-Hartman <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
...
b4 also has options to work on your local mailbox instead of relying on
the external server to apply a-b/r-b. Maybe people should be using it
more instead of relying on patchwork.
Should the Acked-by be recorded on each patch? That is what is usually
done, but if preference would be for a merge commit to be created and
Acked-by recorded in the merge commit, dim would need to learn a
few things. b4 is already prepared:
$ b4 shazam -M -P1-15 [email protected]
Lucas De Marchi
Quoting Vivi, Rodrigo (2022-09-09 19:33:45)
> On Fri, 2022-09-09 at 08:17 -0700, Ceraolo Spurio, Daniele wrote:
> >
> >
> > On 9/9/2022 3:24 AM, Joonas Lahtinen wrote:
> > > Dave, do you have a preference how to deal with the mishap here,
> > > shall I do a
> > > force-push to drm-intel-gt-next to correctly record the Acked-by or
> > > revert and
> > > re-push? Or just leave it as is?
>
> Dave and Daniel, this question is still pertinent.
Discussed with Dave and I did a force-push to add the missing
Acked-by's.
Daniele, I think the tradition is that you have volunteered
yourself to improve dim to nag about missing Acked-by's for
patches outside of i915 when pushing to drm-intel-gt-next.
Regards, Joonas
>
> > >
> > > Quoting Greg Kroah-Hartman (2022-09-01 18:09:09)
> > > > On Sat, Aug 06, 2022 at 03:26:21PM +0300, Tomas Winkler wrote:
> > > > > Add GSC support for XeHP SDV and DG2 platforms.
> > > > >
> > > > > The series includes changes for the mei driver:
> > > > > - add ability to use polling instead of interrupts
> > > > > - add ability to use extended timeouts
> > > > > - setup extended operational memory for GSC
> > > > >
> > > > > The series includes changes for the i915 driver:
> > > > > - allocate extended operational memory for GSC
> > > > > - GSC on XeHP SDV offsets and definitions
> > > > >
> > > > > This patch set should be merged via gfx tree as
> > > > > the auxiliary device belongs there.
> > > > > Greg, your ACK is required for the drives/misc/mei code base,
> > > > > please review the patches.
> > > > With the exception that you all don't know what year it is:
> > > >
> > > > Acked-by: Greg Kroah-Hartman <[email protected]>
> > > Daniele, why were the patches applied without this A-b?
> >
> > Apologies, I usually rely on dim to pick up all the correct r-bs and
> > acks from the ML and to warn me if something is missing, and I didn't
> > realize that it hadn't automagically picked up the ack.
>
> I understand the feeling. Recently I merged a patch from Vinay relying
> on patchwork to get the reviewed-by and I forgot to double check.
>
> dim picks up the "Link:", but I don't believe it picks any ack or rv-b
> from the mailing list. Patchwork does if you use pwclient or something
> like that.
>
> Anyway, lesson to both of us to always double-check, regardless the
> tool used.
>
> >
> > >
> > > I'm just preparing the drm-intel-gt-next pull request and now it
> > > appears
> > > like we're pushing a lot of commits outside of drm without any
> > > Acks.
> > >
> > > Please reach out to the maintainers *before* pushing code for other
> > > subsystems. Unless you get an explicit ack to do so, do not push
> > > such
> > > code.
> >
> > I'm assuming you mean the i915 maintainers here, given that there is
> > an
> > ack from Greg in this email? Rodrigo was in the loop of us needing to
> > merge this via drm, so I thought I was good on that side. I'll make
> > sure
> > to have an explicit ack on the ML next time (which is coming
> > relatively
> > soon, because there are some more mei patches in the DG2 HuC series).
>
> That's my fault indeed. I was following the movement, but I failed
> to step up right after I saw Greg's ack.
> Although I also noticed some re-send and reviews in progress even
> after the ack, I should had been more active there.
>
> Sorry,
> Rodrigo.
>
> >
> > >
> > > Quoting from the committer guidelines[1] the first rule is:
> > > "Only push patches changing drivers/gpu/drm/i915."
> > >
> > > In those cases, please ping a maintainer and don't rush things.
> >
> > Will do. And apologies again for the mistake.
> >
> > Daniele
> >
> > > Regards, Joonas
> > >
> > > [1] https://drm.pages.freedesktop.org/maintainer-tools/committer-
> > > drm-intel.html#high-level-guidelines
> > >
> >
>