drm/tilcdc: Cleanup tilcdc (&tda998x) init sequence

Remove obsolete drm_connector_register() calls from tilcdc_panel.c and tilcdc_tfp410.c. All connectors are registered when drm_dev_register() is called.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 2 --
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 2 --
 2 files changed, 4 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/tilcdc/tilcdc_panel.c index 2134bb20..ad7a0e8 100644

drivers/gpu/drm/tilcdc/tilcdc_panel.c +0 -2

@@ -240,8 +240,6 @@ static struct drm_connector
 *panel_connector_create(struct drm_device *dev, if (ret)
 		goto fail;
 
-	drm_connector_register(connector);
-
 	return connector;
 
 fail:

(quoted diff) +0 -0

b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c index c32c1ef..eea82b4 100644

drivers/gpu/drm/tilcdc/tilcdc_tfp410.c +0 -2

@@ -251,8 +251,6 @@ static struct drm_connector
 *tfp410_connector_create(struct drm_device *dev, if (ret)
 		goto fail;
 
-	drm_connector_register(connector);
-
 	return connector;
 
 fail:

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 <[email protected]>

---
 drivers/gpu/drm/i2c/tda998x_drv.c | 6 ------
 1 file changed, 6 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i2c/tda998x_drv.c index 9798d40..265f854 100644

drivers/gpu/drm/i2c/tda998x_drv.c +0 -6

@@ -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(&priv->connector);
-	if (ret)
-		goto err_sysfs;
-
 	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
 
 	return 0;
 
-err_sysfs:
-	drm_connector_cleanup(&priv->connector);
 err_connector:
 	drm_encoder_cleanup(&priv->encoder);
 err_encoder:

Hi Jyri,

Thank you for the patch.

Quoted 4 lines · 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.

Quoted 1 line

Signed-off-by Jyri Sarha <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

By the way, any chance you would plan porting the driver to drm_bridge ? :-)

Quoted 5 lines · Laurent Pinchart

Quoted 4 lines · 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.

Quoted 6 lines

Quoted 1 line

Signed-off-by Jyri Sarha <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

By the way, any chance you would plan porting the driver to drm_bridge ? :-)

What's the point?

Hi Russell,

Quoted 13 lines · On Wednesday 19 Oct 2016 09:16:30 Russell King - ARM Linux wrote:

Quoted 11 lines · Laurent Pinchart

Quoted 5 lines · 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 <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

By the way, any chance you would plan porting the driver to drm_bridge ? :-)

Quoted 1 line

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.

Quoted 16 lines · Laurent Pinchart

Quoted 14 lines · On Wednesday 19 Oct 2016 09:16:30 Russell King - ARM Linux wrote:

Quoted 11 lines · Laurent Pinchart

Quoted 5 lines · 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 <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

By the way, any chance you would plan porting the driver to drm_bridge ? :-)

What's the point?

Quoted 4 lines

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.

Hi Russell,

Quoted 7 lines · 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.

Quoted 9 lines · Laurent Pinchart

Quoted 7 lines · 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.

Quoted 7 lines

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.

Hi Russell,

Quoted 18 lines · On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:

Quoted 9 lines · Laurent Pinchart

Quoted 7 lines · 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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

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. For the reasons explained above, it should not create a DRM encoder or DRM connector. A step by step approach is obviously needed to get there, one option being moving all external encoders to DRM bridge as a first step without instantiating the DRM encoder in the bridge driver, and then moving connector instantiation out as a second step.

The current situation is a huge mess, we simply can't pretend to ignore the problem.

Quoted 3 lines

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.

I expect several (many? most?) display drivers to handle bridges, encoders and connectors in the same way, so we should obviously share common code in the form of helper functions to keep display drivers simple.

Quoted 61 lines · Laurent Pinchart

Hi Russell,

Quoted 18 lines · On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:

Quoted 9 lines · Laurent Pinchart

Quoted 7 lines · 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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 75 lines · Archit Taneja

Quoted 61 lines · Laurent Pinchart

Hi Russell,

Quoted 18 lines · On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:

Quoted 9 lines · Laurent Pinchart

Quoted 7 lines · 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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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);

Quoted 22 lines · On Thu, 20 Oct 2016 16:56:44 +0530 Archit Taneja wrote:

Quoted 4 lines

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

Quoted 4 lines

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?

3) Rough conversion to bridge:

Quoted 1 line · Archit Taneja

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.

Quoted 82 lines · Russell King - ARM Linux

Quoted 75 lines · Archit Taneja

Quoted 61 lines · Laurent Pinchart

Hi Russell,

Quoted 18 lines · On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:

Quoted 9 lines · Laurent Pinchart

Quoted 7 lines · 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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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);

/*
* Extract the bridge using DT node of the external encoder chip,
* i.e. tda998x
*/

bridge = of_drm_find_bridge(encoder_chip_dev->of_node);

Quoted 107 lines · Archit Taneja

Quoted 82 lines · Russell King - ARM Linux

Quoted 75 lines · Archit Taneja

Quoted 61 lines · Laurent Pinchart

Hi Russell,

Quoted 18 lines · On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:

Quoted 9 lines · Laurent Pinchart

Quoted 7 lines · 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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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,

Quoted 1 line

		 type, NULL);

How does the KMS driver know to create the encoder?

Quoted 7 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line · Laurent Pinchart

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Laurent Pinchart

(sorry, I lost your original mail)

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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.

Quoted 13 lines · Jean-Francois Moine

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Laurent Pinchart

(sorry, I lost your original mail)

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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.

Quoted 7 lines

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

Quoted 25 lines · Russell King - ARM Linux

Quoted 13 lines · Jean-Francois Moine

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Laurent Pinchart

(sorry, I lost your original mail)

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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.

Quoted 7 lines

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:

Quoted 8 lines

	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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

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.

Quoted 32 lines

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);

Quoted 4 lines

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.

Quoted 4 lines · Archit Taneja

Quoted 3 lines · Russell King - ARM Linux

Looking at drm_bridge_disable() and drm_bridge_enable(), the control model appears to be:

Quoted 8 lines

Quoted 8 lines

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

Quoted 7 lines

Quoted 7 lines

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.

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.

