Re: [PATCH 13/14] drm/bridge: synopsys: Add DW HDMI QP TX controller driver

[Cristian Ciocaltea]

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.

Cristian