[PATCH 0/9] Remove cl in struct cmdq_pkt

[PATCH 0/9] Remove cl in struct cmdq_pkt

[Chun-Kuang Hu]

cl in struct cmdq_pkt is used to store struct cmdq_client, but every client
driver has the struct cmdq_client information, so it's not necessary to
store struct cmdq_client in struct cmdq_pkt. Because mailbox maintainer
do not like to mix mailbox patch with other patches in a series, so
mailbox patch [1] would be sent independently.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux.git/commit/?h=mediatek-cmdq8&id=a1b2f7a7488285975c1f439086f1c4cc51a13bb9

Chun-Kuang Hu (9):
  soc: mediatek: cmdq: Remove unused helper funciton
  soc: mediatek: cmdq: Add parameter shift_pa to cmdq_pkt_jump()
  soc: mediatek: cmdq: Add cmdq_pkt_eoc() helper function
  soc: mediatek: cmdq: Add cmdq_pkt_nop() helper function
  drm/mediatek: Drop calling cmdq_pkt_finalize()
  media: platform: mtk-mdp3: drop calling cmdq_pkt_finalize()
  soc: mediatek: cmdq: Remove cmdq_pkt_finalize() helper function
  drm/mediatek: Do not store struct cmdq_client in struct cmdq_pkt
  media: platform: mtk-mdp3: do not store struct cmdq_client in struct
    cmdq_pkt

 drivers/gpu/drm/mediatek/mtk_drm_crtc.c       |  9 +-
 .../platform/mediatek/mdp3/mtk-mdp3-cmdq.c    | 14 ++-
 .../platform/mediatek/mdp3/mtk-mdp3-core.c    |  2 +
 .../platform/mediatek/mdp3/mtk-mdp3-core.h    |  1 +
 drivers/soc/mediatek/mtk-cmdq-helper.c        | 85 +++----------------
 include/linux/soc/mediatek/mtk-cmdq.h         | 50 ++++-------
 6 files changed, 39 insertions(+), 122 deletions(-)

-- 
2.34.1

[PATCH 1/9] soc: mediatek: cmdq: Remove unused helper funciton

[Chun-Kuang Hu]

cmdq_pkt_create(), cmdq_pkt_destroy(), and cmdq_pkt_flush_async()
are not used by all client drivers (MediaTek drm driver and
MediaTek mdp3 driver), so remove them.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 59 --------------------------
 include/linux/soc/mediatek/mtk-cmdq.h  | 40 -----------------
 2 files changed, 99 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index b0cd071c4719..67e17974d1e6 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -105,50 +105,6 @@ void cmdq_mbox_destroy(struct cmdq_client *client)
 }
 EXPORT_SYMBOL(cmdq_mbox_destroy);
 
-struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size)
-{
-	struct cmdq_pkt *pkt;
-	struct device *dev;
-	dma_addr_t dma_addr;
-
-	pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
-	if (!pkt)
-		return ERR_PTR(-ENOMEM);
-	pkt->va_base = kzalloc(size, GFP_KERNEL);
-	if (!pkt->va_base) {
-		kfree(pkt);
-		return ERR_PTR(-ENOMEM);
-	}
-	pkt->buf_size = size;
-	pkt->cl = (void *)client;
-
-	dev = client->chan->mbox->dev;
-	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
-				  DMA_TO_DEVICE);
-	if (dma_mapping_error(dev, dma_addr)) {
-		dev_err(dev, "dma map failed, size=%u\n", (u32)(u64)size);
-		kfree(pkt->va_base);
-		kfree(pkt);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	pkt->pa_base = dma_addr;
-
-	return pkt;
-}
-EXPORT_SYMBOL(cmdq_pkt_create);
-
-void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
-{
-	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
-
-	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
-			 DMA_TO_DEVICE);
-	kfree(pkt->va_base);
-	kfree(pkt);
-}
-EXPORT_SYMBOL(cmdq_pkt_destroy);
-
 static int cmdq_pkt_append_command(struct cmdq_pkt *pkt,
 				   struct cmdq_instruction inst)
 {
@@ -426,19 +382,4 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 }
 EXPORT_SYMBOL(cmdq_pkt_finalize);
 
-int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
-{
-	int err;
-	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
-
-	err = mbox_send_message(client->chan, pkt);
-	if (err < 0)
-		return err;
-	/* We can send next packet immediately, so just call txdone. */
-	mbox_client_txdone(client->chan, 0);
-
-	return 0;
-}
-EXPORT_SYMBOL(cmdq_pkt_flush_async);
-
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 649955d2cf5c..6c42d817d368 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -59,21 +59,6 @@ struct cmdq_client *cmdq_mbox_create(struct device *dev, int index);
  */
 void cmdq_mbox_destroy(struct cmdq_client *client);
 
