lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 23 Sep 2021 20:39:18 +0100 From: Paul Cercueil <paul@...pouillou.net> To: "H. Nikolaus Schaller" <hns@...delico.com> Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>, linux-mips <linux-mips@...r.kernel.org>, list@...ndingux.net, dri-devel <dri-devel@...ts.freedesktop.org>, linux-kernel <linux-kernel@...r.kernel.org>, Paul Boddie <paul@...die.org.uk> Subject: Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders Le jeu., sept. 23 2021 at 20:52:23 +0200, H. Nikolaus Schaller <hns@...delico.com> a écrit : > Hi Paul, > >> Am 23.09.2021 um 15:30 schrieb Paul Cercueil <paul@...pouillou.net>: >> >> Hi Nikolaus, >> >> Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller >> <hns@...delico.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. No, you don't need to do that at all. Just don't set the "ddc-i2c-bus" property. >> >> 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 >> >>> 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. 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. Start by wiring things properly, like in my previously linked DTS, and *test*. If it fails, tell us where it fails. Because your "it doesn't work" arguments have zero weight otherwise. If I can find some time this weekend I will test it myself. Cheers, -Paul > 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 > > >
Powered by blists - more mailing lists