Hi Nicolas， At 2025-05-28 00:12:43, "Nicolas Frattaroli" <nicolas.frattar...@collabora.com> wrote: >Hi Andy, > >thank you for the driver. I'll leave some review comments inline. I don't >have specific knowledge on the DRM subsystem so my comments will be of more >general nature. >
Thank you very much for your review. >On Thursday, 3 April 2025 05:37:31 Central European Summer Time Andy Yan wrote: >> From: Andy Yan <andy....@rock-chips.com> >> >> Add driver extension for Synopsys DesignWare DPTX IP used >> on Rockchip RK3588 SoC. >> >> Signed-off-by: Andy Yan <andy....@rock-chips.com> >> Acked-by: Dmitry Baryshkov <lu...@kernel.org> >> >> --- >> >> (no changes since v2) >> >> Changes in v2: >> - no include uapi path >> - switch to drmm_encoder_init >> >> drivers/gpu/drm/rockchip/Kconfig | 9 ++ >> drivers/gpu/drm/rockchip/Makefile | 1 + >> drivers/gpu/drm/rockchip/dw_dp-rockchip.c | 154 ++++++++++++++++++++ >> drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 1 + >> drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + >> 5 files changed, 166 insertions(+) >> create mode 100644 drivers/gpu/drm/rockchip/dw_dp-rockchip.c >> >> diff --git a/drivers/gpu/drm/rockchip/Kconfig >> b/drivers/gpu/drm/rockchip/Kconfig >> index 26c4410b2407c..00315cc6be5a8 100644 >> --- a/drivers/gpu/drm/rockchip/Kconfig >> +++ b/drivers/gpu/drm/rockchip/Kconfig >> @@ -8,6 +8,7 @@ config DRM_ROCKCHIP >> select DRM_PANEL >> select VIDEOMODE_HELPERS >> select DRM_ANALOGIX_DP if ROCKCHIP_ANALOGIX_DP >> + select DRM_DW_DP if ROCKCHIP_DW_DP >> select DRM_DW_HDMI if ROCKCHIP_DW_HDMI >> select DRM_DW_HDMI_QP if ROCKCHIP_DW_HDMI_QP >> select DRM_DW_MIPI_DSI if ROCKCHIP_DW_MIPI_DSI >> @@ -58,6 +59,14 @@ config ROCKCHIP_CDN_DP >> RK3399 based SoC, you should select this >> option. >> >> +config ROCKCHIP_DW_DP >> + bool "Rockchip specific extensions for Synopsys DW DP" >> + help >> + This selects support for Rockchip SoC specific extensions >> + to enable Synopsys DesignWare Cores based DisplayPort transmit >> + controller support on Rockchip SoC, If you want to enable DP on >> + rk3588 based SoC, you should select this option. >> + >> config ROCKCHIP_DW_HDMI >> bool "Rockchip specific extensions for Synopsys DW HDMI" >> help >> diff --git a/drivers/gpu/drm/rockchip/Makefile >> b/drivers/gpu/drm/rockchip/Makefile >> index 2b867cebbc121..097f062399c7a 100644 >> --- a/drivers/gpu/drm/rockchip/Makefile >> +++ b/drivers/gpu/drm/rockchip/Makefile >> @@ -14,6 +14,7 @@ rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += >> dw_hdmi-rockchip.o >> rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI_QP) += dw_hdmi_qp-rockchip.o >> rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi-rockchip.o >> rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI2) += dw-mipi-dsi2-rockchip.o >> +rockchipdrm-$(CONFIG_ROCKCHIP_DW_DP) += dw_dp-rockchip.o >> rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o >> rockchipdrm-$(CONFIG_ROCKCHIP_LVDS) += rockchip_lvds.o >> rockchipdrm-$(CONFIG_ROCKCHIP_RGB) += rockchip_rgb.o >> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c >> b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c >> new file mode 100644 >> index 0000000000000..5ff8a6a54997e >> --- /dev/null >> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c >> @@ -0,0 +1,154 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2020 Rockchip Electronics Co., Ltd. >> + * >> + * Author: Zhang Yubing <yubing.zh...@rock-chips.com> >> + * Author: Andy Yan <andy....@rock-chips.com> >> + */ >> + >> +#include <linux/component.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <drm/bridge/dw_dp.h> >> +#include <drm/drm_atomic_helper.h> >> +#include <drm/drm_bridge.h> >> +#include <drm/drm_bridge_connector.h> > >I think there's a missing #include <drm/display/drm_dp_helper.h> here. It >gets pulled in implicitly in most configurations, but I think this is what >the s390 build failure from the kernel test robot report is about. I have included this header file in the file dw-dp.c. I think the error occurred during the robot test because the following patch[0] had not been merged yet. Now that this submission has been merged, and dw_dp-rockchip.c does not call functions from that header file, so it should no longer report any errors now. [0]https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=09cdda7a60f45784cebddf1fa2109d6279f9890b > >> +#include <drm/drm_of.h> >> +#include <drm/drm_print.h> >> +#include <drm/drm_probe_helper.h> >> +#include <drm/drm_simple_kms_helper.h> >> + >> +#include <linux/media-bus-format.h> >> +#include <linux/videodev2.h> >> + >> +#include "rockchip_drm_drv.h" >> +#include "rockchip_drm_vop.h" >> + >> +struct rockchip_dw_dp { >> + struct dw_dp *base; >> + struct device *dev; >> + struct rockchip_encoder encoder; >> +}; >> + >> +static inline struct rockchip_dw_dp *encoder_to_dp(struct drm_encoder >> *encoder) >> +{ >> + struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder); >> + >> + return container_of(rkencoder, struct rockchip_dw_dp, encoder); >> +} > >This function appears to be unused, and will generate a warning: > > linux/drivers/gpu/drm/rockchip/dw_dp-rockchip.c:33:38: warning: unused > function 'encoder_to_dp' [-Wunused-function] > >I assume it may be used in a follow-up series. I think it's fine to add it >there when it's needed and avoid the warning for now by removing it. Thank you for catching it， i will remove it in V4. > >> + >> +static int dw_dp_encoder_atomic_check(struct drm_encoder *encoder, >> + struct drm_crtc_state *crtc_state, >> + struct drm_connector_state *conn_state) >> +{ >> + struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state); >> + struct drm_atomic_state *state = conn_state->state; >> + struct drm_display_info *di = &conn_state->connector->display_info; >> + struct drm_bridge *bridge = drm_bridge_chain_get_first_bridge(encoder); >> + struct drm_bridge_state *bridge_state = >> drm_atomic_get_new_bridge_state(state, bridge); >> + u32 bus_format = bridge_state->input_bus_cfg.format; >> + >> + switch (bus_format) { >> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >> + s->output_mode = ROCKCHIP_OUT_MODE_YUV420; >> + break; >> + case MEDIA_BUS_FMT_YUYV10_1X20: >> + case MEDIA_BUS_FMT_YUYV8_1X16: >> + s->output_mode = ROCKCHIP_OUT_MODE_S888_DUMMY; >> + break; >> + case MEDIA_BUS_FMT_RGB101010_1X30: >> + case MEDIA_BUS_FMT_RGB888_1X24: >> + case MEDIA_BUS_FMT_RGB666_1X24_CPADHI: >> + case MEDIA_BUS_FMT_YUV10_1X30: >> + case MEDIA_BUS_FMT_YUV8_1X24: >> + default: >> + s->output_mode = ROCKCHIP_OUT_MODE_AAAA; >> + break; >> + } >> + >> + s->output_type = DRM_MODE_CONNECTOR_DisplayPort; >> + s->bus_format = bus_format; >> + s->bus_flags = di->bus_flags; >> + s->color_space = V4L2_COLORSPACE_DEFAULT; > >Reading the VOP2 code s->color_space gets read by, it seems this results >in the output always using BT.709 as the colour space in YUV output mode. >Is my understanding of the code correct? > >I don't know if DP 1.4 is limited with regards to HDR or if this is just >left for later to implement, but BT.709 in the case of an HDR RGB VOP >input seems wrong, unless VOP2 sets the output color_space to something like >BT.2020 or similar in the case of output_mode == ROCKCHIP_OUT_MODE_AAAA and >I'm not seeing it. In our downstream code, when DP is in HDR output mode, the color_space will be set to V4L2_COLORSPACE_BT2020. Therefore, when we later support HDR output on the mainline, we can make the same adjustment. > >> + >> + return 0; >> +} >> + >> +static const struct drm_encoder_helper_funcs dw_dp_encoder_helper_funcs = { >> + .atomic_check = dw_dp_encoder_atomic_check, >> +}; >> + >> +static int dw_dp_rockchip_bind(struct device *dev, struct device *master, >> void *data) >> +{ >> + struct dw_dp_plat_data plat_data; >> + struct drm_device *drm_dev = data; >> + struct rockchip_dw_dp *dp; >> + struct drm_encoder *encoder; >> + struct drm_connector *connector; >> + int ret; >> + >> + dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL); >> + if (!dp) >> + return -ENOMEM; >> + >> + dp->dev = dev; >> + plat_data.max_link_rate = 810000; >> + encoder = &dp->encoder.encoder; >> + encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev, >> dev->of_node); >> + rockchip_drm_encoder_set_crtc_endpoint_id(&dp->encoder, dev->of_node, >> 0, 0); >> + >> + ret = drmm_encoder_init(drm_dev, encoder, NULL, DRM_MODE_ENCODER_TMDS, >> NULL); >> + if (ret) >> + return ret; >> + drm_encoder_helper_add(encoder, &dw_dp_encoder_helper_funcs); >> + >> + dp->base = dw_dp_bind(dev, encoder, &plat_data); >> + if (IS_ERR(dp->base)) { >> + ret = PTR_ERR(dp->base); >> + return ret; >> + } >> + >> + connector = drm_bridge_connector_init(drm_dev, encoder); >> + if (IS_ERR(connector)) { >> + ret = PTR_ERR(connector); >> + return dev_err_probe(dev, ret, "Failed to init bridge >> connector"); >> + } >> + >> + drm_connector_attach_encoder(connector, encoder); >> + >> + return 0; >> +} >> + >> +static const struct component_ops dw_dp_rockchip_component_ops = { >> + .bind = dw_dp_rockchip_bind, >> +}; >> + >> +static int dw_dp_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + >> + return component_add(dev, &dw_dp_rockchip_component_ops); >> +} >> + >> +static void dw_dp_remove(struct platform_device *pdev) >> +{ >> + struct rockchip_dw_dp *dp = platform_get_drvdata(pdev); > >Does one of the helper functions or something else set drvdata? Otherwise >I don't see how this is ever non-null. Thanks for catching it， will be fixed in V4. > >> + >> + component_del(dp->dev, &dw_dp_rockchip_component_ops); >> +} >> + >> +static const struct of_device_id dw_dp_of_match[] = { >> + { .compatible = "rockchip,rk3588-dp", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, dw_dp_of_match); >> + >> +struct platform_driver dw_dp_driver = { >> + .probe = dw_dp_probe, >> + .remove = dw_dp_remove, >> + .driver = { >> + .name = "dw-dp", >> + .of_match_table = dw_dp_of_match, >> + }, >> +}; >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> index ed88788e04dd2..687bb7b252e8e 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> @@ -531,6 +531,7 @@ static int __init rockchip_drm_init(void) >> ADD_ROCKCHIP_SUB_DRIVER(rockchip_dp_driver, >> CONFIG_ROCKCHIP_ANALOGIX_DP); >> ADD_ROCKCHIP_SUB_DRIVER(cdn_dp_driver, CONFIG_ROCKCHIP_CDN_DP); >> + ADD_ROCKCHIP_SUB_DRIVER(dw_dp_driver, CONFIG_ROCKCHIP_DW_DP); >> ADD_ROCKCHIP_SUB_DRIVER(dw_hdmi_rockchip_pltfm_driver, >> CONFIG_ROCKCHIP_DW_HDMI); >> ADD_ROCKCHIP_SUB_DRIVER(dw_hdmi_qp_rockchip_pltfm_driver, >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h >> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h >> index c183e82a42a51..2e86ad00979c4 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h >> @@ -87,6 +87,7 @@ int rockchip_drm_encoder_set_crtc_endpoint_id(struct >> rockchip_encoder *rencoder, >> struct device_node *np, int port, >> int reg); >> int rockchip_drm_endpoint_is_subdriver(struct device_node *ep); >> extern struct platform_driver cdn_dp_driver; >> +extern struct platform_driver dw_dp_driver; >> extern struct platform_driver dw_hdmi_rockchip_pltfm_driver; >> extern struct platform_driver dw_hdmi_qp_rockchip_pltfm_driver; >> extern struct platform_driver dw_mipi_dsi_rockchip_driver; >> > >Other than that, the driver looks great, thank you! I've tested it on my >ROCK 5T over DP altmode, where it correctly interfaces with a DP monitor >I have through an alt-mode adapter. So feel free to add a > >Tested-by: Nicolas Frattaroli <nicolas.frattar...@collabora.com> Thanks again. > >Kind regards, >Nicolas Frattaroli > > >
