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@xxxxxxxxxxxx>
> > > Tested-by: Alexandre Mergnat <amergnat@xxxxxxxxxxxx>
> > > Acked-by: Sui Jingfeng <sui.jingfeng@xxxxxxxxx>
> > > Tested-by: Michael Walle <mwalle@xxxxxxxxxx> # on kontron-sbc-i1200
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > ---
> >
> > [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:
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[].
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.
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;
> > > +}
> > > +
>
>
>
