From: Jason-jh Lin <[email protected]>
In order to support the upcoming ISP functions, some CMDQ APIs
need to be prepared:
- cmdq_pkt_mem_move(): for memory or register vaule copy
- cmdq_pkt_poll_addr(): extending cmdq_pkt_poll() to support polling
address of register
- cmdq_pkt_acquire_event(): to support MUTEX_LOCK protection between
GCE threads
- cmdq_get_event()/cmdq_set_event(): to support ISP driver runtime control
some GCE events.
Change in RESEND v1:
1. Remove Change-Id in commit message.
Jason-JH.Lin (5):
soc: mediatek: mtk-cmdq: Add specific purpose register definitions for
GCE
soc: mediatek: mtk-cmdq: Add cmdq_pkt_mem_move() function
soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function
soc: mediatek: mtk-cmdq: Add cmdq_pkt_acquire_event() function
mailbox: mtk-cmdq: Add support runtime get and set GCE event
drivers/mailbox/mtk-cmdq-mailbox.c | 37 +++++++++++
drivers/soc/mediatek/mtk-cmdq-helper.c | 79 ++++++++++++++++++++++++
include/linux/mailbox/mtk-cmdq-mailbox.h | 2 +
include/linux/soc/mediatek/mtk-cmdq.h | 44 +++++++++++++
4 files changed, 162 insertions(+)
--
2.18.0
Add cmdq_pkt_mem_move() function to support CMDQ user making
an instruction for moving a value from a source address to a
destination address.
Signed-off-by: Jason-JH.Lin <[email protected]>
---
drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
include/linux/soc/mediatek/mtk-cmdq.h | 10 ++++++++++
2 files changed, 36 insertions(+)
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index b0cd071c4719..3a1e47ad8a41 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -299,6 +299,32 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
}
EXPORT_SYMBOL(cmdq_pkt_write_s_mask_value);
+s32 cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr, dma_addr_t dst_addr)
+{
+ s32 err;
+ const u16 tmp_reg_idx = CMDQ_THR_SPR_IDX0;
+ const u16 swap_reg_idx = CMDQ_THR_SPR_IDX1;
+
+ /* read the value of src_addr into swap_reg_idx */
+ err = cmdq_pkt_assign(pkt, tmp_reg_idx, CMDQ_ADDR_HIGH(src_addr));
+ if (err < 0)
+ return err;
+ err = cmdq_pkt_read_s(pkt, tmp_reg_idx, CMDQ_ADDR_LOW(src_addr), swap_reg_idx);
+ if (err < 0)
+ return err;
+
+ /* write the value of swap_reg_idx into dst_addr */
+ err = cmdq_pkt_assign(pkt, tmp_reg_idx, CMDQ_ADDR_HIGH(dst_addr));
+ if (err < 0)
+ return err;
+ err = cmdq_pkt_write_s(pkt, tmp_reg_idx, CMDQ_ADDR_LOW(dst_addr), swap_reg_idx);
+ if (err < 0)
+ return err;
+
+ return err;
+}
+EXPORT_SYMBOL(cmdq_pkt_mem_move);
+
int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
{
struct cmdq_instruction inst = { {0} };
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 1dae80185f9f..b6dbe2d8f16a 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -182,6 +182,16 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8 high_addr_reg_idx,
u16 addr_low, u32 value, u32 mask);
+/**
+ * cmdq_pkt_mem_move() - append memory move command to the CMDQ packet
+ * @pkt: the CMDQ packet
+ * @src_addr: source address
+ * @dma_addr_t: destination address
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr, dma_addr_t dst_addr);
+
/**
* cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
* @pkt: the CMDQ packet
--
2.18.0
Add specific purpose register definitions for GCE, so CMDQ users can
use them as a buffer to store data.
Signed-off-by: Jason-JH.Lin <[email protected]>
---
include/linux/soc/mediatek/mtk-cmdq.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 649955d2cf5c..1dae80185f9f 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -14,6 +14,15 @@
#define CMDQ_ADDR_HIGH(addr) ((u32)(((addr) >> 16) & GENMASK(31, 0)))
#define CMDQ_ADDR_LOW(addr) ((u16)(addr) | BIT(1))
+/*
+ * Every cmdq thread has its own SPRs (Specific Purpose Registers),
+ * so there are 4 * N (threads) SPRs in GCE that shares the same indexes below.
+ */
+#define CMDQ_THR_SPR_IDX0 (0)
+#define CMDQ_THR_SPR_IDX1 (1)
+#define CMDQ_THR_SPR_IDX2 (2)
+#define CMDQ_THR_SPR_IDX3 (3)
+
struct cmdq_pkt;
struct cmdq_client_reg {
--
2.18.0
Add cmdq_pkt_acquire_event() function to support CMDQ user making
an instruction for acquiring event.
CMDQ users can use cmdq_pkt_acquire_event() and cmdq_pkt_clear_event()
to acquire GCE event and release GCE event and achieve the MUTEX_LOCK
protection between GCE threads.
Signed-off-by: Jason-JH.Lin <[email protected]>
---
drivers/soc/mediatek/mtk-cmdq-helper.c | 15 +++++++++++++++
include/linux/soc/mediatek/mtk-cmdq.h | 9 +++++++++
2 files changed, 24 insertions(+)
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 2e9fc9bb1183..0183b40a0eff 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -342,6 +342,21 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
}
EXPORT_SYMBOL(cmdq_pkt_wfe);
+int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
+{
+ struct cmdq_instruction inst = {};
+
+ if (event >= CMDQ_MAX_EVENT)
+ return -EINVAL;
+
+ inst.op = CMDQ_CODE_WFE;
+ inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE | CMDQ_WFE_WAIT;
+ inst.event = event;
+
+ return cmdq_pkt_append_command(pkt, inst);
+}
+EXPORT_SYMBOL(cmdq_pkt_acquire_event);
+
int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
{
struct cmdq_instruction inst = { {0} };
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 2fe9be240fbc..de93c0a8e8a9 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -202,6 +202,15 @@ int cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr, dma_addr_t dst_
*/
int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
+/**
+ * cmdq_pkt_acquire_event() - append acquire event command to the CMDQ packet
+ * @pkt: the CMDQ packet
+ * @event: the desired event to be acquired
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event);
+
/**
* cmdq_pkt_clear_event() - append clear event command to the CMDQ packet
* @pkt: the CMDQ packet
--
2.18.0
Add cmdq_pkt_poll_addr function to support CMDQ user making
an instruction for polling a specific address of hardware rigster
to check the value with or without mask.
POLL is an old operation in GCE, so it does not support SPR and
CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
to move polling register address to GPR to make an instruction.
This will be done in cmdq_pkt_poll_addr().
Signed-off-by: Jason-JH.Lin <[email protected]>
---
drivers/soc/mediatek/mtk-cmdq-helper.c | 38 ++++++++++++++++++++++++++
include/linux/soc/mediatek/mtk-cmdq.h | 16 +++++++++++
2 files changed, 54 insertions(+)
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 3a1e47ad8a41..2e9fc9bb1183 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -12,6 +12,7 @@
#define CMDQ_WRITE_ENABLE_MASK BIT(0)
#define CMDQ_POLL_ENABLE_MASK BIT(0)
+#define CMDQ_POLL_HIGH_ADDR_GPR (14)
#define CMDQ_EOC_IRQ_EN BIT(0)
#define CMDQ_REG_TYPE 1
#define CMDQ_JUMP_RELATIVE 1
@@ -406,6 +407,43 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
}
EXPORT_SYMBOL(cmdq_pkt_poll_mask);
+int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32 value, u32 mask)
+{
+ struct cmdq_instruction inst = { {0} };
+ int err;
+ u8 use_mask = 0;
+
+ if (mask != U32_MAX) {
+ inst.op = CMDQ_CODE_MASK;
+ inst.mask = ~mask;
+ err = cmdq_pkt_append_command(pkt, inst);
+ if (err != 0)
+ return err;
+ use_mask = CMDQ_POLL_ENABLE_MASK;
+ }
+
+ /*
+ * POLL is an old operation in GCE and it does not support SPR and CMDQ_CODE_LOGIC,
+ * so it can not use cmdq_pkt_assign to keep polling register address to SPR.
+ * It needs to use GPR and CMDQ_CODE_MASK to move polling register address to GPR.
+ */
+ inst.op = CMDQ_CODE_MASK;
+ inst.dst_t = CMDQ_REG_TYPE;
+ inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
+ inst.mask = addr;
+ err = cmdq_pkt_append_command(pkt, inst);
+ if (err < 0)
+ return err;
+
+ inst.op = CMDQ_CODE_POLL;
+ inst.dst_t = CMDQ_REG_TYPE;
+ inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
+ inst.offset = use_mask;
+ inst.value = value;
+ return cmdq_pkt_append_command(pkt, inst);
+}
+EXPORT_SYMBOL(cmdq_pkt_poll_addr);
+
int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
{
struct cmdq_instruction inst = {};
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index b6dbe2d8f16a..2fe9be240fbc 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -253,6 +253,22 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
u16 offset, u32 value, u32 mask);
+/**
+ * cmdq_pkt_poll_addr() - Append polling command to the CMDQ packet, ask GCE to
+ * execute an instruction that wait for a specified
+ * address of hardware register to check for the value
+ * w/ or w/o mask.
+ * All GCE hardware threads will be blocked by this
+ * instruction.
+ * @pkt: the CMDQ packet
+ * @addr: the hardware register address
+ * @value: the specified target register value
+ * @mask: the specified target register mask
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32 value, u32 mask);
+
/**
* cmdq_pkt_assign() - Append logic assign command to the CMDQ packet, ask GCE
* to execute an instruction that set a constant value into
--
2.18.0
Hi, Jason:
On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> Add cmdq_pkt_acquire_event() function to support CMDQ user making
> an instruction for acquiring event.
>
> CMDQ users can use cmdq_pkt_acquire_event() and
> cmdq_pkt_clear_event()
> to acquire GCE event and release GCE event and achieve the MUTEX_LOCK
> protection between GCE threads.
I'm not clear what acquire do in detail. This is what I guess:
cmdq_pkt_acquire_event() would wait for event to be cleared. After
event is cleared, cmdq_pkt_acquire_event() would set event and keep
executing next command. So the mutex would work like this
cmdq_pkt_acquire_event() /* mutex lock */
/* critical secton */
cmdq_pkt_clear_event() /* mutex unlock */
If it's so, describe as detail as this. If not, describe how it do.
As I know, GCE is single core, so multiple thread is served by single
GCE, why need mutex lock?
Regards,
CK
>
> Signed-off-by: Jason-JH.Lin <[email protected]>
> ---
> drivers/soc/mediatek/mtk-cmdq-helper.c | 15 +++++++++++++++
> include/linux/soc/mediatek/mtk-cmdq.h | 9 +++++++++
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 2e9fc9bb1183..0183b40a0eff 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -342,6 +342,21 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16
> event, bool clear)
> }
> EXPORT_SYMBOL(cmdq_pkt_wfe);
>
> +int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
> +{
> + struct cmdq_instruction inst = {};
> +
> + if (event >= CMDQ_MAX_EVENT)
> + return -EINVAL;
> +
> + inst.op = CMDQ_CODE_WFE;
> + inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE |
> CMDQ_WFE_WAIT;
> + inst.event = event;
> +
> + return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_acquire_event);
> +
> int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
> {
> struct cmdq_instruction inst = { {0} };
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> index 2fe9be240fbc..de93c0a8e8a9 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -202,6 +202,15 @@ int cmdq_pkt_mem_move(struct cmdq_pkt *pkt,
> dma_addr_t src_addr, dma_addr_t dst_
> */
> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
>
> +/**
> + * cmdq_pkt_acquire_event() - append acquire event command to the
> CMDQ packet
> + * @pkt: the CMDQ packet
> + * @event: the desired event to be acquired
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event);
> +
> /**
> * cmdq_pkt_clear_event() - append clear event command to the CMDQ
> packet
> * @pkt: the CMDQ packet
Hi, Jason:
On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> Add cmdq_pkt_mem_move() function to support CMDQ user making
> an instruction for moving a value from a source address to a
> destination address.
>
> Signed-off-by: Jason-JH.Lin <[email protected]>
> ---
> drivers/soc/mediatek/mtk-cmdq-helper.c | 26
> ++++++++++++++++++++++++++
> include/linux/soc/mediatek/mtk-cmdq.h | 10 ++++++++++
> 2 files changed, 36 insertions(+)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index b0cd071c4719..3a1e47ad8a41 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -299,6 +299,32 @@ int cmdq_pkt_write_s_mask_value(struct cmdq_pkt
> *pkt, u8 high_addr_reg_idx,
> }
> EXPORT_SYMBOL(cmdq_pkt_write_s_mask_value);
>
> +s32 cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr,
> dma_addr_t dst_addr)
return type int.
> +{
> + s32 err;
It may not error, so I prefer to use 'ret'.
Move this after the next two declaration.
> + const u16 tmp_reg_idx = CMDQ_THR_SPR_IDX0;
> + const u16 swap_reg_idx = CMDQ_THR_SPR_IDX1;
I would like tmp_reg_idx to be high_addr_reg_idx and swap_reg_idx to be
value_reg_idx.
Regards,
CK
> +
> + /* read the value of src_addr into swap_reg_idx */
> + err = cmdq_pkt_assign(pkt, tmp_reg_idx,
> CMDQ_ADDR_HIGH(src_addr));
> + if (err < 0)
> + return err;
> + err = cmdq_pkt_read_s(pkt, tmp_reg_idx,
> CMDQ_ADDR_LOW(src_addr), swap_reg_idx);
> + if (err < 0)
> + return err;
> +
> + /* write the value of swap_reg_idx into dst_addr */
> + err = cmdq_pkt_assign(pkt, tmp_reg_idx,
> CMDQ_ADDR_HIGH(dst_addr));
> + if (err < 0)
> + return err;
> + err = cmdq_pkt_write_s(pkt, tmp_reg_idx,
> CMDQ_ADDR_LOW(dst_addr), swap_reg_idx);
> + if (err < 0)
> + return err;
> +
> + return err;
> +}
> +EXPORT_SYMBOL(cmdq_pkt_mem_move);
> +
> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
> {
> struct cmdq_instruction inst = { {0} };
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> index 1dae80185f9f..b6dbe2d8f16a 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -182,6 +182,16 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt,
> u8 high_addr_reg_idx,
> int cmdq_pkt_write_s_mask_value(struct cmdq_pkt *pkt, u8
> high_addr_reg_idx,
> u16 addr_low, u32 value, u32 mask);
>
> +/**
> + * cmdq_pkt_mem_move() - append memory move command to the CMDQ
> packet
> + * @pkt: the CMDQ packet
> + * @src_addr: source address
> + * @dma_addr_t: destination address
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr,
> dma_addr_t dst_addr);
> +
> /**
> * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> * @pkt: the CMDQ packet
Hi, Jason:
On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> Add cmdq_pkt_poll_addr function to support CMDQ user making
> an instruction for polling a specific address of hardware rigster
> to check the value with or without mask.
>
> POLL is an old operation in GCE, so it does not support SPR and
> CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
> to move polling register address to GPR to make an instruction.
> This will be done in cmdq_pkt_poll_addr().
>
> Signed-off-by: Jason-JH.Lin <[email protected]>
> ---
> drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> ++++++++++++++++++++++++++
> include/linux/soc/mediatek/mtk-cmdq.h | 16 +++++++++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index 3a1e47ad8a41..2e9fc9bb1183 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -12,6 +12,7 @@
>
> #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> #define CMDQ_POLL_ENABLE_MASK BIT(0)
> +#define CMDQ_POLL_HIGH_ADDR_GPR (14)
I think there are multiple GPR and you use #14 to store high addr. I
would like you to list all GPR and do not limit the usage of each GPR.
The question is, why limit #14 to be high addr? If the GPR is shared by
all threads, there should be a mechanism to manage GPR usage for client
driver to allocate/free GPR.
> #define CMDQ_EOC_IRQ_EN BIT(0)
> #define CMDQ_REG_TYPE 1
> #define CMDQ_JUMP_RELATIVE 1
> @@ -406,6 +407,43 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8
> subsys,
> }
> EXPORT_SYMBOL(cmdq_pkt_poll_mask);
>
> +int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32
> value, u32 mask)
> +{
> + struct cmdq_instruction inst = { {0} };
> + int err;
> + u8 use_mask = 0;
> +
> + if (mask != U32_MAX) {
> + inst.op = CMDQ_CODE_MASK;
> + inst.mask = ~mask;
> + err = cmdq_pkt_append_command(pkt, inst);
> + if (err != 0)
> + return err;
> + use_mask = CMDQ_POLL_ENABLE_MASK;
> + }
> +
> + /*
> + * POLL is an old operation in GCE and it does not support SPR
> and CMDQ_CODE_LOGIC,
> + * so it can not use cmdq_pkt_assign to keep polling register
> address to SPR.
> + * It needs to use GPR and CMDQ_CODE_MASK to move polling
> register address to GPR.
> + */
> + inst.op = CMDQ_CODE_MASK;
> + inst.dst_t = CMDQ_REG_TYPE;
> + inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
> + inst.mask = addr;
This is full address, not high address. Why name the GPR to
CMDQ_POLL_HIGH_ADDR_GPR?
The addr is not mask, so I would like inst.value = addr.
Regards,
CK
> + err = cmdq_pkt_append_command(pkt, inst);
> + if (err < 0)
> + return err;
> +
> + inst.op = CMDQ_CODE_POLL;
> + inst.dst_t = CMDQ_REG_TYPE;
> + inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
> + inst.offset = use_mask;
> + inst.value = value;
> + return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_poll_addr);
> +
> int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> {
> struct cmdq_instruction inst = {};
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> index b6dbe2d8f16a..2fe9be240fbc 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -253,6 +253,22 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8
> subsys,
> int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> u16 offset, u32 value, u32 mask);
>
> +/**
> + * cmdq_pkt_poll_addr() - Append polling command to the CMDQ packet,
> ask GCE to
> + * execute an instruction that wait for a
> specified
> + * address of hardware register to check for the
> value
> + * w/ or w/o mask.
> + * All GCE hardware threads will be blocked by
> this
> + * instruction.
> + * @pkt: the CMDQ packet
> + * @addr: the hardware register address
> + * @value: the specified target register value
> + * @mask: the specified target register mask
> + *
> + * Return: 0 for success; else the error code is returned
> + */
> +int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32
> value, u32 mask);
> +
> /**
> * cmdq_pkt_assign() - Append logic assign command to the CMDQ
> packet, ask GCE
> * to execute an instruction that set a constant
> value into
Hi CK,
Thanks for the reviews.
On Mon, 2024-03-04 at 02:11 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
>
> On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > Add cmdq_pkt_acquire_event() function to support CMDQ user making
> > an instruction for acquiring event.
> >
> > CMDQ users can use cmdq_pkt_acquire_event() and
> > cmdq_pkt_clear_event()
> > to acquire GCE event and release GCE event and achieve the
> > MUTEX_LOCK
> > protection between GCE threads.
>
> I'm not clear what acquire do in detail. This is what I guess:
>
> cmdq_pkt_acquire_event() would wait for event to be cleared. After
> event is cleared, cmdq_pkt_acquire_event() would set event and keep
> executing next command. So the mutex would work like this
>
> cmdq_pkt_acquire_event() /* mutex lock */
>
> /* critical secton */
>
> cmdq_pkt_clear_event() /* mutex unlock */
>
> If it's so, describe as detail as this. If not, describe how it do.
>
Yes, they should be used like this.
I'll add more description in commit message.
> As I know, GCE is single core, so multiple thread is served by single
> GCE, why need mutex lock?
>
Although GCE is single core, it will context switch to other GCE
threads with the same priority. The context switch time is set to
GCE_THR_SLOT_CYCLES during GCE initialization.
So we have to use event_lock to protect HW resource if each cmdq_pkt in
different thread may execute more than the context switch time.
Regards,
Jason-JH.Lin
> Regards,
> CK
>
> >
> > Signed-off-by: Jason-JH.Lin <[email protected]>
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 15 +++++++++++++++
> > include/linux/soc/mediatek/mtk-cmdq.h | 9 +++++++++
> > 2 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 2e9fc9bb1183..0183b40a0eff 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -342,6 +342,21 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16
> > event, bool clear)
> > }
> > EXPORT_SYMBOL(cmdq_pkt_wfe);
> >
> > +int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event)
> > +{
> > + struct cmdq_instruction inst = {};
> > +
> > + if (event >= CMDQ_MAX_EVENT)
> > + return -EINVAL;
> > +
> > + inst.op = CMDQ_CODE_WFE;
> > + inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE |
> > CMDQ_WFE_WAIT;
> > + inst.event = event;
> > +
> > + return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_acquire_event);
> > +
> > int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
> > {
> > struct cmdq_instruction inst = { {0} };
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> > b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 2fe9be240fbc..de93c0a8e8a9 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -202,6 +202,15 @@ int cmdq_pkt_mem_move(struct cmdq_pkt *pkt,
> > dma_addr_t src_addr, dma_addr_t dst_
> > */
> > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
> >
> > +/**
> > + * cmdq_pkt_acquire_event() - append acquire event command to the
> > CMDQ packet
> > + * @pkt: the CMDQ packet
> > + * @event: the desired event to be acquired
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_acquire_event(struct cmdq_pkt *pkt, u16 event);
> > +
> > /**
> > * cmdq_pkt_clear_event() - append clear event command to the CMDQ
> > packet
> > * @pkt: the CMDQ packet
Hi CK,
Thanks for the reviews.
On Mon, 2024-03-04 at 02:39 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
>
> On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > Add cmdq_pkt_mem_move() function to support CMDQ user making
> > an instruction for moving a value from a source address to a
> > destination address.
> >
> > Signed-off-by: Jason-JH.Lin <[email protected]>
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 26
> > ++++++++++++++++++++++++++
> > include/linux/soc/mediatek/mtk-cmdq.h | 10 ++++++++++
> > 2 files changed, 36 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index b0cd071c4719..3a1e47ad8a41 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -299,6 +299,32 @@ int cmdq_pkt_write_s_mask_value(struct
> > cmdq_pkt
> > *pkt, u8 high_addr_reg_idx,
> > }
> > EXPORT_SYMBOL(cmdq_pkt_write_s_mask_value);
> >
> > +s32 cmdq_pkt_mem_move(struct cmdq_pkt *pkt, dma_addr_t src_addr,
> > dma_addr_t dst_addr)
>
> return type int.
OK, I'll change it.
>
> > +{
> > + s32 err;
>
> It may not error, so I prefer to use 'ret'.
> Move this after the next two declaration.
OK, Ill change it to 'ret' and move it.
>
> > + const u16 tmp_reg_idx = CMDQ_THR_SPR_IDX0;
> > + const u16 swap_reg_idx = CMDQ_THR_SPR_IDX1;
>
> I would like tmp_reg_idx to be high_addr_reg_idx and swap_reg_idx to
> be
> value_reg_idx.
>
OK, I'll rename them.
Regards,
Jason-JH.Lin
> Regards,
> CK
>
Hi CK,
Thanks for the reviews.
On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
>
> On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > an instruction for polling a specific address of hardware rigster
> > to check the value with or without mask.
> >
> > POLL is an old operation in GCE, so it does not support SPR and
> > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
> > to move polling register address to GPR to make an instruction.
> > This will be done in cmdq_pkt_poll_addr().
> >
> > Signed-off-by: Jason-JH.Lin <[email protected]>
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > ++++++++++++++++++++++++++
> > include/linux/soc/mediatek/mtk-cmdq.h | 16 +++++++++++
> > 2 files changed, 54 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 3a1e47ad8a41..2e9fc9bb1183 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -12,6 +12,7 @@
> >
> > #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> > #define CMDQ_POLL_ENABLE_MASK BIT(0)
> > +#define CMDQ_POLL_HIGH_ADDR_GPR (14)
>
> I think there are multiple GPR and you use #14 to store high addr. I
> would like you to list all GPR and do not limit the usage of each
> GPR.
> The question is, why limit #14 to be high addr? If the GPR is shared
> by
> all threads, there should be a mechanism to manage GPR usage for
> client
> driver to allocate/free GPR.
Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are shared by all
threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.
I think user may not know which GPR is available, so I think CMDQ
driver should manage the usage of GPR instead of configure by the user.
Currently, we only use 1 dedicated GPR in poll, so I defined it in CMDQ
driver to make it simpler.
>
> > #define CMDQ_EOC_IRQ_EN BIT(0)
> > #define CMDQ_REG_TYPE 1
> > #define CMDQ_JUMP_RELATIVE 1
> > @@ -406,6 +407,43 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt,
> > u8
> > subsys,
> > }
> > EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> >
> > +int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32
> > value, u32 mask)
> > +{
> > + struct cmdq_instruction inst = { {0} };
> > + int err;
> > + u8 use_mask = 0;
> > +
> > + if (mask != U32_MAX) {
> > + inst.op = CMDQ_CODE_MASK;
> > + inst.mask = ~mask;
> > + err = cmdq_pkt_append_command(pkt, inst);
> > + if (err != 0)
> > + return err;
> > + use_mask = CMDQ_POLL_ENABLE_MASK;
> > + }
> > +
> > + /*
> > + * POLL is an old operation in GCE and it does not support SPR
> > and CMDQ_CODE_LOGIC,
> > + * so it can not use cmdq_pkt_assign to keep polling register
> > address to SPR.
> > + * It needs to use GPR and CMDQ_CODE_MASK to move polling
> > register address to GPR.
> > + */
> > + inst.op = CMDQ_CODE_MASK;
> > + inst.dst_t = CMDQ_REG_TYPE;
> > + inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
> > + inst.mask = addr;
>
> This is full address, not high address. Why name the GPR to
> CMDQ_POLL_HIGH_ADDR_GPR?
>
My mistake, I'll remove that 'HIGH' word.
> The addr is not mask, so I would like inst.value = addr.
OK, I'll change it.
Regards,
Jason-JH.Lin
>
> Regards,
> CK
>
Hi, Jason:
On Mon, 2024-03-04 at 16:04 +0000, Jason-JH Lin (林睿祥) wrote:
> Hi CK,
>
> Thanks for the reviews.
>
> On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> > Hi, Jason:
> >
> > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > > an instruction for polling a specific address of hardware rigster
> > > to check the value with or without mask.
> > >
> > > POLL is an old operation in GCE, so it does not support SPR and
> > > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
> > > to move polling register address to GPR to make an instruction.
> > > This will be done in cmdq_pkt_poll_addr().
> > >
> > > Signed-off-by: Jason-JH.Lin <[email protected]>
> > > ---
> > > drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > > ++++++++++++++++++++++++++
> > > include/linux/soc/mediatek/mtk-cmdq.h | 16 +++++++++++
> > > 2 files changed, 54 insertions(+)
> > >
> > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > index 3a1e47ad8a41..2e9fc9bb1183 100644
> > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > @@ -12,6 +12,7 @@
> > >
> > > #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> > > #define CMDQ_POLL_ENABLE_MASK BIT(0)
> > > +#define CMDQ_POLL_HIGH_ADDR_GPR (14)
> >
> > I think there are multiple GPR and you use #14 to store high addr.
> > I
> > would like you to list all GPR and do not limit the usage of each
> > GPR.
> > The question is, why limit #14 to be high addr? If the GPR is
> > shared
> > by
> > all threads, there should be a mechanism to manage GPR usage for
> > client
> > driver to allocate/free GPR.
>
> Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are shared by
> all
> threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.
>
> I think user may not know which GPR is available, so I think CMDQ
> driver should manage the usage of GPR instead of configure by the
> user.
>
> Currently, we only use 1 dedicated GPR in poll, so I defined it in
> CMDQ
> driver to make it simpler.
If thread 1 poll addr A first, GPR is set to A. But poll time exceed
GCE_THD_SLOT_CYCLES, change to thread 2 and thread 2 poll addr B, GPR
is set to B. Later switch to thread A and GCE would execute poll
command and GPR is B, so thread 1 would poll addr B, but this is wrong.
How do you solve this problem?
Regards,
CK
>
> >
> > > #define CMDQ_EOC_IRQ_EN BIT(0)
> > > #define CMDQ_REG_TYPE 1
> > > #define CMDQ_JUMP_RELATIVE 1
> > > @@ -406,6 +407,43 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt,
> > > u8
> > > subsys,
> > > }
> > > EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> > >
> > > +int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr,
> > > u32
> > > value, u32 mask)
> > > +{
> > > + struct cmdq_instruction inst = { {0} };
> > > + int err;
> > > + u8 use_mask = 0;
> > > +
> > > + if (mask != U32_MAX) {
> > > + inst.op = CMDQ_CODE_MASK;
> > > + inst.mask = ~mask;
> > > + err = cmdq_pkt_append_command(pkt, inst);
> > > + if (err != 0)
> > > + return err;
> > > + use_mask = CMDQ_POLL_ENABLE_MASK;
> > > + }
> > > +
> > > + /*
> > > + * POLL is an old operation in GCE and it does not support SPR
> > > and CMDQ_CODE_LOGIC,
> > > + * so it can not use cmdq_pkt_assign to keep polling register
> > > address to SPR.
> > > + * It needs to use GPR and CMDQ_CODE_MASK to move polling
> > > register address to GPR.
> > > + */
> > > + inst.op = CMDQ_CODE_MASK;
> > > + inst.dst_t = CMDQ_REG_TYPE;
> > > + inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
> > > + inst.mask = addr;
> >
> > This is full address, not high address. Why name the GPR to
> > CMDQ_POLL_HIGH_ADDR_GPR?
> >
>
> My mistake, I'll remove that 'HIGH' word.
>
> > The addr is not mask, so I would like inst.value = addr.
>
> OK, I'll change it.
>
> Regards,
> Jason-JH.Lin
>
> >
> > Regards,
> > CK
> >
On Tue, 2024-03-05 at 03:26 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
>
> On Mon, 2024-03-04 at 16:04 +0000, Jason-JH Lin (林睿祥) wrote:
> > Hi CK,
> >
> > Thanks for the reviews.
> >
> > On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> > > Hi, Jason:
> > >
> > > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > > > an instruction for polling a specific address of hardware
> > > > rigster
> > > > to check the value with or without mask.
> > > >
> > > > POLL is an old operation in GCE, so it does not support SPR and
> > > > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
> > > > to move polling register address to GPR to make an instruction.
> > > > This will be done in cmdq_pkt_poll_addr().
> > > >
> > > > Signed-off-by: Jason-JH.Lin <[email protected]>
> > > > ---
> > > > drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > > > ++++++++++++++++++++++++++
> > > > include/linux/soc/mediatek/mtk-cmdq.h | 16 +++++++++++
> > > > 2 files changed, 54 insertions(+)
> > > >
> > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > index 3a1e47ad8a41..2e9fc9bb1183 100644
> > > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > @@ -12,6 +12,7 @@
> > > >
> > > > #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> > > > #define CMDQ_POLL_ENABLE_MASK BIT(0)
> > > > +#define CMDQ_POLL_HIGH_ADDR_GPR (14)
> > >
> > > I think there are multiple GPR and you use #14 to store high
> > > addr.
> > > I
> > > would like you to list all GPR and do not limit the usage of each
> > > GPR.
> > > The question is, why limit #14 to be high addr? If the GPR is
> > > shared
> > > by
> > > all threads, there should be a mechanism to manage GPR usage for
> > > client
> > > driver to allocate/free GPR.
> >
> > Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are shared by
> > all
> > threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.
> >
> > I think user may not know which GPR is available, so I think CMDQ
> > driver should manage the usage of GPR instead of configure by the
> > user.
> >
> > Currently, we only use 1 dedicated GPR in poll, so I defined it in
> > CMDQ
> > driver to make it simpler.
>
> If thread 1 poll addr A first, GPR is set to A. But poll time exceed
> GCE_THD_SLOT_CYCLES, change to thread 2 and thread 2 poll addr B, GPR
> is set to B. Later switch to thread A and GCE would execute poll
> command and GPR is B, so thread 1 would poll addr B, but this is
> wrong.
> How do you solve this problem?
>
If POLL instruction has timeout, this may be a problem.
But POLL is a legacy operation, it won't context switch when the
execute time exceed GCE_THR_SLOT_CYCLES. So we add the comment "All GCE
hardware threads will be blocked by this instruction" in the kerneldoc.
And currently, we don't set the GCE hardware timeout in
CMDQ_THR_INST_TIMEOUT_CYCLES, so it'll poll forever until the polling
value is set...
Regards,
Jason-JH.Lin
> Regards,
> CK
>
Hi, Jason:
On Tue, 2024-03-05 at 03:37 +0000, Jason-JH Lin (林睿祥) wrote:
> On Tue, 2024-03-05 at 03:26 +0000, CK Hu (胡俊光) wrote:
> > Hi, Jason:
> >
> > On Mon, 2024-03-04 at 16:04 +0000, Jason-JH Lin (林睿祥) wrote:
> > > Hi CK,
> > >
> > > Thanks for the reviews.
> > >
> > > On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> > > > Hi, Jason:
> > > >
> > > > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > > > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > > > > an instruction for polling a specific address of hardware
> > > > > rigster
> > > > > to check the value with or without mask.
> > > > >
> > > > > POLL is an old operation in GCE, so it does not support SPR
> > > > > and
> > > > > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and
> > > > > CMDQ_CODE_MASK
> > > > > to move polling register address to GPR to make an
> > > > > instruction.
> > > > > This will be done in cmdq_pkt_poll_addr().
> > > > >
> > > > > Signed-off-by: Jason-JH.Lin <[email protected]>
> > > > > ---
> > > > > drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > > > > ++++++++++++++++++++++++++
> > > > > include/linux/soc/mediatek/mtk-cmdq.h | 16 +++++++++++
> > > > > 2 files changed, 54 insertions(+)
> > > > >
> > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > index 3a1e47ad8a41..2e9fc9bb1183 100644
> > > > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > @@ -12,6 +12,7 @@
> > > > >
> > > > > #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> > > > > #define CMDQ_POLL_ENABLE_MASK BIT(0)
> > > > > +#define CMDQ_POLL_HIGH_ADDR_GPR (14)
> > > >
> > > > I think there are multiple GPR and you use #14 to store high
> > > > addr.
> > > > I
> > > > would like you to list all GPR and do not limit the usage of
> > > > each
> > > > GPR.
> > > > The question is, why limit #14 to be high addr? If the GPR is
> > > > shared
> > > > by
> > > > all threads, there should be a mechanism to manage GPR usage
> > > > for
> > > > client
> > > > driver to allocate/free GPR.
> > >
> > > Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are shared
> > > by
> > > all
> > > threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.
> > >
> > > I think user may not know which GPR is available, so I think CMDQ
> > > driver should manage the usage of GPR instead of configure by the
> > > user.
> > >
> > > Currently, we only use 1 dedicated GPR in poll, so I defined it
> > > in
> > > CMDQ
> > > driver to make it simpler.
> >
> > If thread 1 poll addr A first, GPR is set to A. But poll time
> > exceed
> > GCE_THD_SLOT_CYCLES, change to thread 2 and thread 2 poll addr B,
> > GPR
> > is set to B. Later switch to thread A and GCE would execute poll
> > command and GPR is B, so thread 1 would poll addr B, but this is
> > wrong.
> > How do you solve this problem?
> >
>
> If POLL instruction has timeout, this may be a problem.
>
> But POLL is a legacy operation, it won't context switch when the
> execute time exceed GCE_THR_SLOT_CYCLES. So we add the comment "All
> GCE
> hardware threads will be blocked by this instruction" in the
> kerneldoc.
>
> And currently, we don't set the GCE hardware timeout in
> CMDQ_THR_INST_TIMEOUT_CYCLES, so it'll poll forever until the polling
> value is set...
If ISP poll time longer than vblank, it may make display disorder. When
I see display disorder, could I find out ISP poll trigger this
disorder?
Regards,
CK
>
> Regards,
> Jason-JH.Lin
>
> > Regards,
> > CK
> >
On Tue, 2024-03-05 at 03:51 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
>
> On Tue, 2024-03-05 at 03:37 +0000, Jason-JH Lin (林睿祥) wrote:
> > On Tue, 2024-03-05 at 03:26 +0000, CK Hu (胡俊光) wrote:
> > > Hi, Jason:
> > >
> > > On Mon, 2024-03-04 at 16:04 +0000, Jason-JH Lin (林睿祥) wrote:
> > > > Hi CK,
> > > >
> > > > Thanks for the reviews.
> > > >
> > > > On Mon, 2024-03-04 at 03:11 +0000, CK Hu (胡俊光) wrote:
> > > > > Hi, Jason:
> > > > >
> > > > > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > > > > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > > > > > an instruction for polling a specific address of hardware
> > > > > > rigster
> > > > > > to check the value with or without mask.
> > > > > >
> > > > > > POLL is an old operation in GCE, so it does not support SPR
> > > > > > and
> > > > > > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and
> > > > > > CMDQ_CODE_MASK
> > > > > > to move polling register address to GPR to make an
> > > > > > instruction.
> > > > > > This will be done in cmdq_pkt_poll_addr().
> > > > > >
> > > > > > Signed-off-by: Jason-JH.Lin <[email protected]>
> > > > > > ---
> > > > > > drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > > > > > ++++++++++++++++++++++++++
> > > > > > include/linux/soc/mediatek/mtk-cmdq.h | 16 +++++++++++
> > > > > > 2 files changed, 54 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > > index 3a1e47ad8a41..2e9fc9bb1183 100644
> > > > > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > > > @@ -12,6 +12,7 @@
> > > > > >
> > > > > > #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> > > > > > #define CMDQ_POLL_ENABLE_MASK BIT(0)
> > > > > > +#define CMDQ_POLL_HIGH_ADDR_GPR (14)
> > > > >
> > > > > I think there are multiple GPR and you use #14 to store high
> > > > > addr.
> > > > > I
> > > > > would like you to list all GPR and do not limit the usage of
> > > > > each
> > > > > GPR.
> > > > > The question is, why limit #14 to be high addr? If the GPR is
> > > > > shared
> > > > > by
> > > > > all threads, there should be a mechanism to manage GPR usage
> > > > > for
> > > > > client
> > > > > driver to allocate/free GPR.
> > > >
> > > > Yes, there are 16 GPR, from GPR_R0 ~ GPR_R15 and they are
> > > > shared
> > > > by
> > > > all
> > > > threads, but GPR_R0 and GPR_R1 is used by GCE HW itself.
> > > >
> > > > I think user may not know which GPR is available, so I think
> > > > CMDQ
> > > > driver should manage the usage of GPR instead of configure by
> > > > the
> > > > user.
> > > >
> > > > Currently, we only use 1 dedicated GPR in poll, so I defined it
> > > > in
> > > > CMDQ
> > > > driver to make it simpler.
> > >
> > > If thread 1 poll addr A first, GPR is set to A. But poll time
> > > exceed
> > > GCE_THD_SLOT_CYCLES, change to thread 2 and thread 2 poll addr B,
> > > GPR
> > > is set to B. Later switch to thread A and GCE would execute poll
> > > command and GPR is B, so thread 1 would poll addr B, but this is
> > > wrong.
> > > How do you solve this problem?
> > >
> >
> > If POLL instruction has timeout, this may be a problem.
> >
> > But POLL is a legacy operation, it won't context switch when the
> > execute time exceed GCE_THR_SLOT_CYCLES. So we add the comment "All
> > GCE
> > hardware threads will be blocked by this instruction" in the
> > kerneldoc.
> >
> > And currently, we don't set the GCE hardware timeout in
> > CMDQ_THR_INST_TIMEOUT_CYCLES, so it'll poll forever until the
> > polling
> > value is set...
>
> If ISP poll time longer than vblank, it may make display disorder.
> When
> I see display disorder, could I find out ISP poll trigger this
> disorder?
>
If we can get mtk_drm_ddp_irq timeout error when display disorder,
we need to dump all the cmd buffers and current PC in every executing
GCE threads by modifying the CMDQ driver code and see which PC
instruction may cause the problem.
If there is no timeout error when display disorder, then we have no
idea who cause that problem. We can only check the frame sequence id in
ISP driver frame done callback is disorder or not.
Because we don't set CMDQ hardware timeout IRQ or use software timer to
handle software timeout in CMDQ driver currently, so we won't know poll
instruction is blocking. ISP client drivers need to add their timeout
handler for each cmdq_pkt sent to the mailbox channels.
Regards,
Jason-JH.Lin
> Regards,
> CK
>
> >
> > Regards,
> > Jason-JH.Lin
> >
> > > Regards,
> > > CK
> > >