Hi Maxime：
> -----Original Message-----
> From: Maxime Ripard <mripard@xxxxxxxxxx>
> Sent: 2024年5月22日 15:25
> To: Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx>
> Cc: andrzej.hajda@xxxxxxxxx; neil.armstrong@xxxxxxxxxx; rfoss@xxxxxxxxxx;
> Laurent.pinchart@xxxxxxxxxxxxxxxx; jonas@xxxxxxxxx;
> jernej.skrabec@xxxxxxxxx; maarten.lankhorst@xxxxxxxxxxxxxxx;
> tzimmermann@xxxxxxx; airlied@xxxxxxxxx; daniel@xxxxxxxx; robh@xxxxxxxxxx;
> krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; hjc@xxxxxxxxxxxxxx;
> heiko@xxxxxxxxx; andy.yan@xxxxxxxxxxxxxx; Xingyu Wu
> <xingyu.wu@xxxxxxxxxxxxxxxx>; p.zabel@xxxxxxxxxxxxxx; Jack Zhu
> <jack.zhu@xxxxxxxxxxxxxxxx>; Shengyang Chen
> <shengyang.chen@xxxxxxxxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 03/10] drm/rockchip:hdmi: migrate to use inno-hdmi
> bridge driver
>
> Hi,
>
> On Tue, May 21, 2024 at 06:58:10PM GMT, keith wrote:
> > Add the ROCKCHIP inno hdmi driver that uses the Inno DesignWare HDMI
> > TX bridge and remove the old separate one.
> >
> > Signed-off-by: keith <keith.zhao@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/rockchip/Kconfig | 1 +
> > drivers/gpu/drm/rockchip/Makefile | 2 +-
> > drivers/gpu/drm/rockchip/inno_hdmi-rockchip.c | 517 ++++++++
> > .../{inno_hdmi.h => inno_hdmi-rockchip.h} | 45 -
> > drivers/gpu/drm/rockchip/inno_hdmi.c | 1073 -----------------
> > 5 files changed, 519 insertions(+), 1119 deletions(-) create mode
> > 100644 drivers/gpu/drm/rockchip/inno_hdmi-rockchip.c
> > rename drivers/gpu/drm/rockchip/{inno_hdmi.h => inno_hdmi-rockchip.h}
> > (85%) delete mode 100644 drivers/gpu/drm/rockchip/inno_hdmi.c
> >
> > diff --git a/drivers/gpu/drm/rockchip/Kconfig
> > b/drivers/gpu/drm/rockchip/Kconfig
> > index 1bf3e2829cd0..cc6cfd5a30d6 100644
> > --- a/drivers/gpu/drm/rockchip/Kconfig
> > +++ b/drivers/gpu/drm/rockchip/Kconfig
> > @@ -74,6 +74,7 @@ config ROCKCHIP_DW_MIPI_DSI
> >
> > config ROCKCHIP_INNO_HDMI
> > bool "Rockchip specific extensions for Innosilicon HDMI"
> > + select DRM_INNO_HDMI
> > help
> > This selects support for Rockchip SoC specific extensions
> > for the Innosilicon HDMI driver. If you want to enable diff --git
> > a/drivers/gpu/drm/rockchip/Makefile
> > b/drivers/gpu/drm/rockchip/Makefile
> > index 3ff7b21c0414..4b2d0cba8db3 100644
> > --- a/drivers/gpu/drm/rockchip/Makefile
> > +++ b/drivers/gpu/drm/rockchip/Makefile
> > @@ -12,7 +12,7 @@ rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) +=
> > analogix_dp-rockchip.o
> > rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> > rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
> > rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) +=
> dw-mipi-dsi-rockchip.o
> > -rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
> > +rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi-rockchip.o
> > rockchipdrm-$(CONFIG_ROCKCHIP_LVDS) += rockchip_lvds.o
> > rockchipdrm-$(CONFIG_ROCKCHIP_RGB) += rockchip_rgb.o
> > rockchipdrm-$(CONFIG_ROCKCHIP_RK3066_HDMI) += rk3066_hdmi.o diff
> > --git a/drivers/gpu/drm/rockchip/inno_hdmi-rockchip.c
> > b/drivers/gpu/drm/rockchip/inno_hdmi-rockchip.c
> > new file mode 100644
> > index 000000000000..69d0e913e13b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rockchip/inno_hdmi-rockchip.c
> > @@ -0,0 +1,517 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> > + * Zheng Yang <zhengyang@xxxxxxxxxxxxxx>
> > + * Yakir Yang <ykk@xxxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/irq.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/hdmi.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/bridge/inno_hdmi.h>
> > +#include <drm/drm_atomic.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_edid.h>
> > +#include <drm/drm_of.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_simple_kms_helper.h>
> > +
> > +#include "rockchip_drm_drv.h"
> > +
> > +#include "inno_hdmi-rockchip.h"
> > +
> > +#define INNO_HDMI_MIN_TMDS_CLOCK 25000000U
> > +
> > +struct rk_inno_hdmi {
> > + struct rockchip_encoder encoder;
> > + struct inno_hdmi inno_hdmi;
> > + struct clk *pclk;
> > + struct clk *refclk;
> > +};
> > +
> > +static struct inno_hdmi *rk_encoder_to_inno_hdmi(struct drm_encoder
> > +*encoder) {
> > + struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> > + struct rk_inno_hdmi *rk_hdmi = container_of(rkencoder, struct
> > +rk_inno_hdmi, encoder);
> > +
> > + return &rk_hdmi->inno_hdmi;
> > +}
> > +
> > +enum {
> > + CSC_RGB_0_255_TO_ITU601_16_235_8BIT,
> > + CSC_RGB_0_255_TO_ITU709_16_235_8BIT,
> > + CSC_RGB_0_255_TO_RGB_16_235_8BIT,
> > +};
> > +
> > +static const char coeff_csc[][24] = {
> > + /*
> > + * RGB2YUV:601 SD mode:
> > + * Cb = -0.291G - 0.148R + 0.439B + 128
> > + * Y = 0.504G + 0.257R + 0.098B + 16
> > + * Cr = -0.368G + 0.439R - 0.071B + 128
> > + */
> > + {
> > + 0x11, 0x5f, 0x01, 0x82, 0x10, 0x23, 0x00, 0x80,
> > + 0x02, 0x1c, 0x00, 0xa1, 0x00, 0x36, 0x00, 0x1e,
> > + 0x11, 0x29, 0x10, 0x59, 0x01, 0x82, 0x00, 0x80
> > + },
> > + /*
> > + * RGB2YUV:709 HD mode:
> > + * Cb = - 0.338G - 0.101R + 0.439B + 128
> > + * Y = 0.614G + 0.183R + 0.062B + 16
> > + * Cr = - 0.399G + 0.439R - 0.040B + 128
> > + */
> > + {
> > + 0x11, 0x98, 0x01, 0xc1, 0x10, 0x28, 0x00, 0x80,
> > + 0x02, 0x74, 0x00, 0xbb, 0x00, 0x3f, 0x00, 0x10,
> > + 0x11, 0x5a, 0x10, 0x67, 0x01, 0xc1, 0x00, 0x80
> > + },
> > + /*
> > + * RGB[0:255]2RGB[16:235]:
> > + * R' = R x (235-16)/255 + 16;
> > + * G' = G x (235-16)/255 + 16;
> > + * B' = B x (235-16)/255 + 16;
> > + */
> > + {
> > + 0x00, 0x00, 0x03, 0x6F, 0x00, 0x00, 0x00, 0x10,
> > + 0x03, 0x6F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,
> > + 0x00, 0x00, 0x00, 0x00, 0x03, 0x6F, 0x00, 0x10
> > + },
> > +};
> > +
> > +static struct inno_hdmi_phy_config rk3036_hdmi_phy_configs[] = {
> > + { 74250000, 0x3f, 0xbb },
> > + { 165000000, 0x6f, 0xbb },
> > + { ~0UL, 0x00, 0x00 }
> > +};
> > +
> > +static struct inno_hdmi_phy_config rk3128_hdmi_phy_configs[] = {
> > + { 74250000, 0x3f, 0xaa },
> > + { 165000000, 0x5f, 0xaa },
> > + { ~0UL, 0x00, 0x00 }
> > +};
> > +
> > +static int inno_hdmi_find_phy_config(struct inno_hdmi *hdmi,
> > + unsigned long pixelclk)
> > +{
> > + const struct inno_hdmi_phy_config *phy_configs =
> hdmi->plat_data->phy_configs;
> > + int i;
> > +
> > + for (i = 0; phy_configs[i].pixelclock != ~0UL; i++) {
> > + if (pixelclk <= phy_configs[i].pixelclock)
> > + return i;
> > + }
> > +
> > + DRM_DEV_DEBUG(hdmi->dev, "No phy configuration for pixelclock %lu\n",
> > + pixelclk);
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static void inno_hdmi_standby(struct inno_hdmi *hdmi) {
> > + inno_hdmi_sys_power(hdmi, false);
> > +
> > + hdmi_writeb(hdmi, HDMI_PHY_DRIVER, 0x00);
> > + hdmi_writeb(hdmi, HDMI_PHY_PRE_EMPHASIS, 0x00);
> > + hdmi_writeb(hdmi, HDMI_PHY_CHG_PWR, 0x00);
> > + hdmi_writeb(hdmi, HDMI_PHY_SYS_CTL, 0x15); };
> > +
> > +static void inno_hdmi_power_up(struct inno_hdmi *hdmi,
> > + unsigned long mpixelclock)
> > +{
> > + struct inno_hdmi_phy_config *phy_config;
> > + int ret = inno_hdmi_find_phy_config(hdmi, mpixelclock);
> > +
> > + if (ret < 0) {
> > + phy_config = hdmi->plat_data->default_phy_config;
> > + DRM_DEV_ERROR(hdmi->dev,
> > + "Using default phy configuration for TMDS rate %lu",
> > + mpixelclock);
> > + } else {
> > + phy_config = &hdmi->plat_data->phy_configs[ret];
> > + }
> > +
> > + inno_hdmi_sys_power(hdmi, false);
> > +
> > + hdmi_writeb(hdmi, HDMI_PHY_PRE_EMPHASIS,
> phy_config->pre_emphasis);
> > + hdmi_writeb(hdmi, HDMI_PHY_DRIVER,
> phy_config->voltage_level_control);
> > + hdmi_writeb(hdmi, HDMI_PHY_SYS_CTL, 0x15);
> > + hdmi_writeb(hdmi, HDMI_PHY_SYS_CTL, 0x14);
> > + hdmi_writeb(hdmi, HDMI_PHY_SYS_CTL, 0x10);
> > + hdmi_writeb(hdmi, HDMI_PHY_CHG_PWR, 0x0f);
> > + hdmi_writeb(hdmi, HDMI_PHY_SYNC, 0x00);
> > + hdmi_writeb(hdmi, HDMI_PHY_SYNC, 0x01);
> > +
> > + inno_hdmi_sys_power(hdmi, true);
> > +};
> > +
> > +static void inno_hdmi_reset(struct inno_hdmi *hdmi) {
> > + u32 val;
> > + u32 msk;
> > +
> > + hdmi_modb(hdmi, HDMI_SYS_CTRL, m_RST_DIGITAL,
> v_NOT_RST_DIGITAL);
> > + udelay(100);
> > +
> > + hdmi_modb(hdmi, HDMI_SYS_CTRL, m_RST_ANALOG,
> v_NOT_RST_ANALOG);
> > + udelay(100);
> > +
> > + msk = m_REG_CLK_INV | m_REG_CLK_SOURCE | m_POWER | m_INT_POL;
> > + val = v_REG_CLK_INV | v_REG_CLK_SOURCE_SYS | v_PWR_ON |
> v_INT_POL_HIGH;
> > + hdmi_modb(hdmi, HDMI_SYS_CTRL, msk, val);
> > +
> > + inno_hdmi_standby(hdmi);
> > +}
> > +
> > +static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi) {
> > + struct drm_connector *connector = &hdmi->connector;
> > + struct drm_connector_state *conn_state = connector->state;
> > + struct inno_hdmi_connector_state *inno_conn_state =
> > + to_inno_hdmi_conn_state(conn_state);
> > + int c0_c2_change = 0;
> > + int csc_enable = 0;
> > + int csc_mode = 0;
> > + int auto_csc = 0;
> > + int value;
> > + int i;
> > +
> > + /* Input video mode is SDR RGB24bit, data enable signal from external */
> > + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL1, v_DE_EXTERNAL |
> > + v_VIDEO_INPUT_FORMAT(VIDEO_INPUT_SDR_RGB444));
> > +
> > + /* Input color hardcode to RGB, and output color hardcode to RGB888 */
> > + value = v_VIDEO_INPUT_BITS(VIDEO_INPUT_8BITS) |
> > + v_VIDEO_OUTPUT_COLOR(0) |
> > + v_VIDEO_INPUT_CSP(0);
> > + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL2, value);
> > +
> > + if (inno_conn_state->enc_out_format == HDMI_COLORSPACE_RGB) {
> > + if (inno_conn_state->rgb_limited_range) {
> > + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> > + auto_csc = AUTO_CSC_DISABLE;
> > + c0_c2_change = C0_C2_CHANGE_DISABLE;
> > + csc_enable = v_CSC_ENABLE;
> > +
> > + } else {
> > + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > +
> > + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > + return 0;
> > + }
> > + } else {
> > + if (inno_conn_state->colorimetry == HDMI_COLORIMETRY_ITU_601)
> {
> > + if (inno_conn_state->enc_out_format ==
> HDMI_COLORSPACE_YUV444) {
> > + csc_mode = CSC_RGB_0_255_TO_ITU601_16_235_8BIT;
> > + auto_csc = AUTO_CSC_DISABLE;
> > + c0_c2_change = C0_C2_CHANGE_DISABLE;
> > + csc_enable = v_CSC_ENABLE;
> > + }
> > + } else {
> > + if (inno_conn_state->enc_out_format ==
> HDMI_COLORSPACE_YUV444) {
> > + csc_mode = CSC_RGB_0_255_TO_ITU709_16_235_8BIT;
> > + auto_csc = AUTO_CSC_DISABLE;
> > + c0_c2_change = C0_C2_CHANGE_DISABLE;
> > + csc_enable = v_CSC_ENABLE;
> > + }
> > + }
> > + }
> > +
> > + for (i = 0; i < 24; i++)
> > + hdmi_writeb(hdmi, HDMI_VIDEO_CSC_COEF + i,
> > + coeff_csc[csc_mode][i]);
> > +
> > + value = v_SOF_DISABLE | csc_enable |
> v_COLOR_DEPTH_NOT_INDICATED(1);
> > + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL, m_VIDEO_AUTO_CSC |
> > + m_VIDEO_C0_C2_SWAP, v_VIDEO_AUTO_CSC(auto_csc) |
> > + v_VIDEO_C0_C2_SWAP(c0_c2_change));
> > +
> > + return 0;
> > +}
> > +
> > +static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > + struct drm_display_mode *mode)
> > +{
> > + struct drm_display_info *display = &hdmi->connector.display_info;
> > + unsigned long mpixelclock = mode->clock * 1000;
> > +
> > + /* Mute video and audio output */
> > + hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
> > + v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));
> > +
> > + /* Set HDMI Mode */
> > + hdmi_writeb(hdmi, HDMI_HDCP_CTRL,
> > + v_HDMI_DVI(display->is_hdmi));
> > +
> > + inno_hdmi_config_video_timing(hdmi, mode);
> > +
> > + inno_hdmi_config_video_csc(hdmi);
> > +
> > + if (display->is_hdmi)
> > + inno_hdmi_config_video_avi(hdmi, mode);
> > +
> > + /*
> > + * When IP controller have configured to an accurate video
> > + * timing, then the TMDS clock source would be switched to
> > + * DCLK_LCDC, so we need to init the TMDS rate to mode pixel
> > + * clock rate, and reconfigure the DDC clock.
> > + */
> > + inno_hdmi_i2c_init(hdmi, mpixelclock);
> > +
> > + /* Unmute video and audio output */
> > + hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
> > + v_AUDIO_MUTE(0) | v_VIDEO_MUTE(0));
> > +
> > + inno_hdmi_power_up(hdmi, mpixelclock);
> > +
> > + return 0;
> > +}
> >
>
> It's kind of a general comment, but I don't think that's the right abstraction. You
> should create a inno_hdmi bridge that allows to supplement some of the atomic
> hooks, but not reimplement them entirely each time.
>
> You can have a look at how dw-hdmi does it for example. Also, why do you still
> need the encoder and connectors?
>
Hi Maxime：
This series of patches does not consider referencing bridge ,
just split the public interface based on the current structure (encoder + connector),
and then make it into a public API.
The next step is to implement the driver code of the public part based on the bridge architecture.
By the way, does the current situation require the introduction of the next_bridge concept?
dw-hdmi attach:
static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)
{
struct dw_hdmi *hdmi = bridge->driver_private;
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
bridge, flags);
return dw_hdmi_connector_create(hdmi);
}
For inno hdmi , I want to define it like this , will it be ok?
static int inno_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)
{
......
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
DRM_ERROR("Fix bridge driver to make connector optional!");
return -EINVAL;
}
return 0;
}
......
And then , create the connector outside of bridge:
ret = drm_bridge_attach(encoder, &hdmi->bridge, NULL, 0);
if (ret)
return ret;
hdmi->connector = drm_bridge_connector_init(drm, encoder);
if (IS_ERR(hdmi->connector)) {
dev_err(dev, "Unable to create bridge connector\n");
ret = PTR_ERR(hdmi->connector);
return ret;
}
Best regards
> Maxime