Quoted 3 lines

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.

Quoted 37 lines

Quoted 32 lines

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);

Quoted 4 lines

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

Quoted 5 lines

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

Quoted 19 lines · Daniel Vetter

Quoted 4 lines · Archit Taneja

Quoted 3 lines · Russell King - ARM Linux

Looking at drm_bridge_disable() and drm_bridge_enable(), the control model appears to be:

Quoted 8 lines

Quoted 8 lines

	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

Quoted 4 lines

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.

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

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.

Quoted 4 lines

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(-)

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 43 lines

Quoted 37 lines

Quoted 32 lines

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);

Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 6 lines

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.

Hi Russell,

Quoted 5 lines · 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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

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.

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 <[email protected]>

---
drivers/gpu/drm/i2c/tda998x_drv.c | 6 ------

1 file changed, 6 deletions(-)

drivers/gpu/drm/i2c/tda998x_drv.c +0 -6

index 9798d40..265f854 100644
@@ -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(&priv->connector);
-	if (ret)
-		goto err_sysfs;
-
 	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
 
 	return 0;
 
-err_sysfs:
-	drm_connector_cleanup(&priv->connector);

err_connector:

	drm_encoder_cleanup(&priv->encoder);

err_encoder:

Signature · 6 lines

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Quoted 6 lines · Brian Starkey

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]

Quoted 8 lines

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.

Signature · 4 lines

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

Connectors shouldn't be registered until the rest of the whole device is set up, so that consistent state is presented to userspace.

As such, remove the calls to drm_connector_register() and drm_connector_unregister() from tda998x, as these are now handled by drm_dev_(un)register() itself.

To work with this change, the mali-dp and hdlcd bind and unbind sequences have to be reordered, to ensure that the componentised encoder/connector is bound before drm_dev_register() registers all connectors. Similarly, the device must be unregistered before the component is unbound.

Altogether, this allows other drivers using tda998x to be de-midlayered, and to have less racy initialisation of their components.

Splitting this commit into three (one per driver) isn't possible without intermediate breakage, so it is all squashed together here.

Suggested-by Russell King <[email protected]>

Signed-off-by Brian Starkey <[email protected]>

Reviewed-by Liviu Dudau <[email protected]>

drivers/gpu/drm/i2c/tda998x_drv.c +0 -1

index f4315bc..6e6fca2 100644
@@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = {
 
 static void tda998x_connector_destroy(struct drm_connector *connector)
 {
-	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 }
 

drivers/gpu/drm/i2c/tda998x_drv.c +0 -3

 	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
 
 	return 0;
 
-err_sysfs:
-	drm_connector_cleanup(&priv->connector);
 err_connector:
 	drm_encoder_cleanup(&priv->encoder);
 err_encoder:

@@ -1463,7 +1456,6 @@ static void tda998x_unbind(struct device *dev, struct device *master,
 {
 	struct tda998x_priv *priv = dev_get_drvdata(dev);
 
-	drm_connector_unregister(&priv->connector);
 	drm_connector_cleanup(&priv->connector);
 	drm_encoder_cleanup(&priv->encoder);
 	tda998x_destroy(priv);

Signature · 6 lines

1.7.9.5

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 36 lines · Brian Starkey

Connectors shouldn't be registered until the rest of the whole device is set up, so that consistent state is presented to userspace.

As such, remove the calls to drm_connector_register() and drm_connector_unregister() from tda998x, as these are now handled by drm_dev_(un)register() itself.

To work with this change, the mali-dp and hdlcd bind and unbind sequences have to be reordered, to ensure that the componentised encoder/connector is bound before drm_dev_register() registers all connectors. Similarly, the device must be unregistered before the component is unbound.

Altogether, this allows other drivers using tda998x to be de-midlayered, and to have less racy initialisation of their components.

Splitting this commit into three (one per driver) isn't possible without intermediate breakage, so it is all squashed together here.

Suggested-by Russell King <[email protected]>

Signed-off-by Brian Starkey <[email protected]>

Reviewed-by Liviu Dudau <[email protected]>

drivers/gpu/drm/i2c/tda998x_drv.c +0 -1

index f4315bc..6e6fca2 100644
@@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = {
 
 static void tda998x_connector_destroy(struct drm_connector *connector)
 {
-	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 }
 

drivers/gpu/drm/i2c/tda998x_drv.c static int tda998x_bind(struct device *dev, struct device *master, void *data) · line 1447

Quoted 7 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -4

@@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		goto err_connector;
 
-	ret = drm_connector_register(&priv->connector);
-	if (ret)
-		goto err_sysfs;
-

Instead of smashing all these patches into one, what about checking here for midlayer driver set with:

	/* register here for drivers still using midlayer load/unload */
	if (dev->driver->load)
		drm_connector_register(connector),

Similar in other places. That way we wouldn't need to switch the world in one patch.

-Daniel

Quoted 24 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -4

 	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
 
 	return 0;
 
-err_sysfs:
-	drm_connector_cleanup(&priv->connector);
 err_connector:
 	drm_encoder_cleanup(&priv->encoder);
 err_encoder:

@@ -1463,7 +1456,6 @@ static void tda998x_unbind(struct device *dev, struct device *master,
 {
 	struct tda998x_priv *priv = dev_get_drvdata(dev);
 
-	drm_connector_unregister(&priv->connector);
 	drm_connector_cleanup(&priv->connector);
 	drm_encoder_cleanup(&priv->encoder);
 	tda998x_destroy(priv);
--

1.7.9.5

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 29 lines

Quoted 16 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -1

index f4315bc..6e6fca2 100644
@@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
 tda998x_connector_helper_funcs = {
 
 static void tda998x_connector_destroy(struct drm_connector *connector)
 {
-       drm_connector_unregister(connector);
        drm_connector_cleanup(connector);
 }

drivers/gpu/drm/i2c/tda998x_drv.c static int tda998x_bind(struct device *dev, · line 1448

Quoted 8 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -4

@@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
 struct device *master, void *data)
        if (ret)
                goto err_connector;
 
-       ret = drm_connector_register(&priv->connector);
-       if (ret)
-               goto err_sysfs;
-

Instead of smashing all these patches into one, what about checking here for midlayer driver set with:

Quoted 6 lines

        /* register here for drivers still using midlayer load/unload */
        if (dev->driver->load)
                drm_connector_register(connector),

Similar in other places. That way we wouldn't need to switch the world in one patch.

I don't think that helps. If we do that in isolation (first), then mali-dp and hdlcd won't get their connectors registered because their bind order is:

        drm_dev_register();
        component_bind_all();

If we change the mali-dp/hdlcd bind order first, then tda998x will explode on drm_connector_register() until it's patched to remove that.

As I mentioned in my mail to Russell, the only way I can see to avoid patching all three drivers in one go is:
1) Add (probably open-coded) drm_connector_register_all() to the end of bind in hdlcd and mali-dp
2) Patch tda998x to remove drm_connector_register()
3) Reorder hdlcd/mali-dp bind and remove the connector registration added in 1)

We can do that, but it's extra churn for the same result, and none of the 5 patches will really make sense in isolation anyway.

Quoted 55 lines · Brian Starkey

Quoted 29 lines

Quoted 16 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -1

index f4315bc..6e6fca2 100644
@@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
 tda998x_connector_helper_funcs = {
 
 static void tda998x_connector_destroy(struct drm_connector *connector)
 {
-       drm_connector_unregister(connector);
        drm_connector_cleanup(connector);
 }

drivers/gpu/drm/i2c/tda998x_drv.c static int tda998x_bind(struct device *dev, · line 1448

Quoted 8 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -4

@@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
 struct device *master, void *data)
        if (ret)
                goto err_connector;
 
-       ret = drm_connector_register(&priv->connector);
-       if (ret)
-               goto err_sysfs;
-

Instead of smashing all these patches into one, what about checking here for midlayer driver set with:

Quoted 6 lines

        /* register here for drivers still using midlayer load/unload */
        if (dev->driver->load)
                drm_connector_register(connector),

Similar in other places. That way we wouldn't need to switch the world in one patch.

I don't think that helps. If we do that in isolation (first), then mali-dp and hdlcd won't get their connectors registered because their bind order is:

        drm_dev_register();
        component_bind_all();

If we change the mali-dp/hdlcd bind order first, then tda998x will explode on drm_connector_register() until it's patched to remove that.

As I mentioned in my mail to Russell, the only way I can see to avoid patching all three drivers in one go is:
1) Add (probably open-coded) drm_connector_register_all() to the end of bind in hdlcd and mali-dp
2) Patch tda998x to remove drm_connector_register()
3) Reorder hdlcd/mali-dp bind and remove the connector registration added in 1)

