| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0e6afba4-4312-49b4-bd92-23f12faf449a@linaro.org>
Date: Mon, 19 Aug 2024 18:25:24 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Jerome Brunet <jbrunet@...libre.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>
Cc: Kevin Hilman <khilman@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
dri-devel@...ts.freedesktop.org, linux-amlogic@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/9] drm/meson: dw-hdmi: use matched data
On 30/07/2024 14:50, Jerome Brunet wrote:
> Using several string comparisons with if/else if/else clauses
> is fairly inefficient and does not scale well.
Inefficient in which way ? speed ? code size ?
It doesn't scale, but AFAIK Amlogic stopped using the Synopsys DWC controller after the G12B SoCs....
>
> Use matched data to tweak the driver depending on the matched
> SoC instead.
This leads to a very overcomplicated code I'll need some time to review and understand
>
> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> ---
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 209 +++++++++++++++++---------
> 1 file changed, 139 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 7c39e5c99043..ef059c5ef520 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -125,8 +125,17 @@
> #define HHI_HDMI_PHY_CNTL4 0x3b0 /* 0xec */
> #define HHI_HDMI_PHY_CNTL5 0x3b4 /* 0xed */
>
> +struct meson_dw_hdmi_speed {
> + const struct reg_sequence *regs;
> + unsigned int reg_num;
> + unsigned int limit;
> +};
> +
> struct meson_dw_hdmi_data {
> int (*reg_init)(struct device *dev);
> + const struct meson_dw_hdmi_speed *speeds;
> + unsigned int speed_num;
> + bool use_drm_infoframe;
> u32 cntl0_init;
> u32 cntl1_init;
> };
> @@ -185,78 +194,33 @@ struct meson_dw_hdmi {
> struct regmap *top;
> };
>
> -static inline int dw_hdmi_is_compatible(struct meson_dw_hdmi *dw_hdmi,
> - const char *compat)
> -{
> - return of_device_is_compatible(dw_hdmi->dev->of_node, compat);
> -}
> -
> -/* Bridge */
> -
> /* Setup PHY bandwidth modes */
> -static void meson_hdmi_phy_setup_mode(struct meson_dw_hdmi *dw_hdmi,
> +static int meson_hdmi_phy_setup_mode(struct meson_dw_hdmi *dw_hdmi,
> const struct drm_display_mode *mode,
> bool mode_is_420)
> {
> struct meson_drm *priv = dw_hdmi->priv;
> unsigned int pixel_clock = mode->clock;
> + int i;
>
> /* For 420, pixel clock is half unlike venc clock */
> - if (mode_is_420) pixel_clock /= 2;
> -
> - if (dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
> - dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxm-dw-hdmi")) {
> - if (pixel_clock >= 371250) {
> - /* 5.94Gbps, 3.7125Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x333d3282);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2136315b);
> - } else if (pixel_clock >= 297000) {
> - /* 2.97Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33303382);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2036315b);
> - } else if (pixel_clock >= 148500) {
> - /* 1.485Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33303362);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2016315b);
> - } else {
> - /* 742.5Mbps, and below */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33604142);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x0016315b);
> - }
> - } else if (dw_hdmi_is_compatible(dw_hdmi,
> - "amlogic,meson-gxbb-dw-hdmi")) {
> - if (pixel_clock >= 371250) {
> - /* 5.94Gbps, 3.7125Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33353245);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2100115b);
> - } else if (pixel_clock >= 297000) {
> - /* 2.97Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33634283);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0xb000115b);
> - } else {
> - /* 1.485Gbps, and below */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33632122);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2000115b);
> - }
> - } else if (dw_hdmi_is_compatible(dw_hdmi,
> - "amlogic,meson-g12a-dw-hdmi")) {
> - if (pixel_clock >= 371250) {
> - /* 5.94Gbps, 3.7125Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x37eb65c4);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x0000080b);
> - } else if (pixel_clock >= 297000) {
> - /* 2.97Gbps */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33eb6262);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x00000003);
> - } else {
> - /* 1.485Gbps, and below */
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0x33eb4242);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL3, 0x2ab0ff3b);
> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL5, 0x00000003);
> - }
> + if (mode_is_420)
> + pixel_clock /= 2;
> +
> + for (i = 0; i < dw_hdmi->data->speed_num; i++) {
> + if (pixel_clock >= dw_hdmi->data->speeds[i].limit)
> + break;
> }
> +
> + /* No match found - Last entry should have a 0 limit */
> + if (WARN_ON(i == dw_hdmi->data->speed_num))
> + return -EINVAL;
> +
> + regmap_multi_reg_write(priv->hhi,
> + dw_hdmi->data->speeds[i].regs,
> + dw_hdmi->data->speeds[i].reg_num);
> +
> + return 0;
> }
>
> static inline void meson_dw_hdmi_phy_reset(struct meson_dw_hdmi *dw_hdmi)
> @@ -543,22 +507,133 @@ static int meson_dw_init_regmap_g12(struct device *dev)
> return 0;
> }
>
> +static const struct reg_sequence gxbb_3g7_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33353245 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2100115b },
> +};
> +
> +static const struct reg_sequence gxbb_3g_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33634283 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0xb000115b },
> +};
> +
> +static const struct reg_sequence gxbb_def_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33632122 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2000115b },
> +};
> +
> +static const struct meson_dw_hdmi_speed gxbb_speeds[] = {
> + {
> + .limit = 371250,
> + .regs = gxbb_3g7_regs,
> + .reg_num = ARRAY_SIZE(gxbb_3g7_regs)
> + }, {
> + .limit = 297000,
> + .regs = gxbb_3g_regs,
> + .reg_num = ARRAY_SIZE(gxbb_3g_regs)
> + }, {
> + .regs = gxbb_def_regs,
> + .reg_num = ARRAY_SIZE(gxbb_def_regs)
> + }
> +};
> +
> static const struct meson_dw_hdmi_data meson_dw_hdmi_gxbb_data = {
> .reg_init = meson_dw_init_regmap_gx,
> .cntl0_init = 0x0,
> .cntl1_init = PHY_CNTL1_INIT | PHY_INVERT,
> + .use_drm_infoframe = false,
> + .speeds = gxbb_speeds,
> + .speed_num = ARRAY_SIZE(gxbb_speeds),
> +};
> +
> +static const struct reg_sequence gxl_3g7_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x333d3282 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2136315b },
> +};
> +
> +static const struct reg_sequence gxl_3g_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33303382 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2036315b },
> +};
> +
> +static const struct reg_sequence gxl_def_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33303362 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2016315b },
> +};
> +
> +static const struct reg_sequence gxl_270m_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33604142 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x0016315b },
> +};
> +
> +static const struct meson_dw_hdmi_speed gxl_speeds[] = {
> + {
> + .limit = 371250,
> + .regs = gxl_3g7_regs,
> + .reg_num = ARRAY_SIZE(gxl_3g7_regs)
> + }, {
> + .limit = 297000,
> + .regs = gxl_3g_regs,
> + .reg_num = ARRAY_SIZE(gxl_3g_regs)
> + }, {
> + .limit = 148500,
> + .regs = gxl_def_regs,
> + .reg_num = ARRAY_SIZE(gxl_def_regs)
> + }, {
> + .regs = gxl_270m_regs,
> + .reg_num = ARRAY_SIZE(gxl_270m_regs)
> + }
> };
>
> static const struct meson_dw_hdmi_data meson_dw_hdmi_gxl_data = {
> .reg_init = meson_dw_init_regmap_gx,
> .cntl0_init = 0x0,
> .cntl1_init = PHY_CNTL1_INIT,
> + .use_drm_infoframe = true,
> + .speeds = gxl_speeds,
> + .speed_num = ARRAY_SIZE(gxl_speeds),
> +};
> +
> +static const struct reg_sequence g12a_3g7_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x37eb65c4 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
> + { .reg = HHI_HDMI_PHY_CNTL5, .def = 0x0000080b },
> +};
> +
> +static const struct reg_sequence g12a_3g_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33eb6262 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
> + { .reg = HHI_HDMI_PHY_CNTL5, .def = 0x00000003 },
> +};
> +
> +static const struct reg_sequence g12a_def_regs[] = {
> + { .reg = HHI_HDMI_PHY_CNTL0, .def = 0x33eb4242 },
> + { .reg = HHI_HDMI_PHY_CNTL3, .def = 0x2ab0ff3b },
> + { .reg = HHI_HDMI_PHY_CNTL5, .def = 0x00000003 },
> +};
> +
> +static const struct meson_dw_hdmi_speed g12a_speeds[] = {
> + {
> + .limit = 371250,
> + .regs = g12a_3g7_regs,
> + .reg_num = ARRAY_SIZE(g12a_3g7_regs)
> + }, {
> + .limit = 297000,
> + .regs = g12a_3g_regs,
> + .reg_num = ARRAY_SIZE(g12a_3g_regs)
> + }, {
> + .regs = g12a_def_regs,
> + .reg_num = ARRAY_SIZE(g12a_def_regs)
> + }
> };
>
> static const struct meson_dw_hdmi_data meson_dw_hdmi_g12a_data = {
> .reg_init = meson_dw_init_regmap_g12,
> .cntl0_init = 0x000b4242, /* Bandgap */
> .cntl1_init = PHY_CNTL1_INIT,
> + .use_drm_infoframe = true,
> + .speeds = g12a_speeds,
> + .speed_num = ARRAY_SIZE(g12a_speeds),
> };
>
> static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> @@ -590,7 +665,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> platform_set_drvdata(pdev, meson_dw_hdmi);
>
> meson_dw_hdmi->priv = priv;
> - meson_dw_hdmi->dev = dev;
Unrelated change
> meson_dw_hdmi->data = match;
> dw_plat_data = &meson_dw_hdmi->dw_plat_data;
>
> @@ -650,7 +724,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> meson_dw_hdmi_init(meson_dw_hdmi);
>
> /* Bridge / Connector */
> -
Unrelated change
> dw_plat_data->priv_data = meson_dw_hdmi;
> dw_plat_data->phy_ops = &meson_dw_hdmi_phy_ops;
> dw_plat_data->phy_name = "meson_dw_hdmi_phy";
> @@ -659,11 +732,7 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> dw_plat_data->ycbcr_420_allowed = true;
> dw_plat_data->disable_cec = true;
> dw_plat_data->output_port = 1;
> -
> - if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
> - dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
> - dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
> - dw_plat_data->use_drm_infoframe = true;
> + dw_plat_data->use_drm_infoframe = meson_dw_hdmi->data->use_drm_infoframe;
Move this to a separate patch
>
> meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data);
> if (IS_ERR(meson_dw_hdmi->hdmi))
Powered by blists - more mailing lists