-/**
- * cmdq_pkt_create() - create a CMDQ packet
- * @client:	the CMDQ mailbox client
- * @size:	required CMDQ buffer size
- *
- * Return: CMDQ packet pointer
- */
-struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size);
-
-/**
- * cmdq_pkt_destroy() - destroy the CMDQ packet
- * @pkt:	the CMDQ packet
- */
-void cmdq_pkt_destroy(struct cmdq_pkt *pkt);
-
 /**
  * cmdq_pkt_write() - append write command to the CMDQ packet
  * @pkt:	the CMDQ packet
@@ -266,19 +251,6 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr);
  */
 int cmdq_pkt_finalize(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
- * @pkt:	the CMDQ packet
- *
- * Return: 0 for success; else the error code is returned
- *
- * Trigger CMDQ to asynchronously execute the CMDQ packet and call back
- * at the end of done packet. Note that this is an ASYNC function. When the
- * function returned, it may or may not be finished.
- */
-int cmdq_pkt_flush_async(struct cmdq_pkt *pkt);
-
 #else /* IS_ENABLED(CONFIG_MTK_CMDQ) */
 
 static inline int cmdq_dev_get_client_reg(struct device *dev,
@@ -294,13 +266,6 @@ static inline struct cmdq_client *cmdq_mbox_create(struct device *dev, int index
 
 static inline void cmdq_mbox_destroy(struct cmdq_client *client) { }
 
-static inline  struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size)
-{
-	return ERR_PTR(-EINVAL);
-}
-
-static inline void cmdq_pkt_destroy(struct cmdq_pkt *pkt) { }
-
 static inline int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value)
 {
 	return -ENOENT;
@@ -384,11 +349,6 @@ static inline int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 	return -EINVAL;
 }
 
-static inline int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
-{
-	return -EINVAL;
-}
-
 #endif /* IS_ENABLED(CONFIG_MTK_CMDQ) */
 
 #endif	/* __MTK_CMDQ_H__ */
-- 
2.34.1

Re: [PATCH 1/9] soc: mediatek: cmdq: Remove unused helper funciton

[AngeloGioacchino Del Regno]

Il 15/02/24 01:49, Chun-Kuang Hu ha scritto:
> cmdq_pkt_create(), cmdq_pkt_destroy(), and cmdq_pkt_flush_async()
> are not used by all client drivers (MediaTek drm driver and
> MediaTek mdp3 driver),
> 

Hello CK,

We can technically force the hand to say that this is true - but only because
these two functions are copy-pasted in both mediatek-drm and MDP3 drivers with
no meaningful changes, as in, the only change is that `pkt` is supposed to be
preallocated in both of the variants, while the one in mtk-cmdq-helper allocates
it on its own.

Code duplication is something that we want to avoid, not something that we want
to embrace: removing those functions from cmdq-helper with the plan to keep
duplicating them in each MediaTek driver that uses CMDQ packets is plain wrong.

This - especially because I'm sure that we will see yet another copy-paste of
those two functions in a future ISP driver, bringing the duplication count to
3 (or actually, 3 by 2 functions = 6 times).

On the other hand, removing the cmdq_pkt_flush_async() function is something
that I *do* support, as it's only doing two simple calls that are not even
specific to cmdq, but more like "generic stuff".

In short, as it is right now, this is a NACK - but if you change this commit to
remove only cmdq_pkt_flush_async() I would agree.

The right thing to do is to remove the duplicated functions:
  - mtk_drm_cmdq_pkt_create()
  - mtk_drm_cmdq_pkt_destroy()
  - mdp_cmdq_pkt_create()
  - mdp_cmdq_pkt_destroy()

...and migrate both drivers to use the common cmdq helper code instea, but that's
something that can come later.

For now, you can simply perform the ->cl removal on all duplicated functions, then
we can migrate them all to the common helper, removing duplication all along.

Regards,
Angelo

> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> ---
>   drivers/soc/mediatek/mtk-cmdq-helper.c | 59 --------------------------
>   include/linux/soc/mediatek/mtk-cmdq.h  | 40 -----------------
>   2 files changed, 99 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index b0cd071c4719..67e17974d1e6 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -105,50 +105,6 @@ void cmdq_mbox_destroy(struct cmdq_client *client)
>   }
>   EXPORT_SYMBOL(cmdq_mbox_destroy);
>   
> -struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size)
> -{
> -	struct cmdq_pkt *pkt;
> -	struct device *dev;
> -	dma_addr_t dma_addr;
> -
> -	pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> -	if (!pkt)
> -		return ERR_PTR(-ENOMEM);
> -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> -	if (!pkt->va_base) {
> -		kfree(pkt);
> -		return ERR_PTR(-ENOMEM);
> -	}
> -	pkt->buf_size = size;
> -	pkt->cl = (void *)client;
> -
> -	dev = client->chan->mbox->dev;
> -	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
> -				  DMA_TO_DEVICE);
> -	if (dma_mapping_error(dev, dma_addr)) {
> -		dev_err(dev, "dma map failed, size=%u\n", (u32)(u64)size);
> -		kfree(pkt->va_base);
> -		kfree(pkt);
> -		return ERR_PTR(-ENOMEM);
> -	}
> -
> -	pkt->pa_base = dma_addr;
> -
> -	return pkt;
> -}
> -EXPORT_SYMBOL(cmdq_pkt_create);
> -
> -void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
> -{
> -	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> -
> -	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
> -			 DMA_TO_DEVICE);
> -	kfree(pkt->va_base);
> -	kfree(pkt);
> -}
> -EXPORT_SYMBOL(cmdq_pkt_destroy);
> -
>   static int cmdq_pkt_append_command(struct cmdq_pkt *pkt,
>   				   struct cmdq_instruction inst)
>   {
> @@ -426,19 +382,4 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>   }
>   EXPORT_SYMBOL(cmdq_pkt_finalize);
>   
> -int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
> -{
> -	int err;
> -	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> -
> -	err = mbox_send_message(client->chan, pkt);
> -	if (err < 0)
> -		return err;
> -	/* We can send next packet immediately, so just call txdone. */
> -	mbox_client_txdone(client->chan, 0);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(cmdq_pkt_flush_async);
> -
>   MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index 649955d2cf5c..6c42d817d368 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -59,21 +59,6 @@ struct cmdq_client *cmdq_mbox_create(struct device *dev, int index);
>    */
>   void cmdq_mbox_destroy(struct cmdq_client *client);
>   
> -/**
> - * cmdq_pkt_create() - create a CMDQ packet
> - * @client:	the CMDQ mailbox client
> - * @size:	required CMDQ buffer size
> - *
> - * Return: CMDQ packet pointer
> - */
> -struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size);
> -
> -/**
> - * cmdq_pkt_destroy() - destroy the CMDQ packet
> - * @pkt:	the CMDQ packet
> - */
> -void cmdq_pkt_destroy(struct cmdq_pkt *pkt);
> -
>   /**
>    * cmdq_pkt_write() - append write command to the CMDQ packet
>    * @pkt:	the CMDQ packet
> @@ -266,19 +251,6 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr);
>    */
>   int cmdq_pkt_finalize(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
> - * @pkt:	the CMDQ packet
> - *
> - * Return: 0 for success; else the error code is returned
> - *
> - * Trigger CMDQ to asynchronously execute the CMDQ packet and call back
> - * at the end of done packet. Note that this is an ASYNC function. When the
> - * function returned, it may or may not be finished.
> - */
> -int cmdq_pkt_flush_async(struct cmdq_pkt *pkt);
> -
>   #else /* IS_ENABLED(CONFIG_MTK_CMDQ) */
>   
>   static inline int cmdq_dev_get_client_reg(struct device *dev,
> @@ -294,13 +266,6 @@ static inline struct cmdq_client *cmdq_mbox_create(struct device *dev, int index
>   
>   static inline void cmdq_mbox_destroy(struct cmdq_client *client) { }
>   
> -static inline  struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size)
> -{
> -	return ERR_PTR(-EINVAL);
> -}
> -
> -static inline void cmdq_pkt_destroy(struct cmdq_pkt *pkt) { }
> -
>   static inline int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value)
>   {
>   	return -ENOENT;
> @@ -384,11 +349,6 @@ static inline int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>   	return -EINVAL;
>   }
>   
> -static inline int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
> -{
> -	return -EINVAL;
> -}
> -
>   #endif /* IS_ENABLED(CONFIG_MTK_CMDQ) */
>   
>   #endif	/* __MTK_CMDQ_H__ */

Re: [PATCH 1/9] soc: mediatek: cmdq: Remove unused helper funciton

[CK Hu (胡俊光)]

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:
> > cmdq_pkt_create(), cmdq_pkt_destroy(), and cmdq_pkt_flush_async()
> > are not used by all client drivers (MediaTek drm driver and
> > MediaTek mdp3 driver),
> > 
> 
> Hello CK,
> 
> We can technically force the hand to say that this is true - but only
> because
> these two functions are copy-pasted in both mediatek-drm and MDP3
> drivers with
> no meaningful changes, as in, the only change is that `pkt` is
> supposed to be
> preallocated in both of the variants, while the one in mtk-cmdq-
> helper allocates
> it on its own.
> 
> Code duplication is something that we want to avoid, not something
> that we want
> to embrace: removing those functions from cmdq-helper with the plan
> to keep
> duplicating them in each MediaTek driver that uses CMDQ packets is
> plain wrong.
> 
> This - especially because I'm sure that we will see yet another copy-
> paste of
> those two functions in a future ISP driver, bringing the duplication
> count to
> 3 (or actually, 3 by 2 functions = 6 times).
> 
> On the other hand, removing the cmdq_pkt_flush_async() function is
> something
> that I *do* support, as it's only doing two simple calls that are not
> even
> specific to cmdq, but more like "generic stuff".
> 
> In short, as it is right now, this is a NACK - but if you change this
> commit to
> remove only cmdq_pkt_flush_async() I would agree.
> 
> The right thing to do is to remove the duplicated functions:
>   - mtk_drm_cmdq_pkt_create()
>   - mtk_drm_cmdq_pkt_destroy()
>   - mdp_cmdq_pkt_create()
>   - mdp_cmdq_pkt_destroy()
> 
> ...and migrate both drivers to use the common cmdq helper code
> instea, but that's
> something that can come later.
> 
> For now, you can simply perform the ->cl removal on all duplicated
> functions, then
> we can migrate them all to the common helper, removing duplication
> all along.

OK, I would refine cmdq_pkt_create()/cmdq_pkt_destroy() and let client
driver use these common functions.

Regards,
CK

> 
> Regards,
> Angelo
> 
> > Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> > ---
> >   drivers/soc/mediatek/mtk-cmdq-helper.c | 59 -------------------
> > -------
> >   include/linux/soc/mediatek/mtk-cmdq.h  | 40 -----------------
> >   2 files changed, 99 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index b0cd071c4719..67e17974d1e6 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -105,50 +105,6 @@ void cmdq_mbox_destroy(struct cmdq_client
> > *client)
> >   }
> >   EXPORT_SYMBOL(cmdq_mbox_destroy);
> >   
> > -struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client,
> > size_t size)
> > -{
> > -	struct cmdq_pkt *pkt;
> > -	struct device *dev;
> > -	dma_addr_t dma_addr;
> > -
> > -	pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> > -	if (!pkt)
> > -		return ERR_PTR(-ENOMEM);
> > -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> > -	if (!pkt->va_base) {
> > -		kfree(pkt);
> > -		return ERR_PTR(-ENOMEM);
> > -	}
> > -	pkt->buf_size = size;
> > -	pkt->cl = (void *)client;
> > -
> > -	dev = client->chan->mbox->dev;
> > -	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
> > -				  DMA_TO_DEVICE);
> > -	if (dma_mapping_error(dev, dma_addr)) {
> > -		dev_err(dev, "dma map failed, size=%u\n",
> > (u32)(u64)size);
> > -		kfree(pkt->va_base);
> > -		kfree(pkt);
> > -		return ERR_PTR(-ENOMEM);
> > -	}
> > -
> > -	pkt->pa_base = dma_addr;
> > -
> > -	return pkt;
> > -}
> > -EXPORT_SYMBOL(cmdq_pkt_create);
> > -
> > -void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
> > -{
> > -	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> > -
> > -	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt-
> > >buf_size,
> > -			 DMA_TO_DEVICE);
> > -	kfree(pkt->va_base);
> > -	kfree(pkt);
> > -}
> > -EXPORT_SYMBOL(cmdq_pkt_destroy);
> > -
> >   static int cmdq_pkt_append_command(struct cmdq_pkt *pkt,
> >   				   struct cmdq_instruction inst)
> >   {
> > @@ -426,19 +382,4 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >   }
> >   EXPORT_SYMBOL(cmdq_pkt_finalize);
> >   
> > -int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
> > -{
> > -	int err;
> > -	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> > -
> > -	err = mbox_send_message(client->chan, pkt);
> > -	if (err < 0)
> > -		return err;
> > -	/* We can send next packet immediately, so just call txdone. */
> > -	mbox_client_txdone(client->chan, 0);
> > -
> > -	return 0;
> > -}
> > -EXPORT_SYMBOL(cmdq_pkt_flush_async);
> > -
> >   MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> > b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 649955d2cf5c..6c42d817d368 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -59,21 +59,6 @@ struct cmdq_client *cmdq_mbox_create(struct
> > device *dev, int index);
> >    */
> >   void cmdq_mbox_destroy(struct cmdq_client *client);
> >   
> > -/**
> > - * cmdq_pkt_create() - create a CMDQ packet
> > - * @client:	the CMDQ mailbox client
> > - * @size:	required CMDQ buffer size
> > - *
> > - * Return: CMDQ packet pointer
> > - */
> > -struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client,
> > size_t size);
> > -
> > -/**
> > - * cmdq_pkt_destroy() - destroy the CMDQ packet
> > - * @pkt:	the CMDQ packet
> > - */
> > -void cmdq_pkt_destroy(struct cmdq_pkt *pkt);
> > -
> >   /**
> >    * cmdq_pkt_write() - append write command to the CMDQ packet
> >    * @pkt:	the CMDQ packet
> > @@ -266,19 +251,6 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt,
> > dma_addr_t addr);
> >    */
> >   int cmdq_pkt_finalize(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
> > - * @pkt:	the CMDQ packet
> > - *
> > - * Return: 0 for success; else the error code is returned
> > - *
> > - * Trigger CMDQ to asynchronously execute the CMDQ packet and call
> > back
> > - * at the end of done packet. Note that this is an ASYNC function.
> > When the
> > - * function returned, it may or may not be finished.
> > - */
> > -int cmdq_pkt_flush_async(struct cmdq_pkt *pkt);
> > -
> >   #else /* IS_ENABLED(CONFIG_MTK_CMDQ) */
> >   
> >   static inline int cmdq_dev_get_client_reg(struct device *dev,
> > @@ -294,13 +266,6 @@ static inline struct cmdq_client
> > *cmdq_mbox_create(struct device *dev, int index
> >   
> >   static inline void cmdq_mbox_destroy(struct cmdq_client *client)
> > { }
> >   
> > -static inline  struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client
> > *client, size_t size)
> > -{
> > -	return ERR_PTR(-EINVAL);
> > -}
> > -
> > -static inline void cmdq_pkt_destroy(struct cmdq_pkt *pkt) { }
> > -
> >   static inline int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys,
> > u16 offset, u32 value)
> >   {
> >   	return -ENOENT;
> > @@ -384,11 +349,6 @@ static inline int cmdq_pkt_finalize(struct
> > cmdq_pkt *pkt)
> >   	return -EINVAL;
> >   }
> >   
> > -static inline int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
> > -{
> > -	return -EINVAL;
> > -}
> > -
> >   #endif /* IS_ENABLED(CONFIG_MTK_CMDQ) */
> >   
> >   #endif	/* __MTK_CMDQ_H__ */
> 
> 

[PATCH 2/9] soc: mediatek: cmdq: Add parameter shift_pa to cmdq_pkt_jump()

[Chun-Kuang Hu]

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(-)

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;
 }
