On Fri, Oct 21, 2016 at 09:04:39PM +0200, Jean-Francois Moine wrote: > >>>> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote: > (sorry, I lost your original mail) > > >>> DRM bridges indeed don't create encoders. That task is left to the display > > >>> driver. The reason is the same as above: bridges can be chained (including > > >>> with an internal encoder that is not modelled as a bridge, and that's a case > > >>> we have today), while the KMS model exposes a single encoder to userspace. > > >>> Exposing DRM encoder objects as part of the KMS UABI was probably a mistake. > > >>> Better solutions would have been to expose no encoder at all or all encoders > > >>> in the chain. We are however stuck with this model as we can't break the UABI. > > >>> For that reason a DRM encoder object doesn't represent an encoder but a chain > > >>> of encoders. As a DRM bridge models a single encoder, the DRM encoder object > > >>> must be created at a higher level, in the display driver. > > I wonder why you created 'bridge's instead of simply adding links to > the encoders? (that's what ASoC did: the audio CODECs are linked) > This way, in simple cases (most cases), there would have been > crtc -> (encoder -> connector) > instead of > crtc -> (bridge + encoder) -> (bridge + connector) > without any changes in the actual (encoder + connector)s. Looking at drm_bridge_disable() and drm_bridge_enable(), the control model appears to be: crtc -> encoder -> connector `-> bridge `-> bridge `-> bridge Bridges are always attached to an encoder, and there can be multiple bridges attached to one encoder. Bridges can't be attached to the connector. So, in the case of TDA998x, we end up with the TDA998x providing a connector, because it has connector functionality, and providing a bridge. The encoder is left to the KMS driver, which adds additional complexity (100+ lines) to each and every KMS driver, requiring the KMS driver to have much more knowledge of what's attached to the "CRTC", so it can create these encoders itself. I still think this is a backwards step - maybe one step forwards, two backwards. Even if we were to provide helpers to create a dummy encoder to work around the DRM bridge model short-comings, much of the 100+ lines is to do with working out whether or not we need to create an encoder, and parsing the information we need to create that in a way that existing DT doesn't break. Then there's the fact that the bridge approach breaks non-DT at the moment, because DRM bridge fundamentally requires DT. This makes DRM bridge useless for architectures which aren't DT aware, such as x86. So, converting drivers to be a DRM bridge effectively denies their use on other architectures. See drm_bridge.c, and look for the references to bridge_list, noting that there are two: one which adds to the bridge list, and one under #ifdef CONFIG_OF which looks up a DT reference to a bridge. There's another issue with TDA998x - we now have audio support in TDA998x, and converting TDA998x to be a DRM bridge introduces some fundamental (and as yet unsolved) races between the ASoC code and the attachment of the DRM bridge to the DRM encoder, and the detachment when the DRM bridge/connector gets cleaned up. Right now, there's no bridge callback when the encoder or drm_device goes away, so doing stuff like: static int tda998x_audio_get_eld(struct device *dev, void *data, uint8_t *buf, size_t len) { struct tda998x_priv *priv = dev_get_drvdata(dev); struct drm_mode_config *config; struct drm_connector *connector; int ret = -ENODEV; /* FIXME: This is racy */ if (!priv->bridge.encoder || !priv->bridge.encoder->dev) return ret; config = &priv->bridge.encoder->dev->mode_config; mutex_lock(&config->mutex); list_for_each_entry(connector, &config->connector_list, head) { if (priv->bridge.encoder == connector->encoder) { memcpy(buf, connector->eld, min(sizeof(connector->eld), len)); ret = 0; } } mutex_unlock(&config->mutex); feels very unsafe - nothing really guarantees the validity of the priv->bridge.encoder or priv->bridge.encoder->dev pointers. The config->mutex lock does nothing to solve this. The same problem also exists in tda998x_audio_hw_params(). Anyway, the whole bridge conversion thing is moot until every user of the TDA998x code has been updated to cope with the lack of drm_connector_register() inside TDA998x - see Brian Starkey's response to this patch set. It's also moot if it breaks armada-drm. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel