| Message ID | 7ff9cdf7-ee12-c844-ed3d-d956fdcc6c97@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 20 Oct 2016 16:56:44 +0530 Archit Taneja <architt@codeaurora.org> wrote: > > Please show _technically_ how this would work. I want to see code or > > pseudo-code illustrating how a "foreign" DRM encoder could be used with > > either dw-hdmi or tda998x, because right now I can't see any way that > > could work. > > This is something we already do with the adv7511 bridge driver on msm, > rcar and arc (for 4.9) drivers. > > I've shared pseudo code on the kms driver and encoder chip's driver > side. I've also shared a diff that converts the tda998x driver to use > drm_bridge(uncompiled/untested). > > 1) Kms driver side: > > /* > * Create an encoder instance. Depending on the hardware represented > * by the KMS driver, the encoder can ops can either have some > * functionality, or be nops. In the case of tilcdc, the encoder > * funcs would be mostly nops. > */ > drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs); > drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs, > type, NULL); Then, how does this 'kms_priv' know the type of the encoder, this one being tied to the connector type at the end of the bridge chain?
On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote:
> 3) Rough conversion to bridge:
So I thought I might give this a try, and see what's needed to complete
the patch, but...
I thought we'd got past the dark ages of email clients screwing up
patches, but it seems not. This patch is totally screwed that it's
not worth even sending - it has been:
- wrapped
- white-space damaged
which makes it pretty hard to undo all the damage. Please use a
better mail client which doesn't screw up patches, or send it as a
plain text attachment. Thanks.
On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote: > > > On 10/20/2016 02:45 PM, Russell King - ARM Linux wrote: > >On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote: > >> > >> > >>On 10/20/2016 01:50 PM, Laurent Pinchart wrote: > >>>Hi Russell, > >>> > >>>On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote: > >>>>On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote: > >>>>>On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote: > >>>>>>In any case, I don't agree with converting it to a DRM bridge - that > >>>>>>will mean that we have to split the driver into two pieces, the bridge > >>>>>>part handling the mode set specifics, and a connector/encoder part which > >>>>>>handles the detection/edid stuff. > >>>>>> > >>>>>>We might as well keep the whole thing as the classical connector/encoder > >>>>>>rather than introducing this additional layer of complexity. > >>>>> > >>>>>We have different ways to instantiate external HDMI encoders, and that's > >>>>>not good. I believe everybody agrees that drm encoder slave has to go, so > >>>>>that's already one problem solved (or rather solvable, it still requires > >>>>>someone to do the work). We will then be left with two different methods, > >>>>>drm bridge and non-bridge component-based instantiation. We need to > >>>>>somehow merge the two, and I'm open to discussions on how the end result > >>>>>should look like. > >>>> > >>>>I think you're idea really doesn't work - and I think your idea that > >>>>component-based is somehow separate from other methods is wrong. > >>>> > >>>>Look at iMX for example - even converting all the code that could be > >>>>a bridge to be a bridge will not get rid of its use of the component > >>>>instantiation, because you still have other components that need to > >>>>come from elsewhere. The same is true of armada as well. > >>> > >>>Don't get me wrong, I'm certainly not against the component framework itself. > >>>A way to bind multiple independent devices together is needed. We have a > >>>similar framework in V4L2 called v4l2-async, and I'd like to see it moved to > >>>the component framework at some point to unify code. Some changes to the > >>>component framework might be needed to support needs of V4L2 that are > >>>currently not applicable to DRM/KMS, but there's nothing major there. > >>> > >>>>Moreover, as I've already said, converting tda998x to a DRM bridge > >>>>will not get rid of the encoder/connector part, because it _is_ a > >>>>connector in the DRM sense - it provides the detection and EDID > >>>>reading. > >>>> > >>>>So, what would we achieve by splitting the driver into a DRM bridge > >>>>and DRM encoder/connector? > >>> > >>>Please note that DRM bridge doesn't split the DRM connector out of the bridge, > >>>bridges instantiate drm_connector objects. In that sense they don't differ > >>>much from the model implemented by the tda998x driver. > >>> > >>>I however believe that connectors should be split out DRM bridge drivers, for > >>>the simple reason that bridges can be chained. The output of a bridge isn't > >>>guaranteed to be a connector but can be another bridge. We managed not to have > >>>to deal with that in a generic way so far in mainline, but we'll run into the > >>>problem one of these days, and a solution will be needed. There's no rush > >>>right now, and this is unrelated to converting tda998x to DRM bridge. > >>> > >>>>We would still need the component helper to manage the binding of > >>>>the connector part, which would also then have to register a DRM > >>>>bridge in addition to a DRM encoder and providing the DRM connector > >>>>as we can't have two device drivers bound to the same i2c device. > >>>>What we get is more complexity in the driver. > >>> > >>>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. > >> > >>One more thing is that the TDA998x in its current form won't work > >>with KMS drivers that create their own drm_encoder objects to represent > >>their hardware's mipi DPI/RGB interfaces. For such drivers, we would > >>want the TDA998x to tie itself to the existing encoder created by the > >>KMS driver, rather than creating its own. > > > >Please show _technically_ how this would work. I want to see code or > >pseudo-code illustrating how a "foreign" DRM encoder could be used with > >either dw-hdmi or tda998x, because right now I can't see any way that > >could work. > > This is something we already do with the adv7511 bridge driver on msm, > rcar and arc (for 4.9) drivers. > > I've shared pseudo code on the kms driver and encoder chip's driver > side. I've also shared a diff that converts the tda998x driver to use > drm_bridge(uncompiled/untested). > > 1) Kms driver side: > > /* > * Create an encoder instance. Depending on the hardware represented > * by the KMS driver, the encoder can ops can either have some > * functionality, or be nops. In the case of tilcdc, the encoder > * funcs would be mostly nops. > */ > drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs); > drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs, > type, NULL); How does the KMS driver know to create the encoder? > /* > * Extract the bridge using DT node of the external encoder chip, > * i.e. tda998x > */ > > bridge = of_drm_find_bridge(encoder_chip_dev->of_node); What if we're not using DT? There seems to be no solution for that, so this currently breaks non-DT armada-drm. In the case of hardware which is: Dove ===> TDA998x ===> display Who provides this encoder_chip_dev's of_node? The Dove is the CRTC. The TDA998x is the bridge. What's the encoder? Do we need to make up a ficticious DT device for that. That will raise rightful objections, because it means DT is no longer representative of the hardware, but is representing the hardware in an implementation specific way - the split of the encoder from bridge is entirely a software abstraction.
>>>> 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.
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.
Hi Jean-François, On Friday 21 Oct 2016 19:28:47 Jean-Francois Moine wrote: > On Thu, 20 Oct 2016 16:56:44 +0530 Archit Taneja wrote: > >> Please show _technically_ how this would work. I want to see code or > >> pseudo-code illustrating how a "foreign" DRM encoder could be used with > >> either dw-hdmi or tda998x, because right now I can't see any way that > >> could work. > > > > This is something we already do with the adv7511 bridge driver on msm, > > rcar and arc (for 4.9) drivers. > > > > I've shared pseudo code on the kms driver and encoder chip's driver > > side. I've also shared a diff that converts the tda998x driver to use > > drm_bridge(uncompiled/untested). > > > > 1) Kms driver side: > > > > /* > > > > * Create an encoder instance. Depending on the hardware represented > > * by the KMS driver, the encoder can ops can either have some > > * functionality, or be nops. In the case of tilcdc, the encoder > > * funcs would be mostly nops. > > */ > > > > drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs); > > drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs, > > > > type, NULL); > > Then, how does this 'kms_priv' know the type of the encoder, this one > being tied to the connector type at the end of the bridge chain? "[PATCH 4/8] drm: Add encoder_type field to the drm_bridge structure" :-)
On 10/22/2016 12:13 AM, Russell King - ARM Linux wrote: > On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote: >> >> >> On 10/20/2016 02:45 PM, Russell King - ARM Linux wrote: >>> On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote: >>>> >>>> >>>> On 10/20/2016 01:50 PM, Laurent Pinchart wrote: >>>>> Hi Russell, >>>>> >>>>> On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote: >>>>>> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote: >>>>>>> On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote: >>>>>>>> In any case, I don't agree with converting it to a DRM bridge - that >>>>>>>> will mean that we have to split the driver into two pieces, the bridge >>>>>>>> part handling the mode set specifics, and a connector/encoder part which >>>>>>>> handles the detection/edid stuff. >>>>>>>> >>>>>>>> We might as well keep the whole thing as the classical connector/encoder >>>>>>>> rather than introducing this additional layer of complexity. >>>>>>> >>>>>>> We have different ways to instantiate external HDMI encoders, and that's >>>>>>> not good. I believe everybody agrees that drm encoder slave has to go, so >>>>>>> that's already one problem solved (or rather solvable, it still requires >>>>>>> someone to do the work). We will then be left with two different methods, >>>>>>> drm bridge and non-bridge component-based instantiation. We need to >>>>>>> somehow merge the two, and I'm open to discussions on how the end result >>>>>>> should look like. >>>>>> >>>>>> I think you're idea really doesn't work - and I think your idea that >>>>>> component-based is somehow separate from other methods is wrong. >>>>>> >>>>>> Look at iMX for example - even converting all the code that could be >>>>>> a bridge to be a bridge will not get rid of its use of the component >>>>>> instantiation, because you still have other components that need to >>>>>> come from elsewhere. The same is true of armada as well. >>>>> >>>>> Don't get me wrong, I'm certainly not against the component framework itself. >>>>> A way to bind multiple independent devices together is needed. We have a >>>>> similar framework in V4L2 called v4l2-async, and I'd like to see it moved to >>>>> the component framework at some point to unify code. Some changes to the >>>>> component framework might be needed to support needs of V4L2 that are >>>>> currently not applicable to DRM/KMS, but there's nothing major there. >>>>> >>>>>> Moreover, as I've already said, converting tda998x to a DRM bridge >>>>>> will not get rid of the encoder/connector part, because it _is_ a >>>>>> connector in the DRM sense - it provides the detection and EDID >>>>>> reading. >>>>>> >>>>>> So, what would we achieve by splitting the driver into a DRM bridge >>>>>> and DRM encoder/connector? >>>>> >>>>> Please note that DRM bridge doesn't split the DRM connector out of the bridge, >>>>> bridges instantiate drm_connector objects. In that sense they don't differ >>>>> much from the model implemented by the tda998x driver. >>>>> >>>>> I however believe that connectors should be split out DRM bridge drivers, for >>>>> the simple reason that bridges can be chained. The output of a bridge isn't >>>>> guaranteed to be a connector but can be another bridge. We managed not to have >>>>> to deal with that in a generic way so far in mainline, but we'll run into the >>>>> problem one of these days, and a solution will be needed. There's no rush >>>>> right now, and this is unrelated to converting tda998x to DRM bridge. >>>>> >>>>>> We would still need the component helper to manage the binding of >>>>>> the connector part, which would also then have to register a DRM >>>>>> bridge in addition to a DRM encoder and providing the DRM connector >>>>>> as we can't have two device drivers bound to the same i2c device. >>>>>> What we get is more complexity in the driver. >>>>> >>>>> 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. >>>> >>>> One more thing is that the TDA998x in its current form won't work >>>> with KMS drivers that create their own drm_encoder objects to represent >>>> their hardware's mipi DPI/RGB interfaces. For such drivers, we would >>>> want the TDA998x to tie itself to the existing encoder created by the >>>> KMS driver, rather than creating its own. >>> >>> Please show _technically_ how this would work. I want to see code or >>> pseudo-code illustrating how a "foreign" DRM encoder could be used with >>> either dw-hdmi or tda998x, because right now I can't see any way that >>> could work. >> >> This is something we already do with the adv7511 bridge driver on msm, >> rcar and arc (for 4.9) drivers. >> >> I've shared pseudo code on the kms driver and encoder chip's driver >> side. I've also shared a diff that converts the tda998x driver to use >> drm_bridge(uncompiled/untested). >> >> 1) Kms driver side: >> >> /* >> * Create an encoder instance. Depending on the hardware represented >> * by the KMS driver, the encoder can ops can either have some >> * functionality, or be nops. In the case of tilcdc, the encoder >> * funcs would be mostly nops. >> */ >> drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs); >> drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs, >> type, NULL); > > How does the KMS driver know to create the encoder? In the DT case, the Dove CRTC would see that it is connected to a device node which represents either a block of HW within the SoC, or an external chip like TDA998x. The KMS driver would be aware of its capabilities (i.e, whether it can encode RGB to HDMI itself or not). In cases where it can't do the encoding itself, it should create an encoder (with nop funcs) and try to find a bridge. Once it finds a bridge, it links the newly created encoder to the bridge, and attach it. In cases where we have encoding capability within the SoC, but also have another external encoder chip, it creates its own encoder as is (with the ops to configure the HW) and links itself with the external encoder bridge. To summarize, there are two cases: - The HW represented by the bridge driver is the only encoder in the chain: In this case, we create a 'dummy' encoder in the KMS driver, and link it to the bridge. - The HW represented by the bridge driver isn't the first encoder in the chain: In this case, the KMS driver creates its encoder as is, and gets the drm_bridge from the bridge driver, and links the two. > >> /* >> * Extract the bridge using DT node of the external encoder chip, >> * i.e. tda998x >> */ >> >> bridge = of_drm_find_bridge(encoder_chip_dev->of_node); > > What if we're not using DT? There seems to be no solution for that, so > this currently breaks non-DT armada-drm. Yeah, I agree. We haven't looked at non-DT cases yet. Will try to figure out what to do for non-DT scenarios. > > In the case of hardware which is: > > Dove ===> TDA998x ===> display > > Who provides this encoder_chip_dev's of_node? The Dove is the CRTC. > The TDA998x is the bridge. What's the encoder? Do we need to make up > a ficticious DT device for that. That will raise rightful objections, > because it means DT is no longer representative of the hardware, but > is representing the hardware in an implementation specific way - the > split of the encoder from bridge is entirely a software abstraction. The encoder_chip_dev is supposed to be the TDA998x device here. The Dove CRTC's output port would be directly connected to the input port of TDA998x in DT. There would be no encoder node in DT. The DT bindings wouldn't change. Sorry about the choice of name, it made it a bit confusing. Thanks, Archit
On 10/21/2016 11:39 PM, Russell King - ARM Linux wrote: > On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote: >> 3) Rough conversion to bridge: > > So I thought I might give this a try, and see what's needed to complete > the patch, but... > > I thought we'd got past the dark ages of email clients screwing up > patches, but it seems not. This patch is totally screwed that it's > not worth even sending - it has been: > > - wrapped > - white-space damaged > > which makes it pretty hard to undo all the damage. Please use a > better mail client which doesn't screw up patches, or send it as a > plain text attachment. Thanks. Sorry about that. I'll post out a proper patch for this once we resolve the drm_bridge shortcomings you've mentioned. I can create one if you want to try it now. Thanks, Archit
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
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.
On Mon, Oct 24, 2016 at 10:39:27AM +0530, Archit Taneja wrote: > Sorry about that. I'll post out a proper patch for this once we resolve > the drm_bridge shortcomings you've mentioned. I can create one if you > want to try it now. As said elsewhere, I've been away, so I haven't had a chance to look at anything. I'm going to be spending the early part of this week catching up with mail, but probably _not_ touching the kernel tree for any development stuff until I've caught up.
On Mon, Oct 24, 2016 at 08:53:04AM +0200, Daniel Vetter wrote: > 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: > > > 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). That model can't work for TDA998x if the TDA998x is followed by another "bridge" (eg, to convert the TDMS signals to something else) unless there's some way to tell a bridge that it isn't the last in the chain. However, my graph is accurate as it's reflecting the software modelling - it reflects how the various objects are bound together in DRM. The DRM encoder has pointers to the DRM bridge, which has a pointer to the next DRM bridge. The DRM connector doesn't have any pointers to the connector, only to the DRM encoder. So, DRM bridges are childs of the encoder, and the encoder (and attached encoder bridge chain) can be selected by the DRM connector. However, you are correct that for different "tasks" like mode setting, or output probing, the representation is somewhat different - that's not really what I was talking about though, and I certainly was not talking about the userspace representation. What I'm 100% concerned about is how this stuff looks in kernel space and what the driver(s) should look like. > > > 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. Sorry, but I do need exactly what I've written above, I can talk rather definitively because I've actually got the code in front of me. Most of the additional lines is due to the complexity added to the KMS driver to locate (actually for a third time) all the components in the system, specifically parsing the DT tree to find the "encoders" (or rather the TDA998x in this case), creating the DRM encoder objects, and binding the TDA998x bridge. Here's the _exact_ diffstat for the hacky conversion so far (including something like the 10 patches I posted last weekend, which haven't had any comments yet): drivers/gpu/drm/armada/armada_drv.c | 125 +++++ drivers/gpu/drm/i2c/tda998x_drv.c | 904 +++++++++++++++++------------------- 2 files changed, 560 insertions(+), 469 deletions(-) The actual bridge conversion on its own is: drivers/gpu/drm/armada/armada_drv.c | 125 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++---------------------- 2 files changed, 180 insertions(+), 84 deletions(-) > 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. That's not going to happen - and you know exactly why that's not going to happen - I've tried to do it and it failed misterably with all sorts of problems. The idea that it can be done piecemeal as per your guide is a falacy - it can't be. There is no progressive way to do a conversion. It seems that KMS drivers need to be rewritten from scratch, and that means there is a high risk of introducing lots of new bugs. I'm just not prepared to go through that - I'd much rather have a stable kernel driver that actually works than spend the next six months rewriting and debugging stuff just for the latest ideas about how stuff should be done. _OR_ there could be more help from DRM to ease the transition pain from non-atomic to atomic KMS drivers, so that it can be done in appropriately sized steps, so that the driver can be adequately tested to ensure that things don't totally fall apart... you know, like imx-drm has gone from being a stable driver to keep on falling apart now that it's been converted to atomic modeset. > 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. From my point of view, the TDA998x _is_ an encoder - it takes RGB and sync signals, and encodes them into the TDMS format for DVI or HDMI. I guess what I call an encoder is not what DRM calls an encoder though. What's in the Dove is effectively a pair of CRTCs, some muxes, a set of VGA DACs and a parallel RGB bus with pixel clock and sync signals. Apart from the VGA DACs (which aren't used in the TDA998x path) it's pretty hard to imagine what piece of hardware could be called an encoder. So what does the DRM encoder represent, hardware wise in this case? As I say, in my mind, the TDA998x _is_ the 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. Let me repeat: the "DRM connector" is part of the TDA998x - the TDA998x provides the EDID reading capabilities, and the connection detection capabilities. It also provides the CEC communication capabilities as well, but that's not too relevant to this discussion, apart from illustrating that it's an all-in-one single chip solution to providing a full HDMI source implementation. The TDA998x is not a stand-alone "bridge" which just _encodes_ a parallel RGB bus to TDMS signals, it's much more than that. That's why I'm saying we can't separate out the connector functionality from the encoder functionality. > > 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. Sorry, no you can't at the moment. As I've already said, grep for "bridge_list". Read the code in drivers/gpu/drm/drm_bridge.c, and notice that there's two places that this list is accessed: 1. inside drm_bridge_add() 2. inside of_drm_find_bridge() which is only available when CONFIG_OF is enabled, and requires a DT struct device_node pointer to perform the lookup. struct device_node's do not exist without DT. > > > 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). That's easy to say if you're into the "lets rewrite everything all at the same time" mentality, which from your response I think sums up your position on everything from atomic mode set to this problem. Sorry, I really hate the rewrite mentality, that's not good programming practice, especially when existing implementations work. What's instead required are a series of incremental steps to effect the full outcome, especially when multiple drivers are involved. If you look at the problems surrounding the removal of the drm_connector_register() from TDA998x, you'll see why this is important: it's not the drivers _with_ the mid-layer that's a problem here, but those which were converted prematurely, or written without using the mid-layer that are blocking the removal of drm_connector_register(). And the removal of drm_connector_register() from TDA998x blocks the removal of the mid-layer from armada, because removing the mid-layer _now_ causes the kernel to WARN - I know, I've tried it already: [ 1.933854] WARNING: CPU: 0 PID: 13 at /home/rmk/git/linux-cubox/lib/kobject.c:244 kobject_add_internal+0xfc/0x2d8 [ 1.944286] kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0) But... the mid-layer issue you raise is a complete red herring, the race has absolutely nothing to do with that. What causes the race is that during the KMS driver's probing, we get to the point where tda998x_bind() is called. This registers the DRM bridge so that the KMS driver can later find and attach to the bridge. However, just before creating the DRM bridge, it also creates a platform device for the audio codec side. As soon as that platform device is registered, ASoC is free to bind the audio subsystem and make it available to userspace. This means that any of the tda998x_audio_* functions are able to be called from the point that this platform device is registered. At this point, priv->bridge.encoder will be NULL, which means that some of the tda998x_audio_* functions should fail due to that. KMS driver initialisation can continue, writing the various pointers, and that happens without locking - but at that stage, it should only be going from NULL pointers to non-NULL pointers pointing at valid memory. However, there are no barriers to ensure that the various writes occur in the expected order (we're talking about writes in the KMS driver being visible to reads in the TDA998x audio side, possibly by another CPU, so locking isn't the answer - I can't see any way such a lock could be shared between TDA998x and various KMS drivers, or even some generic dummy DRM encoder helper. Barriers may be the answer, we need to ensure that encoder->dev is always valid before bridge->encoder is valid.) However, we need to also consider the initialisation failure and error clean up paths, assuming we have got this far - and that's where the worry is. drm_encoder_cleanup() memsets the entire encoder to zero. So, from the above, a compiler is perfectly at liberty to re-read the priv->bridge.encoder->dev pointer between these two statements: if (!priv->bridge.encoder || !priv->bridge.encoder->dev) return ret; config = &priv->bridge.encoder->dev->mode_config; and if such a re-read co-incides with the memset() in drm_encoder_cleanup() becoming visible, this is a possible oops waiting to happen. It gets worse if the KMS driver is responsible for freeing the DRM encoder that it created to attach to the TDA998x - if it frees that memory before tda998x_unbind() has been called, the audio subsystem will still be visible to userspace, and creates a potential use-after-free. So, none of this has anything what so ever to do with "is the KMS driver mid-layered or not" - this problem can exist irrespective of whether I have armada mid-layered or de-mid-layered. NB. It doesn't actually exist with armada, because armada is not used with the audio stuff on the cubox, we feed SPDIF to the TDA998x and let the TDA998x sort itself out, no audio codec is required there, but the point is that the complexities here are spread between TDA998x and associated KMS drivers - both have to be doing the right things for there not to be any subtle bugs here, and that is a really bad model. As I've already said, the problem does not exist as the driver stands in mainline today, only once it is converted to drm bridge, and it's purely down to the way the bridge code works. It is solvable, provided the connector remains part of TDA998x. So, like everything, we need to go through a series of steps to make these changes, and these steps need to happen in the right order, not as one huge great big lets-change-everything-at-once kind of approach. It's either going to take time, feeding changes into the kernel slowly, or it's going to need a lot of co-operation between different device driver authors, and sharing of stable commits between different git trees. Right now, the drm_connector_register() thing is basically blocking everything, and that needs to be handled in a way that's acceptable to all parties. The drm bridge conversion is something that can only happen once all the ducks are properly aligned - iow, drm_connector_register() gone, audio problems solved (eg, via the 10 patch series) and we have a way to convert TDA998x to a bridge without requiring every KMS user of TDA998x to simultaneously grow its own drm encoders.
On Mon, Oct 31, 2016 at 12:09:23AM +0000, Russell King - ARM Linux wrote: > On Mon, Oct 24, 2016 at 08:53:04AM +0200, Daniel Vetter wrote: > > 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: > > > > 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). > > That model can't work for TDA998x if the TDA998x is followed by > another "bridge" (eg, to convert the TDMS signals to something else) > unless there's some way to tell a bridge that it isn't the last in > the chain. > > However, my graph is accurate as it's reflecting the software > modelling - it reflects how the various objects are bound together in > DRM. The DRM encoder has pointers to the DRM bridge, which has a > pointer to the next DRM bridge. The DRM connector doesn't have any > pointers to the connector, only to the DRM encoder. So, DRM bridges > are childs of the encoder, and the encoder (and attached encoder > bridge chain) can be selected by the DRM connector. Small note: The connector -> encoder pointer is only used for legacy modesetting drivers. In atomic we shoveled it into drm_connector_state as as derived state of the connector->crtc link (which is what setCrtc and atomic ioctl set). > However, you are correct that for different "tasks" like mode setting, > or output probing, the representation is somewhat different - that's > not really what I was talking about though, and I certainly was not > talking about the userspace representation. > > What I'm 100% concerned about is how this stuff looks in kernel space > and what the driver(s) should look like. Ah, I missed that. Some shared code and pointers in generic drivers to untangle which exact drm_bridge owns the connector would certainly be useful. Otoh I'm not aware of any real-world chaining existing yet, I guess that's why this is unsolved. > > > > 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. > > Sorry, but I do need exactly what I've written above, I can talk rather > definitively because I've actually got the code in front of me. Most of > the additional lines is due to the complexity added to the KMS driver to > locate (actually for a third time) all the components in the system, > specifically parsing the DT tree to find the "encoders" (or rather the > TDA998x in this case), creating the DRM encoder objects, and binding the > TDA998x bridge. > > Here's the _exact_ diffstat for the hacky conversion so far (including > something like the 10 patches I posted last weekend, which haven't had > any comments yet): > > drivers/gpu/drm/armada/armada_drv.c | 125 +++++ > drivers/gpu/drm/i2c/tda998x_drv.c | 904 +++++++++++++++++------------------- > 2 files changed, 560 insertions(+), 469 deletions(-) > > The actual bridge conversion on its own is: > > drivers/gpu/drm/armada/armada_drv.c | 125 ++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++---------------------- > 2 files changed, 180 insertions(+), 84 deletions(-) Hm, this doesn't look good indeed ... > > 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. ... but I can't justify the effort really in making non-atomic drivers look good personally. Not going to reject patches from others of course. > That's not going to happen - and you know exactly why that's not going > to happen - I've tried to do it and it failed misterably with all sorts > of problems. The idea that it can be done piecemeal as per your guide > is a falacy - it can't be. There is no progressive way to do a > conversion. It seems that KMS drivers need to be rewritten from > scratch, and that means there is a high risk of introducing lots of > new bugs. > > I'm just not prepared to go through that - I'd much rather have a stable > kernel driver that actually works than spend the next six months rewriting > and debugging stuff just for the latest ideas about how stuff should be > done. gpus unfortunately change current idea of how stuff should work every few years. It's part of the job ... > _OR_ there could be more help from DRM to ease the transition pain from > non-atomic to atomic KMS drivers, so that it can be done in appropriately > sized steps, so that the driver can be adequately tested to ensure that > things don't totally fall apart... you know, like imx-drm has gone from > being a stable driver to keep on falling apart now that it's been > converted to atomic modeset. drm changes fast and I'm not entirely surprised that the conversion guide isn't fully up-to-date any more. I can help with debugging issues (preferrably on irc), but I can't magically fix bugs I'm not aware of. But in the end it's your call to either convert to atomic, refactor the core/helpers to need less dummy code for old modeset code or just carry some dummy code around in your driver. Atomic is definitely the way to go (since stuff like Android outright requires it to be useable). > > 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. > > From my point of view, the TDA998x _is_ an encoder - it takes RGB and > sync signals, and encodes them into the TDMS format for DVI or HDMI. > I guess what I call an encoder is not what DRM calls an encoder though. > > What's in the Dove is effectively a pair of CRTCs, some muxes, a set of > VGA DACs and a parallel RGB bus with pixel clock and sync signals. > Apart from the VGA DACs (which aren't used in the TDA998x path) it's > pretty hard to imagine what piece of hardware could be called an > encoder. > > So what does the DRM encoder represent, hardware wise in this case? > As I say, in my mind, the TDA998x _is_ the encoder. Rule of thumb: If the encoder is created as an integrated part of the overall display IP block, by the same IP company, it's probably best represented by a drm_encoder. If otoh it's an external IP block, reused in a bunch of places, it should be a drm_bridge. Think s/drm_bridge/drm_non_integrated_encoder/ or similar. It would of course be neat if the drm_encoder could be entirely no-op'ed out in that case, but because drm_encoders are also part of the uabi (imo a design mistake) you need to carry a dummy one around. Other guideline: The split between drm_crtc and drm_encoder should represent the display pipeline to outputs cross-bar (if you have one), since that's how helpers handle different outputs. For some chips there's a bit of generic per-output stuff, and hence it makes sense to both have encoder code and a separate drm_bridge. > > 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. > > Let me repeat: the "DRM connector" is part of the TDA998x - the TDA998x > provides the EDID reading capabilities, and the connection detection > capabilities. It also provides the CEC communication capabilities as > well, but that's not too relevant to this discussion, apart from > illustrating that it's an all-in-one single chip solution to providing > a full HDMI source implementation. > > The TDA998x is not a stand-alone "bridge" which just _encodes_ a parallel > RGB bus to TDMS signals, it's much more than that. That's why I'm saying > we can't separate out the connector functionality from the encoder > functionality. drm_bridge is meant to contain the connector, design-wise. Agreed that the code and helpers leaves a few things to be desired in this area. > > > 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. > > Sorry, no you can't at the moment. As I've already said, grep for > "bridge_list". Read the code in drivers/gpu/drm/drm_bridge.c, and > notice that there's two places that this list is accessed: > > 1. inside drm_bridge_add() > 2. inside of_drm_find_bridge() which is only available when CONFIG_OF > is enabled, and requires a DT struct device_node pointer to perform > the lookup. struct device_node's do not exist without DT. Well, then it needs to be added. It's open-source after 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). > > That's easy to say if you're into the "lets rewrite everything all at > the same time" mentality, which from your response I think sums up > your position on everything from atomic mode set to this problem. > > Sorry, I really hate the rewrite mentality, that's not good programming > practice, especially when existing implementations work. What's > instead required are a series of incremental steps to effect the > full outcome, especially when multiple drivers are involved. We've been "rewriting" i915 to be atomic in small steps for 2 years now. It works. > If you look at the problems surrounding the removal of the > drm_connector_register() from TDA998x, you'll see why this is important: > it's not the drivers _with_ the mid-layer that's a problem here, but > those which were converted prematurely, or written without using the > mid-layer that are blocking the removal of drm_connector_register(). > > And the removal of drm_connector_register() from TDA998x blocks the > removal of the mid-layer from armada, because removing the mid-layer > _now_ causes the kernel to WARN - I know, I've tried it already: > > [ 1.933854] WARNING: CPU: 0 PID: 13 at /home/rmk/git/linux-cubox/lib/kobject.c:244 kobject_add_internal+0xfc/0x2d8 > [ 1.944286] kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0) > > But... the mid-layer issue you raise is a complete red herring, the > race has absolutely nothing to do with that. > > What causes the race is that during the KMS driver's probing, we get > to the point where tda998x_bind() is called. This registers the > DRM bridge so that the KMS driver can later find and attach to the > bridge. > > However, just before creating the DRM bridge, it also creates a > platform device for the audio codec side. As soon as that platform > device is registered, ASoC is free to bind the audio subsystem and > make it available to userspace. > > This means that any of the tda998x_audio_* functions are able to > be called from the point that this platform device is registered. > > At this point, priv->bridge.encoder will be NULL, which means that > some of the tda998x_audio_* functions should fail due to that. > > KMS driver initialisation can continue, writing the various pointers, > and that happens without locking - but at that stage, it should only > be going from NULL pointers to non-NULL pointers pointing at valid > memory. However, there are no barriers to ensure that the various > writes occur in the expected order (we're talking about writes in the > KMS driver being visible to reads in the TDA998x audio side, possibly > by another CPU, so locking isn't the answer - I can't see any way such > a lock could be shared between TDA998x and various KMS drivers, or > even some generic dummy DRM encoder helper. Barriers may be the > answer, we need to ensure that encoder->dev is always valid before > bridge->encoder is valid.) > > However, we need to also consider the initialisation failure and > error clean up paths, assuming we have got this far - and that's > where the worry is. drm_encoder_cleanup() memsets the entire > encoder to zero. So, from the above, a compiler is perfectly at > liberty to re-read the priv->bridge.encoder->dev pointer between > these two statements: > > if (!priv->bridge.encoder || !priv->bridge.encoder->dev) > return ret; > > config = &priv->bridge.encoder->dev->mode_config; > > and if such a re-read co-incides with the memset() in > drm_encoder_cleanup() becoming visible, this is a possible oops > waiting to happen. > > It gets worse if the KMS driver is responsible for freeing the DRM > encoder that it created to attach to the TDA998x - if it frees that > memory before tda998x_unbind() has been called, the audio subsystem > will still be visible to userspace, and creates a potential > use-after-free. > > So, none of this has anything what so ever to do with "is the KMS > driver mid-layered or not" - this problem can exist irrespective of > whether I have armada mid-layered or de-mid-layered. Not entirely clear to me from your description, but I think if the audio platform device registration and unregistration is put into the new connector register_late/unregister_early callbacks, and if the load/unload sequence is fixed to register everything as the last step/unregister as the first, then this should be fixed. And if the bridge owns the connector, it can set these callbacks. If it's not fixed then I need to take another look at your code, because fixing these kind of issues was exactly the goal with the load/unload reorg. We have a very similar problem in i915 on connectors, but with the backlight interfaces. > NB. It doesn't actually exist with armada, because armada is not used > with the audio stuff on the cubox, we feed SPDIF to the TDA998x and > let the TDA998x sort itself out, no audio codec is required there, > but the point is that the complexities here are spread between > TDA998x and associated KMS drivers - both have to be doing the > right things for there not to be any subtle bugs here, and that is > a really bad model. As I've already said, the problem does not > exist as the driver stands in mainline today, only once it is > converted to drm bridge, and it's purely down to the way the bridge > code works. It is solvable, provided the connector remains part of > TDA998x. > > So, like everything, we need to go through a series of steps to make > these changes, and these steps need to happen in the right order, > not as one huge great big lets-change-everything-at-once kind of > approach. > > It's either going to take time, feeding changes into the kernel slowly, > or it's going to need a lot of co-operation between different device > driver authors, and sharing of stable commits between different git > trees. > > Right now, the drm_connector_register() thing is basically blocking > everything, and that needs to be handled in a way that's acceptable to > all parties. The drm bridge conversion is something that can only > happen once all the ducks are properly aligned - iow, > drm_connector_register() gone, audio problems solved (eg, via the > 10 patch series) and we have a way to convert TDA998x to a bridge > without requiring every KMS user of TDA998x to simultaneously grow > its own drm encoders. tbh I think I'm lost in all the actual conversion issues at hand here. I jumped into the discussion since there seemed to be some confusion going on at higher levels. Aside: This should be all documented in kernel-doc somewhere. If not please raise this, I'll try to improve the docs - (rfc) doc patches very much welcome of course too. Thanks, Daniel
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 9798d40..bdca061 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -60,7 +60,7 @@ struct tda998x_priv { wait_queue_head_t edid_delay_waitq; bool edid_delay_active; - struct drm_encoder encoder; + struct drm_bridge bridge; struct drm_connector connector; struct tda998x_audio_port audio_port[2]; @@ -69,8 +69,8 @@ struct tda998x_priv { #define conn_to_tda998x_priv(x) \ container_of(x, struct tda998x_priv, connector) -#define enc_to_tda998x_priv(x) \ - container_of(x, struct tda998x_priv, encoder) +#define bridge_to_tda998x_priv(x) \ + container_of(x, struct tda998x_priv, bridge) /* The TDA9988 series of devices use a paged register scheme.. to simplify * things we encode the page # in upper bits of the register #. To read/ @@ -614,7 +614,7 @@ static void tda998x_detect_work(struct work_struct *work) { struct tda998x_priv *priv = container_of(work, struct tda998x_priv, detect_work); - struct drm_device *dev = priv->encoder.dev; + struct drm_device *dev = priv->connector.dev; if (dev)