-- 
2.34.1

Re: [PATCH 2/9] soc: mediatek: cmdq: Add parameter shift_pa to cmdq_pkt_jump()

[AngeloGioacchino Del Regno]

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;
>   }

Re: [PATCH 2/9] soc: mediatek: cmdq: Add parameter shift_pa to cmdq_pkt_jump()

[CK Hu (胡俊光)]

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;
> >   }
> 
> 
> 
> 

[PATCH 3/9] soc: mediatek: cmdq: Add cmdq_pkt_eoc() helper function

[Chun-Kuang Hu]

cmdq_pkt_eoc() append eoc command to CMDQ packet. eoc command
would ask GCE to generate IRQ. It's usually appended to the end
of packet to notify all command in the packet is done.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 10 ++++++++++
 include/linux/soc/mediatek/mtk-cmdq.h  | 15 +++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index ed4ef95adf5b..e982997117c2 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -359,6 +359,16 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa)
 }
 EXPORT_SYMBOL(cmdq_pkt_jump);
 
+int cmdq_pkt_eoc(struct cmdq_pkt *pkt)
+{
+	struct cmdq_instruction inst = { {0} };
+
+	inst.op = CMDQ_CODE_EOC;
+	inst.value = CMDQ_EOC_IRQ_EN;
+	return cmdq_pkt_append_command(pkt, inst);
+}
+EXPORT_SYMBOL(cmdq_pkt_eoc);
+
 int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 {
 	struct cmdq_instruction inst = { {0} };
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 6215191a328d..a67f719dec0b 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -245,6 +245,16 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
  */
 int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa);
 
+/**
+ * cmdq_pkt_eoc() - Append eoc command to the CMDQ packet, ask GCE
+ *		    to generate IRQ. It's usually appended to the end of
+ *		    packet to notify that all command in the packet is done.
+ * @pkt:	the CMDQ packet
+ *
+ * Return: 0 for success; else the error code is returned
+ */
+int cmdq_pkt_eoc(struct cmdq_pkt *pkt);
+
 /**
  * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
  * @pkt:	the CMDQ packet
@@ -346,6 +356,11 @@ static inline int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_
 	return -EINVAL;
 }
 
+static inline int cmdq_pkt_eoc(struct cmdq_pkt *pkt)
+{
+	return -EINVAL;
+}
+
 static inline int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 {
 	return -EINVAL;
-- 
2.34.1

Re: [PATCH 3/9] soc: mediatek: cmdq: Add cmdq_pkt_eoc() helper function

[AngeloGioacchino Del Regno]

Il 15/02/24 01:49, Chun-Kuang Hu ha scritto:
> cmdq_pkt_eoc() append eoc command to CMDQ packet. eoc command
> would ask GCE to generate IRQ. It's usually appended to the end
> of packet to notify all command in the packet is done.
> 
> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> ---
>   drivers/soc/mediatek/mtk-cmdq-helper.c | 10 ++++++++++
>   include/linux/soc/mediatek/mtk-cmdq.h  | 15 +++++++++++++++
>   2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index ed4ef95adf5b..e982997117c2 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -359,6 +359,16 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa)
>   }
>   EXPORT_SYMBOL(cmdq_pkt_jump);
>   
> +int cmdq_pkt_eoc(struct cmdq_pkt *pkt)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +
> +	inst.op = CMDQ_CODE_EOC;
> +	inst.value = CMDQ_EOC_IRQ_EN;
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_eoc);
> +
>   int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>   {
>   	struct cmdq_instruction inst = { {0} };
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index 6215191a328d..a67f719dec0b 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -245,6 +245,16 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
>    */
>   int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa);
>   
> +/**
> + * cmdq_pkt_eoc() - Append eoc command to the CMDQ packet, ask GCE
> + *		    to generate IRQ. It's usually appended to the end of
> + *		    packet to notify that all command in the packet is done.
> + * @pkt:	the CMDQ packet
> + *
> + * Return: 0 for success; else the error code is returned

/**
  * cmdq_pkt_eoc() - Append EOC and ask GCE to generate an IRQ at end of execution
  * @pkt:	The CMDQ packet
  *
  * Appends an End Of Code (EOC) command to the CMDQ packet and asks the GCE
  * to generate an interrupt at the end of the execution of all commands in
  * the pipeline.
  * The EOC command is usually appended to the end of the pipeline to notify
  * that all commands are done.
  *
  * Return: 0 for success or negative error number
  */

Apart from that,

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> + */
> +int cmdq_pkt_eoc(struct cmdq_pkt *pkt);
> +
>   /**
>    * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
>    * @pkt:	the CMDQ packet
> @@ -346,6 +356,11 @@ static inline int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_
>   	return -EINVAL;
>   }
>   
> +static inline int cmdq_pkt_eoc(struct cmdq_pkt *pkt)
> +{
> +	return -EINVAL;
> +}
> +
>   static inline int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>   {
>   	return -EINVAL;

[PATCH 4/9] soc: mediatek: cmdq: Add cmdq_pkt_nop() helper function

[Chun-Kuang Hu]

cmdq_pkt_nop() append nop command to the packet. nop command ask
GCE to do no operation.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 11 +++++++++++
 include/linux/soc/mediatek/mtk-cmdq.h  | 16 ++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index e982997117c2..1be950b4ec7f 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -369,6 +369,17 @@ int cmdq_pkt_eoc(struct cmdq_pkt *pkt)
 }
 EXPORT_SYMBOL(cmdq_pkt_eoc);
 
+int cmdq_pkt_nop(struct cmdq_pkt *pkt, u8 shift_pa)
+{
+	struct cmdq_instruction inst = { {0} };
+
+	/* Jumping to next instruction is equal to no operation */
+	inst.op = CMDQ_CODE_JUMP;
+	inst.value = CMDQ_INST_SIZE >> shift_pa;
+	return cmdq_pkt_append_command(pkt, inst);
+}
+EXPORT_SYMBOL(cmdq_pkt_nop);
+
 int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 {
 	struct cmdq_instruction inst = { {0} };
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index a67f719dec0b..8179ba5238f9 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -255,6 +255,17 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa);
  */
 int cmdq_pkt_eoc(struct cmdq_pkt *pkt);
 