Quoted 2 lines

We can do that, but it's extra churn for the same result, and none of the 5 patches will really make sense in isolation anyway.

I thought there's also armada to take care of, which this patch would break? Maybe even another driver, so the hack would still be useful for those other drivers.

And it would have been useful if malidp/hdlcd wouldn't have started out with the wrong init ordering ;-)

-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Hi Daniel,

Quoted 56 lines · Daniel Vetter

Quoted 55 lines · Brian Starkey

Quoted 29 lines

Quoted 16 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -1

index f4315bc..6e6fca2 100644
@@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
 tda998x_connector_helper_funcs = {
 
 static void tda998x_connector_destroy(struct drm_connector *connector)
 {
-       drm_connector_unregister(connector);
        drm_connector_cleanup(connector);
 }

drivers/gpu/drm/i2c/tda998x_drv.c static int tda998x_bind(struct device *dev, · line 1448

Quoted 8 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -4

@@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
 struct device *master, void *data)
        if (ret)
                goto err_connector;
 
-       ret = drm_connector_register(&priv->connector);
-       if (ret)
-               goto err_sysfs;
-

Instead of smashing all these patches into one, what about checking here for midlayer driver set with:

Quoted 6 lines

        /* register here for drivers still using midlayer load/unload */
        if (dev->driver->load)
                drm_connector_register(connector),

Similar in other places. That way we wouldn't need to switch the world in one patch.

I don't think that helps. If we do that in isolation (first), then mali-dp and hdlcd won't get their connectors registered because their bind order is:

        drm_dev_register();
        component_bind_all();

If we change the mali-dp/hdlcd bind order first, then tda998x will explode on drm_connector_register() until it's patched to remove that.

As I mentioned in my mail to Russell, the only way I can see to avoid patching all three drivers in one go is:
1) Add (probably open-coded) drm_connector_register_all() to the end of bind in hdlcd and mali-dp
2) Patch tda998x to remove drm_connector_register()
3) Reorder hdlcd/mali-dp bind and remove the connector registration added in 1)

Quoted 6 lines

Quoted 2 lines

We can do that, but it's extra churn for the same result, and none of the 5 patches will really make sense in isolation anyway.

I thought there's also armada to take care of, which this patch would break? Maybe even another driver, so the hack would still be useful for those other drivers.

OK it seems like this situation has got very confused. In short, this patch does not break armada. Russell previously tested the tda998x patch against armada and reported no issues.
Drivers with a ->load() callback don't need to register the connector anymore, because drm_dev_register() does it for them.

As far as I know, this patch touching these three drivers is sufficient to allow all current users of tda998x to continue using it, whilst also allowing armada and tilcdc to be de-midlayered.

On Tue, Oct 25, 2016 at 10:52:33AM +0100, Brian Starkey wrote:

Ah, makes sense. Should I apply this to drm-misc so it's in a shared tree?

Quoted 8 lines

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.

-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 72 lines · Brian Starkey

Hi Daniel,

Quoted 56 lines · Daniel Vetter

Quoted 55 lines · Brian Starkey

Quoted 29 lines

Quoted 16 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -1

