[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call

[Russell King - ARM Linux]

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.