[PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

[H. Nikolaus Schaller]

Hi Paul,

> Am 23.09.2021 um 15:30 schrieb Paul Cercueil <paul at crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller <hns at goldelico.com> a écrit :
>> Hi Laurent,
>> Ah, ok.
>> But then we still have issues.
>> Firstly I would assume that get_edid only works properly if it is initialized
>> through dw_hdmi_connector_create().
>> Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to
>> dw_hdmi_bridge_attach() indeed does not call dw_hdmi_connector_create()
>> but returns 0.
>> This patch 6/6 makes drm/ingenic unconditionally require a connector
>> to be attached which is defined somewhere else (device tree e.g. "connector-hdmi")
>> unrelated to dw-hdmi. Current upstream code for drm/ingenic does not init/attach
>> such a connector on its own so it did work before.
>> I.e. I think we can't just use parts of dw-hdmi.
> 
> The fact that Laurent is using dw-hdmi with DRM_BRIDGE_ATTACH_NO_CONNECTOR on Renesas makes me think that it's possible here as well. There's no reason why it shouldn't work with ingenic-drm.

That is interesting and Laurent can probably comment on differences between
his setup (I wasn't able to deduce what device you are referring to) and dw-hdmi.

For jz4780 we tried that first. I do not remember the exact reasons but we wasted
weeks trying to but failed to get it working. While the dw-hdmi connector simply works
on top of upstream and fails only if we apply your patch.

Another issue is how you want to tell connector-hdmi to use the extra i2c bus driver
for ddc which is not available directly as a standard i2c controller of the jz4780.

hdmi-connector.yaml defines:

  ddc-i2c-bus:
	description: phandle link to the I2C controller used for DDC EDID probing
	$ref: /schemas/types.yaml#/definitions/phandle

So we would need some ddc-i2c-bus = <&i2c-controller-inside-the dw-hdmi>.

But that i2c-controller-inside-the dw-hdmi does not exist in device tree
and can not be added unless someone significantly rewrites dw-hdmi to
register and expose it as i2c controller.

> 
> The ingenic-drm driver does not need to create any connector. The "connector-hdmi" is connected to dw-hdmi as the "next bridge" in the list.

Sure. It does not *create* a connector. It expects that it can safely call
drm_bridge_connector_init() to get a pointer to a newly created connector.

But if we use the dw-hdmi connector, there is no such connector and "next bridge".

Or can you tell me how to make the dw-hdmi connector created by
dw_hdmi_connector_create() become the "next bridge" in the list for your driver?
But without significantly rewriting dw-hdmi.c (and testing).

> 
>> If drm_bridge_attach() would return some errno if DRM_BRIDGE_ATTACH_NO_CONNECTOR
>> is set, initialization in ingenic_drm_bind() would fail likewise with "Unable to attach bridge".
>> So in any case dw-hdmi is broken by this drm/ingenic patch unless someone
>> reworks it to make it compatible.
> 
> Where would the errno be returned? Why would drm_bridge_attach() return an error code?

Currently dw_hdmi_bridge_attach() returns 0 if it is asked to support
DRM_BRIDGE_ATTACH_NO_CONNECTOR.

This is not treated as an error by drm_bridge_attach().

Here it could return an error (-ENOTSUPP?) instead, to allow for error handling
by its caller.

But that raises an error message like "failed to attach bridge to encoder" and
the bridge is reset and detached.

> 
>> Another issue is that dw_hdmi_connector_create() does not only do dcd/edid
>> but appears to detects hot plug and does some special initialization.
>> So we probably loose hotplug detect if we just use drm_bridge_funcs.get_edid().
> 
> There's drm_bridge_funcs.detect().

You mean in dw-hdmi? Yes, it calls dw_hdmi_bridge_detect() which calls dw_hdmi_detect().
This does some read_hpd.

Anyways, this does not solve the problem that with your drm/ingenic proposal the
dw-hdmi subsystem (hdmi + ddc) can no longer be initialized properly unless someone
fixes either.

So IMHO this should be treated as a significant blocking point for your patch
because it breaks something that is working upstream and there seems to be no
rationale to change it.

Your commit message just says:
"All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR."
but gives no reason why.

I fully understand that you want to change it and Laurent said that it will become
standard in the far future. Therefore I suggest to find a way that you can find out
if a connector has already been created by drm_bridge_attach() to stay compatible
with current upstream code.

I even want to help here but I don't know how to detect the inverse of
drm_connector_attach_encoder(), i.e. is_drm_encoder_attached_to_any_connector().

BR and thanks,
Nikolaus