Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.21\))
Subject: Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
From:   "H. Nikolaus Schaller" <hns@goldelico.com>
In-Reply-To: <RTPVZQ.WN90B9MHPMZ13@crapouillou.net>
Date:   Thu, 23 Sep 2021 11:19:23 +0200
Cc:     David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
        linux-mips <linux-mips@vger.kernel.org>, list@opendingux.net,
        dri-devel <dri-devel@lists.freedesktop.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <896D04E4-4058-474B-8BD2-7F21B1C754E4@goldelico.com>
References: <20210922205555.496871-1-paul@crapouillou.net>
 <20210922205555.496871-7-paul@crapouillou.net>
 <32234186-1802-4FDF-801A-B14E48FB86D8@goldelico.com>
 <RTPVZQ.WN90B9MHPMZ13@crapouillou.net>
To:     Paul Cercueil <paul@crapouillou.net>
Precedence: bulk

Hi Paul,

> Am 23.09.2021 um 10:49 schrieb Paul Cercueil <paul@crapouillou.net>:
>=20
> Hi Nikolaus,
>=20
> Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller =
<hns@goldelico.com> a =C3=A9crit :
>> Hi Paul,
>> thanks for another update.
>> We have been delayed to rework the CI20 HDMI code on top of your =
series
>> but it basically works in some situations. There is for example a =
problem
>> if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be =
outside
>> of your series.
>=20
> I think the SoC can output YCbCr as well, but I never tried to use it.

Maybe there is code missing or something else. We have not yet deeply =
researched.
Except that when ignoring DRM_COLOR_FORMAT_YCRCB422 capability it uses =
RGB
and works.

>=20
>>> +		ret =3D drm_bridge_attach(encoder, &ib->bridge, NULL,
>>> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> 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).
>=20
> 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.
>=20
>> 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.
>=20
> 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?

>=20
>> 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.
>=20
> 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 DRM_BRIDGE_ATTACH_NO_CONNECTOR first
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?

Just some ideas how to solve without touching hdmi drivers.

BR and thanks,
Nikolaus=