index f4315bc..6e6fca2 100644
@@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
 tda998x_connector_helper_funcs = {
 
 static void tda998x_connector_destroy(struct drm_connector *connector)
 {
-       drm_connector_unregister(connector);
        drm_connector_cleanup(connector);
 }

drivers/gpu/drm/i2c/tda998x_drv.c static int tda998x_bind(struct device *dev, · line 1448

Quoted 8 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -4

@@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
 struct device *master, void *data)
        if (ret)
                goto err_connector;
 
-       ret = drm_connector_register(&priv->connector);
-       if (ret)
-               goto err_sysfs;
-

Instead of smashing all these patches into one, what about checking here for midlayer driver set with:

Quoted 6 lines

        /* register here for drivers still using midlayer load/unload */
        if (dev->driver->load)
                drm_connector_register(connector),

Similar in other places. That way we wouldn't need to switch the world in one patch.

I don't think that helps. If we do that in isolation (first), then mali-dp and hdlcd won't get their connectors registered because their bind order is:

        drm_dev_register();
        component_bind_all();

If we change the mali-dp/hdlcd bind order first, then tda998x will explode on drm_connector_register() until it's patched to remove that.

As I mentioned in my mail to Russell, the only way I can see to avoid patching all three drivers in one go is:
1) Add (probably open-coded) drm_connector_register_all() to the end of bind in hdlcd and mali-dp
2) Patch tda998x to remove drm_connector_register()
3) Reorder hdlcd/mali-dp bind and remove the connector registration added in 1)

Quoted 6 lines

Quoted 2 lines

We can do that, but it's extra churn for the same result, and none of the 5 patches will really make sense in isolation anyway.

I thought there's also armada to take care of, which this patch would break? Maybe even another driver, so the hack would still be useful for those other drivers.

OK it seems like this situation has got very confused. In short, this patch does not break armada. Russell previously tested the tda998x patch against armada and reported no issues.
Drivers with a ->load() callback don't need to register the connector anymore, because drm_dev_register() does it for them.

Quoted 3 lines

As far as I know, this patch touching these three drivers is sufficient to allow all current users of tda998x to continue using it, whilst also allowing armada and tilcdc to be de-midlayered.

Ah, makes sense. Should I apply this to drm-misc so it's in a shared tree?

I need the patch against v4.8. I'm happy to pick it up and add it to my drm-tda998x-devel branch, which you can then merge into drm-misc if you wish.

Quoted 55 lines · Brian Starkey

Quoted 29 lines

Quoted 16 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -1

index f4315bc..6e6fca2 100644
@@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
 tda998x_connector_helper_funcs = {
 
 static void tda998x_connector_destroy(struct drm_connector *connector)
 {
-       drm_connector_unregister(connector);
        drm_connector_cleanup(connector);
 }

drivers/gpu/drm/i2c/tda998x_drv.c static int tda998x_bind(struct device *dev, · line 1448

Quoted 8 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -4

@@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
 struct device *master, void *data)
        if (ret)
                goto err_connector;
 
-       ret = drm_connector_register(&priv->connector);
-       if (ret)
-               goto err_sysfs;
-

Instead of smashing all these patches into one, what about checking here for midlayer driver set with:

Quoted 6 lines

        /* register here for drivers still using midlayer load/unload */
        if (dev->driver->load)
                drm_connector_register(connector),

Similar in other places. That way we wouldn't need to switch the world in one patch.

I don't think that helps. If we do that in isolation (first), then mali-dp and hdlcd won't get their connectors registered because their bind order is:

        drm_dev_register();
        component_bind_all();

If we change the mali-dp/hdlcd bind order first, then tda998x will explode on drm_connector_register() until it's patched to remove that.

As I mentioned in my mail to Russell, the only way I can see to avoid patching all three drivers in one go is:
1) Add (probably open-coded) drm_connector_register_all() to the end of bind in hdlcd and mali-dp
2) Patch tda998x to remove drm_connector_register()
3) Reorder hdlcd/mali-dp bind and remove the connector registration added in 1)

Quoted 2 lines

We can do that, but it's extra churn for the same result, and none of the 5 patches will really make sense in isolation anyway.

I thought there's also armada to take care of, which this patch would break?

Maybe even another driver, so the hack would still be useful for those other drivers. And it would have been useful if malidp/hdlcd wouldn't have started out with the wrong init ordering ;-)

Quoted 56 lines · Daniel Vetter

Quoted 55 lines · Brian Starkey

Quoted 29 lines

Quoted 16 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -1

index f4315bc..6e6fca2 100644
@@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
 tda998x_connector_helper_funcs = {
 
 static void tda998x_connector_destroy(struct drm_connector *connector)
 {
-       drm_connector_unregister(connector);
        drm_connector_cleanup(connector);
 }

drivers/gpu/drm/i2c/tda998x_drv.c static int tda998x_bind(struct device *dev, · line 1448

Quoted 8 lines

drivers/gpu/drm/i2c/tda998x_drv.c +0 -4

@@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
 struct device *master, void *data)
        if (ret)
                goto err_connector;
 
-       ret = drm_connector_register(&priv->connector);
-       if (ret)
-               goto err_sysfs;
-

Instead of smashing all these patches into one, what about checking here for midlayer driver set with:

Quoted 6 lines

        /* register here for drivers still using midlayer load/unload */
        if (dev->driver->load)
                drm_connector_register(connector),

Similar in other places. That way we wouldn't need to switch the world in one patch.

I don't think that helps. If we do that in isolation (first), then mali-dp and hdlcd won't get their connectors registered because their bind order is:

        drm_dev_register();
        component_bind_all();

If we change the mali-dp/hdlcd bind order first, then tda998x will explode on drm_connector_register() until it's patched to remove that.

As I mentioned in my mail to Russell, the only way I can see to avoid patching all three drivers in one go is:
1) Add (probably open-coded) drm_connector_register_all() to the end of bind in hdlcd and mali-dp
2) Patch tda998x to remove drm_connector_register()
3) Reorder hdlcd/mali-dp bind and remove the connector registration added in 1)

Quoted 5 lines

Quoted 2 lines

We can do that, but it's extra churn for the same result, and none of the 5 patches will really make sense in isolation anyway.

I thought there's also armada to take care of, which this patch would break?

NO NO NO NO NO. I've said this several times. Let's try it again, and see if it sticks.

Because Armada has not been converted from a mid-layered driver, it is _IMMUNE_ from any patch removing the drm_connector_register() call in TDA998x. It does _NOT_ break in any way.

Only those drivers which are de-mid-layered, and worked around the drm_connector_register() call inside TDA998x (eg, mali) break, because of the order in which they are _forced_ to call stuff.

In a de-mid-layered driver, with the drm_connector_register() call in place in TDA998x, drm_dev_register() _MUST_ be called prior to component_bind_all(), otherwise you get a WARN_ON() dump from the kobject code. With the drm_connector_register() call removed, drm_dev_register() _MUST_ be called after component_bind_all() so that the connector is registered.

It's the de-mid-layered drivers which are the problem here, not the mid-layered ones like Armada.

Quoted 3 lines

Maybe even another driver, so the hack would still be useful for those other drivers. And it would have been useful if malidp/hdlcd wouldn't have started out with the wrong init ordering ;-)

It's forced into the "wrong init ordering" due to the kobject WARN_ON.

Hm, I entirely missed that part of the troubles. Anyway, if you all agree on a patch I certainly won't block it, feel free to merge through suitable trees (or I can smash it into drm-misc if that's wanted).

Quoted 3 lines · Daniel Vetter

Hm, I entirely missed that part of the troubles. Anyway, if you all agree on a patch I certainly won't block it, feel free to merge through suitable trees (or I can smash it into drm-misc if that's wanted).

I think those who are interested in seeing the drm_connector_register() call disappear from tda998x only care about that happening, but not how it happens.

We have agreement between myself, Brian and Liviu on this approach, and I think everyone else is waiting for me to push out the commit so it can be used as the basis for their work. I think everyone else is waiting for me to push something out which gets us past this log-jam.

I don't understand the connectivity between drm-misc and David's drm tree - so I'm going to let you make the decision on whether to merge this into drm-misc. I normally send my pull requests for Armada and TDA998x changes to David, which means when I send my other TDA998x changes, the mali/tda998x commit will be included in that pull request too. So I'm wondering whether it would make more sense for me to send it to David instead, or whether I need to send my other changes through drm-misc instead. I find the whole drm vs drm-misc thing rather confusing.

I think we should get this accepted into drm trees before anyone bases their work on this commit (which is why I've been holding off during the last week, waiting for DRM folk to get back from Santa Fe and readjust to the higher atmospheric pressure!)

Anyway, here is my pull request for the mali/hdlcd/tda998x commit which I'd normally send to David - I don't mind which tree it goes into as long as things work out nicely.

8<===

David,

Please incorporate the latest TDA998x I2C driver (drm-tda998x-mali branch), which can be found at:

git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-mali

with SHA1 90731c24d2db7ec04df43ddbcee9605183d05187.

This change removes the call to drm_connector_register() which has been blocking the proper de-midlayer conversion of other DRM drivers. Unfortunately, hdlcd and mali have intimate dependencies on this change, which is why these drivers need to be fixed up in the same commit - they can't be separate commits without these drivers breaking. All other DRM drivers which make use of tda998x (to my knowledge - armada, tilcdc) cope with this change.

This will update the following files:

 drivers/gpu/drm/arm/hdlcd_drv.c   | 19 +++++++++++--------
 drivers/gpu/drm/arm/malidp_drv.c  | 18 +++++++++++-------
 drivers/gpu/drm/i2c/tda998x_drv.c |  8 --------
 3 files changed, 22 insertions(+), 23 deletions(-)

through these changes:

Brian Starkey (1):
drm/i2c: tda998x: mali-dp: hdlcd: refactor connector registration

Many thanks.

Signature · 3 lines

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.

Quoted 3 lines · Daniel Vetter

Hm, I entirely missed that part of the troubles. Anyway, if you all agree on a patch I certainly won't block it, feel free to merge through suitable trees (or I can smash it into drm-misc if that's wanted).

I think those who are interested in seeing the drm_connector_register() call disappear from tda998x only care about that happening, but not how it happens.

We have agreement between myself, Brian and Liviu on this approach, and I think everyone else is waiting for me to push out the commit so it can be used as the basis for their work. I think everyone else is waiting for me to push something out which gets us past this log-jam.

I don't understand the connectivity between drm-misc and David's drm tree - so I'm going to let you make the decision on whether to merge this into drm-misc. I normally send my pull requests for Armada and TDA998x changes to David, which means when I send my other TDA998x changes, the mali/tda998x commit will be included in that pull request too. So I'm wondering whether it would make more sense for me to send it to David instead, or whether I need to send my other changes through drm-misc instead. I find the whole drm vs drm-misc thing rather confusing.

I think we should get this accepted into drm trees before anyone bases their work on this commit (which is why I've been holding off during the last week, waiting for DRM folk to get back from Santa Fe and readjust to the higher atmospheric pressure!)

Anyway, here is my pull request for the mali/hdlcd/tda998x commit which I'd normally send to David - I don't mind which tree it goes into as long as things work out nicely.

8<===

David,

Please incorporate the latest TDA998x I2C driver (drm-tda998x-mali branch), which can be found at:

git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-mali

with SHA1 90731c24d2db7ec04df43ddbcee9605183d05187.

This change removes the call to drm_connector_register() which has been blocking the proper de-midlayer conversion of other DRM drivers. Unfortunately, hdlcd and mali have intimate dependencies on this change, which is why these drivers need to be fixed up in the same commit - they can't be separate commits without these drivers breaking. All other DRM drivers which make use of tda998x (to my knowledge - armada, tilcdc) cope with this change.

This will update the following files:

 drivers/gpu/drm/arm/hdlcd_drv.c   | 19 +++++++++++--------
 drivers/gpu/drm/arm/malidp_drv.c  | 18 +++++++++++-------
 drivers/gpu/drm/i2c/tda998x_drv.c |  8 --------
 3 files changed, 22 insertions(+), 23 deletions(-)

through these changes:

Brian Starkey (1):
drm/i2c: tda998x: mali-dp: hdlcd: refactor connector registration

Many thanks.

Signature · 3 lines

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.

I guess Dave must have missed this as I can't see it in drm-next, so I'm resending the pull request.

Quoted 70 lines · Russell King - ARM Linux

Quoted 3 lines · Daniel Vetter

Hm, I entirely missed that part of the troubles. Anyway, if you all agree on a patch I certainly won't block it, feel free to merge through suitable trees (or I can smash it into drm-misc if that's wanted).

