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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.


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









[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux