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

[Archit Taneja]

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

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.

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

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.

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

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project