Re: [PATCH v14 6/6] soc: mediatek: mutex: add functions that operate registers by CMDQ

[moudy.ho <moudy.ho@mediatek.com>]

On Wed, 2022-04-13 at 10:41 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/04/22 09:24, Moudy Ho ha scritto:
> > Due to HW limitations, MDP3 is necessary to enable MUTEX in each
> > frame
> > for SOF triggering and cooperate with CMDQ control to reduce the
> > amount
> > of interrupts generated(also, reduce frame latency).
> > 
> > In response to the above situation, a new interface
> > "mtk_mutex_enable_by_cmdq" has been added to achieve the purpose.
> > 
> > Signed-off-by: Moudy Ho <moudy.ho@mediatek.com>
> > ---
> >   drivers/soc/mediatek/mtk-mutex.c       | 42
> > +++++++++++++++++++++++++-
> >   include/linux/soc/mediatek/mtk-mutex.h |  2 ++
> >   2 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-mutex.c
> > b/drivers/soc/mediatek/mtk-mutex.c
> > index fc9ba2749946..1811beaf399d 100644
> > --- a/drivers/soc/mediatek/mtk-mutex.c
> > +++ b/drivers/soc/mediatek/mtk-mutex.c
> > @@ -7,10 +7,12 @@
> >   #include <linux/iopoll.h>
> >   #include <linux/module.h>
> >   #include <linux/of_device.h>
> > +#include <linux/of_address.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/regmap.h>
> >   #include <linux/soc/mediatek/mtk-mmsys.h>
> >   #include <linux/soc/mediatek/mtk-mutex.h>
> > +#include <linux/soc/mediatek/mtk-cmdq.h>
> >   
> >   #define MT2701_MUTEX0_MOD0			0x2c
> >   #define MT2701_MUTEX0_SOF0			0x30
> > @@ -176,6 +178,9 @@ struct mtk_mutex_ctx {
> >   	void __iomem			*regs;
> >   	struct mtk_mutex		mutex[10];
> >   	const struct mtk_mutex_data	*data;
> > +	phys_addr_t			addr;
> > +	struct cmdq_client_reg		cmdq_reg;
> > +	bool				has_gce_client_reg;
> >   };
> >   
> >   static const unsigned int mt2701_mutex_mod[DDP_COMPONENT_ID_MAX]
> > = {
> > @@ -618,6 +623,28 @@ void mtk_mutex_enable(struct mtk_mutex *mutex)
> >   }
> >   EXPORT_SYMBOL_GPL(mtk_mutex_enable);
> >   
> > +void mtk_mutex_enable_by_cmdq(struct mtk_mutex *mutex, void *pkt)
> > +{
> > +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > +	struct mtk_mutex_ctx *mtx = container_of(mutex, struct
> > mtk_mutex_ctx,
> > +						 mutex[mutex->id]);
> > +	struct cmdq_pkt *cmdq_pkt = (struct cmdq_pkt *)pkt;
> > +
> > +	WARN_ON(&mtx->mutex[mutex->id] != mutex);
> > +
> > +	if (!mtx->has_gce_client_reg) {
> > +		dev_dbg(mtx->dev, "mediatek,gce-client-reg hasn't been
> > set in dts");
> > +		return;
> 
> Is this really supposed to happen?
> 
> Reiterating: when the gce-client-reg is not set, this function should
> either never
> be called from the principle, or it should actually fail.
> If a driver relies on mtk_mutex_enable_by_cmdq() and does *not* fall
> back to
> register write from cpu, then no change will occur at all, leading to
> a random
> breakage with no apparent reason.
> 
> This means that - for safety - this function should return -EINVAL
> when it gets
> called while no gce client reg is declared.
> Of course, this would also mean that the dev_dbg() should be a
> dev_err(), and
> that efforts should be done to avoid triggering this error by adding
> fallbacks
> in the drivers that will call this.
> 
> Another option would be to keep this function void, keep the print a
> dev_dbg(),
> but automatically fallback to mtk_mutex_enable(), so that here:
> 
> if (!mtx->has_gce_client_reg) {
> 	dev_dbg(mtx->dev,
> 		"No GCE client register found: falling back to cpu
> write.\n");
> 	mtk_mutex_enable(mutex);
> 	return;
> }
> 
> ...you're free to choose whichever of the two options, but this has
> to be fixed
> to remove this fragility.
> 
> > +	}
> > +
> > +	cmdq_pkt_write(cmdq_pkt, mtx->cmdq_reg.subsys,
> > +		       mtx->addr + DISP_REG_MUTEX_EN(mutex->id), 1);
> > +#else
> > +	dev_err(mtx->dev, "Not support for enable MUTEX by CMDQ");
> 
> ...obviously, if mtk_cmdq not reachable, instead of simply letting
> drivers break,
> you should, also here, fall back to the less performant and very
> suboptimal (for
> the specific case of mdp3 and some others) mtk_mutex_enable().
> 
> 
> Thanks,
> Angelo

Hi Angelo,

Thanks for your review.
Considering that the CPU writing flow already exists, the new CMDQ
method should be simplified to avoid confusion.
I will use the second method to directly return the function and print
an error message.
Users (currently only MDP3 needs to use CMDQ because it is single-mode
totally) should pay attention to this function.

Thanks,
Moudy


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek