[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
On Mon, Oct 31, 2016 at 12:09:23AM +, 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
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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. -- 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.
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
Hi Russell, On Sat, Oct 22, 2016 at 02:40:22PM +0100, Russell King - ARM Linux wrote: >On Wed, Oct 19, 2016 at 10:46:48AM +0100, Brian Starkey wrote: >> Hi Jyri, >> >> I believe this will break mali-dp and hdlcd, unless something changed >> while I wasn't looking. Please see this previous thread where I did >> the same thing and then had to have it reverted: [1] >> >> Before removing this, we need to refactor (at least) mali-dp and hdlcd >> to move drm_dev_register() to the end of their ->bind() callback. >> >> That could be done without moving drm_dev_unregister() to the start >> of ->unbind() if you really want to nuke the drm_connector_register() >> call, but to maintain symmetry (and introduce correctness) I was >> putting it off until I had a chance to remove drm_vblank_cleanup() >> from drm_dev_unregister() (because [2]). > >So what is the status of this - when is it going to happen? Without >this happening, I can't de-midlayer armada-drm, and I can't apply >these TDA998x patches. > >As armada-drm stands at the moment, it can cope with the TDA998x >driver having the drm_connector_register(), or with it eliminated. >When armada-drm is de-midlayered without changing TDA998x, the >drm_connector_register() call in TDA998x produces a warning: > >WARNING: CPU: 0 PID: 13 at lib/kobject.c:244 kobject_add_internal+0xfc/0x2d8 >kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0) > >I suspect that you will end up with the same problem when you move >the drm_dev_register() call after component_bind_all() - and I think >the answer is... you shouldn't have de-midlayered until TDA998x had >been updated to cope with de-midlayering, iow having had the >drm_connector_register() call removed. > Well technically neither hdlcd or mali-dp ever had a midlayer, but yes it would have been nice to have never introduced this problem in the first place, I agree. >Given that, I don't think we can avoid breaking mali-dp and hdlcd, >except by combining the change into a single patch, changing all three >drivers simultaneously (and any other driver which uses TDA998x which >has also been de-midlayered.) > There is a way - we can explicitly register the connectors in hdlcd and mali-dp (drm_connector_register_all used to be exposed for that job, afaik to work around the kind of issue we face here). But, that would introduce some extra churn, so probably a single patch for all three works just fine. I couldn't spot any other drivers I think will be affected. tilcdc isn't de-midlayered. >So, what I would like to see is a single patch against Linus' 4.8.0 >commit fixing mali-dp, hdlcd and any other driver, together with >removing drm_connector_register() from TDA998x. This is so the patch >can be shared between all interested parties without forcing everyone >to 4.9-rc1. Looking at the diff between 4.8 and 4.9-rc1 for >drivers/gpu/drm/arm, that shouldn't result in any merge conflicts - >and if you want to follow on from that with 4.9-rc1 development, you >can always merge 4.9-rc1 on top of that commit. > >I'm happy to take such a patch and publish it via my git tree as part >of the TDA998x development if that helps - but either way we need it >shared between all parties. OK, I will follow up to this email with that patch. I note that it will conflict with the series you sent out yesterday (23rd October). Hopefully that's an agreeable solution for you, and everyone else. Thanks, -Brian > >-- >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. >
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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 -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
On Wed, Oct 19, 2016 at 10:46:48AM +0100, Brian Starkey wrote: > Hi Jyri, > > I believe this will break mali-dp and hdlcd, unless something changed > while I wasn't looking. Please see this previous thread where I did > the same thing and then had to have it reverted: [1] > > Before removing this, we need to refactor (at least) mali-dp and hdlcd > to move drm_dev_register() to the end of their ->bind() callback. > > That could be done without moving drm_dev_unregister() to the start > of ->unbind() if you really want to nuke the drm_connector_register() > call, but to maintain symmetry (and introduce correctness) I was > putting it off until I had a chance to remove drm_vblank_cleanup() > from drm_dev_unregister() (because [2]). So what is the status of this - when is it going to happen? Without this happening, I can't de-midlayer armada-drm, and I can't apply these TDA998x patches. As armada-drm stands at the moment, it can cope with the TDA998x driver having the drm_connector_register(), or with it eliminated. When armada-drm is de-midlayered without changing TDA998x, the drm_connector_register() call in TDA998x produces a warning: WARNING: CPU: 0 PID: 13 at lib/kobject.c:244 kobject_add_internal+0xfc/0x2d8 kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0) I suspect that you will end up with the same problem when you move the drm_dev_register() call after component_bind_all() - and I think the answer is... you shouldn't have de-midlayered until TDA998x had been updated to cope with de-midlayering, iow having had the drm_connector_register() call removed. Given that, I don't think we can avoid breaking mali-dp and hdlcd, except by combining the change into a single patch, changing all three drivers simultaneously (and any other driver which uses TDA998x which has also been de-midlayered.) So, what I would like to see is a single patch against Linus' 4.8.0 commit fixing mali-dp, hdlcd and any other driver, together with removing drm_connector_register() from TDA998x. This is so the patch can be shared between all interested parties without forcing everyone to 4.9-rc1. Looking at the diff between 4.8 and 4.9-rc1 for drivers/gpu/drm/arm, that shouldn't result in any merge conflicts - and if you want to follow on from that with 4.9-rc1 development, you can always merge 4.9-rc1 on top of that commit. I'm happy to take such a patch and publish it via my git tree as part of the TDA998x development if that helps - but either way we need it shared between all parties. -- 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.
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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(_priv->encoder, _encoder_helper_funcs); > > drm_encoder_init(kms_pirv->drm, _priv->encoder, _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" :-) -- Regards, Laurent Pinchart
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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 = >bridge.encoder->dev->mode_config;
mutex_lock(>mutex);
list_for_each_entry(connector, >connector_list, head) {
if (priv->bridge.encoder == connector->encoder) {
memcpy(buf, connector->eld,
min(sizeof(connector->eld), len));
ret = 0;
}
}
mutex_unlock(>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
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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. -- Ken ar c'hentañ| ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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(_priv->encoder, _encoder_helper_funcs); > drm_encoder_init(kms_pirv->drm, _priv->encoder, _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? -- Ken ar c'hentañ| ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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. -- 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.
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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. Thanks, Archit > >> We can see this with what happened to the DW-HDMI driver - that still >> needs the component helper, and it provides a DRM bridge, DRM encoder >> and DRM connector parts. > > To be precise, the DW-HDMI driver core doesn't create a DRM encoder, it's the > glue layers implemented as part of the Rockchip and iMX display drivers that > do. Nonetheless, that's a mistake, the encoder should be created by the > display drivers. > >> The only reason it made sense to split the DW-HDMI driver was due
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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. > We can see this with what happened to the DW-HDMI driver - that still > needs the component helper, and it provides a DRM bridge, DRM encoder > and DRM connector parts. To be precise, the DW-HDMI driver core doesn't create a DRM encoder, it's the glue layers implemented as part of the Rockchip and iMX display drivers that do. Nonetheless, that's a mistake, the encoder should be created by the display drivers. > The only reason it made sense to split the DW-HDMI driver was due to the > differences between the Rockchip and Freescale implementations. Such > differences do not exist for TDA998x. So even here, your idea that "DRM > bridge" vs "non-DRM bridge component based" doesn't work - we have "DRM > bridge component based" because of the problem that I'm illustrating here. > > So, again, I ask: what's the point of needlessly splitting the code > between an encoder/connector and a bridge? We need to standardize on one
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
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. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
On Thu, Oct 20, 2016 at 11:20:05AM +0300, Laurent Pinchart wrote: > Hi Russell, > > On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote: > > 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. So, we what you're saying is that we from a component based DRM encoder plus DRM connector to a component based DRM bridge plus DRM connector and some nebulous DRM encoder provider. How does the DRM encoder get associated with the DRM connector? DRM requires the presence of at least one DRM encoder for each DRM connector, and the DRM connector needs to provide a .best_encoder method to pick the appropriate DRM encoder. If (as you said elsewhere in your message) that the display driver is responsible for providing the DRM encoder in your view, how does a bridge DRM connector get to that DRM encoder, and how does it know which DRM encoder to pick? > 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. You've not said how that could be achieved with TDA998x, so I'm still opposed to the idea. Remember, you're the one asking for this, you must have a view on how to achieve it if you want it to happen. > > We can see this with what happened to the DW-HDMI driver - that still > > needs the component helper, and it provides a DRM bridge, DRM encoder > > and DRM connector parts. > > To be precise, the DW-HDMI driver core doesn't create a DRM encoder, it's the > glue layers implemented as part of the Rockchip and iMX display drivers that > do. Nonetheless, that's a mistake, the encoder should be created by the > display drivers. If we're being precise, the "glue layer" creates the DRM encoder, but the bridge code is responsible for binding itself to the DRM encoder, and binding the DRM encoder to its associated DRM connector. Let's assume it's a mistake. Please illustrate how you would solve this mistake. > > The only reason it made sense to split the DW-HDMI driver was due to the > > differences between the Rockchip and Freescale implementations. Such > > differences do not exist for TDA998x. So even here, your idea that "DRM > > bridge" vs "non-DRM bridge component based" doesn't work - we have "DRM > > bridge component based" because of the problem that I'm illustrating here. > > > > So, again, I ask: what's the point of needlessly splitting the code > > between an encoder/connector and a bridge? > > We need to standardize on one model. I don't care much about how the end > result is named, as long as it fulfils the task at hand. I don't care about the name either, that's not what I'm asking here. All I seem to be getting is some hand-waving "we want one model" and "the current situation is a huge mess" (which is not a sentiment I agree with) but with no technical response about how to achieve it. Please do the technical response bits and stop hand-waving. -- 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.
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
Hi Russell, On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote: > On Wed, Oct 19, 2016 at 11:52:15AM +0300, Laurent Pinchart wrote: > > On Wednesday 19 Oct 2016 09:16:30 Russell King - ARM Linux wrote: > >> On Wed, Oct 19, 2016 at 10:54:06AM +0300, Laurent Pinchart wrote: > >>> On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote: > Remove obsolete drm_connector_register() call from tda998x_bind(). > All connectors are registered when drm_dev_register() is called by the > master drm_device driver. > > Signed-off-by: Jyri Sarha > >>> > >>> Acked-by: Laurent Pinchart > >>> > >>> By the way, any chance you would plan porting the driver to drm_bridge > >>> ? :-) > >> > >> What's the point? > > > > Avoiding code duplication. We currently have three APIs to handle external > > encoders (drm encoder slave, drm bridge and the component-based method > > used by tda998x only), which requires display drivers that want to > > support multiple external encoders to use up to three APIs. > > tda998x doesn't use the encoder slave anymore. You somehow list component- > based as a third alternative - it isn't, that's the native DRM non-bridge > method. The order in my list was random, component-based instantiation of external encoders is one method, not specifically the third one :-) > 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. -- Regards, Laurent Pinchart
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
Hi Russell, On Wednesday 19 Oct 2016 09:16:30 Russell King - ARM Linux wrote: > On Wed, Oct 19, 2016 at 10:54:06AM +0300, Laurent Pinchart wrote: > > On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote: > >> Remove obsolete drm_connector_register() call from tda998x_bind(). All > >> connectors are registered when drm_dev_register() is called by the > >> master drm_device driver. > >> > >> Signed-off-by: Jyri Sarha > > > > Acked-by: Laurent Pinchart > > > > By the way, any chance you would plan porting the driver to drm_bridge ? > > :-) > > What's the point? Avoiding code duplication. We currently have three APIs to handle external encoders (drm encoder slave, drm bridge and the component-based method used by tda998x only), which requires display drivers that want to support multiple external encoders to use up to three APIs. -- Regards, Laurent Pinchart
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
Hi Jyri, Thank you for the patch. On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote: > Remove obsolete drm_connector_register() call from tda998x_bind(). All > connectors are registered when drm_dev_register() is called by the > master drm_device driver. > > Signed-off-by: Jyri Sarha Acked-by: Laurent Pinchart By the way, any chance you would plan porting the driver to drm_bridge ? :-) > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 6 -- > 1 file changed, 6 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > b/drivers/gpu/drm/i2c/tda998x_drv.c index 9798d40..265f854 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -1656,16 +1656,10 @@ static int tda998x_bind(struct device *dev, struct > device *master, void *data) if (ret) > goto err_connector; > > - ret = drm_connector_register(>connector); > - if (ret) > - goto err_sysfs; > - > drm_mode_connector_attach_encoder(>connector, >encoder); > > return 0; > > -err_sysfs: > - drm_connector_cleanup(>connector); > err_connector: > drm_encoder_cleanup(>encoder); > err_encoder: -- Regards, Laurent Pinchart
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
Hi Jyri, I believe this will break mali-dp and hdlcd, unless something changed while I wasn't looking. Please see this previous thread where I did the same thing and then had to have it reverted: [1] Before removing this, we need to refactor (at least) mali-dp and hdlcd to move drm_dev_register() to the end of their ->bind() callback. That could be done without moving drm_dev_unregister() to the start of ->unbind() if you really want to nuke the drm_connector_register() call, but to maintain symmetry (and introduce correctness) I was putting it off until I had a chance to remove drm_vblank_cleanup() from drm_dev_unregister() (because [2]). Thanks, Brian [1] https://lists.freedesktop.org/archives/dri-devel/2016-September/119038.html [2] https://lists.freedesktop.org/archives/dri-devel/2016-September/119268.html On Wed, Oct 19, 2016 at 12:33:54AM +0300, Jyri Sarha wrote: >Remove obsolete drm_connector_register() call from tda998x_bind(). All >connectors are registered when drm_dev_register() is called by the >master drm_device driver. > >Signed-off-by: Jyri Sarha >--- > drivers/gpu/drm/i2c/tda998x_drv.c | 6 -- > 1 file changed, 6 deletions(-) > >diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c >b/drivers/gpu/drm/i2c/tda998x_drv.c >index 9798d40..265f854 100644 >--- a/drivers/gpu/drm/i2c/tda998x_drv.c >+++ b/drivers/gpu/drm/i2c/tda998x_drv.c >@@ -1656,16 +1656,10 @@ static int tda998x_bind(struct device *dev, struct >device *master, void *data) > if (ret) > goto err_connector; > >- ret = drm_connector_register(>connector); >- if (ret) >- goto err_sysfs; >- > drm_mode_connector_attach_encoder(>connector, >encoder); > > return 0; > >-err_sysfs: >- drm_connector_cleanup(>connector); > err_connector: > drm_encoder_cleanup(>encoder); > err_encoder: >-- >1.9.1 > >___ >dri-devel mailing list >dri-devel at lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote: > Hi Russell, > > 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. 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? 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. We can see this with what happened to the DW-HDMI driver - that still needs the component helper, and it provides a DRM bridge, DRM encoder and DRM connector parts. The only reason it made sense to split the DW-HDMI driver was due to the differences between the Rockchip and Freescale implementations. Such differences do not exist for TDA998x. So even here, your idea that "DRM bridge" vs "non-DRM bridge component based" doesn't work - we have "DRM bridge component based" because of the problem that I'm illustrating here. So, again, I ask: what's the point of needlessly splitting the code between an encoder/connector and a bridge? You seem to be forcing the DRM bridge model onto drivers with no regard for its appropriateness for those drivers. If it pushes more complexity into drivers, the model is wrong. -- 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.
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
On Wed, Oct 19, 2016 at 11:52:15AM +0300, Laurent Pinchart wrote: > Hi Russell, > > On Wednesday 19 Oct 2016 09:16:30 Russell King - ARM Linux wrote: > > On Wed, Oct 19, 2016 at 10:54:06AM +0300, Laurent Pinchart wrote: > > > On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote: > > >> Remove obsolete drm_connector_register() call from tda998x_bind(). All > > >> connectors are registered when drm_dev_register() is called by the > > >> master drm_device driver. > > >> > > >> Signed-off-by: Jyri Sarha > > > > > > Acked-by: Laurent Pinchart > > > > > > By the way, any chance you would plan porting the driver to drm_bridge ? > > > :-) > > > > What's the point? > > Avoiding code duplication. We currently have three APIs to handle external > encoders (drm encoder slave, drm bridge and the component-based method used > by > tda998x only), which requires display drivers that want to support multiple > external encoders to use up to three APIs. tda998x doesn't use the encoder slave anymore. You somehow list component- based as a third alternative - it isn't, that's the native DRM non-bridge method. 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. -- 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.
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
On Wed, Oct 19, 2016 at 10:54:06AM +0300, Laurent Pinchart wrote: > Hi Jyri, > > Thank you for the patch. > > On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote: > > Remove obsolete drm_connector_register() call from tda998x_bind(). All > > connectors are registered when drm_dev_register() is called by the > > master drm_device driver. > > > > Signed-off-by: Jyri Sarha > > Acked-by: Laurent Pinchart > > By the way, any chance you would plan porting the driver to drm_bridge ? :-) What's the point? -- 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.
[PATCH 2/4] drm/i2c: tda998x: Remove obsolete drm_connector_register() call
Remove obsolete drm_connector_register() call from tda998x_bind(). All connectors are registered when drm_dev_register() is called by the master drm_device driver. Signed-off-by: Jyri Sarha --- drivers/gpu/drm/i2c/tda998x_drv.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 9798d40..265f854 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1656,16 +1656,10 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data) if (ret) goto err_connector; - ret = drm_connector_register(>connector); - if (ret) - goto err_sysfs; - drm_mode_connector_attach_encoder(>connector, >encoder); return 0; -err_sysfs: - drm_connector_cleanup(>connector); err_connector: drm_encoder_cleanup(>encoder); err_encoder: -- 1.9.1
