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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Thu, 23 Sep 2021 14:30:07 +0100
From:   Paul Cercueil <>
To:     "H. Nikolaus Schaller" <>
Cc:     Laurent Pinchart <>,
        David Airlie <>,
        Daniel Vetter <>,
        linux-mips <>,,
        dri-devel <>,
        linux-kernel <>,
        Paul Boddie <>
Subject: Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

Hi Nikolaus,

Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller 
<> a écrit :
> Hi Laurent,
>>  Am 23.09.2021 um 12:03 schrieb Laurent Pinchart 
>> <>:
>>  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 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 
>>>>>  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.

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 

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 

> If drm_bridge_attach() would return some errno if 
> 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?

> 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().


> 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

Powered by blists - more mailing lists