Hi Paul, > Am 23.09.2021 um 21:39 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>: > > > > Le jeu., sept. 23 2021 at 20:52:23 +0200, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> a écrit : >> Hi Paul, >>> Am 23.09.2021 um 15:30 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>: >>> Hi Nikolaus, >>> Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> 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. > > No, you don't need to do that at all. Just don't set the "ddc-i2c-bus" property. And then ddc from dw-hdmi works? > >>> 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". > > We don't want to use the dw-hdmi connector. Your "next bridge" is the "hdmi-connector" that should be wired in DTS. > >> 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). > > Wire it to the LCD node in DTS... > > See how we do it for the IT66121 driver: > https://github.com/OpenDingux/linux/blob/jz-5.15/arch/mips/boot/dts/ingenic/rg350m.dts#L114-L134 That is how we already tried. > >>>> 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. > > And why would you do that? If you don't want to attach a connector, then drm_bridge_attach() doesn't need to do much. So it's normal that it returns zero. > >> 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. > > No, absolutely not. There is nothing upstream yet that can bind the ingenic-drm driver with the dw-hdmi driver. We have proposed it a while ago using nothing special. > This is your downstream patch. I'm not breaking anything that's upstream, so there is no blocking point. > > Besides, even with your downstream patch I don't see any reason why the dw-hdmi driver wouldn't work with this patch, provided it's wired properly, and you never did show a proof of failure either. You come up with "possible points where it will fail" but these are based on your assumptions on how the drivers should be working together, and I think you somehow miss the whole picture. Yes, maybe. > > Start by wiring things properly, like in my previously linked DTS, and *test*. If it fails, tell us where it fails. Well, I tell where drm_bridge_attach fails with DRM_BRIDGE_ATTACH_NO_CONNECTOR... > Because your "it doesn't work" arguments have zero weight otherwise. I hope I still can find it. So I can't promise anything. We have had it complete in DTS and added code to parse it. It may have been wiped out by cleaning up patch series during rebase. > > If I can find some time this weekend I will test it myself. You may be faster than me. BR and thanks, Nikolaus
