Re: [PATCH v4 3/3] drm/mediatek: Implement OF graphs support for display paths
Hi Angelo, > >> 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. > > > > paths might be optional, see comment in mtk_drm_kms_init(). But with > > this patch, you'll get an -EINVAL with a disabled path. See my > > proposals how to fix that below. > > I might not be understanding the reason behind allowing that but, per my > logic, if > a board does have a path, then it's written in devicetree and enabled - > otherwise, > it should not be there at all, in principle. > > > Can you explain a bit more extensively the reason(s) why we need to account > for disabled paths? Paths should be (and this was already supported before this patch with the hardcoded paths) disabled with the status property. This way you can have a common board configuration where all the paths are already described but are disabled. An overlay (or maybe another dts variant) can then just enable the pipeline/output port by overwriting the status property. Also, this is the usual DT usage, as a node with status = "disabled" should just be skipped. Without handling this, the current code will return -EINVAL during probe (IIRC, my vacation might have reset my memory :o). -michael signature.asc Description: PGP signature
Re: [PATCH v4 3/3] drm/mediatek: Implement OF graphs support for display paths
Reviewed-by: Alexandre Mergnat Tested-by: Alexandre Mergnat On 16/05/2024 10:11, 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. -- Regards, Alexandre
Re: [PATCH v4 3/3] drm/mediatek: Implement OF graphs support for display paths
Il 17/05/24 11:49, Michael Walle ha scritto:
Hi Angelo,
On Thu May 16, 2024 at 10:11 AM CEST, AngeloGioacchino Del Regno wrote:
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.
paths might be optional, see comment in mtk_drm_kms_init(). But with
this patch, you'll get an -EINVAL with a disabled path. See my
proposals how to fix that below.
I might not be understanding the reason behind allowing that but, per my logic,
if
a board does have a path, then it's written in devicetree and enabled -
otherwise,
it should not be there at all, in principle.
Can you explain a bit more extensively the reason(s) why we need to account
for disabled paths?
With these changes and the following two patches I was able to get
DisplayPort working on vdosys1. vdosys0 wasn't used at all.
https://lore.kernel.org/r/20240516145824.1669263-1-mwa...@kernel.org/
https://lore.kernel.org/r/20240517093024.1702750-1-mwa...@kernel.org/
I've already successfully tested a former version with DSI output on
vdosys0.
Thanks for working on this!
Thank you for helping with the testing! :-)
Cheers,
Angelo
+/**
+ * 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, , _path[idx]);
+ if (ret) {
+ if (next && temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
+ dev_err(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, ,
_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) {
+ if (!ovl_adaptor_comp_added)
+ ovl_adaptor_comp_added = true;
+ else
+ idx--;
+ }
+ } while (++idx < DDP_COMPONENT_DRM_ID_MAX);
/* The device might not be disabled. In that case, don't check the last
* entry but just report the missing device. */
if (ret == -ENODEV)
return ret;
+
+ /* If the last entry is not a final display output, the configuration
is
Re: [PATCH v4 3/3] drm/mediatek: Implement OF graphs support for display paths
Hi Angelo,
On Thu May 16, 2024 at 10:11 AM CEST, AngeloGioacchino Del Regno wrote:
> 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.
paths might be optional, see comment in mtk_drm_kms_init(). But with
this patch, you'll get an -EINVAL with a disabled path. See my
proposals how to fix that below.
With these changes and the following two patches I was able to get
DisplayPort working on vdosys1. vdosys0 wasn't used at all.
https://lore.kernel.org/r/20240516145824.1669263-1-mwa...@kernel.org/
https://lore.kernel.org/r/20240517093024.1702750-1-mwa...@kernel.org/
I've already successfully tested a former version with DSI output on
vdosys0.
Thanks for working on this!
> +/**
> + * 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, , _path[idx]);
> + if (ret) {
> + if (next && temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
> + dev_err(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, ,
> _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) {
> + if (!ovl_adaptor_comp_added)
> + ovl_adaptor_comp_added = true;
> + else
> + idx--;
> + }
> + } while (++idx < DDP_COMPONENT_DRM_ID_MAX);
/* The device might not be disabled. In that case, don't check the last
* entry but just report the missing device. */
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;
> +