+/**
+ * cmdq_pkt_nop() - Append nop command to the CMDQ packet, ask GCE
+ *		    to do no operation.
+ * @pkt:	the CMDQ packet
+ * @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_nop(struct cmdq_pkt *pkt, u8 shift_pa);
+
 /**
  * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
  * @pkt:	the CMDQ packet
@@ -361,6 +372,11 @@ static inline int cmdq_pkt_eoc(struct cmdq_pkt *pkt)
 	return -EINVAL;
 }
 
+static inline int cmdq_pkt_nop(struct cmdq_pkt *pkt, u8 shift_pa)
+{
+	return -EINVAL;
+}
+
 static inline int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 {
 	return -EINVAL;
-- 
2.34.1

Re: [PATCH 4/9] soc: mediatek: cmdq: Add cmdq_pkt_nop() helper function

[AngeloGioacchino Del Regno]

Il 15/02/24 01:49, Chun-Kuang Hu ha scritto:
> cmdq_pkt_nop() append nop command to the packet. nop command ask
> GCE to do no operation.
> 
> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> ---
>   drivers/soc/mediatek/mtk-cmdq-helper.c | 11 +++++++++++
>   include/linux/soc/mediatek/mtk-cmdq.h  | 16 ++++++++++++++++
>   2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index e982997117c2..1be950b4ec7f 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -369,6 +369,17 @@ int cmdq_pkt_eoc(struct cmdq_pkt *pkt)
>   }
>   EXPORT_SYMBOL(cmdq_pkt_eoc);
>   
> +int cmdq_pkt_nop(struct cmdq_pkt *pkt, u8 shift_pa)
> +{
> +	struct cmdq_instruction inst = { {0} };
> +
> +	/* Jumping to next instruction is equal to no operation */
> +	inst.op = CMDQ_CODE_JUMP;
> +	inst.value = CMDQ_INST_SIZE >> shift_pa;
> +	return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_nop);
> +
>   int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>   {
>   	struct cmdq_instruction inst = { {0} };
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index a67f719dec0b..8179ba5238f9 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -255,6 +255,17 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, dma_addr_t addr, u8 shift_pa);
>    */
>   int cmdq_pkt_eoc(struct cmdq_pkt *pkt);
>   
> +/**
> + * cmdq_pkt_nop() - Append nop command to the CMDQ packet, ask GCE
> + *		    to do no operation.

  * cmdq_pkt_nop() - Append No-Operation (NOP) command to a CMDQ packet

After which...
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 4/9] soc: mediatek: cmdq: Add cmdq_pkt_nop() helper function

[CK Hu (胡俊光)]

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:
> > cmdq_pkt_nop() append nop command to the packet. nop command ask
> > GCE to do no operation.
> > 
> > Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> > ---
> >   drivers/soc/mediatek/mtk-cmdq-helper.c | 11 +++++++++++
> >   include/linux/soc/mediatek/mtk-cmdq.h  | 16 ++++++++++++++++
> >   2 files changed, 27 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index e982997117c2..1be950b4ec7f 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -369,6 +369,17 @@ int cmdq_pkt_eoc(struct cmdq_pkt *pkt)
> >   }
> >   EXPORT_SYMBOL(cmdq_pkt_eoc);
> >   
> > +int cmdq_pkt_nop(struct cmdq_pkt *pkt, u8 shift_pa)
> > +{
> > +	struct cmdq_instruction inst = { {0} };
> > +
> > +	/* Jumping to next instruction is equal to no operation */
> > +	inst.op = CMDQ_CODE_JUMP;
> > +	inst.value = CMDQ_INST_SIZE >> shift_pa;
> > +	return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_nop);
> > +
> >   int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >   {
> >   	struct cmdq_instruction inst = { {0} };
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> > b/include/linux/soc/mediatek/mtk-cmdq.h
> > index a67f719dec0b..8179ba5238f9 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -255,6 +255,17 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt,
> > dma_addr_t addr, u8 shift_pa);
> >    */
> >   int cmdq_pkt_eoc(struct cmdq_pkt *pkt);
> >   
> > +/**
> > + * cmdq_pkt_nop() - Append nop command to the CMDQ packet, ask GCE
> > + *		    to do no operation.
> 
>   * cmdq_pkt_nop() - Append No-Operation (NOP) command to a CMDQ
> packet
> 
> After which...
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> 
> 

Because I would create cmdq_pkt_jump_abs(), I think cmdq_pkt_nop() is a
special case of cmdq_pkt_jump_rel(), so I would change cmdq_pkt_nop()
to cmdq_pkt_jump_rel() for general use.

Regards,
CK

> 

[PATCH 5/9] drm/mediatek: Drop calling cmdq_pkt_finalize()

[Chun-Kuang Hu]

For some client driver, it want to reduce latency between excuting
previous packet command and next packet command, so append jump
command to the end of previous packet and the jump destination
address is the start address of next packet command buffer. Before
next packet exist, the previous packet has no information of where
to jump to, so append nop command first. When next packet exist,
change nop command to jump command. For mediatek drm driver, it
never has next packet, so appending nop command is redundant.
Because cmdq_pkt_finalize() would append nop command, so change
calling cmdq_pkt_finalize() to cmdq_pkt_eoc() to prevent append
redundant nop command.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index c729af3b9822..df693fa268ce 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -593,7 +593,7 @@ static void mtk_drm_crtc_update_config(struct mtk_drm_crtc *mtk_crtc,
 		cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
 		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event, false);
 		mtk_crtc_ddp_config(crtc, cmdq_handle);
-		cmdq_pkt_finalize(cmdq_handle);
+		cmdq_pkt_eoc(cmdq_handle);
 		dma_sync_single_for_device(mtk_crtc->cmdq_client.chan->mbox->dev,
 					   cmdq_handle->pa_base,
 					   cmdq_handle->cmd_buf_size,
-- 
2.34.1

Re: [PATCH 5/9] drm/mediatek: Drop calling cmdq_pkt_finalize()

[AngeloGioacchino Del Regno]

Il 15/02/24 01:49, Chun-Kuang Hu ha scritto:
> For some client driver, it want to reduce latency between excuting
> previous packet command and next packet command, so append jump
> command to the end of previous packet and the jump destination
> address is the start address of next packet command buffer. Before
> next packet exist, the previous packet has no information of where
> to jump to, so append nop command first. When next packet exist,
> change nop command to jump command. For mediatek drm driver, it
> never has next packet, so appending nop command is redundant.
> Because cmdq_pkt_finalize() would append nop command, so change
> calling cmdq_pkt_finalize() to cmdq_pkt_eoc() to prevent append
> redundant nop command.
> 
> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>

Makes sense.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH 6/9] media: platform: mtk-mdp3: drop calling cmdq_pkt_finalize()

[Chun-Kuang Hu]

Because client driver has the information of struct cmdq_client, so
it's not necessary to store struct cmdq_client in struct cmdq_pkt.
cmdq_pkt_finalize() use struct cmdq_client in struct cmdq_pkt, so it's
going to be abandoned. Therefore, use cmdq_pkt_eoc() and cmdq_pkt_nop()
to replace cmdq_pkt_finalize().

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c | 3 ++-
 drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c | 2 ++
 drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
index 6adac857a477..a420d492d879 100644
--- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
+++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
@@ -471,7 +471,8 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param)
 		dev_err(dev, "mdp_path_config error\n");
 		goto err_free_path;
 	}
-	cmdq_pkt_finalize(&cmd->pkt);
+	cmdq_pkt_eoc(&cmd->pkt);
+	cmdq_pkt_nop(&cmd->pkt, mdp->cmdq_shift_pa);
 
 	for (i = 0; i < num_comp; i++)
 		memcpy(&comps[i], path->comps[i].comp,
diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
index 94f4ed78523b..2214744c937c 100644
--- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
+++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
@@ -231,6 +231,8 @@ static int mdp_probe(struct platform_device *pdev)
 		goto err_put_scp;
 	}
 
+	mdp->cmdq_shift_pa = cmdq_get_shift_pa(mdp->cmdq_clt->chan);
+
 	init_waitqueue_head(&mdp->callback_wq);
 	ida_init(&mdp->mdp_ida);
 	platform_set_drvdata(pdev, mdp);
diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
index 7e21d226ceb8..ed61e0bb69ee 100644
--- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
+++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
@@ -83,6 +83,7 @@ struct mdp_dev {
 	u32					id_count;
 	struct ida				mdp_ida;
 	struct cmdq_client			*cmdq_clt;
+	u8					cmdq_shift_pa;
 	wait_queue_head_t			callback_wq;
 
 	struct v4l2_device			v4l2_dev;
-- 
2.34.1

Re: [PATCH 6/9] media: platform: mtk-mdp3: drop calling cmdq_pkt_finalize()

[AngeloGioacchino Del Regno]

Il 15/02/24 01:49, Chun-Kuang Hu ha scritto:
> Because client driver has the information of struct cmdq_client, so
> it's not necessary to store struct cmdq_client in struct cmdq_pkt.
> cmdq_pkt_finalize() use struct cmdq_client in struct cmdq_pkt, so it's
> going to be abandoned. Therefore, use cmdq_pkt_eoc() and cmdq_pkt_nop()
> to replace cmdq_pkt_finalize().

No, it's not because cmdq_pkt_finalize() has cmdq_client, but because we want
finer grain control over the CMDQ packets, as not all cases require the NOP
packet to be appended after EOC.

Besides, honestly I'm not even sure if the NOP is always required in MDP3, so...

...Moudy, you know the MDP3 way better than anyone else - can you please
check if NOP is actually needed here?

Thanks!
Angelo

> 
> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> ---
>   drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c | 3 ++-
>   drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c | 2 ++
>   drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h | 1 +
>   3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> index 6adac857a477..a420d492d879 100644
> --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> @@ -471,7 +471,8 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param)
>   		dev_err(dev, "mdp_path_config error\n");
>   		goto err_free_path;
>   	}
> -	cmdq_pkt_finalize(&cmd->pkt);
> +	cmdq_pkt_eoc(&cmd->pkt);
> +	cmdq_pkt_nop(&cmd->pkt, mdp->cmdq_shift_pa);
>   
>   	for (i = 0; i < num_comp; i++)
>   		memcpy(&comps[i], path->comps[i].comp,
> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> index 94f4ed78523b..2214744c937c 100644
> --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> @@ -231,6 +231,8 @@ static int mdp_probe(struct platform_device *pdev)
>   		goto err_put_scp;
>   	}
>   
> +	mdp->cmdq_shift_pa = cmdq_get_shift_pa(mdp->cmdq_clt->chan);
> +
>   	init_waitqueue_head(&mdp->callback_wq);
>   	ida_init(&mdp->mdp_ida);
>   	platform_set_drvdata(pdev, mdp);
> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> index 7e21d226ceb8..ed61e0bb69ee 100644
> --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> @@ -83,6 +83,7 @@ struct mdp_dev {
>   	u32					id_count;
>   	struct ida				mdp_ida;
>   	struct cmdq_client			*cmdq_clt;
> +	u8					cmdq_shift_pa;
>   	wait_queue_head_t			callback_wq;
>   
>   	struct v4l2_device			v4l2_dev;

Re: [PATCH 6/9] media: platform: mtk-mdp3: drop calling cmdq_pkt_finalize()

[Moudy Ho (何宗原)]

On Thu, 2024-02-15 at 11:29 +0100, AngeloGioacchino Del Regno wrote:
> Il 15/02/24 01:49, Chun-Kuang Hu ha scritto:
> > Because client driver has the information of struct cmdq_client, so
> > it's not necessary to store struct cmdq_client in struct cmdq_pkt.
> > cmdq_pkt_finalize() use struct cmdq_client in struct cmdq_pkt, so
> > it's
> > going to be abandoned. Therefore, use cmdq_pkt_eoc() and
> > cmdq_pkt_nop()
> > to replace cmdq_pkt_finalize().
> 
> No, it's not because cmdq_pkt_finalize() has cmdq_client, but because
> we want
> finer grain control over the CMDQ packets, as not all cases require
> the NOP
> packet to be appended after EOC.
> 
> Besides, honestly I'm not even sure if the NOP is always required in
> MDP3, so...
> 
> ...Moudy, you know the MDP3 way better than anyone else - can you
> please
> check if NOP is actually needed here?
> 
> Thanks!
> Angelo
> 

Hi Angelo,

After confirming with the hardware designer and performing the video
playback test, it was discovered that MDP3 is capable of excluding the
NOP(jump) instruction.

Sincerely,
Moudy
> > 
> > Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> > ---
> >   drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c | 3 ++-
> >   drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c | 2 ++
> >   drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h | 1 +
> >   3 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> > b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> > index 6adac857a477..a420d492d879 100644
> > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> > @@ -471,7 +471,8 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct
> > mdp_cmdq_param *param)
> >   		dev_err(dev, "mdp_path_config error\n");
> >   		goto err_free_path;
> >   	}
> > -	cmdq_pkt_finalize(&cmd->pkt);
> > +	cmdq_pkt_eoc(&cmd->pkt);
> > +	cmdq_pkt_nop(&cmd->pkt, mdp->cmdq_shift_pa);
> >   
> >   	for (i = 0; i < num_comp; i++)
> >   		memcpy(&comps[i], path->comps[i].comp,
> > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> > b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> > index 94f4ed78523b..2214744c937c 100644
> > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> > @@ -231,6 +231,8 @@ static int mdp_probe(struct platform_device
> > *pdev)
> >   		goto err_put_scp;
> >   	}
> >   
> > +	mdp->cmdq_shift_pa = cmdq_get_shift_pa(mdp->cmdq_clt->chan);
> > +
> >   	init_waitqueue_head(&mdp->callback_wq);
> >   	ida_init(&mdp->mdp_ida);
> >   	platform_set_drvdata(pdev, mdp);
> > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> > b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> > index 7e21d226ceb8..ed61e0bb69ee 100644
> > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> > @@ -83,6 +83,7 @@ struct mdp_dev {
> >   	u32					id_count;
> >   	struct ida				mdp_ida;
> >   	struct cmdq_client			*cmdq_clt;
> > +	u8					cmdq_shift_pa;
> >   	wait_queue_head_t			callback_wq;
> >   
> >   	struct v4l2_device			v4l2_dev;
> 
> 

Re: [PATCH 6/9] media: platform: mtk-mdp3: drop calling cmdq_pkt_finalize()

[AngeloGioacchino Del Regno]

Il 16/02/24 07:20, Moudy Ho (何宗原) ha scritto:
> On Thu, 2024-02-15 at 11:29 +0100, AngeloGioacchino Del Regno wrote:
>> Il 15/02/24 01:49, Chun-Kuang Hu ha scritto:
>>> Because client driver has the information of struct cmdq_client, so
>>> it's not necessary to store struct cmdq_client in struct cmdq_pkt.
>>> cmdq_pkt_finalize() use struct cmdq_client in struct cmdq_pkt, so
>>> it's
>>> going to be abandoned. Therefore, use cmdq_pkt_eoc() and
>>> cmdq_pkt_nop()
>>> to replace cmdq_pkt_finalize().
>>
>> No, it's not because cmdq_pkt_finalize() has cmdq_client, but because
>> we want
>> finer grain control over the CMDQ packets, as not all cases require
>> the NOP
>> packet to be appended after EOC.
>>
>> Besides, honestly I'm not even sure if the NOP is always required in
>> MDP3, so...
>>
>> ...Moudy, you know the MDP3 way better than anyone else - can you
>> please
>> check if NOP is actually needed here?
>>
>> Thanks!
>> Angelo
>>
> 
> Hi Angelo,
> 
> After confirming with the hardware designer and performing the video
> playback test, it was discovered that MDP3 is capable of excluding the
> NOP(jump) instruction.
> 

Thank you for this extremely fast clarification.

Cheers,
Angelo

> Sincerely,
> Moudy
>>>
>>> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
>>> ---
>>>    drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c | 3 ++-
>>>    drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c | 2 ++
>>>    drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h | 1 +
>>>    3 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
>>> b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
>>> index 6adac857a477..a420d492d879 100644
>>> --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
>>> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
>>> @@ -471,7 +471,8 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct
>>> mdp_cmdq_param *param)
>>>    		dev_err(dev, "mdp_path_config error\n");
>>>    		goto err_free_path;
>>>    	}
>>> -	cmdq_pkt_finalize(&cmd->pkt);
>>> +	cmdq_pkt_eoc(&cmd->pkt);
>>> +	cmdq_pkt_nop(&cmd->pkt, mdp->cmdq_shift_pa);
>>>    
>>>    	for (i = 0; i < num_comp; i++)
>>>    		memcpy(&comps[i], path->comps[i].comp,
>>> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
>>> b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
>>> index 94f4ed78523b..2214744c937c 100644
>>> --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
>>> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
>>> @@ -231,6 +231,8 @@ static int mdp_probe(struct platform_device
>>> *pdev)
>>>    		goto err_put_scp;
>>>    	}
>>>    
>>> +	mdp->cmdq_shift_pa = cmdq_get_shift_pa(mdp->cmdq_clt->chan);
>>> +
>>>    	init_waitqueue_head(&mdp->callback_wq);
>>>    	ida_init(&mdp->mdp_ida);
>>>    	platform_set_drvdata(pdev, mdp);
>>> diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
>>> b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
>>> index 7e21d226ceb8..ed61e0bb69ee 100644
>>> --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
>>> +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
>>> @@ -83,6 +83,7 @@ struct mdp_dev {
>>>    	u32					id_count;
>>>    	struct ida				mdp_ida;
>>>    	struct cmdq_client			*cmdq_clt;
>>> +	u8					cmdq_shift_pa;
>>>    	wait_queue_head_t			callback_wq;
>>>    
>>>    	struct v4l2_device			v4l2_dev;
>>
>>

Re: [PATCH 6/9] media: platform: mtk-mdp3: drop calling cmdq_pkt_finalize()

[CK Hu (胡俊光)]

Hi, Moudy:

On Fri, 2024-02-16 at 08:56 +0100, AngeloGioacchino Del Regno wrote:
> Il 16/02/24 07:20, Moudy Ho (何宗原) ha scritto:
> > On Thu, 2024-02-15 at 11:29 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > Il 15/02/24 01:49, Chun-Kuang Hu ha scritto:
> > > > Because client driver has the information of struct
> > > > cmdq_client, so
> > > > it's not necessary to store struct cmdq_client in struct
> > > > cmdq_pkt.
> > > > cmdq_pkt_finalize() use struct cmdq_client in struct cmdq_pkt,
> > > > so
> > > > it's
> > > > going to be abandoned. Therefore, use cmdq_pkt_eoc() and
> > > > cmdq_pkt_nop()
> > > > to replace cmdq_pkt_finalize().
> > > 
> > > No, it's not because cmdq_pkt_finalize() has cmdq_client, but
> > > because
> > > we want
> > > finer grain control over the CMDQ packets, as not all cases
> > > require
> > > the NOP
> > > packet to be appended after EOC.
> > > 
> > > Besides, honestly I'm not even sure if the NOP is always required
> > > in
> > > MDP3, so...
> > > 
> > > ...Moudy, you know the MDP3 way better than anyone else - can you
> > > please
> > > check if NOP is actually needed here?
> > > 
> > > Thanks!
> > > Angelo
> > > 
> > 
> > Hi Angelo,
> > 
> > After confirming with the hardware designer and performing the
> > video
> > playback test, it was discovered that MDP3 is capable of excluding
> > the
> > NOP(jump) instruction.
> > 
> 
> Thank you for this extremely fast clarification.

As discuss with Jason, there is one precondition that mdp3 could drop
nop. mpd3 still need to append jump command in the end of packet. The
precondition is that if the jump command is append by mailbox
controller, then client driver is not necessary to append nop. But
currently, mailbox controller just modify nop to jump, not append jump.
Because this modification is not related to this series of removing
pkt->cl, so I would keep append nop in the end of packet for mdp3
driver.

Regards,
CK

> 
> Cheers,
> Angelo
> 
> > Sincerely,
> > Moudy
> > > > 
> > > > Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> > > > ---
> > > >    drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c | 3 ++-
> > > >    drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c | 2 ++
> > > >    drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h | 1 +
> > > >    3 files changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-
> > > > cmdq.c
> > > > b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> > > > index 6adac857a477..a420d492d879 100644
> > > > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> > > > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
> > > > @@ -471,7 +471,8 @@ int mdp_cmdq_send(struct mdp_dev *mdp,
> > > > struct
> > > > mdp_cmdq_param *param)
> > > >    		dev_err(dev, "mdp_path_config error\n");
> > > >    		goto err_free_path;
> > > >    	}
> > > > -	cmdq_pkt_finalize(&cmd->pkt);
> > > > +	cmdq_pkt_eoc(&cmd->pkt);
> > > > +	cmdq_pkt_nop(&cmd->pkt, mdp->cmdq_shift_pa);
> > > >    
> > > >    	for (i = 0; i < num_comp; i++)
> > > >    		memcpy(&comps[i], path->comps[i].comp,
> > > > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-
> > > > core.c
> > > > b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> > > > index 94f4ed78523b..2214744c937c 100644
> > > > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> > > > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> > > > @@ -231,6 +231,8 @@ static int mdp_probe(struct platform_device
> > > > *pdev)
> > > >    		goto err_put_scp;
> > > >    	}
> > > >    
> > > > +	mdp->cmdq_shift_pa = cmdq_get_shift_pa(mdp->cmdq_clt-
> > > > >chan);
> > > > +
> > > >    	init_waitqueue_head(&mdp->callback_wq);
> > > >    	ida_init(&mdp->mdp_ida);
> > > >    	platform_set_drvdata(pdev, mdp);
> > > > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-
> > > > core.h
> > > > b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> > > > index 7e21d226ceb8..ed61e0bb69ee 100644
> > > > --- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> > > > +++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.h
> > > > @@ -83,6 +83,7 @@ struct mdp_dev {
> > > >    	u32					id_count;
> > > >    	struct ida				mdp_ida;
> > > >    	struct cmdq_client			*cmdq_clt;
> > > > +	u8					cmdq_shift_pa;
> > > >    	wait_queue_head_t			callback_wq;
> > > >    
> > > >    	struct v4l2_device			v4l2_dev;
> > > 
> > > 
> 
> 
> 
> 

[PATCH 7/9] soc: mediatek: cmdq: Remove cmdq_pkt_finalize() helper function

[Chun-Kuang Hu]

Because client driver has the information of struct cmdq_client, so
it's not necessary to store struct cmdq_client in struct cmdq_pkt.
cmdq_pkt_finalize() use struct cmdq_client in struct cmdq_pkt, so
client driver use cmdq_pkt_eoc() and cmdq_pkt_nop() to replace
cmdq_pkt_finalize() and this function could be removed.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/soc/mediatek/mtk-cmdq-helper.c | 22 ----------------------
 include/linux/soc/mediatek/mtk-cmdq.h  | 13 -------------
 2 files changed, 35 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 1be950b4ec7f..9b937db7398c 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -380,26 +380,4 @@ int cmdq_pkt_nop(struct cmdq_pkt *pkt, u8 shift_pa)
 }
 EXPORT_SYMBOL(cmdq_pkt_nop);
 
-int cmdq_pkt_finalize(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 end */
-	inst.op = CMDQ_CODE_JUMP;
-	inst.value = CMDQ_JUMP_PASS >>
-		cmdq_get_shift_pa(((struct cmdq_client *)pkt->cl)->chan);
-	err = cmdq_pkt_append_command(pkt, inst);
-
-	return err;
-}
-EXPORT_SYMBOL(cmdq_pkt_finalize);
-
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 8179ba5238f9..f1d571475b52 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -266,14 +266,6 @@ int cmdq_pkt_eoc(struct cmdq_pkt *pkt);
  */
 int cmdq_pkt_nop(struct cmdq_pkt *pkt, u8 shift_pa);
 
