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

[Daniel Vetter]

On Mon, Oct 24, 2016 at 11:58:00AM +0530, Archit Taneja wrote:
> On 10/22/2016 03:25 PM, Russell King - ARM Linux wrote:
> > 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.

In helpers connectors are no-op objects. We _never_ call any connector
callbacks when doing a modeset. Connectors are only used to probe output
state, and as the userspace-visisble endpoint representation. Hence the
real graph is

crtc -> encoder [ -> bridge [ -> bridge [...]]] -> connector

with the last bridge owning the connector. And that last bridge probably
needs to store a pointer to its connector(s).

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

We've stubbed out everything that's in an encoder, you definitely don't
need hundreds of lines to write one any more. If there's still a callback
left around drm_encoder which has not yet suitable default handling, then
that's a bug.

Note: Only applies to atomic though, I'm not going to bother with old
legacy crtc helpers. I guess armada needs to switch to atomic, otherwise
encoders are indeed a bit a pain.

> The majority of KMS drivers do end up creating their own encoders (i.e,
> encoders are exclusive to the HW represented by the KMS driver, not an
> external chip/IP that can be used by other KMS drivers).
> 
> The additional complexity is more for KMS drivers like armada-drm,
> where the encoder HW isn't provided by the main display HW altogether.
> 
> From the initial commits that added drm_bridge, it was mainly created
> to piggy-back onto an existing drm_encoder. Later, some infrastructure
> was added to create independent bridge drivers that could attach to
> a KMS driver. What was completely missed out when implementing drm_bridge
> was the case where the KMS driver doesn't have a drm_encoder
> at all. I feel it should be fixable with additional helpers, though.
> If not, I think we still aren't too late to come up with some other
> way of representing encoder chains, since there still aren't too many
> bridge drivers and no presence of it in user space.

Imo encoders should be that part which is baked into your core ip. If
there's nothing, then you're perfectly fine with a no-op encoder. Maybe we
could do a helper for creating those, if the few lines are copypasted too
often. Then all the external IP should be bridges (and chained). And with
chains either you need another bridge, or you're the last bridge, and then
you're supposed to register the connector as the final endpoint.

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

It should be pretty easy to add some other lookup thing for drm_bridge. I
don't see any reasons why drm_bridge fundamentally requires dt.

> Maybe for the non-DT case, we could add a field in drm_bridge that is
> populated by dev->platform_data which tells the KMS driver which
> encoder it needs to bind to.
> 
> Could you point me to what the dev->platform_data retrieved by
> armada_drm_probe from a board file looks like? I couldn't find
> anything when I grep-ed "armada-drm"/"armada-510-drm"
> 
> I do agree that it's a step backward that we now have to search for
> a corresponding bridge, which we didn't have to do when the chip
> was represented as an encoder.

You can still do the exact same thing with bridges as with encoders using
the component framework. Should not be a step back at all.

> > 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().
> 
> Maybe we could ensure that the bridge attachment/detachment is
> contained within drm_encoder_init/cleanup funcs, so that their
> life is tied to the encoder drm_mode_object. It wouldn't be as
> straightforward, since the drm_bridges create connectors too.
> Will look more into this.

I don't see any issue with the above at all. Or well, if there is one
there's a larger issue: You can't reach this code if you unregister your
driver's interface _before_ you tear down anything. This is fixed by
getting rid of the load/unload callbacks. And for additional interfaces
there's new register/unregister callbacks on connectors (which the bridge
also should own).

I think fundamentally the issue here is that armada still has the midlayer
callbacks, and with those this kind of stuff isn't really fixable.
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch