| Message ID | 20230918192204.32263-9-jason-jh.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add CMDQ secure driver for SVP | expand |
Hi, Jason: On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote: > Add cmdq_pkt_finalize_loop to CMDQ driver. > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > jump to start of command buffer instruction to make the command > buffer loopable. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++ > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 4be2a18a4a02..bbb127620bb3 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > } > EXPORT_SYMBOL(cmdq_pkt_finalize); > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > +{ > + struct cmdq_instruction inst = { {0} }; > + int err; > + > + /* insert EOC and generate IRQ for each command iteration */ > + inst.op = CMDQ_CODE_EOC; > + inst.value = CMDQ_EOC_IRQ_EN; > + err = cmdq_pkt_append_command(pkt, inst); > + if (err < 0) > + return err; > + > + /* JUMP to start of pkt */ > + err = cmdq_pkt_jump(pkt, pkt->pa_base); > + if (err < 0) > + return err; Could you explain the case that a loop thread would trigger an interrupt? In DRM crc function, the loop thread need not to trigger interrupt, so I'm curious about this. Regards, CK > + > + pkt->loop = true; > + > + return err; > +} > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); > + > int cmdq_pkt_flush_async(struct cmdq_pkt *pkt) > { > int err; > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h > b/include/linux/soc/mediatek/mtk-cmdq.h > index 837ad8656adc..38a8e47da338 100644 > --- a/include/linux/soc/mediatek/mtk-cmdq.h > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > @@ -323,6 +323,14 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, > dma_addr_t addr); > */ > int cmdq_pkt_finalize(struct cmdq_pkt *pkt); > > +/** > + * cmdq_pkt_finalize_loop() - Append EOC and jump to start command. > + * @pkt: the CMDQ packet > + * > + * Return: 0 for success; else the error code is returned > + */ > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt); > + > /** > * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute > the CMDQ > * packet and call back at the end of done > packet
On Tue, 2023-09-19 at 01:38 +0000, CK Hu (胡俊光) wrote: > Hi, Jason: > > On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote: > > Add cmdq_pkt_finalize_loop to CMDQ driver. > > > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > > jump to start of command buffer instruction to make the command > > buffer loopable. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 > > +++++++++++++++++++++++ > > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > > 2 files changed, 31 insertions(+) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > > b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index 4be2a18a4a02..bbb127620bb3 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > > } > > EXPORT_SYMBOL(cmdq_pkt_finalize); > > > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > > +{ > > + struct cmdq_instruction inst = { {0} }; > > + int err; > > + > > + /* insert EOC and generate IRQ for each command iteration */ > > + inst.op = CMDQ_CODE_EOC; > > + inst.value = CMDQ_EOC_IRQ_EN; > > + err = cmdq_pkt_append_command(pkt, inst); > > + if (err < 0) > > + return err; > > + > > + /* JUMP to start of pkt */ > > + err = cmdq_pkt_jump(pkt, pkt->pa_base); > > + if (err < 0) > > + return err; > > Could you explain the case that a loop thread would trigger an > interrupt? In DRM crc function, the loop thread need not to trigger > interrupt, so I'm curious about this. > The looping thread in DRM crc funtion is for update CRC value to DRAM during every vblank event coming. It doesn't need to handle the software flow after the EOC. The looping thread in cmdq_sec_irq_notify_start() is waiting for every CMDQ_SYNC_TOKEN_SEC_THR_EOF being set, that means the GCE in secure world has finished all commands in a command buffer. Then it needs a GCE irq to trigger secure mailbox rx_callback() to handle the task of secure cmdq_pkt done in software. Regards, Jason-JH.Lin > Regards, > CK > > > + > > + pkt->loop = true; > > + > > + return err; > > +} > > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); > > + > > int cmdq_pkt_flush_async(struct cmdq_pkt *pkt) > > { > > int err; > > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h > > b/include/linux/soc/mediatek/mtk-cmdq.h > > index 837ad8656adc..38a8e47da338 100644 > > --- a/include/linux/soc/mediatek/mtk-cmdq.h > > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > > @@ -323,6 +323,14 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, > > dma_addr_t addr); > > */ > > int cmdq_pkt_finalize(struct cmdq_pkt *pkt); > > > > +/** > > + * cmdq_pkt_finalize_loop() - Append EOC and jump to start > > command. > > + * @pkt: the CMDQ packet > > + * > > + * Return: 0 for success; else the error code is returned > > + */ > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt); > > + > > /** > > * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute > > the CMDQ > > * packet and call back at the end of > > done > > packet
On 18/09/2023 21:21, Jason-JH.Lin wrote: > Add cmdq_pkt_finalize_loop to CMDQ driver. > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > jump to start of command buffer instruction to make the command > buffer loopable. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++ > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 4be2a18a4a02..bbb127620bb3 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > } > EXPORT_SYMBOL(cmdq_pkt_finalize); > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > +{ > + struct cmdq_instruction inst = { {0} }; > + int err; > + > + /* insert EOC and generate IRQ for each command iteration */ > + inst.op = CMDQ_CODE_EOC; > + inst.value = CMDQ_EOC_IRQ_EN; > + err = cmdq_pkt_append_command(pkt, inst); > + if (err < 0) > + return err; > + > + /* JUMP to start of pkt */ > + err = cmdq_pkt_jump(pkt, pkt->pa_base); > + if (err < 0) > + return err; > + > + pkt->loop = true; > + > + return err; > +} > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); 1. Missing GPL 2. Missing kerneldoc 3. Missing user 4. Are you going to split the patchset into one function per patch? No. Best regards, Krzysztof
On 18/09/2023 21:21, Jason-JH.Lin wrote: > Add cmdq_pkt_finalize_loop to CMDQ driver. > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > jump to start of command buffer instruction to make the command > buffer loopable. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++ > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 4be2a18a4a02..bbb127620bb3 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > } > EXPORT_SYMBOL(cmdq_pkt_finalize); > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > +{ > + struct cmdq_instruction inst = { {0} }; > + int err; > + > + /* insert EOC and generate IRQ for each command iteration */ > + inst.op = CMDQ_CODE_EOC; > + inst.value = CMDQ_EOC_IRQ_EN; > + err = cmdq_pkt_append_command(pkt, inst); > + if (err < 0) > + return err; > + > + /* JUMP to start of pkt */ > + err = cmdq_pkt_jump(pkt, pkt->pa_base); > + if (err < 0) > + return err; > + > + pkt->loop = true; > + > + return err; > +} > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); NAK. No users (and please carefully think before you answer that your other patch uses it). Best regards, Krzysztof
Hi Krzysztof, Thanks for the reviews. On Sat, 2023-09-23 at 20:08 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 18/09/2023 21:21, Jason-JH.Lin wrote: > > Add cmdq_pkt_finalize_loop to CMDQ driver. > > > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > > jump to start of command buffer instruction to make the command > > buffer loopable. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 > +++++++++++++++++++++++ > > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > > 2 files changed, 31 insertions(+) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index 4be2a18a4a02..bbb127620bb3 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > > } > > EXPORT_SYMBOL(cmdq_pkt_finalize); > > > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > > +{ > > +struct cmdq_instruction inst = { {0} }; > > +int err; > > + > > +/* insert EOC and generate IRQ for each command iteration */ > > +inst.op = CMDQ_CODE_EOC; > > +inst.value = CMDQ_EOC_IRQ_EN; > > +err = cmdq_pkt_append_command(pkt, inst); > > +if (err < 0) > > +return err; > > + > > +/* JUMP to start of pkt */ > > +err = cmdq_pkt_jump(pkt, pkt->pa_base); > > +if (err < 0) > > +return err; > > + > > +pkt->loop = true; > > + > > +return err; > > +} > > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); > > NAK. No users (and please carefully think before you answer that your > other patch uses it). > As I know, the API with EXPORT_SYMBOL means it can be used by a dynamic loaded module. Do you means that mtk-cmdq-sec-mailbox.c in [PATCH 10/15] is a built-in module currently, so I should not add EXPORT_SYMBOL to this API? Regards, Jason-JH.Lin > Best regards, > Krzysztof > >
On 25/09/2023 08:04, Jason-JH Lin (林睿祥) wrote: > Hi Krzysztof, > > Thanks for the reviews. > > On Sat, 2023-09-23 at 20:08 +0200, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 18/09/2023 21:21, Jason-JH.Lin wrote: >>> Add cmdq_pkt_finalize_loop to CMDQ driver. >>> >>> cmdq_pkt_finalize_loop appends end of command(EOC) instruction and >>> jump to start of command buffer instruction to make the command >>> buffer loopable. >>> >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> >>> --- >>> drivers/soc/mediatek/mtk-cmdq-helper.c | 23 >> +++++++++++++++++++++++ >>> include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ >>> 2 files changed, 31 insertions(+) >>> >>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c >> b/drivers/soc/mediatek/mtk-cmdq-helper.c >>> index 4be2a18a4a02..bbb127620bb3 100644 >>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c >>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c >>> @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) >>> } >>> EXPORT_SYMBOL(cmdq_pkt_finalize); >>> >>> +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) >>> +{ >>> +struct cmdq_instruction inst = { {0} }; >>> +int err; >>> + >>> +/* insert EOC and generate IRQ for each command iteration */ >>> +inst.op = CMDQ_CODE_EOC; >>> +inst.value = CMDQ_EOC_IRQ_EN; >>> +err = cmdq_pkt_append_command(pkt, inst); >>> +if (err < 0) >>> +return err; >>> + >>> +/* JUMP to start of pkt */ >>> +err = cmdq_pkt_jump(pkt, pkt->pa_base); >>> +if (err < 0) >>> +return err; >>> + >>> +pkt->loop = true; >>> + >>> +return err; >>> +} >>> +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); >> >> NAK. No users (and please carefully think before you answer that your >> other patch uses it). >> > > As I know, the API with EXPORT_SYMBOL means it can be used by a dynamic > loaded module. > > Do you means that mtk-cmdq-sec-mailbox.c in [PATCH 10/15] is a built-in > module currently, so I should not add EXPORT_SYMBOL to this API? I mean there is no need for this. Please name the name of module where this function will be defined and name of module(s) using it. ".c" is not a module. ".ko" is. Best regards, Krzysztof
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c index 4be2a18a4a02..bbb127620bb3 100644 --- a/drivers/soc/mediatek/mtk-cmdq-helper.c +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) } EXPORT_SYMBOL(cmdq_pkt_finalize); +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) +{ + struct cmdq_instruction inst = { {0} }; + int err; + + /* insert EOC and generate IRQ for each command iteration */ + inst.op = CMDQ_CODE_EOC; + inst.value = CMDQ_EOC_IRQ_EN; + err = cmdq_pkt_append_command(pkt, inst); + if (err < 0) + return err; + + /* JUMP to start of pkt */ + err = cmdq_pkt_jump(pkt, pkt->pa_base); + if (err < 0) + return err; + + pkt->loop = true; + + return err; +} +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); + int cmdq_pkt_flush_async(struct cmdq_pkt *pkt) { int err; diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h index 837ad8656adc..38a8e47da338 100644 --- a/include/linux/soc/mediatek/mtk-cmdq.h +++ b/include/linux/soc/mediatek/mtk-cmdq.h @@ -323,6 +323,14 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr); */ int cmdq_pkt_finalize(struct cmdq_pkt *pkt); +/** + * cmdq_pkt_finalize_loop() - Append EOC and jump to start command. + * @pkt: the CMDQ packet + * + * Return: 0 for success; else the error code is returned + */ +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt); + /** * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ * packet and call back at the end of done packet
Add cmdq_pkt_finalize_loop to CMDQ driver. cmdq_pkt_finalize_loop appends end of command(EOC) instruction and jump to start of command buffer instruction to make the command buffer loopable. Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> --- drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++ include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ 2 files changed, 31 insertions(+)