-/**
- * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
- * @pkt:	the CMDQ packet
- *
- * Return: 0 for success; else the error code is returned
- */
-int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
-
 #else /* IS_ENABLED(CONFIG_MTK_CMDQ) */
 
 static inline int cmdq_dev_get_client_reg(struct device *dev,
@@ -377,11 +369,6 @@ static inline int cmdq_pkt_nop(struct cmdq_pkt *pkt, u8 shift_pa)
 	return -EINVAL;
 }
 
-static inline int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
-{
-	return -EINVAL;
-}
-
 #endif /* IS_ENABLED(CONFIG_MTK_CMDQ) */
 
 #endif	/* __MTK_CMDQ_H__ */
-- 
2.34.1

[PATCH 8/9] drm/mediatek: Do not store struct cmdq_client in struct cmdq_pkt

[Chun-Kuang Hu]

MediaTek drm driver has the struct cmdq_client information, so
it's not necessary to store this information in struct cmdq_pkt.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index df693fa268ce..0d54cbefee0b 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -123,7 +123,6 @@ static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *
 		return -ENOMEM;
 
 	pkt->buf_size = size;
-	pkt->cl = (void *)client;
 
 	dev = client->chan->mbox->dev;
 	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
@@ -139,10 +138,8 @@ static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *
 	return 0;
 }
 
