On 14/06/2024 08:56, Andy Yan wrote:
Hi Neil，
At 2024-06-05 19:48:09, "Neil Armstrong" <neil.armstrong@xxxxxxxxxx> wrote:
On 05/06/2024 12:11, Cristian Ciocaltea wrote:
On 6/5/24 12:34 AM, Cristian Ciocaltea wrote:
On 6/4/24 11:41 PM, Sam Ravnborg wrote:
Hi Cristian.
On Tue, Jun 04, 2024 at 10:32:04PM +0300, Cristian Ciocaltea wrote:
Hi Sam,
On 6/1/24 5:32 PM, Sam Ravnborg wrote:
Hi Cristian,
a few drive-by comments below.
Sam
+
+static const struct drm_connector_funcs dw_hdmi_qp_connector_funcs = {
+ .fill_modes = drm_helper_probe_single_connector_modes,
+ .detect = dw_hdmi_connector_detect,
+ .destroy = drm_connector_cleanup,
+ .force = dw_hdmi_qp_connector_force,
+ .reset = drm_atomic_helper_connector_reset,
+ .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int dw_hdmi_qp_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, &dw_hdmi_qp_connector_funcs);
+}
Are there any users left that requires the display driver to create the
connector?
In other words - could this driver fail if DRM_BRIDGE_ATTACH_NO_CONNECTOR
is not passed and drop dw_hdmi_connector_create()?
I did not try to verify this - just a naive question.
I've just tested this and it doesn't work - dw_hdmi_connector_create()
is still needed.
Hmm, seems the display driver or some other bridge driver fails to
support "DRM_BRIDGE_ATTACH_NO_CONNECTOR".
what other drivers are involved?
Could it be related to the glue driver (updated in the next patch) which
is also responsible for setting up the encoder?
Note that my comments here should be seen as potential future
improvements, and do not block the patch from being used.
Thanks for the heads up! Will try to get back to this soon and investigate.
IIUC, modern bridges should not create the connector but rely on display
drivers to take care of, which in this case is the VOP2 driver. However,
it also handles some of the older SoCs relying on the non-QP variant of
DW HDMI IP. Hence the existing dw-hdmi driver would be also impacted in
order to come up with a proper solution.
A quick check shows there are several users of this IP:
$ git grep -E '= dw_hdmi_(bind|probe)\('
drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c: hdmi->dw_hdmi = dw_hdmi_probe(pdev, plat_data);
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, plat_data);
drivers/gpu/drm/imx/ipuv3/dw_hdmi-imx.c: hdmi->hdmi = dw_hdmi_probe(pdev, match->data);
drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c: hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
drivers/gpu/drm/meson/meson_dw_hdmi.c: meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data);
drivers/gpu/drm/renesas/rcar-du/rcar_dw_hdmi.c: hdmi = dw_hdmi_probe(pdev, &rcar_dw_hdmi_plat_data);
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c: hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
I didn't check which display drivers would be involved, I'd guess there
are quite a few of them as well. So it seems this ends up being a pretty
complex task.
If this would be a brand new driver, then it should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR,
so you should not create a connector from the driver.
The fact dw-hdmi accepts an attach without the flag is for legacy purpose
since some DRM drivers haven't switched to DRM_BRIDGE_ATTACH_NO_CONNECTOR yes,
but it's a requirement for new bridges so at some point you'll need to make
sure the rockchip glue and drm driver supports DRM_BRIDGE_ATTACH_NO_CONNECTOR.
This will greatly simplify the driver!
Based on the previous discussion， the DW HDMI QP drivers will be implemented like this：
Core bridge library：
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
Rockchip platform specific glue：
drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
As a new bridge driver should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR,
Is it acceptable if we implement the connector at the rockchip glue dw_hdmi_qp-rockchip.c ？
Our current combination is a bit complex：
The display controller driver is drivers/gpu/drm/rockchip/rockchip_drm_vop2.c ，which shared
by rk3568， rk3588 and some upcoming soc like rk3528/rk3562.
For rk3588， we have totally new HDMI、DP、DSI2 IP， they need brand new bridge driver that
should only support DRM_BRIDGE_ATTACH_NO_CONNECTOR， and there is also an eDP on rk3588
use analogix_dp_core.c that create connector by analogix_dp bridge。
For rk3568， the HDMI/eDP/DSI IP are based on old IP， the corresponding drivers are dw-hdmi，analogix_dp
and dw-mipi-dsi, they both create drm connector by it's bridge driver. And rk3528/rk3562 are like this too。
So if we can create drm_connector at glue side (such as dw_hdmi_qp-rockchip.c), let the interface driver decide
if it should create drm_connector or not will make the vop2 driver simpler。
I think you should start migration to drm_bridge_connector instead of hacking dw_hdmi_qp-rockchip.c into
fitting into DRM_BRIDGE_ATTACH_NO_CONNECTOR.
You'll add technical dept, and the migration will be even harder afterwards.
But in any case, bridge/synopsys/dw-hdmi-qp.c and rockchip/dw_hdmi_qp-rockchip.c should be send
in two separate patchsets, so how rockchip DRM_BRIDGE_ATTACH_NO_CONNECTOR is a different story.
Neil
Neil
Cristian
