| Message ID | 20240301144403.2977-4-jason-jh.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add CMDQ API for upcoming ISP feature | expand |
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 <jason-jh.lin@mediatek.com> > --- > 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 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 <jason-jh.lin@mediatek.com> > > --- > > 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 <jason-jh.lin@mediatek.com> > > > --- > > > 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 <jason-jh.lin@mediatek.com> > > > > --- > > > > 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 <jason-jh.lin@mediatek.com> > > > > > --- > > > > > 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 <jason-jh.lin@mediatek.com> > > > > > > --- > > > > > > 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 > > >
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
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 <jason-jh.lin@mediatek.com> --- drivers/soc/mediatek/mtk-cmdq-helper.c | 38 ++++++++++++++++++++++++++ include/linux/soc/mediatek/mtk-cmdq.h | 16 +++++++++++ 2 files changed, 54 insertions(+)