Messages in this thread | | | Date | Tue, 6 Feb 2024 14:33:10 +0100 | Subject | Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions | From | 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
| |