-static void mtk_drm_cmdq_pkt_destroy(struct cmdq_pkt *pkt)
+static void mtk_drm_cmdq_pkt_destroy(struct cmdq_client *client, struct cmdq_pkt *pkt)
 {
-	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
-
 	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
 			 DMA_TO_DEVICE);
 	kfree(pkt->va_base);
@@ -156,7 +153,7 @@ static void mtk_drm_crtc_destroy(struct drm_crtc *crtc)
 
 	mtk_mutex_put(mtk_crtc->mutex);
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	mtk_drm_cmdq_pkt_destroy(&mtk_crtc->cmdq_handle);
+	mtk_drm_cmdq_pkt_destroy(&mtk_crtc->cmdq_client, &mtk_crtc->cmdq_handle);
 
 	if (mtk_crtc->cmdq_client.chan) {
 		mbox_free_channel(mtk_crtc->cmdq_client.chan);
-- 
2.34.1

Re: [PATCH 8/9] drm/mediatek: Do not store struct cmdq_client in struct cmdq_pkt

[AngeloGioacchino Del Regno]

Il 15/02/24 01:49, Chun-Kuang Hu ha scritto:
> MediaTek drm driver has the struct cmdq_client information, so
> it's not necessary to store this information in struct cmdq_pkt.
> 
> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>

As said in patch [1/9], this should be done in the common function, not in each
copy-pasted function in mtk_drm and mtk-mdp3.

Regards,
Angelo

[PATCH 9/9] media: platform: mtk-mdp3: do not store struct cmdq_client in struct cmdq_pkt

[Chun-Kuang Hu]

MediaTek mdp3 driver has the struct cmdq_client information, so
it's not necessary to store struct cmdq_client in struct cmdq_pkt.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
index a420d492d879..6aa32ab018b4 100644
--- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
+++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
@@ -298,7 +298,6 @@ static int mdp_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *pkt,
 		return -ENOMEM;
 
 	pkt->buf_size = size;
-	pkt->cl = (void *)client;
 
 	dev = client->chan->mbox->dev;
 	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
@@ -314,10 +313,8 @@ static int mdp_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *pkt,
 	return 0;
 }
 
