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!
Neil
Cristian
