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
