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

[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.

> Hence it checks for ! DRM_BRIDGE_ATTACH_NO_CONNECTOR which seems to be the
> right thing to do on current platforms that use it.
> 
> For CI20/jz4780 we just add a specialisation of the generic dw-hdmi to
> make HDMI work.
> 
> Now this patch for drm/ingenic wants the opposite definition and create its own
> connector. This fails even if we remove the check (then we have two interfering
> connectors).
> 
> > We're moving
> > towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it
> > will have to be done eventually.
> 
> So from my view drm/ingenic wants to already enforce this rule and breaks dw-hdmi.
> 
> IMHO it should either handle this situation gracefully or include a fix for
> dw-hdmi.c to keep it compatible.

-- 
Regards,

Laurent Pinchart