Message ID | 20250218054405.2017918-7-jason-jh.lin@mediatek.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | Add GCE support for MT8196 | expand |
Il 18/02/25 06:41, Jason-JH Lin ha scritto: > To support hardware without subsys IDs on new SoCs, add a programming > flow that checks whether the subsys ID is valid. If the subsys ID is > invalid, the flow will call 2 alternative CMDQ APIs: > cmdq_pkt_assign() and cmdq_pkt_write_s_value() to achieve the same > functionality. > > Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com> > --- > drivers/soc/mediatek/mtk-mmsys.c | 14 +++++++++++--- > drivers/soc/mediatek/mtk-mutex.c | 11 +++++++++-- > 2 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c > index bb4639ca0b8c..ce949b863b05 100644 > --- a/drivers/soc/mediatek/mtk-mmsys.c > +++ b/drivers/soc/mediatek/mtk-mmsys.c > @@ -167,9 +167,17 @@ static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, > u32 tmp; > > if (mmsys->cmdq_base.size && cmdq_pkt) { > - ret = cmdq_pkt_write_mask(cmdq_pkt, mmsys->cmdq_base.subsys, > - mmsys->cmdq_base.offset + offset, val, > - mask); > + offset += mmsys->cmdq_base.offset; > + if (mmsys->cmdq_base.subsys != CMDQ_SUBSYS_INVALID) { You're still anyway passing the subsys to cmdq_pkt_write_mask(), right?! Why don't you just handle this in cmdq_pkt_write_mask() then? ;-) I can see this pattern being repeated over and over in both drm/mediatek and MDP3 drivers, and it's not necessary to duplicate this many times when you can write it just once. Would've also been faster for you to implement... :-D Cheers, Angelo > + ret = cmdq_pkt_write_mask(cmdq_pkt, mmsys->cmdq_base.subsys, > + offset, val, mask); > + } else { > + /* only MMIO access, no need to check mminfro_offset */ > + ret = cmdq_pkt_assign(cmdq_pkt, CMDQ_THR_SPR_IDX0, > + CMDQ_ADDR_HIGH(mmsys->cmdq_base.pa_base)); > + ret |= cmdq_pkt_write_s_mask_value(cmdq_pkt, CMDQ_THR_SPR_IDX0, > + CMDQ_ADDR_LOW(offset), val, mask); > + } > if (ret) > pr_debug("CMDQ unavailable: using CPU write\n"); > else > diff --git a/drivers/soc/mediatek/mtk-mutex.c b/drivers/soc/mediatek/mtk-mutex.c > index 5250c1d702eb..3788b16efbf4 100644 > --- a/drivers/soc/mediatek/mtk-mutex.c > +++ b/drivers/soc/mediatek/mtk-mutex.c > @@ -963,6 +963,7 @@ int mtk_mutex_enable_by_cmdq(struct mtk_mutex *mutex, void *pkt) > struct mtk_mutex_ctx *mtx = container_of(mutex, struct mtk_mutex_ctx, > mutex[mutex->id]); > struct cmdq_pkt *cmdq_pkt = (struct cmdq_pkt *)pkt; > + dma_addr_t en_addr = mtx->addr + DISP_REG_MUTEX_EN(mutex->id); > > WARN_ON(&mtx->mutex[mutex->id] != mutex); > > @@ -971,8 +972,14 @@ int mtk_mutex_enable_by_cmdq(struct mtk_mutex *mutex, void *pkt) > return -ENODEV; > } > > - cmdq_pkt_write(cmdq_pkt, mtx->cmdq_reg.subsys, > - mtx->addr + DISP_REG_MUTEX_EN(mutex->id), 1); > + if (mtx->cmdq_reg.subsys != CMDQ_SUBSYS_INVALID) { > + cmdq_pkt_write(cmdq_pkt, mtx->cmdq_reg.subsys, en_addr, 1); > + } else { > + /* only MMIO access, no need to check mminfro_offset */ > + cmdq_pkt_assign(cmdq_pkt, CMDQ_THR_SPR_IDX0, CMDQ_ADDR_HIGH(en_addr)); > + cmdq_pkt_write_s_value(cmdq_pkt, CMDQ_THR_SPR_IDX0, CMDQ_ADDR_LOW(en_addr), 1); > + } > + > return 0; > } > EXPORT_SYMBOL_GPL(mtk_mutex_enable_by_cmdq);
On Tue, 2025-03-04 at 10:41 +0100, AngeloGioacchino Del Regno wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Il 18/02/25 06:41, Jason-JH Lin ha scritto: > > To support hardware without subsys IDs on new SoCs, add a > > programming > > flow that checks whether the subsys ID is valid. If the subsys ID > > is > > invalid, the flow will call 2 alternative CMDQ APIs: > > cmdq_pkt_assign() and cmdq_pkt_write_s_value() to achieve the same > > functionality. > > > > Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/soc/mediatek/mtk-mmsys.c | 14 +++++++++++--- > > drivers/soc/mediatek/mtk-mutex.c | 11 +++++++++-- > > 2 files changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c > > b/drivers/soc/mediatek/mtk-mmsys.c > > index bb4639ca0b8c..ce949b863b05 100644 > > --- a/drivers/soc/mediatek/mtk-mmsys.c > > +++ b/drivers/soc/mediatek/mtk-mmsys.c > > @@ -167,9 +167,17 @@ static void mtk_mmsys_update_bits(struct > > mtk_mmsys *mmsys, u32 offset, u32 mask, > > u32 tmp; > > > > if (mmsys->cmdq_base.size && cmdq_pkt) { > > - ret = cmdq_pkt_write_mask(cmdq_pkt, mmsys- > > >cmdq_base.subsys, > > - mmsys->cmdq_base.offset + > > offset, val, > > - mask); > > + offset += mmsys->cmdq_base.offset; > > + if (mmsys->cmdq_base.subsys != CMDQ_SUBSYS_INVALID) { > > You're still anyway passing the subsys to cmdq_pkt_write_mask(), > right?! > Why don't you just handle this in cmdq_pkt_write_mask() then? ;-) > > I can see this pattern being repeated over and over in both > drm/mediatek and MDP3 > drivers, and it's not necessary to duplicate this many times when you > can write it > just once. > > Would've also been faster for you to implement... :-D > I think did it in the series V1: https://patchwork.kernel.org/project/linux-mediatek/patch/20241121042602.32730-5-jason-jh.lin@mediatek.com/ Because it'll need to passing the base_pa and that will need to change the interface for original APIs. And CK think that's not a necessary to change the APIs. It can be done by cmdq_pkt_assign() + cmdq_pkt_write_s_mask_value() in the client drivers. Then you can see this pattern in everywhere. :-) Regards, Jason-JH Lin > Cheers, > Angelo
Il 05/03/25 17:12, Jason-JH Lin (林睿祥) ha scritto: > On Tue, 2025-03-04 at 10:41 +0100, AngeloGioacchino Del Regno wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> >> >> Il 18/02/25 06:41, Jason-JH Lin ha scritto: >>> To support hardware without subsys IDs on new SoCs, add a >>> programming >>> flow that checks whether the subsys ID is valid. If the subsys ID >>> is >>> invalid, the flow will call 2 alternative CMDQ APIs: >>> cmdq_pkt_assign() and cmdq_pkt_write_s_value() to achieve the same >>> functionality. >>> >>> Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com> >>> --- >>> drivers/soc/mediatek/mtk-mmsys.c | 14 +++++++++++--- >>> drivers/soc/mediatek/mtk-mutex.c | 11 +++++++++-- >>> 2 files changed, 20 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c >>> b/drivers/soc/mediatek/mtk-mmsys.c >>> index bb4639ca0b8c..ce949b863b05 100644 >>> --- a/drivers/soc/mediatek/mtk-mmsys.c >>> +++ b/drivers/soc/mediatek/mtk-mmsys.c >>> @@ -167,9 +167,17 @@ static void mtk_mmsys_update_bits(struct >>> mtk_mmsys *mmsys, u32 offset, u32 mask, >>> u32 tmp; >>> >>> if (mmsys->cmdq_base.size && cmdq_pkt) { >>> - ret = cmdq_pkt_write_mask(cmdq_pkt, mmsys- >>>> cmdq_base.subsys, >>> - mmsys->cmdq_base.offset + >>> offset, val, >>> - mask); >>> + offset += mmsys->cmdq_base.offset; >>> + if (mmsys->cmdq_base.subsys != CMDQ_SUBSYS_INVALID) { >> >> You're still anyway passing the subsys to cmdq_pkt_write_mask(), >> right?! >> Why don't you just handle this in cmdq_pkt_write_mask() then? ;-) >> >> I can see this pattern being repeated over and over in both >> drm/mediatek and MDP3 >> drivers, and it's not necessary to duplicate this many times when you >> can write it >> just once. >> >> Would've also been faster for you to implement... :-D >> > > I think did it in the series V1: > https://patchwork.kernel.org/project/linux-mediatek/patch/20241121042602.32730-5-jason-jh.lin@mediatek.com/ > > Because it'll need to passing the base_pa and that will need to change > the interface for original APIs. > > And CK think that's not a necessary to change the APIs. It can be done > by cmdq_pkt_assign() + cmdq_pkt_write_s_mask_value() in the client > drivers. Then you can see this pattern in everywhere. :-) > Using likely(x) and unlikely(x) should be avoided, really, unless it's something that is really really really really ... really ... rea.... likely or unlikely :-) Btw. Changing the APIs is a bit difficult, but I disagree with CK about not "inventing" a new API for the unsupported-subsys flow. It's true, it is not *strictly* needed to add a function, but it's good for any kind of future maintainability - as I explained, it's easier then to fix a problem if there's one.... and well, I can see that you agree with me, because effectively you did it the first time :-) CK mentioned using cmdq_pkt_write() *or* cmdq_pkt_assignwrite/cmdq_pkt_write_pa() (however you wanna call it, it's fine for me), in drivers that know that there always is or there always isn't a subsys ID: that's a good suggestion, as this can be eventually done with assigning a function pointer, so, no conditionals at each operation. My point of view, finally, is: - This is just another way of doing cmdq_pkt_write() - This, at the end of the day, does exactly what cmdq_pkt_write() is doing, except it's doing it with two instructions instead of one; - The same thing can be done in two different ways (depending on SoC) - This same thing should have a function that does it. A function that does it can be int cmdq_pkt_write_pa(struct cmdq_pkt *pkt, u8 subsys /*unused*/, u32 pa_base, u16 offset, u32 value) { err = cmdq_pkt_assign(pkt, 0, CMDQ_ADDR_HIGH(pa_base)); if (err < 0) return err; return cmdq_pkt_write_s_value( .... etc) } int cmdq_pkt_write() <--- unchanged, scheduled for removal after all drivers migrated int cmdq_pkt_write_subsys(struct cmdq_pkt *pkt, u8 subsys, u32 pa_base /*unused*/, u16 offset, u32 value) { /* This function will get the contents of cmdq_pkt_write once removed, but, in the meanwhile, to avoid duplication we just call that: */ return cmdq_pkt_write(pkt, subsys, offset, value); } - Are we adding one more function parameter? Yes - Is this impacting performance overall? Not really After all, we're living in an ARMv8 (actually, ARMv9 for new ones) world, so one more function param won't hurt anyone. I think that's the best of both worlds, and makes everyone happy. Are you happy with that? :-) Cheers, Angelo
On Wed, 2025-03-05 at 19:08 +0100, AngeloGioacchino Del Regno wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Il 05/03/25 17:12, Jason-JH Lin (林睿祥) ha scritto: > > On Tue, 2025-03-04 at 10:41 +0100, AngeloGioacchino Del Regno > > wrote: > > > > > > External email : Please do not click links or open attachments > > > until > > > you have verified the sender or the content. > > > > > > > > > Il 18/02/25 06:41, Jason-JH Lin ha scritto: > > > > To support hardware without subsys IDs on new SoCs, add a > > > > programming > > > > flow that checks whether the subsys ID is valid. If the subsys > > > > ID > > > > is > > > > invalid, the flow will call 2 alternative CMDQ APIs: > > > > cmdq_pkt_assign() and cmdq_pkt_write_s_value() to achieve the > > > > same > > > > functionality. > > > > > > > > Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com> > > > > --- > > > > drivers/soc/mediatek/mtk-mmsys.c | 14 +++++++++++--- > > > > drivers/soc/mediatek/mtk-mutex.c | 11 +++++++++-- > > > > 2 files changed, 20 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c > > > > b/drivers/soc/mediatek/mtk-mmsys.c > > > > index bb4639ca0b8c..ce949b863b05 100644 > > > > --- a/drivers/soc/mediatek/mtk-mmsys.c > > > > +++ b/drivers/soc/mediatek/mtk-mmsys.c > > > > @@ -167,9 +167,17 @@ static void mtk_mmsys_update_bits(struct > > > > mtk_mmsys *mmsys, u32 offset, u32 mask, > > > > u32 tmp; > > > > > > > > if (mmsys->cmdq_base.size && cmdq_pkt) { > > > > - ret = cmdq_pkt_write_mask(cmdq_pkt, mmsys- > > > > > cmdq_base.subsys, > > > > - mmsys->cmdq_base.offset > > > > + > > > > offset, val, > > > > - mask); > > > > + offset += mmsys->cmdq_base.offset; > > > > + if (mmsys->cmdq_base.subsys != > > > > CMDQ_SUBSYS_INVALID) { > > > > > > You're still anyway passing the subsys to cmdq_pkt_write_mask(), > > > right?! > > > Why don't you just handle this in cmdq_pkt_write_mask() then? ;-) > > > > > > I can see this pattern being repeated over and over in both > > > drm/mediatek and MDP3 > > > drivers, and it's not necessary to duplicate this many times when > > > you > > > can write it > > > just once. > > > > > > Would've also been faster for you to implement... :-D > > > > > > > I think did it in the series V1: > > https://patchwork.kernel.org/project/linux-mediatek/patch/20241121042602.32730-5-jason-jh.lin@mediatek.com > > > > Because it'll need to passing the base_pa and that will need to > > change > > the interface for original APIs. > > > > And CK think that's not a necessary to change the APIs. It can be > > done > > by cmdq_pkt_assign() + cmdq_pkt_write_s_mask_value() in the client > > drivers. Then you can see this pattern in everywhere. :-) > > > > Using likely(x) and unlikely(x) should be avoided, really, unless > it's something > that is really really really really ... really ... rea.... likely or > unlikely :-) > > Btw. Changing the APIs is a bit difficult, but I disagree with CK > about not > "inventing" a new API for the unsupported-subsys flow. > > It's true, it is not *strictly* needed to add a function, but it's > good for any > kind of future maintainability - as I explained, it's easier then to > fix a problem > if there's one.... and well, I can see that you agree with me, > because effectively > you did it the first time :-) > > CK mentioned using cmdq_pkt_write() *or* > cmdq_pkt_assignwrite/cmdq_pkt_write_pa() > (however you wanna call it, it's fine for me), in drivers that know > that there > always is or there always isn't a subsys ID: that's a good > suggestion, as this can > be eventually done with assigning a function pointer, so, no > conditionals at each > operation. > > My point of view, finally, is: > - This is just another way of doing cmdq_pkt_write() > - This, at the end of the day, does exactly what cmdq_pkt_write() > is doing, > except it's doing it with two instructions instead of one; > - The same thing can be done in two different ways (depending on > SoC) > - This same thing should have a function that does it. > > A function that does it can be > > int cmdq_pkt_write_pa(struct cmdq_pkt *pkt, u8 subsys /*unused*/, u32 > pa_base, u16 > offset, u32 value) > { > err = cmdq_pkt_assign(pkt, 0, CMDQ_ADDR_HIGH(pa_base)); > if (err < 0) > return err; > > return cmdq_pkt_write_s_value( .... etc) > } > > int cmdq_pkt_write() <--- unchanged, scheduled for removal after all > drivers migrated > > int cmdq_pkt_write_subsys(struct cmdq_pkt *pkt, u8 subsys, u32 > pa_base /*unused*/, > u16 offset, u32 value) > { > /* This function will get the contents of cmdq_pkt_write once > removed, > but, in the meanwhile, to avoid duplication we just call > that: */ > > return cmdq_pkt_write(pkt, subsys, offset, value); > } > > - Are we adding one more function parameter? Yes > - Is this impacting performance overall? Not really > > After all, we're living in an ARMv8 (actually, ARMv9 for new ones) > world, so > one more function param won't hurt anyone. > > I think that's the best of both worlds, and makes everyone happy. > Are you happy with that? :-) > Yes, I am happy with that. :-) And thanks for your detail coding. I'll change it in the next version. regards, Jason-JH Lin > Cheers, > Angelo >
diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c index bb4639ca0b8c..ce949b863b05 100644 --- a/drivers/soc/mediatek/mtk-mmsys.c +++ b/drivers/soc/mediatek/mtk-mmsys.c @@ -167,9 +167,17 @@ static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 tmp; if (mmsys->cmdq_base.size && cmdq_pkt) { - ret = cmdq_pkt_write_mask(cmdq_pkt, mmsys->cmdq_base.subsys, - mmsys->cmdq_base.offset + offset, val, - mask); + offset += mmsys->cmdq_base.offset; + if (mmsys->cmdq_base.subsys != CMDQ_SUBSYS_INVALID) { + ret = cmdq_pkt_write_mask(cmdq_pkt, mmsys->cmdq_base.subsys, + offset, val, mask); + } else { + /* only MMIO access, no need to check mminfro_offset */ + ret = cmdq_pkt_assign(cmdq_pkt, CMDQ_THR_SPR_IDX0, + CMDQ_ADDR_HIGH(mmsys->cmdq_base.pa_base)); + ret |= cmdq_pkt_write_s_mask_value(cmdq_pkt, CMDQ_THR_SPR_IDX0, + CMDQ_ADDR_LOW(offset), val, mask); + } if (ret) pr_debug("CMDQ unavailable: using CPU write\n"); else diff --git a/drivers/soc/mediatek/mtk-mutex.c b/drivers/soc/mediatek/mtk-mutex.c index 5250c1d702eb..3788b16efbf4 100644 --- a/drivers/soc/mediatek/mtk-mutex.c +++ b/drivers/soc/mediatek/mtk-mutex.c @@ -963,6 +963,7 @@ int mtk_mutex_enable_by_cmdq(struct mtk_mutex *mutex, void *pkt) struct mtk_mutex_ctx *mtx = container_of(mutex, struct mtk_mutex_ctx, mutex[mutex->id]); struct cmdq_pkt *cmdq_pkt = (struct cmdq_pkt *)pkt; + dma_addr_t en_addr = mtx->addr + DISP_REG_MUTEX_EN(mutex->id); WARN_ON(&mtx->mutex[mutex->id] != mutex); @@ -971,8 +972,14 @@ int mtk_mutex_enable_by_cmdq(struct mtk_mutex *mutex, void *pkt) return -ENODEV; } - cmdq_pkt_write(cmdq_pkt, mtx->cmdq_reg.subsys, - mtx->addr + DISP_REG_MUTEX_EN(mutex->id), 1); + if (mtx->cmdq_reg.subsys != CMDQ_SUBSYS_INVALID) { + cmdq_pkt_write(cmdq_pkt, mtx->cmdq_reg.subsys, en_addr, 1); + } else { + /* only MMIO access, no need to check mminfro_offset */ + cmdq_pkt_assign(cmdq_pkt, CMDQ_THR_SPR_IDX0, CMDQ_ADDR_HIGH(en_addr)); + cmdq_pkt_write_s_value(cmdq_pkt, CMDQ_THR_SPR_IDX0, CMDQ_ADDR_LOW(en_addr), 1); + } + return 0; } EXPORT_SYMBOL_GPL(mtk_mutex_enable_by_cmdq);
To support hardware without subsys IDs on new SoCs, add a programming flow that checks whether the subsys ID is valid. If the subsys ID is invalid, the flow will call 2 alternative CMDQ APIs: cmdq_pkt_assign() and cmdq_pkt_write_s_value() to achieve the same functionality. Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com> --- drivers/soc/mediatek/mtk-mmsys.c | 14 +++++++++++--- drivers/soc/mediatek/mtk-mutex.c | 11 +++++++++-- 2 files changed, 20 insertions(+), 5 deletions(-)