| Message ID | 20240215004931.3808-3-chunkuang.hu@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove cl in struct cmdq_pkt | expand |
Il 15/02/24 01:49, Chun-Kuang Hu ha scritto: > In original design, cmdq_pkt_jump() call cmdq_get_shift_pa() every > time to get shift_pa. But the shift_pa is constant value for each > SoC, so client driver just need to call cmdq_get_shift_pa() once > and pass shift_pa to cmdq_pkt_jump() to prevent frequent function > call. > As far as I understand, the CMDQ supports both relative and absolute jumps, right? Here's my proposal: - Add a new function cmdq_pkt_jump_rel() or cmdq_pkt_jump_relative() * note: I prefer "rel", as maybe in a future we'll get a jump_abs function? :-) - Don't touch the cmdq_pkt_jump() function for one cycle - Migrate mediatek-drm to use cmdq_pkt_jump_rel() - Remove cmdq_pkt_jump() in the next cycle. What do you think? Regards, Angelo > Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org> > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 5 ++--- > include/linux/soc/mediatek/mtk-cmdq.h | 6 ++++-- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 67e17974d1e6..ed4ef95adf5b 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -348,14 +348,13 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value) > } > EXPORT_SYMBOL(cmdq_pkt_assign); > > -int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr) > +int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa) > { > struct cmdq_instruction inst = {}; > > inst.op = CMDQ_CODE_JUMP; > inst.offset = CMDQ_JUMP_RELATIVE; > - inst.value = addr >> > - cmdq_get_shift_pa(((struct cmdq_client *)pkt->cl)->chan); > + inst.value = addr >> shift_pa; > return cmdq_pkt_append_command(pkt, inst); > } > EXPORT_SYMBOL(cmdq_pkt_jump); > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h > index 6c42d817d368..6215191a328d 100644 > --- a/include/linux/soc/mediatek/mtk-cmdq.h > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > @@ -238,10 +238,12 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value); > * a physical address which should contains more instruction. > * @pkt: the CMDQ packet > * @addr: physical address of target instruction buffer > + * @shift_pa: shift bits of physical address in CMDQ instruction. This value > + * is got by cmdq_get_shift_pa(). > * > * Return: 0 for success; else the error code is returned > */ > -int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr); > +int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa); > > /** > * cmdq_pkt_finalize() - Append EOC and jump command to pkt. > @@ -339,7 +341,7 @@ static inline int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value) > return -EINVAL; > } > > -static inline int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr) > +static inline int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa) > { > return -EINVAL; > }
Hi, Angelo: On Thu, 2024-02-15 at 11:40 +0100, AngeloGioacchino Del Regno wrote: > Il 15/02/24 01:49, Chun-Kuang Hu ha scritto: > > In original design, cmdq_pkt_jump() call cmdq_get_shift_pa() every > > time to get shift_pa. But the shift_pa is constant value for each > > SoC, so client driver just need to call cmdq_get_shift_pa() once > > and pass shift_pa to cmdq_pkt_jump() to prevent frequent function > > call. > > > > As far as I understand, the CMDQ supports both relative and absolute > jumps, right? > > Here's my proposal: > - Add a new function cmdq_pkt_jump_rel() or > cmdq_pkt_jump_relative() > * note: I prefer "rel", as maybe in a future we'll get a jump_abs > function? :-) > - Don't touch the cmdq_pkt_jump() function for one cycle > - Migrate mediatek-drm to use cmdq_pkt_jump_rel() > - Remove cmdq_pkt_jump() in the next cycle. > > What do you think? Good idea. I would create cmdq_pkt_jump_abs() to replace cmdq_pkt_jump() and fix the wrong definition of CMDQ_JUMP_RELATIVE as: #define CMDQ_JUMP_RELATIVE 0 #define CMDQ_JUMP_ABSOLUTE 1 Regards, CK > > Regards, > Angelo > > > Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org> > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 5 ++--- > > include/linux/soc/mediatek/mtk-cmdq.h | 6 ++++-- > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > > b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index 67e17974d1e6..ed4ef95adf5b 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -348,14 +348,13 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 > > reg_idx, u32 value) > > } > > EXPORT_SYMBOL(cmdq_pkt_assign); > > > > -int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr) > > +int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 > > shift_pa) > > { > > struct cmdq_instruction inst = {}; > > > > inst.op = CMDQ_CODE_JUMP; > > inst.offset = CMDQ_JUMP_RELATIVE; > > - inst.value = addr >> > > - cmdq_get_shift_pa(((struct cmdq_client *)pkt->cl)- > > >chan); > > + inst.value = addr >> shift_pa; > > return cmdq_pkt_append_command(pkt, inst); > > } > > EXPORT_SYMBOL(cmdq_pkt_jump); > > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h > > b/include/linux/soc/mediatek/mtk-cmdq.h > > index 6c42d817d368..6215191a328d 100644 > > --- a/include/linux/soc/mediatek/mtk-cmdq.h > > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > > @@ -238,10 +238,12 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 > > reg_idx, u32 value); > > * a physical address which should contains > > more instruction. > > * @pkt: the CMDQ packet > > * @addr: physical address of target instruction buffer > > + * @shift_pa: shift bits of physical address in CMDQ > > instruction. This value > > + * is got by cmdq_get_shift_pa(). > > * > > * Return: 0 for success; else the error code is returned > > */ > > -int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr); > > +int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 > > shift_pa); > > > > /** > > * cmdq_pkt_finalize() - Append EOC and jump command to pkt. > > @@ -339,7 +341,7 @@ static inline int cmdq_pkt_assign(struct > > cmdq_pkt *pkt, u16 reg_idx, u32 value) > > return -EINVAL; > > } > > > > -static inline int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t > > addr) > > +static inline int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t > > addr, u8 shift_pa) > > { > > return -EINVAL; > > } > > > >
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c index 67e17974d1e6..ed4ef95adf5b 100644 --- a/drivers/soc/mediatek/mtk-cmdq-helper.c +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c @@ -348,14 +348,13 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value) } EXPORT_SYMBOL(cmdq_pkt_assign); -int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr) +int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa) { struct cmdq_instruction inst = {}; inst.op = CMDQ_CODE_JUMP; inst.offset = CMDQ_JUMP_RELATIVE; - inst.value = addr >> - cmdq_get_shift_pa(((struct cmdq_client *)pkt->cl)->chan); + inst.value = addr >> shift_pa; return cmdq_pkt_append_command(pkt, inst); } EXPORT_SYMBOL(cmdq_pkt_jump); diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h index 6c42d817d368..6215191a328d 100644 --- a/include/linux/soc/mediatek/mtk-cmdq.h +++ b/include/linux/soc/mediatek/mtk-cmdq.h @@ -238,10 +238,12 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value); * a physical address which should contains more instruction. * @pkt: the CMDQ packet * @addr: physical address of target instruction buffer + * @shift_pa: shift bits of physical address in CMDQ instruction. This value + * is got by cmdq_get_shift_pa(). * * Return: 0 for success; else the error code is returned */ -int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr); +int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa); /** * cmdq_pkt_finalize() - Append EOC and jump command to pkt. @@ -339,7 +341,7 @@ static inline int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value) return -EINVAL; } -static inline int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr) +static inline int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa) { return -EINVAL; }
In original design, cmdq_pkt_jump() call cmdq_get_shift_pa() every time to get shift_pa. But the shift_pa is constant value for each SoC, so client driver just need to call cmdq_get_shift_pa() once and pass shift_pa to cmdq_pkt_jump() to prevent frequent function call. Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org> --- drivers/soc/mediatek/mtk-cmdq-helper.c | 5 ++--- include/linux/soc/mediatek/mtk-cmdq.h | 6 ++++-- 2 files changed, 6 insertions(+), 5 deletions(-)