I think those who are interested in seeing the drm_connector_register() call disappear from tda998x only care about that happening, but not how it happens.

We have agreement between myself, Brian and Liviu on this approach, and I think everyone else is waiting for me to push out the commit so it can be used as the basis for their work. I think everyone else is waiting for me to push something out which gets us past this log-jam.

I don't understand the connectivity between drm-misc and David's drm tree - so I'm going to let you make the decision on whether to merge this into drm-misc. I normally send my pull requests for Armada and TDA998x changes to David, which means when I send my other TDA998x changes, the mali/tda998x commit will be included in that pull request too. So I'm wondering whether it would make more sense for me to send it to David instead, or whether I need to send my other changes through drm-misc instead. I find the whole drm vs drm-misc thing rather confusing.

I think we should get this accepted into drm trees before anyone bases their work on this commit (which is why I've been holding off during the last week, waiting for DRM folk to get back from Santa Fe and readjust to the higher atmospheric pressure!)

Anyway, here is my pull request for the mali/hdlcd/tda998x commit which I'd normally send to David - I don't mind which tree it goes into as long as things work out nicely.

8<===

David,

Please incorporate the latest TDA998x I2C driver (drm-tda998x-mali branch), which can be found at:

git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-mali

with SHA1 90731c24d2db7ec04df43ddbcee9605183d05187.

This change removes the call to drm_connector_register() which has been blocking the proper de-midlayer conversion of other DRM drivers. Unfortunately, hdlcd and mali have intimate dependencies on this change, which is why these drivers need to be fixed up in the same commit - they can't be separate commits without these drivers breaking. All other DRM drivers which make use of tda998x (to my knowledge - armada, tilcdc) cope with this change.

This will update the following files:

 drivers/gpu/drm/arm/hdlcd_drv.c   | 19 +++++++++++--------
 drivers/gpu/drm/arm/malidp_drv.c  | 18 +++++++++++-------
 drivers/gpu/drm/i2c/tda998x_drv.c |  8 --------
 3 files changed, 22 insertions(+), 23 deletions(-)

through these changes:

Brian Starkey (1):
drm/i2c: tda998x: mali-dp: hdlcd: refactor connector registration

Many thanks.

Signature · 3 lines

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.

Signature · 3 lines

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.

Stop using struct drm_driver load() callback. The load() callback should not be used anymore. Instead the drm_device is allocated with drm_dev_alloc() and registered with drm_dev_register() only after the driver is completely initialized.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 103 ++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 45 deletions(-)

drivers/gpu/drm/tilcdc/tilcdc_drv.c +56 -41

index 231f2c7..2dca822 100644
@@ -230,25 +230,31 @@ static int tilcdc_unload(struct drm_device *dev)
 	return 0;
 }
 
-static int tilcdc_load(struct drm_device *dev, unsigned long flags)
+static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 {
-	struct platform_device *pdev = dev->platformdev;
-	struct device_node *node = pdev->dev.of_node;
+	struct drm_device *ddev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct device_node *node = dev->of_node;
 	struct tilcdc_drm_private *priv;
 	struct resource *res;
 	u32 bpp = 0;
 	int ret;
 
-	priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
-		dev_err(dev->dev, "failed to allocate private data\n");
+		dev_err(dev, "failed to allocate private data\n");
 		return -ENOMEM;
 	}
 
-	dev->dev_private = priv;
+	ddev = drm_dev_alloc(ddrv, dev);
+	if (IS_ERR(ddev))
+		return PTR_ERR(ddev);
+
+	ddev->platformdev = pdev;
+	ddev->dev_private = priv;
 
 	priv->is_componentized =
-		tilcdc_get_external_components(dev->dev, NULL) > 0;
+		tilcdc_get_external_components(dev, NULL) > 0;
 
 	priv->wq = alloc_ordered_workqueue("tilcdc", 0);
 	if (!priv->wq) {

@@ -258,21 +264,21 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
-		dev_err(dev->dev, "failed to get memory resource\n");
+		dev_err(dev, "failed to get memory resource\n");
 		ret = -EINVAL;
 		goto fail_free_wq;
 	}
 
 	priv->mmio = ioremap_nocache(res->start, resource_size(res));
 	if (!priv->mmio) {
-		dev_err(dev->dev, "failed to ioremap\n");
+		dev_err(dev, "failed to ioremap\n");
 		ret = -ENOMEM;
 		goto fail_free_wq;
 	}
 
-	priv->clk = clk_get(dev->dev, "fck");
+	priv->clk = clk_get(dev, "fck");
 	if (IS_ERR(priv->clk)) {
-		dev_err(dev->dev, "failed to get functional clock\n");
+		dev_err(dev, "failed to get functional clock\n");
 		ret = -ENODEV;
 		goto fail_iounmap;
 	}

@@ -282,7 +288,7 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 	ret = cpufreq_register_notifier(&priv->freq_transition,
 			CPUFREQ_TRANSITION_NOTIFIER);
 	if (ret) {
-		dev_err(dev->dev, "failed to register cpufreq notifier\n");
+		dev_err(dev, "failed to register cpufreq notifier\n");
 		goto fail_put_clk;
 	}
 #endif

@@ -303,11 +309,11 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 
 	DBG("Maximum Pixel Clock Value %dKHz", priv->max_pixelclock);
 
-	pm_runtime_enable(dev->dev);
+	pm_runtime_enable(dev);
 
 	/* Determine LCD IP Version */
-	pm_runtime_get_sync(dev->dev);
-	switch (tilcdc_read(dev, LCDC_PID_REG)) {
+	pm_runtime_get_sync(dev);
+	switch (tilcdc_read(ddev, LCDC_PID_REG)) {
 	case 0x4c100102:
 		priv->rev = 1;
 		break;

@@ -316,14 +322,14 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 		priv->rev = 2;
 		break;
 	default:
-		dev_warn(dev->dev, "Unknown PID Reg value 0x%08x, "
-				"defaulting to LCD revision 1\n",
-				tilcdc_read(dev, LCDC_PID_REG));
+		dev_warn(dev, "Unknown PID Reg value 0x%08x, "
+			"defaulting to LCD revision 1\n",
+			tilcdc_read(ddev, LCDC_PID_REG));
 		priv->rev = 1;
 		break;
 	}
 
-	pm_runtime_put_sync(dev->dev);
+	pm_runtime_put_sync(dev);
 
 	if (priv->rev == 1) {
 		DBG("Revision 1 LCDC supports only RGB565 format");

@@ -356,74 +362,82 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 		}
 	}
 
