From: Paul Cercueil <paul@crapouillou.net>
To: "H. Nikolaus Schaller" <hns@goldelico.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
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>,
Paul Boddie <paul@boddie.org.uk>
Subject: Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders
Date: Thu, 23 Sep 2021 14:30:07 +0100 [thread overview]
Message-ID: <7U2WZQ.D8DTPCJ0ZPKO3@crapouillou.net> (raw)
In-Reply-To: <B7C9EEE8-F999-4105-B805-1B32619A3847@goldelico.com>
Hi Nikolaus,
Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller
<hns@goldelico.com> a écrit :
> Hi Laurent,
>
>> Am 23.09.2021 um 12:03 schrieb Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com>:
>>
>> 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);
>>>>>>>
>>>>>>> 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
>>>>> 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?
>>>>
>>>> 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
ingenic-drm.
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.
> 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?
> 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().
Cheers,
-Paul
> 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
>
>
next prev parent reply other threads:[~2021-09-23 13:30 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-22 20:55 [PATCH v3 0/6] drm/ingenic: Various improvements v3 Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 1/6] drm/ingenic: Simplify code by using hwdescs array Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 2/6] drm/ingenic: Add support for private objects Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 3/6] drm/ingenic: Move IPU scale settings to private state Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 4/6] drm/ingenic: Set DMA descriptor chain register when starting CRTC Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 5/6] drm/ingenic: Upload palette before frame Paul Cercueil
2021-09-22 20:55 ` [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders Paul Cercueil
2021-09-23 5:52 ` H. Nikolaus Schaller
2021-09-23 8:49 ` Paul Cercueil
2021-09-23 9:19 ` H. Nikolaus Schaller
2021-09-23 9:27 ` Laurent Pinchart
2021-09-23 9:55 ` H. Nikolaus Schaller
2021-09-23 10:03 ` Laurent Pinchart
2021-09-23 11:41 ` H. Nikolaus Schaller
2021-09-23 11:56 ` H. Nikolaus Schaller
2021-09-23 13:30 ` Paul Cercueil [this message]
[not found] ` <ABE75744-46FE-4F37-A14C-D996F36B7B0E@goldelico.com>
2021-09-23 19:39 ` Paul Cercueil
2021-09-23 20:23 ` H. Nikolaus Schaller
2021-09-23 22:51 ` Paul Boddie
2021-09-24 8:29 ` Paul Cercueil
2021-09-25 15:55 ` Paul Boddie
2021-09-25 19:08 ` Paul Cercueil
2021-09-25 19:26 ` H. Nikolaus Schaller
2021-09-25 19:39 ` Paul Cercueil
2021-09-27 16:18 ` H. Nikolaus Schaller
2021-09-24 11:40 ` H. Nikolaus Schaller
2021-09-23 9:23 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7U2WZQ.D8DTPCJ0ZPKO3@crapouillou.net \
--to=paul@crapouillou.net \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=hns@goldelico.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=list@opendingux.net \
--cc=paul@boddie.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).