-static void mdp_cmdq_pkt_destroy(struct cmdq_pkt *pkt)
+static void mdp_cmdq_pkt_destroy(struct cmdq_client *client, struct cmdq_pkt *pkt)
 {
-	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
-
 	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
 			 DMA_TO_DEVICE);
 	kfree(pkt->va_base);
@@ -341,7 +338,7 @@ static void mdp_auto_release_work(struct work_struct *work)
 	atomic_dec(&mdp->job_count);
 	wake_up(&mdp->callback_wq);
 
-	mdp_cmdq_pkt_destroy(&cmd->pkt);
+	mdp_cmdq_pkt_destroy(mdp->cmdq_clt, &cmd->pkt);
 	kfree(cmd->comps);
 	cmd->comps = NULL;
 	kfree(cmd);
@@ -388,7 +385,7 @@ static void mdp_handle_cmdq_callback(struct mbox_client *cl, void *mssg)
 		atomic_dec(&mdp->job_count);
 		wake_up(&mdp->callback_wq);
 
-		mdp_cmdq_pkt_destroy(&cmd->pkt);
+		mdp_cmdq_pkt_destroy(mdp->cmdq_clt, &cmd->pkt);
 		kfree(cmd->comps);
 		cmd->comps = NULL;
 		kfree(cmd);
@@ -513,7 +510,7 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param)
 err_free_comps:
 	kfree(comps);
 err_destroy_pkt:
-	mdp_cmdq_pkt_destroy(&cmd->pkt);
+	mdp_cmdq_pkt_destroy(mdp->cmdq_clt, &cmd->pkt);
 err_free_cmd:
 	kfree(cmd);
 err_cancel_job:
-- 
2.34.1