-	ret = modeset_init(dev);
+	ret = modeset_init(ddev);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to initialize mode setting\n");
+		dev_err(dev, "failed to initialize mode setting\n");
 		goto fail_cpufreq_unregister;
 	}
 
-	platform_set_drvdata(pdev, dev);
+	platform_set_drvdata(pdev, ddev);
 
 	if (priv->is_componentized) {
-		ret = component_bind_all(dev->dev, dev);
+		ret = component_bind_all(dev, ddev);
 		if (ret < 0)
 			goto fail_mode_config_cleanup;
 
-		ret = tilcdc_add_external_encoders(dev);
+		ret = tilcdc_add_external_encoders(ddev);
 		if (ret < 0)
 			goto fail_component_cleanup;
 	}
 
 	if ((priv->num_encoders == 0) || (priv->num_connectors == 0)) {
-		dev_err(dev->dev, "no encoders/connectors found\n");
+		dev_err(dev, "no encoders/connectors found\n");
 		ret = -ENXIO;
 		goto fail_external_cleanup;
 	}
 
-	ret = drm_vblank_init(dev, 1);
+	ret = drm_vblank_init(ddev, 1);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to initialize vblank\n");
+		dev_err(dev, "failed to initialize vblank\n");
 		goto fail_external_cleanup;
 	}
 
-	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
+	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to install IRQ handler\n");
+		dev_err(dev, "failed to install IRQ handler\n");
 		goto fail_vblank_cleanup;
 	}
 
-	drm_mode_config_reset(dev);
+	drm_mode_config_reset(ddev);
 
-	priv->fbdev = drm_fbdev_cma_init(dev, bpp,
-			dev->mode_config.num_crtc,
-			dev->mode_config.num_connector);
+	priv->fbdev = drm_fbdev_cma_init(ddev, bpp,
+			ddev->mode_config.num_crtc,
+			ddev->mode_config.num_connector);
 	if (IS_ERR(priv->fbdev)) {
 		ret = PTR_ERR(priv->fbdev);
 		goto fail_irq_uninstall;
 	}
 
-	drm_kms_helper_poll_init(dev);
+	drm_kms_helper_poll_init(ddev);
+
+	ret = drm_dev_register(ddev, 0);
+	if (ret)
+		goto fail_platform_init;
 
 	return 0;
 
+fail_platform_init:
+	drm_kms_helper_poll_fini(ddev);
+	drm_fbdev_cma_fini(priv->fbdev);
+
 fail_irq_uninstall:
-	drm_irq_uninstall(dev);
+	drm_irq_uninstall(ddev);
 
 fail_vblank_cleanup:
-	drm_vblank_cleanup(dev);
+	drm_vblank_cleanup(ddev);
 
 fail_component_cleanup:
 	if (priv->is_componentized)
-		component_unbind_all(dev->dev, dev);
+		component_unbind_all(dev, dev);
 
 fail_mode_config_cleanup:
-	drm_mode_config_cleanup(dev);
+	drm_mode_config_cleanup(ddev);
 
 fail_external_cleanup:
-	tilcdc_remove_external_encoders(dev);
+	tilcdc_remove_external_encoders(ddev);
 
 fail_cpufreq_unregister:
-	pm_runtime_disable(dev->dev);
+	pm_runtime_disable(dev);
 #ifdef CONFIG_CPU_FREQ
 	cpufreq_unregister_notifier(&priv->freq_transition,
 			CPUFREQ_TRANSITION_NOTIFIER);

@@ -440,7 +454,8 @@ fail_free_wq:
 	destroy_workqueue(priv->wq);
 
 fail_unset_priv:
-	dev->dev_private = NULL;
+	ddev->dev_private = NULL;
+	drm_dev_unref(ddev);
 
 	return ret;
 }

drivers/gpu/drm/tilcdc/tilcdc_drv.c +2 -3

 	.lastclose          = tilcdc_lastclose,
 	.irq_handler        = tilcdc_irq,

