Hi,
On Fri, Jun 14, 2024 at 02:56:00PM GMT, Andy Yan wrote:
> 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
Note that the bridge HDMI helpers Dmitry was mentioning have now been
merged, so I'd expect the core bridge library to be fairly minimal. And
the new driver should obviously use it as much as it can.
Maxime
Attachment:
signature.asc
Description: PGP signature
