Re: [RESEND, PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr() function

[Jason-JH Lin (林睿祥)]

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@xxxxxxxxxxxx>
> > > > > > ---
> > > > > >  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
> > >