@@ -662,10 +676,9 @@ static const struct dev_pm_ops tilcdc_pm_ops = {
 /*
  * Platform driver:
  */
-
 static int tilcdc_bind(struct device *dev)
 {
-	return drm_platform_init(&tilcdc_driver, to_platform_device(dev));
+	return tilcdc_init(&tilcdc_driver, dev);
 }
 
 static void tilcdc_unbind(struct device *dev)

@@ -699,7 +712,7 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 	else if (ret == 0)
-		return drm_platform_init(&tilcdc_driver, pdev);
+		return tilcdc_init(&tilcdc_driver, &pdev->dev);
 	else
 		return component_master_add_with_match(&pdev->dev,
 						       &tilcdc_comp_ops,

Signature · 1 line

1.9.1

Use unload to handle initialization failures instead of complex goto label mess. To do this the initialization sequence needed slight reordering and some unload functions needed to become conditional.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 91 +++++++++++-----------------------
 1 file changed, 28 insertions(+), 63 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 2dca822..8820133 100644

drivers/gpu/drm/tilcdc/tilcdc_drv.c +12 -9

@@ -160,8 +160,6 @@ static int modeset_init(struct drm_device *dev)
 	struct tilcdc_drm_private *priv = dev->dev_private;
 	struct tilcdc_module *mod;
 
-	drm_mode_config_init(dev);
-
 	priv->crtc = tilcdc_crtc_create(dev);
 
 	list_for_each_entry(mod, &module_list, list) {

@@ -200,18 +198,19 @@ static int tilcdc_unload(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
-	tilcdc_remove_external_encoders(dev);
+	if (priv->fbdev)
+		drm_fbdev_cma_fini(priv->fbdev);
 
-	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
 	drm_mode_config_cleanup(dev);
 	drm_vblank_cleanup(dev);
-
 	drm_irq_uninstall(dev);
+	tilcdc_remove_external_encoders(dev);
 
 #ifdef CONFIG_CPU_FREQ
-	cpufreq_unregister_notifier(&priv->freq_transition,
-			CPUFREQ_TRANSITION_NOTIFIER);
+	if (priv->freq_transition.notifier_call)
+		cpufreq_unregister_notifier(&priv->freq_transition,
+					    CPUFREQ_TRANSITION_NOTIFIER);
 #endif
 
 	if (priv->clk)

@@ -220,8 +219,10 @@ static int tilcdc_unload(struct drm_device *dev)
 	if (priv->mmio)
 		iounmap(priv->mmio);
 
-	flush_workqueue(priv->wq);
-	destroy_workqueue(priv->wq);
+	if (priv->wq) {
+		flush_workqueue(priv->wq);
+		destroy_workqueue(priv->wq);
+	}
 
 	dev->dev_private = NULL;
 

@@ -252,6 +253,8 @@ static int tilcdc_init(struct drm_driver *ddrv, struct
 device *dev)
 
 	ddev->platformdev = pdev;
 	ddev->dev_private = priv;
+	platform_set_drvdata(pdev, ddev);
+	drm_mode_config_init(ddev);
 
 	priv->is_componentized =

drivers/gpu/drm/tilcdc/tilcdc_drv.c +16 -54

 		tilcdc_get_external_components(dev, NULL) > 0;

@@ -259,28 +262,28 @@ static int tilcdc_init(struct drm_driver *ddrv, struct
 device *dev) priv->wq = alloc_ordered_workqueue("tilcdc", 0);
 	if (!priv->wq) {
 		ret = -ENOMEM;
-		goto fail_unset_priv;
+		goto init_failed;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "failed to get memory resource\n");
 		ret = -EINVAL;
-		goto fail_free_wq;
+		goto init_failed;
 	}
 
 	priv->mmio = ioremap_nocache(res->start, resource_size(res));
 	if (!priv->mmio) {
 		dev_err(dev, "failed to ioremap\n");
 		ret = -ENOMEM;
-		goto fail_free_wq;
+		goto init_failed;
 	}
 
 	priv->clk = clk_get(dev, "fck");
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "failed to get functional clock\n");
 		ret = -ENODEV;
-		goto fail_iounmap;
+		goto init_failed;
 	}
 
 #ifdef CONFIG_CPU_FREQ

@@ -289,7 +292,8 @@ static int tilcdc_init(struct drm_driver *ddrv, struct
 device *dev) CPUFREQ_TRANSITION_NOTIFIER);
 	if (ret) {
 		dev_err(dev, "failed to register cpufreq notifier\n");
-		goto fail_put_clk;
+		priv->freq_transition.notifier_call = NULL;
+		goto init_failed;
 	}
 #endif
 

@@ -365,37 +369,35 @@ static int tilcdc_init(struct drm_driver *ddrv, struct
 device *dev) ret = modeset_init(ddev);
 	if (ret < 0) {
 		dev_err(dev, "failed to initialize mode setting\n");
-		goto fail_cpufreq_unregister;
+		goto init_failed;
 	}
 
-	platform_set_drvdata(pdev, ddev);
-
 	if (priv->is_componentized) {
 		ret = component_bind_all(dev, ddev);
 		if (ret < 0)
-			goto fail_mode_config_cleanup;
+			goto init_failed;
 
 		ret = tilcdc_add_external_encoders(ddev);
 		if (ret < 0)
-			goto fail_component_cleanup;
+			goto init_failed;
 	}
 
 	if ((priv->num_encoders == 0) || (priv->num_connectors == 0)) {
 		dev_err(dev, "no encoders/connectors found\n");
 		ret = -ENXIO;
-		goto fail_external_cleanup;
+		goto init_failed;
 	}
 
 	ret = drm_vblank_init(ddev, 1);
 	if (ret < 0) {
 		dev_err(dev, "failed to initialize vblank\n");
-		goto fail_external_cleanup;
+		goto init_failed;
 	}
 
 	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
 	if (ret < 0) {
 		dev_err(dev, "failed to install IRQ handler\n");
-		goto fail_vblank_cleanup;
+		goto init_failed;
 	}
 
 	drm_mode_config_reset(ddev);

@@ -405,56 +407,19 @@ static int tilcdc_init(struct drm_driver *ddrv, struct
 device *dev) ddev->mode_config.num_connector);
 	if (IS_ERR(priv->fbdev)) {
 		ret = PTR_ERR(priv->fbdev);
-		goto fail_irq_uninstall;
+		goto init_failed;
 	}
 
 	drm_kms_helper_poll_init(ddev);
 
 	ret = drm_dev_register(ddev, 0);
 	if (ret)
-		goto fail_platform_init;
+		goto init_failed;
 
 	return 0;
 
-fail_platform_init:
-	drm_kms_helper_poll_fini(ddev);
-	drm_fbdev_cma_fini(priv->fbdev);
-
-fail_irq_uninstall:
-	drm_irq_uninstall(ddev);
-
-fail_vblank_cleanup:
-	drm_vblank_cleanup(ddev);
-
-fail_component_cleanup:
-	if (priv->is_componentized)
-		component_unbind_all(dev, dev);
-
-fail_mode_config_cleanup:
-	drm_mode_config_cleanup(ddev);
-
-fail_external_cleanup:
-	tilcdc_remove_external_encoders(ddev);
-
-fail_cpufreq_unregister:
-	pm_runtime_disable(dev);
-#ifdef CONFIG_CPU_FREQ
-	cpufreq_unregister_notifier(&priv->freq_transition,
-			CPUFREQ_TRANSITION_NOTIFIER);
-
-fail_put_clk:
-#endif
-	clk_put(priv->clk);
-
-fail_iounmap:
-	iounmap(priv->mmio);
-
-fail_free_wq:
-	flush_workqueue(priv->wq);
-	destroy_workqueue(priv->wq);
-
-fail_unset_priv:
-	ddev->dev_private = NULL;
+init_failed:
+	tilcdc_unload(ddev);
 	drm_dev_unref(ddev);
 
 	return ret;