Re: [PATCH 0/3] MediaTek DRM: Clean up CMDQ support and ifdefs

Re: [PATCH 0/3] MediaTek DRM: Clean up CMDQ support and ifdefs

[AngeloGioacchino Del Regno]

Il 23/06/23 14:49, Alexandre Mergnat ha scritto:
> 
> 
> On 23/06/2023 11:49, AngeloGioacchino Del Regno wrote:
>> This series changes MediaTek CMDQ support to use the mtk-cmdq-helper
>> functions, removing duplicated cmdq setup code in mtk-drm and also
>> removing all instances of `#if IS_REACHABLE(CONFIG_MTK_CMDQ)` while
>> keeping compatibility with both CONFIG_MTK_CMDQ=n and =m/=y.
>>
>> This applies on top of [1] and [2].
>>
>> [1]:https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
>> [2]:https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com
> 
> Hi Angelo,
> 
> Can you provide a public branch to test it please ?
> I tried to apply the dependencies but still have an issue with the 3rd one:
> 
> https://lore.kernel.org/lkml/20230523104234.7849-1-angelogioacchino.delregno@collabora.com
> OK
> 
> https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
> OK
> 
> https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com
> KO
> 
> Thanks
> 

Sorry for the very late reply; I've somehow lost this email in the haystack...

There you go:
https://gitlab.collabora.com/google/chromeos-kernel/-/commits/for-kernelci/

Cheers,
Angelo

Re: [PATCH 0/3] MediaTek DRM: Clean up CMDQ support and ifdefs

[Alexandre Mergnat]

On 19/07/2023 09:41, AngeloGioacchino Del Regno wrote:
> Il 23/06/23 14:49, Alexandre Mergnat ha scritto:
>>
>>
>> On 23/06/2023 11:49, AngeloGioacchino Del Regno wrote:
>>> This series changes MediaTek CMDQ support to use the mtk-cmdq-helper
>>> functions, removing duplicated cmdq setup code in mtk-drm and also
>>> removing all instances of `#if IS_REACHABLE(CONFIG_MTK_CMDQ)` while
>>> keeping compatibility with both CONFIG_MTK_CMDQ=n and =m/=y.
>>>
>>> This applies on top of [1] and [2].
>>>
>>> [1]:https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
>>> [2]:https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com
>>
>> Hi Angelo,
>>
>> Can you provide a public branch to test it please ?
>> I tried to apply the dependencies but still have an issue with the 3rd 
>> one:
>>
>> https://lore.kernel.org/lkml/20230523104234.7849-1-angelogioacchino.delregno@collabora.com
>> OK
>>
>> https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
>> OK
>>
>> https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com
>> KO
>>
>> Thanks
>>
> 
> Sorry for the very late reply; I've somehow lost this email in the 
> haystack...
> 
> There you go:
> https://gitlab.collabora.com/google/chromeos-kernel/-/commits/for-kernelci/
> 

Thanks Angelo,

I build/boot your branch for the i350-evk board.

I had to revert this commit from your branch:
"15f12798e218 TODO: soc: mediatek: mtk-pm-domains: Support CPU Power 
Domains"
Because it conflict with the "soc: mediatek: MT8365 power support" serie 
[3].

Also, I change back the defconfig to be aligned with the v6.5-rc1

After that, the board boot and drm/display seems working like a charm.

[3]: 
https://lore.kernel.org/linux-arm-kernel/20230713150414.891893-1-msp@baylibre.com/

-- 
Regards,
Alexandre

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[CK Hu (胡俊光)]

Hi, Angelo:

