Messages in this thread | | | From | CK Hu (胡俊光) <> | Subject | Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions | Date | Wed, 7 Feb 2024 03:49:19 +0000 |
| |
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 > > >
| |