<html><body><p> <pre> 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 <amergnat@baylibre.com> > > > > > Tested-by: Alexandre Mergnat <amergnat@baylibre.com> > > > > > Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev> > > > > > Tested-by: Michael Walle <mwalle@kernel.org> # on kontron-sbc-i1200 > > > > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > > > > --- > > > > > > > > [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. 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; > > > > > +} > > > > > + > > > > > > > > > > > </pre> </p></body></html><pre>************* MEDIATEK Confidentiality Notice ******************** The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you! </pre>