On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Instead of stack allocating the cmdq_client and cmdq_handle
> structures
> switch them to pointers, allowing us to migrate this driver to use
> the
> common functions provided by mtk-cmdq-helper.
> In order to do this, it was also necessary to add a `priv` pointer to
> struct cmdq_client, as that's used to pass (in this case) a mtk_crtc
> handle to the ddp_cmdq_cb() mailbox RX callback function.
> 
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++---------------
> --
>  include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
>  2 files changed, 32 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 0df62b076f49..b63289ab6787 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
>  	bool				pending_async_planes;
>  
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -	struct cmdq_client		cmdq_client;
> -	struct cmdq_pkt			cmdq_handle;
> +	struct cmdq_client		*cmdq_client;
> +	struct cmdq_pkt			*cmdq_handle;
>  	u32				cmdq_event;
>  	u32				cmdq_vblank_cnt;
>  	wait_queue_head_t		cb_blocking_queue;
> @@ -108,47 +108,6 @@ static void mtk_drm_finish_page_flip(struct
> mtk_drm_crtc *mtk_crtc)
>  	}
>  }
>  
> -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client,
> struct cmdq_pkt *pkt,
> -				   size_t size)
> -{
> -	struct device *dev;
> -	dma_addr_t dma_addr;
> -
> -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> -	if (!pkt->va_base) {
> -		kfree(pkt);
> -		return -ENOMEM;
> -	}
> -	pkt->buf_size = size;
> -	pkt->cl = (void *)client;

I have a plan to remove cl in struct cmdq_pkt. But this modification
would make this plan more difficult. So I would pending this patch
until cl is removed from struct cmdq_pkt.

Regards,
CK

> -
> -	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 -ENOMEM;
> -	}
> -
> -	pkt->pa_base = dma_addr;
> -
> -	return 0;
> -}
> -
> -static void mtk_drm_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);
> -}
> -#endif
> -
>  static void mtk_drm_crtc_destroy(struct drm_crtc *crtc)
>  {
>  	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> @@ -156,12 +115,9 @@ 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);
> -
> -	if (mtk_crtc->cmdq_client.chan) {
> -		mbox_free_channel(mtk_crtc->cmdq_client.chan);
> -		mtk_crtc->cmdq_client.chan = NULL;
> -	}
> +	cmdq_pkt_destroy(mtk_crtc->cmdq_handle);
> +	cmdq_mbox_destroy(mtk_crtc->cmdq_client);
> +	mtk_crtc->cmdq_client = NULL;
>  #endif
>  
>  	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> @@ -288,7 +244,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> void *mssg)
>  {
>  	struct cmdq_cb_data *data = mssg;
>  	struct cmdq_client *cmdq_cl = container_of(cl, struct
> cmdq_client, client);
> -	struct mtk_drm_crtc *mtk_crtc = container_of(cmdq_cl, struct
> mtk_drm_crtc, cmdq_client);
> +	struct mtk_drm_crtc *mtk_crtc = (struct mtk_drm_crtc *)cmdq_cl-
> >priv;
>  	struct mtk_crtc_state *state;
>  	unsigned int i;
>  
> @@ -546,7 +502,7 @@ static void mtk_drm_crtc_update_config(struct
> mtk_drm_crtc *mtk_crtc,
>  				       bool needs_vblank)
>  {
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -	struct cmdq_pkt *cmdq_handle = &mtk_crtc->cmdq_handle;
> +	struct cmdq_pkt *cmdq_handle = mtk_crtc->cmdq_handle;
>  #endif
>  	struct drm_crtc *crtc = &mtk_crtc->base;
>  	struct mtk_drm_private *priv = crtc->dev->dev_private;
> @@ -584,14 +540,14 @@ static void mtk_drm_crtc_update_config(struct
> mtk_drm_crtc *mtk_crtc,
>  		mtk_mutex_release(mtk_crtc->mutex);
>  	}
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -	if (mtk_crtc->cmdq_client.chan) {
> -		mbox_flush(mtk_crtc->cmdq_client.chan, 2000);
> +	if (mtk_crtc->cmdq_client) {
> +		mbox_flush(mtk_crtc->cmdq_client->chan, 2000);
>  		cmdq_handle->cmd_buf_size = 0;
>  		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);
> -		dma_sync_single_for_device(mtk_crtc->cmdq_client.chan-
> >mbox->dev,
> +		dma_sync_single_for_device(mtk_crtc->cmdq_client->chan-
> >mbox->dev,
>  					   cmdq_handle->pa_base,
>  					   cmdq_handle->cmd_buf_size,
>  					   DMA_TO_DEVICE);
> @@ -604,8 +560,8 @@ static void mtk_drm_crtc_update_config(struct
> mtk_drm_crtc *mtk_crtc,
>  		 */
>  		mtk_crtc->cmdq_vblank_cnt = 3;
>  
> -		mbox_send_message(mtk_crtc->cmdq_client.chan,
> cmdq_handle);
> -		mbox_client_txdone(mtk_crtc->cmdq_client.chan, 0);
> +		mbox_send_message(mtk_crtc->cmdq_client->chan,
> cmdq_handle);
> +		mbox_client_txdone(mtk_crtc->cmdq_client->chan, 0);
>  	}
>  #endif
>  	mtk_crtc->config_updating = false;
> @@ -619,7 +575,7 @@ static void mtk_crtc_ddp_irq(void *data)
>  	struct mtk_drm_private *priv = crtc->dev->dev_private;
>  
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -	if (!priv->data->shadow_register && !mtk_crtc-
> >cmdq_client.chan)
> +	if (!priv->data->shadow_register && !mtk_crtc->cmdq_client)
>  		mtk_crtc_ddp_config(crtc, NULL);
>  	else if (mtk_crtc->cmdq_vblank_cnt > 0 && --mtk_crtc-
> >cmdq_vblank_cnt == 0)
>  		DRM_ERROR("mtk_crtc %d CMDQ execute command
> timeout!\n",
> @@ -722,7 +678,7 @@ static void mtk_drm_crtc_atomic_disable(struct
> drm_crtc *crtc,
>  	mtk_drm_crtc_update_config(mtk_crtc, false);
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>  	/* Wait for planes to be disabled by cmdq */
> -	if (mtk_crtc->cmdq_client.chan)
> +	if (mtk_crtc->cmdq_client)
>  		wait_event_timeout(mtk_crtc->cb_blocking_queue,
>  				   mtk_crtc->cmdq_vblank_cnt == 0,
>  				   msecs_to_jiffies(500));
> @@ -1002,19 +958,20 @@ int mtk_drm_crtc_create(struct drm_device
> *drm_dev,
>  
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>  	i = priv->mbox_index++;
> -	mtk_crtc->cmdq_client.client.dev = mtk_crtc->mmsys_dev;
> -	mtk_crtc->cmdq_client.client.tx_block = false;
> -	mtk_crtc->cmdq_client.client.knows_txdone = true;
> -	mtk_crtc->cmdq_client.client.rx_callback = ddp_cmdq_cb;
> -	mtk_crtc->cmdq_client.chan =
> -			mbox_request_channel(&mtk_crtc-
> >cmdq_client.client, i);
> -	if (IS_ERR(mtk_crtc->cmdq_client.chan)) {
> -		dev_dbg(dev, "mtk_crtc %d failed to create mailbox
> client, writing register by CPU now\n",
> -			drm_crtc_index(&mtk_crtc->base));
> -		mtk_crtc->cmdq_client.chan = NULL;
> +
> +	mtk_crtc->cmdq_client = cmdq_mbox_create(mtk_crtc->mmsys_dev,
> i);
> +	if (IS_ERR(mtk_crtc->cmdq_client)) {
> +		ret = PTR_ERR(mtk_crtc->cmdq_client);
> +		dev_dbg(dev, "Failed to create CMDQ client: %d\n",
> ret);
> +		mtk_crtc->cmdq_client = NULL;
> +		return 0;
>  	}
>  
> -	if (mtk_crtc->cmdq_client.chan) {
> +	/* Setup the CMDQ handler callback */
> +	mtk_crtc->cmdq_client->priv = mtk_crtc;
> +	mtk_crtc->cmdq_client->client.rx_callback = ddp_cmdq_cb;
> +
> +	if (mtk_crtc->cmdq_client) {
>  		ret = of_property_read_u32_index(priv->mutex_node,
>  						 "mediatek,gce-events",
>  						 i,
> @@ -1022,17 +979,15 @@ int mtk_drm_crtc_create(struct drm_device
> *drm_dev,
>  		if (ret) {
>  			dev_dbg(dev, "mtk_crtc %d failed to get
> mediatek,gce-events property\n",
>  				drm_crtc_index(&mtk_crtc->base));
> -			mbox_free_channel(mtk_crtc->cmdq_client.chan);
> -			mtk_crtc->cmdq_client.chan = NULL;
> +			cmdq_mbox_destroy(mtk_crtc->cmdq_client);
> +			mtk_crtc->cmdq_client = NULL;
>  		} else {
> -			ret = mtk_drm_cmdq_pkt_create(&mtk_crtc-
> >cmdq_client,
> -						      &mtk_crtc-
> >cmdq_handle,
> -						      PAGE_SIZE);
> +			mtk_crtc->cmdq_handle =
> cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
>  			if (ret) {
>  				dev_dbg(dev, "mtk_crtc %d failed to
> create cmdq packet\n",
>  					drm_crtc_index(&mtk_crtc-
> >base));
> -				mbox_free_channel(mtk_crtc-
> >cmdq_client.chan);
> -				mtk_crtc->cmdq_client.chan = NULL;
> +				cmdq_mbox_destroy(mtk_crtc-
> >cmdq_client);
> +				mtk_crtc->cmdq_client = NULL;
>  			}
>  		}
>  
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> index 649955d2cf5c..2a1dc8b12db3 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -25,6 +25,7 @@ struct cmdq_client_reg {
>  struct cmdq_client {
>  	struct mbox_client client;
>  	struct mbox_chan *chan;
> +	void *priv;
>  };
>  
>  #if IS_ENABLED(CONFIG_MTK_CMDQ)
> -- 
> 2.40.1

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[AngeloGioacchino Del Regno]

Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno wrote:
>>   	
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>   Instead of stack allocating the cmdq_client and cmdq_handle
>> structures
>> switch them to pointers, allowing us to migrate this driver to use
>> the
>> common functions provided by mtk-cmdq-helper.
>> In order to do this, it was also necessary to add a `priv` pointer to
>> struct cmdq_client, as that's used to pass (in this case) a mtk_crtc
>> handle to the ddp_cmdq_cb() mailbox RX callback function.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <
>> angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++---------------
>> --
>>   include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
>>   2 files changed, 32 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> index 0df62b076f49..b63289ab6787 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
>>   	bool				pending_async_planes;
>>   
>>   #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>> -	struct cmdq_client		cmdq_client;
>> -	struct cmdq_pkt			cmdq_handle;
>> +	struct cmdq_client		*cmdq_client;
>> +	struct cmdq_pkt			*cmdq_handle;
>>   	u32				cmdq_event;
>>   	u32				cmdq_vblank_cnt;
>>   	wait_queue_head_t		cb_blocking_queue;
>> @@ -108,47 +108,6 @@ static void mtk_drm_finish_page_flip(struct
>> mtk_drm_crtc *mtk_crtc)
>>   	}
>>   }
>>   
>> -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
>> -static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client,
>> struct cmdq_pkt *pkt,
>> -				   size_t size)
>> -{
>> -	struct device *dev;
>> -	dma_addr_t dma_addr;
>> -
>> -	pkt->va_base = kzalloc(size, GFP_KERNEL);
>> -	if (!pkt->va_base) {
>> -		kfree(pkt);
>> -		return -ENOMEM;
>> -	}
>> -	pkt->buf_size = size;
>> -	pkt->cl = (void *)client;
> 
> I have a plan to remove cl in struct cmdq_pkt. But this modification
> would make this plan more difficult. So I would pending this patch
> until cl is removed from struct cmdq_pkt.
> 

I think that this ifdef cleanup is more urgent than the removal of `cl` from
struct cmdq_pkt, as those ifdefs shouldn't have reached upstream in the first
place, don't you agree?

Regards,
Angelo

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[CK Hu (胡俊光)]

Hi, Angelo:

On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno wrote:
> Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno
> > wrote:
> > >   	
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > >   Instead of stack allocating the cmdq_client and cmdq_handle
> > > structures
> > > switch them to pointers, allowing us to migrate this driver to
> > > use
> > > the
> > > common functions provided by mtk-cmdq-helper.
> > > In order to do this, it was also necessary to add a `priv`
> > > pointer to
> > > struct cmdq_client, as that's used to pass (in this case) a
> > > mtk_crtc
> > > handle to the ddp_cmdq_cb() mailbox RX callback function.
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@collabora.com>
> > > ---
> > >   drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++--------
> > > -------
> > > --
> > >   include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
> > >   2 files changed, 32 insertions(+), 76 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > index 0df62b076f49..b63289ab6787 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
> > >   	bool				pending_async_planes;
> > >   
> > >   #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > -	struct cmdq_client		cmdq_client;
> > > -	struct cmdq_pkt			cmdq_handle;
> > > +	struct cmdq_client		*cmdq_client;
> > > +	struct cmdq_pkt			*cmdq_handle;
> > >   	u32				cmdq_event;
> > >   	u32				cmdq_vblank_cnt;
> > >   	wait_queue_head_t		cb_blocking_queue;
> > > @@ -108,47 +108,6 @@ static void mtk_drm_finish_page_flip(struct
> > > mtk_drm_crtc *mtk_crtc)
> > >   	}
> > >   }
> > >   
> > > -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > -static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client,
> > > struct cmdq_pkt *pkt,
> > > -				   size_t size)
> > > -{
> > > -	struct device *dev;
> > > -	dma_addr_t dma_addr;
> > > -
> > > -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> > > -	if (!pkt->va_base) {
> > > -		kfree(pkt);
> > > -		return -ENOMEM;
> > > -	}
> > > -	pkt->buf_size = size;
> > > -	pkt->cl = (void *)client;
> > 
> > I have a plan to remove cl in struct cmdq_pkt. But this
> > modification
> > would make this plan more difficult. So I would pending this patch
> > until cl is removed from struct cmdq_pkt.
> > 
> 
> I think that this ifdef cleanup is more urgent than the removal of
> `cl` from
> struct cmdq_pkt, as those ifdefs shouldn't have reached upstream in
> the first
> place, don't you agree?

I think removing ifdefs and using helper function are different things.
You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().

Regards,
CK

> 
> Regards,
> Angelo
> 

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[AngeloGioacchino Del Regno]

Il 03/08/23 08:28, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno wrote:
>> Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
>>> Hi, Angelo:
>>>
>>> On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno
>>> wrote:
>>>>    	
>>>> External email : Please do not click links or open attachments
>>>> until
>>>> you have verified the sender or the content.
>>>>    Instead of stack allocating the cmdq_client and cmdq_handle
>>>> structures
>>>> switch them to pointers, allowing us to migrate this driver to
>>>> use
>>>> the
>>>> common functions provided by mtk-cmdq-helper.
>>>> In order to do this, it was also necessary to add a `priv`
>>>> pointer to
>>>> struct cmdq_client, as that's used to pass (in this case) a
>>>> mtk_crtc
>>>> handle to the ddp_cmdq_cb() mailbox RX callback function.
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <
>>>> angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>    drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++--------
>>>> -------
>>>> --
>>>>    include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
>>>>    2 files changed, 32 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>> index 0df62b076f49..b63289ab6787 100644
>>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>> @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
>>>>    	bool				pending_async_planes;
>>>>    
>>>>    #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>>>> -	struct cmdq_client		cmdq_client;
>>>> -	struct cmdq_pkt			cmdq_handle;
>>>> +	struct cmdq_client		*cmdq_client;
>>>> +	struct cmdq_pkt			*cmdq_handle;
>>>>    	u32				cmdq_event;
>>>>    	u32				cmdq_vblank_cnt;
>>>>    	wait_queue_head_t		cb_blocking_queue;
>>>> @@ -108,47 +108,6 @@ static void mtk_drm_finish_page_flip(struct
>>>> mtk_drm_crtc *mtk_crtc)
>>>>    	}
>>>>    }
>>>>    
>>>> -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
>>>> -static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client,
>>>> struct cmdq_pkt *pkt,
>>>> -				   size_t size)
>>>> -{
>>>> -	struct device *dev;
>>>> -	dma_addr_t dma_addr;
>>>> -
>>>> -	pkt->va_base = kzalloc(size, GFP_KERNEL);
>>>> -	if (!pkt->va_base) {
>>>> -		kfree(pkt);
>>>> -		return -ENOMEM;
>>>> -	}
>>>> -	pkt->buf_size = size;
>>>> -	pkt->cl = (void *)client;
>>>
>>> I have a plan to remove cl in struct cmdq_pkt. But this
>>> modification
>>> would make this plan more difficult. So I would pending this patch
>>> until cl is removed from struct cmdq_pkt.
>>>
>>
>> I think that this ifdef cleanup is more urgent than the removal of
>> `cl` from
>> struct cmdq_pkt, as those ifdefs shouldn't have reached upstream in
>> the first
>> place, don't you agree?
> 
> I think removing ifdefs and using helper function are different things.
> You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().
> 

I chose to do it like that because this function would otherwise be a
100% duplicate of the related cmdq helper :-)


> Regards,
> CK
> 
>>
>> Regards,
>> Angelo
>>

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[CK Hu (胡俊光)]

Hi, Angelo:

On Thu, 2023-08-03 at 10:25 +0200, AngeloGioacchino Del Regno wrote:
> Il 03/08/23 08:28, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
> > > > Hi, Angelo:
> > > > 
> > > > On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno
> > > > wrote:
> > > > >    	
> > > > > External email : Please do not click links or open
> > > > > attachments
> > > > > until
> > > > > you have verified the sender or the content.
> > > > >    Instead of stack allocating the cmdq_client and
> > > > > cmdq_handle
> > > > > structures
> > > > > switch them to pointers, allowing us to migrate this driver
> > > > > to
> > > > > use
> > > > > the
> > > > > common functions provided by mtk-cmdq-helper.
> > > > > In order to do this, it was also necessary to add a `priv`
> > > > > pointer to
> > > > > struct cmdq_client, as that's used to pass (in this case) a
> > > > > mtk_crtc
> > > > > handle to the ddp_cmdq_cb() mailbox RX callback function.
> > > > > 
> > > > > Signed-off-by: AngeloGioacchino Del Regno <
> > > > > angelogioacchino.delregno@collabora.com>
> > > > > ---
> > > > >    drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++-----
> > > > > ---
> > > > > -------
> > > > > --
> > > > >    include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
> > > > >    2 files changed, 32 insertions(+), 76 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > index 0df62b076f49..b63289ab6787 100644
> > > > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
> > > > >    	bool				pending_async_planes;
> > > > >    
> > > > >    #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > > > -	struct cmdq_client		cmdq_client;
> > > > > -	struct cmdq_pkt			cmdq_handle;
> > > > > +	struct cmdq_client		*cmdq_client;
> > > > > +	struct cmdq_pkt			*cmdq_handle;
> > > > >    	u32				cmdq_event;
> > > > >    	u32				cmdq_vblank_cnt;
> > > > >    	wait_queue_head_t		cb_blocking_queue;
> > > > > @@ -108,47 +108,6 @@ static void
> > > > > mtk_drm_finish_page_flip(struct
> > > > > mtk_drm_crtc *mtk_crtc)
> > > > >    	}
> > > > >    }
> > > > >    
> > > > > -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > > > -static int mtk_drm_cmdq_pkt_create(struct cmdq_client
> > > > > *client,
> > > > > struct cmdq_pkt *pkt,
> > > > > -				   size_t size)
> > > > > -{
> > > > > -	struct device *dev;
> > > > > -	dma_addr_t dma_addr;
> > > > > -
> > > > > -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> > > > > -	if (!pkt->va_base) {
> > > > > -		kfree(pkt);
> > > > > -		return -ENOMEM;
> > > > > -	}
> > > > > -	pkt->buf_size = size;
> > > > > -	pkt->cl = (void *)client;
> > > > 
> > > > I have a plan to remove cl in struct cmdq_pkt. But this
> > > > modification
> > > > would make this plan more difficult. So I would pending this
> > > > patch
> > > > until cl is removed from struct cmdq_pkt.
> > > > 
> > > 
> > > I think that this ifdef cleanup is more urgent than the removal
> > > of
> > > `cl` from
> > > struct cmdq_pkt, as those ifdefs shouldn't have reached upstream
> > > in
> > > the first
> > > place, don't you agree?
> > 
> > I think removing ifdefs and using helper function are different
> > things.
> > You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().
> > 
> 
> I chose to do it like that because this function would otherwise be a
> 100% duplicate of the related cmdq helper :-)

Removing cl would change the interface of cmdq_pkt_create(). And this
is related to different maintainer's tree. So it would be a long time
to process. For you, only removing ifdes is urgent, so use
cmdq_pkt_create() is not urgent. So let's keep
mtk_drm_cmdq_pkt_create() and you could remove ifdefs.

Regards,
CK

> 
> 
> > Regards,
> > CK
> > 
> > > 
> > > Regards,
> > > Angelo
> > > 
> 
> 

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[AngeloGioacchino Del Regno]

Il 03/08/23 10:37, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Thu, 2023-08-03 at 10:25 +0200, AngeloGioacchino Del Regno wrote:
>> Il 03/08/23 08:28, CK Hu (胡俊光) ha scritto:
>>> Hi, Angelo:
>>>
>>> On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno
>>> wrote:
>>>> Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
>>>>> Hi, Angelo:
>>>>>
>>>>> On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno
>>>>> wrote:
>>>>>>     	
>>>>>> External email : Please do not click links or open
>>>>>> attachments
>>>>>> until
>>>>>> you have verified the sender or the content.
>>>>>>     Instead of stack allocating the cmdq_client and
>>>>>> cmdq_handle
>>>>>> structures
>>>>>> switch them to pointers, allowing us to migrate this driver
>>>>>> to
>>>>>> use
>>>>>> the
>>>>>> common functions provided by mtk-cmdq-helper.
>>>>>> In order to do this, it was also necessary to add a `priv`
>>>>>> pointer to
>>>>>> struct cmdq_client, as that's used to pass (in this case) a
>>>>>> mtk_crtc
>>>>>> handle to the ddp_cmdq_cb() mailbox RX callback function.
>>>>>>
>>>>>> Signed-off-by: AngeloGioacchino Del Regno <
>>>>>> angelogioacchino.delregno@collabora.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++-----
>>>>>> ---
>>>>>> -------
>>>>>> --
>>>>>>     include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
>>>>>>     2 files changed, 32 insertions(+), 76 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>>>> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>>>> index 0df62b076f49..b63289ab6787 100644
>>>>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>>>> @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
>>>>>>     	bool				pending_async_planes;
>>>>>>     
>>>>>>     #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>>>>>> -	struct cmdq_client		cmdq_client;
>>>>>> -	struct cmdq_pkt			cmdq_handle;
>>>>>> +	struct cmdq_client		*cmdq_client;
>>>>>> +	struct cmdq_pkt			*cmdq_handle;
>>>>>>     	u32				cmdq_event;
>>>>>>     	u32				cmdq_vblank_cnt;
>>>>>>     	wait_queue_head_t		cb_blocking_queue;
>>>>>> @@ -108,47 +108,6 @@ static void
>>>>>> mtk_drm_finish_page_flip(struct
>>>>>> mtk_drm_crtc *mtk_crtc)
>>>>>>     	}
>>>>>>     }
>>>>>>     
>>>>>> -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
>>>>>> -static int mtk_drm_cmdq_pkt_create(struct cmdq_client
>>>>>> *client,
>>>>>> struct cmdq_pkt *pkt,
>>>>>> -				   size_t size)
>>>>>> -{
>>>>>> -	struct device *dev;
>>>>>> -	dma_addr_t dma_addr;
>>>>>> -
>>>>>> -	pkt->va_base = kzalloc(size, GFP_KERNEL);
>>>>>> -	if (!pkt->va_base) {
>>>>>> -		kfree(pkt);
>>>>>> -		return -ENOMEM;
>>>>>> -	}
>>>>>> -	pkt->buf_size = size;
>>>>>> -	pkt->cl = (void *)client;
>>>>>
>>>>> I have a plan to remove cl in struct cmdq_pkt. But this
>>>>> modification
>>>>> would make this plan more difficult. So I would pending this
>>>>> patch
>>>>> until cl is removed from struct cmdq_pkt.
>>>>>
>>>>
>>>> I think that this ifdef cleanup is more urgent than the removal
>>>> of
>>>> `cl` from
>>>> struct cmdq_pkt, as those ifdefs shouldn't have reached upstream
>>>> in
>>>> the first
>>>> place, don't you agree?
>>>
>>> I think removing ifdefs and using helper function are different
>>> things.
>>> You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().
>>>
>>
>> I chose to do it like that because this function would otherwise be a
>> 100% duplicate of the related cmdq helper :-)
> 
> Removing cl would change the interface of cmdq_pkt_create(). And this
> is related to different maintainer's tree. So it would be a long time
> to process. For you, only removing ifdes is urgent, so use
> cmdq_pkt_create() is not urgent. So let's keep
> mtk_drm_cmdq_pkt_create() and you could remove ifdefs.
> 

Hello CK,

my CMDQ cleanup has been stuck on your intention to remove `cl` from the CMDQ
helpers for ** six months ** now.

Are you performing that removal, or can we just get this cleanup finally done?

Regards,
Angelo

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[CK Hu (胡俊光)]

Hi, Angelo:

On Tue, 2024-02-06 at 14:33 +0100, AngeloGioacchino Del Regno wrote:
> Il 03/08/23 10:37, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Thu, 2023-08-03 at 10:25 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 03/08/23 08:28, CK Hu (胡俊光) ha scritto:
> > > > Hi, Angelo:
> > > > 
> > > > On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno
> > > > wrote:
> > > > > Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
> > > > > > Hi, Angelo:
> > > > > > 
> > > > > > On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del
> > > > > > Regno
> > > > > > wrote:
> > > > > > >     	
> > > > > > > External email : Please do not click links or open
> > > > > > > attachments
> > > > > > > until
> > > > > > > you have verified the sender or the content.
> > > > > > >     Instead of stack allocating the cmdq_client and
> > > > > > > cmdq_handle
> > > > > > > structures
> > > > > > > switch them to pointers, allowing us to migrate this
> > > > > > > driver
> > > > > > > to
> > > > > > > use
> > > > > > > the
> > > > > > > common functions provided by mtk-cmdq-helper.
> > > > > > > In order to do this, it was also necessary to add a
> > > > > > > `priv`
> > > > > > > pointer to
> > > > > > > struct cmdq_client, as that's used to pass (in this case)
> > > > > > > a
> > > > > > > mtk_crtc
> > > > > > > handle to the ddp_cmdq_cb() mailbox RX callback function.
> > > > > > > 
> > > > > > > Signed-off-by: AngeloGioacchino Del Regno <
> > > > > > > angelogioacchino.delregno@collabora.com>
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107
> > > > > > > +++++++-----
> > > > > > > ---
> > > > > > > -------
> > > > > > > --
> > > > > > >     include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
> > > > > > >     2 files changed, 32 insertions(+), 76 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > > > index 0df62b076f49..b63289ab6787 100644
> > > > > > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > > > @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
> > > > > > >     	bool				pending_async_pla
> > > > > > > nes;
> > > > > > >     
> > > > > > >     #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > > > > > -	struct cmdq_client		cmdq_client;
> > > > > > > -	struct cmdq_pkt			cmdq_handle;
> > > > > > > +	struct cmdq_client		*cmdq_client;
> > > > > > > +	struct cmdq_pkt			*cmdq_handle;
> > > > > > >     	u32				cmdq_event;
> > > > > > >     	u32				cmdq_vblank_cnt;
> > > > > > >     	wait_queue_head_t		cb_blocking_queue
> > > > > > > ;
> > > > > > > @@ -108,47 +108,6 @@ static void
> > > > > > > mtk_drm_finish_page_flip(struct
> > > > > > > mtk_drm_crtc *mtk_crtc)
> > > > > > >     	}
> > > > > > >     }
> > > > > > >     
> > > > > > > -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > > > > > -static int mtk_drm_cmdq_pkt_create(struct cmdq_client
> > > > > > > *client,
> > > > > > > struct cmdq_pkt *pkt,
> > > > > > > -				   size_t size)
> > > > > > > -{
> > > > > > > -	struct device *dev;
> > > > > > > -	dma_addr_t dma_addr;
> > > > > > > -
> > > > > > > -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> > > > > > > -	if (!pkt->va_base) {
> > > > > > > -		kfree(pkt);
> > > > > > > -		return -ENOMEM;
> > > > > > > -	}
> > > > > > > -	pkt->buf_size = size;
> > > > > > > -	pkt->cl = (void *)client;
> > > > > > 
> > > > > > I have a plan to remove cl in struct cmdq_pkt. But this
> > > > > > modification
> > > > > > would make this plan more difficult. So I would pending
> > > > > > this
> > > > > > patch
> > > > > > until cl is removed from struct cmdq_pkt.
> > > > > > 
> > > > > 
> > > > > I think that this ifdef cleanup is more urgent than the
> > > > > removal
> > > > > of
> > > > > `cl` from
> > > > > struct cmdq_pkt, as those ifdefs shouldn't have reached
> > > > > upstream
> > > > > in
> > > > > the first
> > > > > place, don't you agree?
> > > > 
> > > > I think removing ifdefs and using helper function are different
> > > > things.
> > > > You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().
> > > > 
> > > 
> > > I chose to do it like that because this function would otherwise
> > > be a
> > > 100% duplicate of the related cmdq helper :-)
> > 
> > Removing cl would change the interface of cmdq_pkt_create(). And
> > this
> > is related to different maintainer's tree. So it would be a long
> > time
> > to process. For you, only removing ifdes is urgent, so use
> > cmdq_pkt_create() is not urgent. So let's keep
> > mtk_drm_cmdq_pkt_create() and you could remove ifdefs.
> > 
> 
> Hello CK,
> 
> my CMDQ cleanup has been stuck on your intention to remove `cl` from
> the CMDQ
> helpers for ** six months ** now.
> 
> Are you performing that removal, or can we just get this cleanup
> finally done?

It's busy last year and I almost forget this. Thank you to notice me. I
would send patch after Chinese new year (Feb 8th ~ Feb 14th).

Regards,
CK


> 
> Regards,
> Angelo
> 
> 
>