Hi Nikolaus,
On Thu, Sep 23, 2021 at 11:55:56AM +0200, H. Nikolaus Schaller wrote:
Am 23.09.2021 um 11:27 schrieb Laurent Pinchart: On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote:
ret = drm_bridge_attach(encoder, &ib->bridge, NULL,DRM_BRIDGE_ATTACH_NO_CONNECTOR);DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible with synopsys/dw_hdmi.c That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present, since it wants to register its own connector through dw_hdmi_connector_create(). It does it for a reason: the dw-hdmi is a multi-function driver which does HDMI and DDC/EDID stuff in a single driver (because I/O registers and power management seem to be shared).
The IT66121 driver does all of that too, and does not need DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has callbacks to handle cable detection and DDC stuff.
Since I do not see who could split this into a separate bridge and a connector driver and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not our turf.
You could have a field in the dw-hdmi pdata structure, that would instruct the driver whether or not it should use the new API. Ugly, I know, and would probably duplicate a lot of code, but that would allow other drivers to be updated at a later date.
Yes, would be very ugly.
But generally who has the knowledge (and time) to do this work? And has a working platform to test (jz4780 isn't a good development environment)?
The driver seems to have a turbulent history starting 2013 in staging/imx and apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer?
"Maintainer" would be an overstatement. I've worked on that driver in the past, and I still use it, but don't have time to really maintain it. I've also been told that Synopsys required all patches for that driver developed using documentation under NDA to be submitted internally to them first before being published, so I decided to stop contributing instead of agreeing with this insane process. There's public documentation about the IP in some NXP reference manuals though, so it should be possible to still move forward without abiding by this rule.
Therefore the code here should be able to detect if drm_bridge_attach() already creates and attaches a connector and then skip the code below.
Not that easy, unfortunately. On one side we have dw-hdmi which checks that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the other side we have other drivers like the IT66121 which will fail if this flag is not set.
Ok, I see. You have to handle contradicting cases here.
Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first and retry if it fails without?
But IMHO the return value (in error case) is not well defined. So there must be a test if a connector has been created (I do not know how this would work).
Another suggestion: can you check if there is a downstream connector defined in device tree (dw-hdmi does not need such a definition)? If not we call it with 0 and if there is one we call it with DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one?
I haven't followed the ful conversation, what the reason why DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ?
The synopsys driver creates its own connector through dw_hdmi_connector_create() because the IP handles DDC/EDID directly.
That doesn't require creating a connector though. The driver implements drm_bridge_funcs.get_edid(), which is used to get the EDID without the need to create a connector in the dw-hdmi driver.
Hence it checks for ! DRM_BRIDGE_ATTACH_NO_CONNECTOR which seems to be the right thing to do on current platforms that use it.
For CI20/jz4780 we just add a specialisation of the generic dw-hdmi to make HDMI work.
Now this patch for drm/ingenic wants the opposite definition and create its own connector. This fails even if we remove the check (then we have two interfering connectors).
We're moving towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it will have to be done eventually.
So from my view drm/ingenic wants to already enforce this rule and breaks dw-hdmi.
IMHO it should either handle this situation gracefully or include a fix for dw-hdmi.c to keep it compatible.