Hi Laurent, > Am 23.09.2021 um 12:03 schrieb Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>: > > 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. 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. 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. 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(). I come to the conclusion that not creating a specific connector in dw-hdmi and relying on a generic connector does not seem to be an option with current code proposals. In such a situation the question is what the least invasive surgery is to avoid complications and lenghty regression tests on unknown platforms. IMHO it is leaving (mature) dw-hdmi untouched and make attachment of a connector in ingenic_drm_bind() depend on some condition. BR and thanks, Nikolaus
