From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C3FC618CBF4; Mon, 7 Oct 2024 09:08:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728292138; cv=none; b=oTgMBT7iwcm7yO5Jd0JWNLPkEYNFxMRsl4ECIoBEmemy7lsJgc0562oHY6kAdPK9DM1sg2LQphZY/2KBloU8w7bUad+bDr4be47uOuU09elZLH5YVGXTJIXGWPFpYS5oKw4gR3t/Y+8Vah7eA196N2sqlGAKAwxN1UVsV/WJ2qg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728292138; c=relaxed/simple; bh=IIry1dEYTifF6DEPd31BX5nVO/lm731GurpTFpXJErI=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=C2meX06pYRjyjdUcutNpEzBL3xhY74zLgeDQi5aFZ/kwd2BLUofGT119qqaoKhThPnQO6x9ojlOcGDc/7bRCrNHwatrfc98JUvR9MmRhqDyKvuzR8c3w+TYR/qiKg8cecWpoSUEHJqCx37m0owC1ypL6erY69S9DO9Bw9O41i6U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=TpV6g2tT; arc=none smtp.client-ip=148.251.105.195 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="TpV6g2tT" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1728292133; bh=IIry1dEYTifF6DEPd31BX5nVO/lm731GurpTFpXJErI=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=TpV6g2tTzqJSEfvFXUsJr+805MendttYBqk7k5qFBc78STg3QgoyOHMj/tfTp2kAR tFn0pLf4NrU6kjGtIMmu+jyrfcbKX7a4jOXaoCjNc3ABwwAaVjrZKHJXo8Dgzlghf8 /Nijpl3eVUSXaCb/JzYDnxERsKMZpUICzoCJ9gsPmjTbJYqOFYWk88QQlrp2nwXl8I abIgY2hF2vC2q1MxdRbLLl552O/9/26akUWC2q7ST4WAXPjXIUX6H2kRDCYR5QsclR adQBHhqzUZ6YxZvbMRPAZecf2w4hfwNzKt8DCiFWsbsQndiwX/5QKOOaZeaissIbP4 QwuCjP56dagTw== Received: from [192.168.1.100] (2-237-20-237.ip236.fastwebnet.it [2.237.20.237]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: kholk11) by bali.collaboradmins.com (Postfix) with ESMTPSA id C956517E10D6; Mon, 7 Oct 2024 11:08:52 +0200 (CEST) Message-ID: <9bda89e0-d236-40e2-a109-0de5fb4bd228@collabora.com> Date: Mon, 7 Oct 2024 11:08:52 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v10 3/3] drm/mediatek: Implement OF graphs support for display paths To: =?UTF-8?B?Q0sgSHUgKOiDoeS/iuWFiSk=?= , "chunkuang.hu@kernel.org" Cc: "linux-kernel@vger.kernel.org" , "linux-mediatek@lists.infradead.org" , "sui.jingfeng@linux.dev" , "wenst@chromium.org" , Sjoerd Simons , "devicetree@vger.kernel.org" , "tzimmermann@suse.de" , =?UTF-8?B?U2hhd24gU3VuZyAo5a6L5a2d6KyZKQ==?= , "mripard@kernel.org" , =?UTF-8?B?Sml0YW8gU2hpICjnn7PorrDmtpsp?= , "michael@walle.cc" , "daniel@ffwll.ch" , "p.zabel@pengutronix.de" , "conor+dt@kernel.org" , "maarten.lankhorst@linux.intel.com" , "robh@kernel.org" , "dri-devel@lists.freedesktop.org" , "airlied@gmail.com" , "krzysztof.kozlowski+dt@linaro.org" , "kernel@collabora.com" , "matthias.bgg@gmail.com" , =?UTF-8?B?WXUtY2hhbmcgTGVlICjmnY7nprnnkosp?= , "mwalle@kernel.org" , "linux-arm-kernel@lists.infradead.org" , Alexandre Mergnat References: <20240910105054.125005-1-angelogioacchino.delregno@collabora.com> <01020191db8f22cd-0f5d733b-420e-453c-8607-a3e9f70f32d6-000000@eu-west-1.amazonses.com> <56c4e87c-6774-4542-8ae7-dab89750081c@collabora.com> <58ee09aeb5a224dbc8faee236ed1a77ce3fbd011.camel@mediatek.com> <04f1506b23b41c775e0735b5b3189b4118500715.camel@mediatek.com> From: AngeloGioacchino Del Regno Content-Language: en-US In-Reply-To: <04f1506b23b41c775e0735b5b3189b4118500715.camel@mediatek.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Il 07/10/24 08:57, CK Hu (胡俊光) ha scritto: > Hi, Angelo: > > On Fri, 2024-10-04 at 12:22 +0200, AngeloGioacchino Del Regno wrote: >> Il 04/10/24 08:03, CK Hu (胡俊光) ha scritto: >>> Hi, Angelo: >>> >>> On Tue, 2024-10-01 at 13:33 +0200, AngeloGioacchino Del Regno wrote: >>>> Il 01/10/24 12:07, CK Hu (胡俊光) ha scritto: >>>>> Hi, Angelo: >>>>> >>>>> On Tue, 2024-09-10 at 10:51 +0000, AngeloGioacchino Del Regno wrote: >>>>>> It is impossible to add each and every possible DDP path combination >>>>>> for each and every possible combination of SoC and board: right now, >>>>>> this driver hardcodes configuration for 10 SoCs and this is going to >>>>>> grow larger and larger, and with new hacks like the introduction of >>>>>> mtk_drm_route which is anyway not enough for all final routes as the >>>>>> DSI cannot be connected to MERGE if it's not a dual-DSI, or enabling >>>>>> DSC preventively doesn't work if the display doesn't support it, or >>>>>> others. >>>>>> >>>>>> Since practically all display IPs in MediaTek SoCs support being >>>>>> interconnected with different instances of other, or the same, IPs >>>>>> or with different IPs and in different combinations, the final DDP >>>>>> pipeline is effectively a board specific configuration. >>>>>> >>>>>> Implement OF graphs support to the mediatek-drm drivers, allowing to >>>>>> stop hardcoding the paths, and preventing this driver to get a huge >>>>>> amount of arrays for each board and SoC combination, also paving the >>>>>> way to share the same mtk_mmsys_driver_data between multiple SoCs, >>>>>> making it more straightforward to add support for new chips. >>>>>> >>>>>> Reviewed-by: Alexandre Mergnat >>>>>> Tested-by: Alexandre Mergnat >>>>>> Acked-by: Sui Jingfeng >>>>>> Tested-by: Michael Walle # on kontron-sbc-i1200 >>>>>> Signed-off-by: AngeloGioacchino Del Regno >>>>>> --- >>>>> >>>>> [snip] >>>>> >>>>>> + >>>>>> +bool mtk_ovl_adaptor_is_comp_present(struct device_node *node) >>>>>> +{ >>>>>> + enum mtk_ovl_adaptor_comp_type type; >>>>>> + int ret; >>>>>> + >>>>>> + ret = ovl_adaptor_of_get_ddp_comp_type(node, &type); >>>>>> + if (ret) >>>>>> + return false; >>>>>> + >>>>>> + if (type >= OVL_ADAPTOR_TYPE_NUM) >>>>>> + return false; >>>>>> + >>>>>> + /* >>>>>> + * ETHDR and Padding are used exclusively in OVL Adaptor: if this >>>>>> + * component is not one of those, it's likely not an OVL Adaptor path. >>>>>> + */ >>>>> >>>>> I don't know your logic here. >>>>> The OVL Adaptor pipeline is: >>>>> >>>>> mdp_rdma -> padding ---+ +-------+ >>>>> Merge -> | | >>>>> mdp_rdma -> padding ---+ | | >>>>> | | >>>>> mdp_rdma -> padding ---+ | | >>>>> Merge -> | | >>>>> mdp_rdma -> padding ---+ | | >>>>> | ETHDR | >>>>> mdp_rdma -> padding ---+ | | >>>>> Merge -> | | >>>>> mdp_rdma -> padding ---+ | | >>>>> | | >>>>> mdp_rdma -> padding ---+ | | >>>>> Merge -> | | >>>>> mdp_rdma -> padding ---+ +-------+ >>>>> >>>>> So mdp_rdma and merge is not OVL Adaptor? >>>>> >>>> >>>> Yes, and in device tree, you express that exactly like you just pictured. >>>> >>>> OVL Adaptor is treated like a software component internally, and manages >>>> its own merge pipes exactly like before this commit. >>>> >>>> In case there will be any need to express OVL Adaptor as hardware component, >>>> it will be possible to do so with no modification to the bindings. >>>> >>>>> >>>>>> + return type == OVL_ADAPTOR_TYPE_ETHDR || type == OVL_ADAPTOR_TYPE_PADDING; >>>>>> +} >>>>>> + >>>>>> >>>>> >>>>> [snip] >>>>> >>>>>> + >>>>>> +/** >>>>>> + * mtk_drm_of_ddp_path_build_one - Build a Display HW Pipeline for a CRTC Path >>>>>> + * @dev: The mediatek-drm device >>>>>> + * @cpath: CRTC Path relative to a VDO or MMSYS >>>>>> + * @out_path: Pointer to an array that will contain the new pipeline >>>>>> + * @out_path_len: Number of entries in the pipeline array >>>>>> + * >>>>>> + * MediaTek SoCs can use different DDP hardware pipelines (or paths) depending >>>>>> + * on the board-specific desired display configuration; this function walks >>>>>> + * through all of the output endpoints starting from a VDO or MMSYS hardware >>>>>> + * instance and builds the right pipeline as specified in device trees. >>>>>> + * >>>>>> + * Return: >>>>>> + * * %0 - Display HW Pipeline successfully built and validated >>>>>> + * * %-ENOENT - Display pipeline was not specified in device tree >>>>>> + * * %-EINVAL - Display pipeline built but validation failed >>>>>> + * * %-ENOMEM - Failure to allocate pipeline array to pass to the caller >>>>>> + */ >>>>>> +static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path cpath, >>>>>> + const unsigned int **out_path, >>>>>> + unsigned int *out_path_len) >>>>>> +{ >>>>>> + struct device_node *next, *prev, *vdo = dev->parent->of_node; >>>>>> + unsigned int temp_path[DDP_COMPONENT_DRM_ID_MAX] = { 0 }; >>>>>> + unsigned int *final_ddp_path; >>>>>> + unsigned short int idx = 0; >>>>>> + bool ovl_adaptor_comp_added = false; >>>>>> + int ret; >>>>>> + >>>>>> + /* Get the first entry for the temp_path array */ >>>>>> + ret = mtk_drm_of_get_ddp_ep_cid(vdo, 0, cpath, &next, &temp_path[idx]); >>>>>> + if (ret) { >>>>>> + if (next && temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) { >>>>> >>>>> mdp_rdma would not be DDP_COMPONENT_DRM_OVL_ADAPTOR. >>>> >>>> This piece of code just avoids adding OVL_ADAPTOR more than once to the pipeline. >>>> >>>>> >>>>>> + dev_dbg(dev, "Adding OVL Adaptor for %pOF\n", next); >>>>>> + ovl_adaptor_comp_added = true; >>>>>> + } else { >>>>>> + if (next) >>>>>> + dev_err(dev, "Invalid component %pOF\n", next); >>>>>> + else >>>>>> + dev_err(dev, "Cannot find first endpoint for path %d\n", cpath); >>>>>> + >>>>>> + return ret; >>>>>> + } >>>>>> + } >>>>>> + idx++; >>>>>> + >>>>>> + /* >>>>>> + * Walk through port outputs until we reach the last valid mediatek-drm component. >>>>>> + * To be valid, this must end with an "invalid" component that is a display node. >>>>>> + */ >>>>>> + do { >>>>>> + prev = next; >>>>>> + ret = mtk_drm_of_get_ddp_ep_cid(next, 1, cpath, &next, &temp_path[idx]); >>>>>> + of_node_put(prev); >>>>>> + if (ret) { >>>>>> + of_node_put(next); >>>>>> + break; >>>>>> + } >>>>>> + >>>>>> + /* >>>>>> + * If this is an OVL adaptor exclusive component and one of those >>>>>> + * was already added, don't add another instance of the generic >>>>>> + * DDP_COMPONENT_OVL_ADAPTOR, as this is used only to decide whether >>>>>> + * to probe that component master driver of which only one instance >>>>>> + * is needed and possible. >>>>>> + */ >>>>>> + if (temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) { >>>>> >>>>> merge would not be DDP_COMPONENT_DRM_OVL_ADAPTOR. >>>>> Finally, the path would be: >>>>> >>>>> mdp_rdma -> ovl adaptor (padding) -> merge -> (ethdr is skipped here) ... >>>>> >>>> >>>> Again, the path in the OF graph is expressed exactly like you said. >>> >>> I know the OF graph is expressed like I said. >>> But I care about the path in driver should like this: >> >> Ok, now I understand your concern. >> >>> >>> static const unsigned int mt8195_mtk_ddp_ext[] = { >>> DDP_COMPONENT_DRM_OVL_ADAPTOR, >>> DDP_COMPONENT_MERGE5, >>> DDP_COMPONENT_DP_INTF1, >>> }; >>> >>> In OF graph, the first component is mdp_rdma and mtk_ovl_adaptor_is_comp_present() would return false for mdp_rdma. >>> So I think this would make mtk_drm_of_ddp_path_build_one() return error and the path is not created. >>> If I'm wrong, please explain how this code would result in the path like mt8195_mtk_ddp_ext[]. >>> >> >> The MDP_RDMA usage in mtk_disp_ovl_adaptor is hardcoded: in function >> mtk_ovl_adaptor_layer_config(), the rdma_l/r are always OVL_ADAPTOR_MDP_RDMAx, >> then function mtk_ovl_adaptor_dma_dev_get(), always returns the MDP_RDMA0 >> component, same for mtk_ovl_adaptor_get_{num_formats,formats}() which always >> call mtk_mdp_rdma_get_formats() for OVL_ADAPTOR_MDP_RDMA0. >> >> I have just rechecked how I expressed the path for MT8195 Tomato, where the >> external display works with OF Graphs, and it was missing MDP_RDMA entirely. >> >> The path was ethdr -> merge -> dp_intf1 ... and it should be mdp_rdma -> (etc). >> >> Effectively, that is indeed wrong, as all of the steps must be expressed >> inside of the graph. >> >> Since the OVL Adaptor's RDMA instances' compatible strings do *not* collide >> with the others, as OVL Adaptor uses compatible mediatek,mt8195-vdo1-rdma, >> and the regular one uses compatible mediatek,mt8195-disp-rdma, we can resolve >> this issue by changing function mtk_ovl_adaptor_is_comp_present() >> >> from >> >> return type == OVL_ADAPTOR_TYPE_ETHDR || type == OVL_ADAPTOR_TYPE_PADDING; >> >> to >> >> return type == OVL_ADAPTOR_TYPE_ETHDR || type == OVL_ADAPTOR_TYPE_PADDING || >> type == OVL_ADAPTOR_TYPE_MDP_RDMA; >> >> is that okay for you? > > I just want the path to be like mt8195_mtk_ddp_ext[]. If so, I'm ok. > Yes, that makes the path that you described to be exactly like mt8195_mtk_ddp_ext[]. I will send a v11 later today. Cheers, Angelo > Regards, > CK > >> >>> If you does not test this with mt8195 external display path, maybe we could just drop the code about OVL adaptor with a TODO comment. >>> >> >> And yes, as I said, external display paths were tested on 8195, actually both >> on Kontron i1200 by Michael Walle and on MT8195 Tomato by myself. >> >> Thanks again, >> Angelo >> >>> Regards, >>> CK >>> >>>> >>>> Regards, >>>> Angelo >>>> >>>>> Regards, >>>>> CK >>>>> >>>>>> + if (!ovl_adaptor_comp_added) >>>>>> + ovl_adaptor_comp_added = true; >>>>>> + else >>>>>> + idx--; >>>>>> + } >>>>>> + } while (++idx < DDP_COMPONENT_DRM_ID_MAX); >>>>>> + >>>>>> + /* >>>>>> + * The device component might not be enabled: in that case, don't >>>>>> + * check the last entry and just report that the device is missing. >>>>>> + */ >>>>>> + if (ret == -ENODEV) >>>>>> + return ret; >>>>>> + >>>>>> + /* If the last entry is not a final display output, the configuration is wrong */ >>>>>> + switch (temp_path[idx - 1]) { >>>>>> + case DDP_COMPONENT_DP_INTF0: >>>>>> + case DDP_COMPONENT_DP_INTF1: >>>>>> + case DDP_COMPONENT_DPI0: >>>>>> + case DDP_COMPONENT_DPI1: >>>>>> + case DDP_COMPONENT_DSI0: >>>>>> + case DDP_COMPONENT_DSI1: >>>>>> + case DDP_COMPONENT_DSI2: >>>>>> + case DDP_COMPONENT_DSI3: >>>>>> + break; >>>>>> + default: >>>>>> + dev_err(dev, "Invalid display hw pipeline. Last component: %d (ret=%d)\n", >>>>>> + temp_path[idx - 1], ret); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + final_ddp_path = devm_kmemdup(dev, temp_path, idx * sizeof(temp_path[0]), GFP_KERNEL); >>>>>> + if (!final_ddp_path) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + dev_dbg(dev, "Display HW Pipeline built with %d components.\n", idx); >>>>>> + >>>>>> + /* Pipeline built! */ >>>>>> + *out_path = final_ddp_path; >>>>>> + *out_path_len = idx; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>> >>>> >>>> >> >> -- AngeloGioacchino Del Regno Senior Software Engineer Collabora Ltd. Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK Registered in England & Wales, no. 5513718