[PATCH 0/4] drm/tilcdc: Cleanup tilcdc (&tda998x) init sequence

[PATCH 0/4] drm/tilcdc: Cleanup tilcdc (&tda998x) init sequence

[Jyri Sarha]

A patch[1] that crashed all drm drivers using load() hook was briefly
part of linux-next, so became awave that tilcdc should stop using
that too.

The two first patches should be merged before the third can be
merged. So the merge order should be agreed with tda998x before going
forward.

The last patch is just a cleanup, but depends on earlier ones.

Best regards,
Jyri

[1] https://patchwork.kernel.org/patch/9322771/

Jyri Sarha (4):
  drm/tilcdc: Remove obsolete drm_connector_register() calls
  drm/i2c: tda998x: Remove obsolete drm_connector_register() call
  drm/tilcdc: Stop using struct drm_driver load() callback
  drm/tilcdc: Use unload to handle initialization failures

 drivers/gpu/drm/i2c/tda998x_drv.c      |   6 --
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 168 ++++++++++++++-------------------
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  |   2 -
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c |   2 -
 4 files changed, 73 insertions(+), 105 deletions(-)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/tilcdc: Remove obsolete drm_connector_register() calls

[Jyri Sarha]

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 <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 2 --
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index 2134bb20..ad7a0e8 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -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:
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index c32c1ef..eea82b4 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -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:
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/tilcdc: Remove obsolete drm_connector_register() calls

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Wednesday 19 Oct 2016 00:33:53 Jyri Sarha wrote:
> 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 <jsarha@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 2 --
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> b/drivers/gpu/drm/tilcdc/tilcdc_panel.c index 2134bb20..ad7a0e8 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
> @@ -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:
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c index c32c1ef..eea82b4 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> @@ -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:

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jyri Sarha]

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 <jsarha@ti.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9798d40..265f854 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1656,16 +1656,10 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		goto err_connector;
 
-	ret = drm_connector_register(&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:
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote:
> Remove obsolete drm_connector_register() call from tda998x_bind(). All
> connectors are registered when drm_dev_register() is called by the
> master drm_device driver.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c index 9798d40..265f854 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1656,16 +1656,10 @@ static int tda998x_bind(struct device *dev, struct
> device *master, void *data) if (ret)
>  		goto err_connector;
> 
> -	ret = drm_connector_register(&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:

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Wed, Oct 19, 2016 at 10:54:06AM +0300, Laurent Pinchart wrote:
> Hi Jyri,
> 
> Thank you for the patch.
> 
> On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote:
> > Remove obsolete drm_connector_register() call from tda998x_bind(). All
> > connectors are registered when drm_dev_register() is called by the
> > master drm_device driver.
> > 
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> By the way, any chance you would plan porting the driver to drm_bridge ? :-)

What's the point?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Russell,

On Wednesday 19 Oct 2016 09:16:30 Russell King - ARM Linux wrote:
> On Wed, Oct 19, 2016 at 10:54:06AM +0300, Laurent Pinchart wrote:
> > On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote:
> >> Remove obsolete drm_connector_register() call from tda998x_bind(). All
> >> connectors are registered when drm_dev_register() is called by the
> >> master drm_device driver.
> >> 
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > By the way, any chance you would plan porting the driver to drm_bridge ?
> > :-)
>
> What's the point?

Avoiding code duplication. We currently have three APIs to handle external 
encoders (drm encoder slave, drm bridge and the component-based method used by 
tda998x only), which requires display drivers that want to support multiple 
external encoders to use up to three APIs.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Wed, Oct 19, 2016 at 11:52:15AM +0300, Laurent Pinchart wrote:
> Hi Russell,
> 
> On Wednesday 19 Oct 2016 09:16:30 Russell King - ARM Linux wrote:
> > On Wed, Oct 19, 2016 at 10:54:06AM +0300, Laurent Pinchart wrote:
> > > On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote:
> > >> Remove obsolete drm_connector_register() call from tda998x_bind(). All
> > >> connectors are registered when drm_dev_register() is called by the
> > >> master drm_device driver.
> > >> 
> > >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > > 
> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > By the way, any chance you would plan porting the driver to drm_bridge ?
> > > :-)
> >
> > What's the point?
> 
> Avoiding code duplication. We currently have three APIs to handle external 
> encoders (drm encoder slave, drm bridge and the component-based method used by 
> tda998x only), which requires display drivers that want to support multiple 
> external encoders to use up to three APIs.

tda998x doesn't use the encoder slave anymore.  You somehow list component-
based as a third alternative - it isn't, that's the native DRM non-bridge
method.

In any case, I don't agree with converting it to a DRM bridge - that will
mean that we have to split the driver into two pieces, the bridge part
handling the mode set specifics, and a connector/encoder part which
handles the detection/edid stuff.

We might as well keep the whole thing as the classical connector/encoder
rather than introducing this additional layer of complexity.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Russell,

On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
> On Wed, Oct 19, 2016 at 11:52:15AM +0300, Laurent Pinchart wrote:
> > On Wednesday 19 Oct 2016 09:16:30 Russell King - ARM Linux wrote:
> >> On Wed, Oct 19, 2016 at 10:54:06AM +0300, Laurent Pinchart wrote:
> >>> On Wednesday 19 Oct 2016 00:33:54 Jyri Sarha wrote:
> >>>> Remove obsolete drm_connector_register() call from tda998x_bind().
> >>>> All connectors are registered when drm_dev_register() is called by the
> >>>> master drm_device driver.
> >>>> 
> >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >>> 
> >>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> 
> >>> By the way, any chance you would plan porting the driver to drm_bridge
> >>> ? :-)
> >>
> >> What's the point?
> > 
> > Avoiding code duplication. We currently have three APIs to handle external
> > encoders (drm encoder slave, drm bridge and the component-based method
> > used by tda998x only), which requires display drivers that want to
> > support multiple external encoders to use up to three APIs.
> 
> tda998x doesn't use the encoder slave anymore.  You somehow list component-
> based as a third alternative - it isn't, that's the native DRM non-bridge
> method.

The order in my list was random, component-based instantiation of external 
encoders is one method, not specifically the third one :-)

> In any case, I don't agree with converting it to a DRM bridge - that will
> mean that we have to split the driver into two pieces, the bridge part
> handling the mode set specifics, and a connector/encoder part which
> handles the detection/edid stuff.
> 
> We might as well keep the whole thing as the classical connector/encoder
> rather than introducing this additional layer of complexity.

We have different ways to instantiate external HDMI encoders, and that's not 
good. I believe everybody agrees that drm encoder slave has to go, so that's 
already one problem solved (or rather solvable, it still requires someone to 
do the work). We will then be left with two different methods, drm bridge and 
non-bridge component-based instantiation. We need to somehow merge the two, 
and I'm open to discussions on how the end result should look like.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
> Hi Russell,
> 
> On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
> > In any case, I don't agree with converting it to a DRM bridge - that will
> > mean that we have to split the driver into two pieces, the bridge part
> > handling the mode set specifics, and a connector/encoder part which
> > handles the detection/edid stuff.
> > 
> > We might as well keep the whole thing as the classical connector/encoder
> > rather than introducing this additional layer of complexity.
> 
> We have different ways to instantiate external HDMI encoders, and that's not 
> good. I believe everybody agrees that drm encoder slave has to go, so that's 
> already one problem solved (or rather solvable, it still requires someone to 
> do the work). We will then be left with two different methods, drm bridge and 
> non-bridge component-based instantiation. We need to somehow merge the two, 
> and I'm open to discussions on how the end result should look like.

I think you're idea really doesn't work - and I think your idea that
component-based is somehow separate from other methods is wrong.

Look at iMX for example - even converting all the code that could be
a bridge to be a bridge will not get rid of its use of the component
instantiation, because you still have other components that need to
come from elsewhere.  The same is true of armada as well.

Moreover, as I've already said, converting tda998x to a DRM bridge
will not get rid of the encoder/connector part, because it _is_ a
connector in the DRM sense - it provides the detection and EDID
reading.

So, what would we achieve by splitting the driver into a DRM bridge
and DRM encoder/connector?

We would still need the component helper to manage the binding of
the connector part, which would also then have to register a DRM
bridge in addition to a DRM encoder and providing the DRM connector
as we can't have two device drivers bound to the same i2c device.
What we get is more complexity in the driver.

We can see this with what happened to the DW-HDMI driver - that still
needs the component helper, and it provides a DRM bridge, DRM encoder
and DRM connector parts.  The only reason it made sense to split the
DW-HDMI driver was due to the differences between the Rockchip and
Freescale implementations.  Such differences do not exist for TDA998x.
So even here, your idea that "DRM bridge" vs "non-DRM bridge component
based" doesn't work - we have "DRM bridge component based" because of
the problem that I'm illustrating here.

So, again, I ask: what's the point of needlessly splitting the code
between an encoder/connector and a bridge?

You seem to be forcing the DRM bridge model onto drivers with no
regard for its appropriateness for those drivers.  If it pushes
more complexity into drivers, the model is wrong.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Russell,

On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
> > On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
> >> In any case, I don't agree with converting it to a DRM bridge - that
> >> will mean that we have to split the driver into two pieces, the bridge
> >> part handling the mode set specifics, and a connector/encoder part which
> >> handles the detection/edid stuff.
> >> 
> >> We might as well keep the whole thing as the classical connector/encoder
> >> rather than introducing this additional layer of complexity.
> > 
> > We have different ways to instantiate external HDMI encoders, and that's
> > not good. I believe everybody agrees that drm encoder slave has to go, so
> > that's already one problem solved (or rather solvable, it still requires
> > someone to do the work). We will then be left with two different methods,
> > drm bridge and non-bridge component-based instantiation. We need to
> > somehow merge the two, and I'm open to discussions on how the end result
> > should look like.
>
> I think you're idea really doesn't work - and I think your idea that
> component-based is somehow separate from other methods is wrong.
> 
> Look at iMX for example - even converting all the code that could be
> a bridge to be a bridge will not get rid of its use of the component
> instantiation, because you still have other components that need to
> come from elsewhere.  The same is true of armada as well.

Don't get me wrong, I'm certainly not against the component framework itself. 
A way to bind multiple independent devices together is needed. We have a 
similar framework in V4L2 called v4l2-async, and I'd like to see it moved to 
the component framework at some point to unify code. Some changes to the 
component framework might be needed to support needs of V4L2 that are 
currently not applicable to DRM/KMS, but there's nothing major there.

> Moreover, as I've already said, converting tda998x to a DRM bridge
> will not get rid of the encoder/connector part, because it _is_ a
> connector in the DRM sense - it provides the detection and EDID
> reading.
> 
> So, what would we achieve by splitting the driver into a DRM bridge
> and DRM encoder/connector?

Please note that DRM bridge doesn't split the DRM connector out of the bridge, 
bridges instantiate drm_connector objects. In that sense they don't differ 
much from the model implemented by the tda998x driver.

I however believe that connectors should be split out DRM bridge drivers, for 
the simple reason that bridges can be chained. The output of a bridge isn't 
guaranteed to be a connector but can be another bridge. We managed not to have 
to deal with that in a generic way so far in mainline, but we'll run into the 
problem one of these days, and a solution will be needed. There's no rush 
right now, and this is unrelated to converting tda998x to DRM bridge.

> We would still need the component helper to manage the binding of
> the connector part, which would also then have to register a DRM
> bridge in addition to a DRM encoder and providing the DRM connector
> as we can't have two device drivers bound to the same i2c device.
> What we get is more complexity in the driver.

DRM bridges indeed don't create encoders. That task is left to the display 
driver. The reason is the same as above: bridges can be chained (including 
with an internal encoder that is not modelled as a bridge, and that's a case 
we have today), while the KMS model exposes a single encoder to userspace. 
Exposing DRM encoder objects as part of the KMS UABI was probably a mistake. 
Better solutions would have been to expose no encoder at all or all encoders 
in the chain. We are however stuck with this model as we can't break the UABI. 
For that reason a DRM encoder object doesn't represent an encoder but a chain 
of encoders. As a DRM bridge models a single encoder, the DRM encoder object 
must be created at a higher level, in the display driver.

> We can see this with what happened to the DW-HDMI driver - that still
> needs the component helper, and it provides a DRM bridge, DRM encoder
> and DRM connector parts.

To be precise, the DW-HDMI driver core doesn't create a DRM encoder, it's the 
glue layers implemented as part of the Rockchip and iMX display drivers that 
do. Nonetheless, that's a mistake, the encoder should be created by the 
display drivers.

> The only reason it made sense to split the DW-HDMI driver was due to the
> differences between the Rockchip and Freescale implementations.  Such
> differences do not exist for TDA998x. So even here, your idea that "DRM
> bridge" vs "non-DRM bridge component based" doesn't work - we have "DRM
> bridge component based" because of the problem that I'm illustrating here.
> 
> So, again, I ask: what's the point of needlessly splitting the code
> between an encoder/connector and a bridge?

We need to standardize on one 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.

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

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Archit Taneja]

On 10/20/2016 01:50 PM, Laurent Pinchart wrote:
> Hi Russell,
>
> On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
>> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
>>> On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
>>>> In any case, I don't agree with converting it to a DRM bridge - that
>>>> will mean that we have to split the driver into two pieces, the bridge
>>>> part handling the mode set specifics, and a connector/encoder part which
>>>> handles the detection/edid stuff.
>>>>
>>>> We might as well keep the whole thing as the classical connector/encoder
>>>> rather than introducing this additional layer of complexity.
>>>
>>> We have different ways to instantiate external HDMI encoders, and that's
>>> not good. I believe everybody agrees that drm encoder slave has to go, so
>>> that's already one problem solved (or rather solvable, it still requires
>>> someone to do the work). We will then be left with two different methods,
>>> drm bridge and non-bridge component-based instantiation. We need to
>>> somehow merge the two, and I'm open to discussions on how the end result
>>> should look like.
>>
>> I think you're idea really doesn't work - and I think your idea that
>> component-based is somehow separate from other methods is wrong.
>>
>> Look at iMX for example - even converting all the code that could be
>> a bridge to be a bridge will not get rid of its use of the component
>> instantiation, because you still have other components that need to
>> come from elsewhere.  The same is true of armada as well.
>
> Don't get me wrong, I'm certainly not against the component framework itself.
> A way to bind multiple independent devices together is needed. We have a
> similar framework in V4L2 called v4l2-async, and I'd like to see it moved to
> the component framework at some point to unify code. Some changes to the
> component framework might be needed to support needs of V4L2 that are
> currently not applicable to DRM/KMS, but there's nothing major there.
>
>> Moreover, as I've already said, converting tda998x to a DRM bridge
>> will not get rid of the encoder/connector part, because it _is_ a
>> connector in the DRM sense - it provides the detection and EDID
>> reading.
>>
>> So, what would we achieve by splitting the driver into a DRM bridge
>> and DRM encoder/connector?
>
> Please note that DRM bridge doesn't split the DRM connector out of the bridge,
> bridges instantiate drm_connector objects. In that sense they don't differ
> much from the model implemented by the tda998x driver.
>
> I however believe that connectors should be split out DRM bridge drivers, for
> the simple reason that bridges can be chained. The output of a bridge isn't
> guaranteed to be a connector but can be another bridge. We managed not to have
> to deal with that in a generic way so far in mainline, but we'll run into the
> problem one of these days, and a solution will be needed. There's no rush
> right now, and this is unrelated to converting tda998x to DRM bridge.
>
>> We would still need the component helper to manage the binding of
>> the connector part, which would also then have to register a DRM
>> bridge in addition to a DRM encoder and providing the DRM connector
>> as we can't have two device drivers bound to the same i2c device.
>> What we get is more complexity in the driver.
>
> DRM bridges indeed don't create encoders. That task is left to the display
> driver. The reason is the same as above: bridges can be chained (including
> with an internal encoder that is not modelled as a bridge, and that's a case
> we have today), while the KMS model exposes a single encoder to userspace.
> Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
> Better solutions would have been to expose no encoder at all or all encoders
> in the chain. We are however stuck with this model as we can't break the UABI.
> For that reason a DRM encoder object doesn't represent an encoder but a chain
> of encoders. As a DRM bridge models a single encoder, the DRM encoder object
> must be created at a higher level, in the display driver.

One more thing is that the TDA998x in its current form won't work
with KMS drivers that create their own drm_encoder objects to represent
their hardware's mipi DPI/RGB interfaces. For such drivers, we would
want the TDA998x to tie itself to the existing encoder created by the
KMS driver, rather than creating its own.

Thanks,
Archit

>
>> We can see this with what happened to the DW-HDMI driver - that still
>> needs the component helper, and it provides a DRM bridge, DRM encoder
>> and DRM connector parts.
>
> To be precise, the DW-HDMI driver core doesn't create a DRM encoder, it's the
> glue layers implemented as part of the Rockchip and iMX display drivers that
> do. Nonetheless, that's a mistake, the encoder should be created by the
> display drivers.
>
>> The only reason it made sense to split the DW-HDMI driver was due 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.
>
>> 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.
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote:
> 
> 
> On 10/20/2016 01:50 PM, Laurent Pinchart wrote:
> >Hi Russell,
> >
> >On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
> >>On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
> >>>On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
> >>>>In any case, I don't agree with converting it to a DRM bridge - that
> >>>>will mean that we have to split the driver into two pieces, the bridge
> >>>>part handling the mode set specifics, and a connector/encoder part which
> >>>>handles the detection/edid stuff.
> >>>>
> >>>>We might as well keep the whole thing as the classical connector/encoder
> >>>>rather than introducing this additional layer of complexity.
> >>>
> >>>We have different ways to instantiate external HDMI encoders, and that's
> >>>not good. I believe everybody agrees that drm encoder slave has to go, so
> >>>that's already one problem solved (or rather solvable, it still requires
> >>>someone to do the work). We will then be left with two different methods,
> >>>drm bridge and non-bridge component-based instantiation. We need to
> >>>somehow merge the two, and I'm open to discussions on how the end result
> >>>should look like.
> >>
> >>I think you're idea really doesn't work - and I think your idea that
> >>component-based is somehow separate from other methods is wrong.
> >>
> >>Look at iMX for example - even converting all the code that could be
> >>a bridge to be a bridge will not get rid of its use of the component
> >>instantiation, because you still have other components that need to
> >>come from elsewhere.  The same is true of armada as well.
> >
> >Don't get me wrong, I'm certainly not against the component framework itself.
> >A way to bind multiple independent devices together is needed. We have a
> >similar framework in V4L2 called v4l2-async, and I'd like to see it moved to
> >the component framework at some point to unify code. Some changes to the
> >component framework might be needed to support needs of V4L2 that are
> >currently not applicable to DRM/KMS, but there's nothing major there.
> >
> >>Moreover, as I've already said, converting tda998x to a DRM bridge
> >>will not get rid of the encoder/connector part, because it _is_ a
> >>connector in the DRM sense - it provides the detection and EDID
> >>reading.
> >>
> >>So, what would we achieve by splitting the driver into a DRM bridge
> >>and DRM encoder/connector?
> >
> >Please note that DRM bridge doesn't split the DRM connector out of the bridge,
> >bridges instantiate drm_connector objects. In that sense they don't differ
> >much from the model implemented by the tda998x driver.
> >
> >I however believe that connectors should be split out DRM bridge drivers, for
> >the simple reason that bridges can be chained. The output of a bridge isn't
> >guaranteed to be a connector but can be another bridge. We managed not to have
> >to deal with that in a generic way so far in mainline, but we'll run into the
> >problem one of these days, and a solution will be needed. There's no rush
> >right now, and this is unrelated to converting tda998x to DRM bridge.
> >
> >>We would still need the component helper to manage the binding of
> >>the connector part, which would also then have to register a DRM
> >>bridge in addition to a DRM encoder and providing the DRM connector
> >>as we can't have two device drivers bound to the same i2c device.
> >>What we get is more complexity in the driver.
> >
> >DRM bridges indeed don't create encoders. That task is left to the display
> >driver. The reason is the same as above: bridges can be chained (including
> >with an internal encoder that is not modelled as a bridge, and that's a case
> >we have today), while the KMS model exposes a single encoder to userspace.
> >Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
> >Better solutions would have been to expose no encoder at all or all encoders
> >in the chain. We are however stuck with this model as we can't break the UABI.
> >For that reason a DRM encoder object doesn't represent an encoder but a chain
> >of encoders. As a DRM bridge models a single encoder, the DRM encoder object
> >must be created at a higher level, in the display driver.
> 
> One more thing is that the TDA998x in its current form won't work
> with KMS drivers that create their own drm_encoder objects to represent
> their hardware's mipi DPI/RGB interfaces. For such drivers, we would
> want the TDA998x to tie itself to the existing encoder created by the
> KMS driver, rather than creating its own.

Please show _technically_ how this would work.  I want to see code or
pseudo-code illustrating how a "foreign" DRM encoder could be used with
either dw-hdmi or tda998x, because right now I can't see any way that
could work.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Archit Taneja]

On 10/20/2016 02:45 PM, Russell King - ARM Linux wrote:
> On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote:
>>
>>
>> On 10/20/2016 01:50 PM, Laurent Pinchart wrote:
>>> Hi Russell,
>>>
>>> On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
>>>> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
>>>>> On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
>>>>>> In any case, I don't agree with converting it to a DRM bridge - that
>>>>>> will mean that we have to split the driver into two pieces, the bridge
>>>>>> part handling the mode set specifics, and a connector/encoder part which
>>>>>> handles the detection/edid stuff.
>>>>>>
>>>>>> We might as well keep the whole thing as the classical connector/encoder
>>>>>> rather than introducing this additional layer of complexity.
>>>>>
>>>>> We have different ways to instantiate external HDMI encoders, and that's
>>>>> not good. I believe everybody agrees that drm encoder slave has to go, so
>>>>> that's already one problem solved (or rather solvable, it still requires
>>>>> someone to do the work). We will then be left with two different methods,
>>>>> drm bridge and non-bridge component-based instantiation. We need to
>>>>> somehow merge the two, and I'm open to discussions on how the end result
>>>>> should look like.
>>>>
>>>> I think you're idea really doesn't work - and I think your idea that
>>>> component-based is somehow separate from other methods is wrong.
>>>>
>>>> Look at iMX for example - even converting all the code that could be
>>>> a bridge to be a bridge will not get rid of its use of the component
>>>> instantiation, because you still have other components that need to
>>>> come from elsewhere.  The same is true of armada as well.
>>>
>>> Don't get me wrong, I'm certainly not against the component framework itself.
>>> A way to bind multiple independent devices together is needed. We have a
>>> similar framework in V4L2 called v4l2-async, and I'd like to see it moved to
>>> the component framework at some point to unify code. Some changes to the
>>> component framework might be needed to support needs of V4L2 that are
>>> currently not applicable to DRM/KMS, but there's nothing major there.
>>>
>>>> Moreover, as I've already said, converting tda998x to a DRM bridge
>>>> will not get rid of the encoder/connector part, because it _is_ a
>>>> connector in the DRM sense - it provides the detection and EDID
>>>> reading.
>>>>
>>>> So, what would we achieve by splitting the driver into a DRM bridge
>>>> and DRM encoder/connector?
>>>
>>> Please note that DRM bridge doesn't split the DRM connector out of the bridge,
>>> bridges instantiate drm_connector objects. In that sense they don't differ
>>> much from the model implemented by the tda998x driver.
>>>
>>> I however believe that connectors should be split out DRM bridge drivers, for
>>> the simple reason that bridges can be chained. The output of a bridge isn't
>>> guaranteed to be a connector but can be another bridge. We managed not to have
>>> to deal with that in a generic way so far in mainline, but we'll run into the
>>> problem one of these days, and a solution will be needed. There's no rush
>>> right now, and this is unrelated to converting tda998x to DRM bridge.
>>>
>>>> We would still need the component helper to manage the binding of
>>>> the connector part, which would also then have to register a DRM
>>>> bridge in addition to a DRM encoder and providing the DRM connector
>>>> as we can't have two device drivers bound to the same i2c device.
>>>> What we get is more complexity in the driver.
>>>
>>> DRM bridges indeed don't create encoders. That task is left to the display
>>> driver. The reason is the same as above: bridges can be chained (including
>>> with an internal encoder that is not modelled as a bridge, and that's a case
>>> we have today), while the KMS model exposes a single encoder to userspace.
>>> Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
>>> Better solutions would have been to expose no encoder at all or all encoders
>>> in the chain. We are however stuck with this model as we can't break the UABI.
>>> For that reason a DRM encoder object doesn't represent an encoder but a chain
>>> of encoders. As a DRM bridge models a single encoder, the DRM encoder object
>>> must be created at a higher level, in the display driver.
>>
>> One more thing is that the TDA998x in its current form won't work
>> with KMS drivers that create their own drm_encoder objects to represent
>> their hardware's mipi DPI/RGB interfaces. For such drivers, we would
>> want the TDA998x to tie itself to the existing encoder created by the
>> KMS driver, rather than creating its own.
>
> Please show _technically_ how this would work.  I want to see code or
> pseudo-code illustrating how a "foreign" DRM encoder could be used with
> either dw-hdmi or tda998x, because right now I can't see any way that
> could work.

This is something we already do with the adv7511 bridge driver on msm,
rcar and arc (for 4.9) drivers.

I've shared pseudo code on the kms driver and encoder chip's driver
side. I've also shared a diff that converts the tda998x driver to use
drm_bridge(uncompiled/untested).

1) Kms driver side:

/*
  * Create an encoder instance. Depending on the hardware represented
  * by the KMS driver, the encoder can ops can either have some
  * functionality, or be nops. In the case of tilcdc, the encoder
  * funcs would be mostly nops.
  */
drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs);
drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs,
		 type, NULL);

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

bridge = of_drm_find_bridge(encoder_chip_dev->of_node);

/*
  * Tell our newly created drm_encoder that it is connected
  * to a bridge. The atomic helpers and legacy crtc helpers
  * check if the encoder is connected to a bridge. If so,
  * it'll call the bridge ops along with the encoder ops.
  */

bridge->encoder = &kms_priv->encoder;
kms_priv->encoder.bridge = bridge;

drm_bridge_attach(kms_priv->drm, bridge);

...
...

2) On the encoder chip driver side:

/*
  * The drm bridge driver creates only drm_bridge and connector,
  * it assumes that the drm_encoder the bridge is tied to is
  * created by the kms driver. When the KMS driver calls
  * drm_bridge_attach with the bridge pointer, it assumes that
  * we have already set up the links between the encoder
  * and the bridge.
  */

static int tda998x_bridge_attach(struct drm_bridge *bridge)
{
	/* set up connector here, you can peek at the
	 * drm_encoder by referencing bridge->encoder
	 */
	...
}

static struct drm_bridge_funcs tda998x_bridge_funcs = {
	/* ops that are very similar to encoder ops */
	.enable = tda998x_bridge_enable,
	.disable = tda998x_bridge_disable,
	.mode_set = tda998x_bridge_mode_set,
	/* the attach op that binds the bridge to the kms device */
	.attach = tda998x_bridge_attach,
};

static int tda998x_bind(...)
{
	...
	...

	priv->bridge.funcs = &tda998x_bridge_funcs;
	priv->bridge.of_node = dev->of_node;
	ret = drm_bridge_add(&priv->bridge);
	...
	...
}

3) Rough conversion to bridge:

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9798d40..bdca061 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -60,7 +60,7 @@ struct tda998x_priv {
  	wait_queue_head_t edid_delay_waitq;
  	bool edid_delay_active;

-	struct drm_encoder encoder;
+	struct drm_bridge bridge;
  	struct drm_connector connector;

  	struct tda998x_audio_port audio_port[2];
@@ -69,8 +69,8 @@ struct tda998x_priv {
  #define conn_to_tda998x_priv(x) \
  	container_of(x, struct tda998x_priv, connector)

-#define enc_to_tda998x_priv(x) \
-	container_of(x, struct tda998x_priv, encoder)
+#define bridge_to_tda998x_priv(x) \
+	container_of(x, struct tda998x_priv, bridge)

  /* The TDA9988 series of devices use a paged register scheme.. to simplify
   * things we encode the page # in upper bits of the register #.  To read/
@@ -614,7 +614,7 @@ static void tda998x_detect_work(struct work_struct 
*work)
  {
  	struct tda998x_priv *priv =
  		container_of(work, struct tda998x_priv, detect_work);
-	struct drm_device *dev = priv->encoder.dev;
+	struct drm_device *dev = priv->connector.dev;

  	if (dev)
  		drm_kms_helper_hotplug_event(dev);
@@ -840,9 +840,9 @@ static void tda998x_encoder_set_config(struct 
tda998x_priv *priv,
  	priv->audio_params = p->audio_params;
  }

-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+static void tda998x_encoder_dpms(struct drm_bridge *bridge, int mode)
  {
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);

  	/* we only care about on or off: */
  	if (mode != DRM_MODE_DPMS_ON)
@@ -889,11 +889,11 @@ static int tda998x_connector_mode_valid(struct 
drm_connector *connector,
  }

  static void
-tda998x_encoder_mode_set(struct drm_encoder *encoder,
-			 struct drm_display_mode *mode,
-			 struct drm_display_mode *adjusted_mode)
+tda998x_bridge_mode_set(struct drm_bridge *bridge,
+			struct drm_display_mode *mode,
+			struct drm_display_mode *adjusted_mode)
  {
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
  	u16 ref_pix, ref_line, n_pix, n_line;
  	u16 hs_pix_s, hs_pix_e;
  	u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1227,7 +1227,8 @@ static int tda998x_audio_hw_params(struct device 
*dev, void *data,
  		.cea = params->cea,
  	};

-	if (!priv->encoder.crtc)
+	/* ?? */
+	if (!priv->bridge.encoder->crtc)
  		return -ENODEV;

  	memcpy(audio.status, params->iec.status,
@@ -1267,7 +1268,7 @@ static int tda998x_audio_hw_params(struct device 
*dev, void *data,
  	mutex_lock(&priv->audio_mutex);
  	ret = tda998x_configure_audio(priv,
  				      &audio,
-				      priv->encoder.crtc->hwmode.clock);
+				      priv->bridge.encoder->crtc->hwmode.clock);

  	if (ret == 0)
  		priv->audio_params = audio;
@@ -1538,41 +1539,12 @@ static int tda998x_create(struct i2c_client 
*client, struct tda998x_priv *priv)
  	return -ENXIO;
  }

-static void tda998x_encoder_prepare(struct drm_encoder *encoder)
-{
-	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
-}
-
-static void tda998x_encoder_commit(struct drm_encoder *encoder)
-{
-	tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
-}
-
-static const struct drm_encoder_helper_funcs 
tda998x_encoder_helper_funcs = {
-	.dpms = tda998x_encoder_dpms,
-	.prepare = tda998x_encoder_prepare,
-	.commit = tda998x_encoder_commit,
-	.mode_set = tda998x_encoder_mode_set,
-};
-
-static void tda998x_encoder_destroy(struct drm_encoder *encoder)
-{
-	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
-
-	tda998x_destroy(priv);
-	drm_encoder_cleanup(encoder);
-}
-
-static const struct drm_encoder_funcs tda998x_encoder_funcs = {
-	.destroy = tda998x_encoder_destroy,
-};
-
  static struct drm_encoder *
  tda998x_connector_best_encoder(struct drm_connector *connector)
  {
  	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);

-	return &priv->encoder;
+	return priv->bridge.encoder;
  }

  static
@@ -1584,7 +1556,6 @@ static void tda998x_encoder_destroy(struct 
drm_encoder *encoder)

  static void tda998x_connector_destroy(struct drm_connector *connector)
  {
-	drm_connector_unregister(connector);
  	drm_connector_cleanup(connector);
  }

@@ -1606,13 +1577,50 @@ static int tda998x_connector_dpms(struct 
drm_connector *connector, int mode)
  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
  };

+static void tda998x_bridge_enable(struct drm_bridge *bridge)
+{
+	tda998x_encoder_dpms(bridge, DRM_MODE_DPMS_ON);
+}
+
+static void tda998x_bridge_disable(struct drm_bridge *bridge)
+{
+	tda998x_encoder_dpms(bridge, DRM_MODE_DPMS_OFF);
+}
+
+static int tda998x_bridge_attach(struct drm_bridge *bridge)
+{
+	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+	struct drm_connector *connector = &priv->connector;
+	struct drm_device *dev = dev_get_drvdata(
+
+	connector->interlace_allowed = 1;
+	tda998x_encoder_set_polling(priv, connector);
+
+	drm_connector_helper_add(connector, &tda998x_connector_helper_funcs);
+	ret = drm_connector_init(drm, connector,
+				 &tda998x_connector_funcs,
+				 DRM_MODE_CONNECTOR_HDMIA);
+	if (ret)
+		return ret;
+
+	drm_mode_connector_attach_encoder(&priv->connector, priv->bridge.encoder);
+
+	return 0;
+}
+
+static struct drm_bridge_funcs tda998x_bridge_funcs = {
+	.enable = tda998x_bridge_enable,
+	.disable = tda998x_bridge_disable,
+	.mode_set = tda998x_bridge_mode_set,
+	.attach = tda998x_bridge_attach,
+};
+
  static int tda998x_bind(struct device *dev, struct device *master, 
void *data)
  {
  	struct tda998x_encoder_params *params = dev->platform_data;
  	struct i2c_client *client = to_i2c_client(dev);
  	struct drm_device *drm = data;
  	struct tda998x_priv *priv;
-	u32 crtcs = 0;
  	int ret;

  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -1621,17 +1629,7 @@ static int tda998x_bind(struct device *dev, 
struct device *master, void *data)

  	dev_set_drvdata(dev, priv);

-	if (dev->of_node)
-		crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
-
-	/* If no CRTCs were found, fall back to our old behaviour */
-	if (crtcs == 0) {
-		dev_warn(dev, "Falling back to first CRTC\n");
-		crtcs = 1 << 0;
-	}
-
-	priv->connector.interlace_allowed = 1;
-	priv->encoder.possible_crtcs = crtcs;
+	/* find possible crtcs in the KMS driver */

  	ret = tda998x_create(client, priv);
  	if (ret)
@@ -1640,35 +1638,18 @@ static int tda998x_bind(struct device *dev, 
struct device *master, void *data)
  	if (!dev->of_node && params)
  		tda998x_encoder_set_config(priv, params);

-	tda998x_encoder_set_polling(priv, &priv->connector);
-
-	drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs);
-	ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs,
-			       DRM_MODE_ENCODER_TMDS, NULL);
-	if (ret)
-		goto err_encoder;
-
-	drm_connector_helper_add(&priv->connector,
-				 &tda998x_connector_helper_funcs);
-	ret = drm_connector_init(drm, &priv->connector,
-				 &tda998x_connector_funcs,
-				 DRM_MODE_CONNECTOR_HDMIA);
-	if (ret)
-		goto err_connector;
-
-	ret = drm_connector_register(&priv->connector);
-	if (ret)
-		goto err_sysfs;
+	priv->bridge.funcs = &tda998x_bridge_funcs;
+	priv->bridge.of_node = dev->of_node;

-	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
+	ret = drm_bridge_add(&priv->bridge);
+	if (ret) {
+		dev_err(dev, "failed to add adv7511 bridge\n");
+		goto err_bridge_add;
+	}

  	return 0;

-err_sysfs:
-	drm_connector_cleanup(&priv->connector);
-err_connector:
-	drm_encoder_cleanup(&priv->encoder);
-err_encoder:
+err_bridge_add:
  	tda998x_destroy(priv);
  	return ret;
  }
@@ -1678,9 +1659,8 @@ 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);
+	drm_bridge_remove(&priv->bridge);
  	tda998x_destroy(priv);
  }

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jean-Francois Moine]

On Thu, 20 Oct 2016 16:56:44 +0530
Archit Taneja <architt@codeaurora.org> wrote:

> > Please show _technically_ how this would work.  I want to see code or
> > pseudo-code illustrating how a "foreign" DRM encoder could be used with
> > either dw-hdmi or tda998x, because right now I can't see any way that
> > could work.
> 
> This is something we already do with the adv7511 bridge driver on msm,
> rcar and arc (for 4.9) drivers.
> 
> I've shared pseudo code on the kms driver and encoder chip's driver
> side. I've also shared a diff that converts the tda998x driver to use
> drm_bridge(uncompiled/untested).
> 
> 1) Kms driver side:
> 
> /*
>   * Create an encoder instance. Depending on the hardware represented
>   * by the KMS driver, the encoder can ops can either have some
>   * functionality, or be nops. In the case of tilcdc, the encoder
>   * funcs would be mostly nops.
>   */
> drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs);
> drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs,
> 		 type, NULL);

Then, how does this 'kms_priv' know the type of the encoder, this one
being tied to the connector type at the end of the bridge chain?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Jean-François,

On Friday 21 Oct 2016 19:28:47 Jean-Francois Moine wrote:
> On Thu, 20 Oct 2016 16:56:44 +0530 Archit Taneja wrote:
> >> Please show _technically_ how this would work.  I want to see code or
> >> pseudo-code illustrating how a "foreign" DRM encoder could be used with
> >> either dw-hdmi or tda998x, because right now I can't see any way that
> >> could work.
> > 
> > This is something we already do with the adv7511 bridge driver on msm,
> > rcar and arc (for 4.9) drivers.
> > 
> > I've shared pseudo code on the kms driver and encoder chip's driver
> > side. I've also shared a diff that converts the tda998x driver to use
> > drm_bridge(uncompiled/untested).
> > 
> > 1) Kms driver side:
> > 
> > /*
> > 
> >   * Create an encoder instance. Depending on the hardware represented
> >   * by the KMS driver, the encoder can ops can either have some
> >   * functionality, or be nops. In the case of tilcdc, the encoder
> >   * funcs would be mostly nops.
> >   */
> > 
> > drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs);
> > drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs,
> > 
> > 		 type, NULL);
> 
> Then, how does this 'kms_priv' know the type of the encoder, this one
> being tied to the connector type at the end of the bridge chain?

"[PATCH 4/8] drm: Add encoder_type field to the drm_bridge structure"

:-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote:
> 3) Rough conversion to bridge:

So I thought I might give this a try, and see what's needed to complete
the patch, but...

I thought we'd got past the dark ages of email clients screwing up
patches, but it seems not.  This patch is totally screwed that it's
not worth even sending - it has been:

- wrapped
- white-space damaged

which makes it pretty hard to undo all the damage.  Please use a
better mail client which doesn't screw up patches, or send it as a
plain text attachment.  Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Archit Taneja]

On 10/21/2016 11:39 PM, Russell King - ARM Linux wrote:
> On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote:
>> 3) Rough conversion to bridge:
>
> So I thought I might give this a try, and see what's needed to complete
> the patch, but...
>
> I thought we'd got past the dark ages of email clients screwing up
> patches, but it seems not.  This patch is totally screwed that it's
> not worth even sending - it has been:
>
> - wrapped
> - white-space damaged
>
> which makes it pretty hard to undo all the damage.  Please use a
> better mail client which doesn't screw up patches, or send it as a
> plain text attachment.  Thanks.

Sorry about that. I'll post out a proper patch for this once we resolve
the drm_bridge shortcomings you've mentioned. I can create one if you
want to try it now.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Mon, Oct 24, 2016 at 10:39:27AM +0530, Archit Taneja wrote:
> Sorry about that. I'll post out a proper patch for this once we resolve
> the drm_bridge shortcomings you've mentioned. I can create one if you
> want to try it now.

As said elsewhere, I've been away, so I haven't had a chance to look
at anything.

I'm going to be spending the early part of this week catching up with
mail, but probably _not_ touching the kernel tree for any development
stuff until I've caught up.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote:
> 
> 
> On 10/20/2016 02:45 PM, Russell King - ARM Linux wrote:
> >On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote:
> >>
> >>
> >>On 10/20/2016 01:50 PM, Laurent Pinchart wrote:
> >>>Hi Russell,
> >>>
> >>>On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
> >>>>On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
> >>>>>On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
> >>>>>>In any case, I don't agree with converting it to a DRM bridge - that
> >>>>>>will mean that we have to split the driver into two pieces, the bridge
> >>>>>>part handling the mode set specifics, and a connector/encoder part which
> >>>>>>handles the detection/edid stuff.
> >>>>>>
> >>>>>>We might as well keep the whole thing as the classical connector/encoder
> >>>>>>rather than introducing this additional layer of complexity.
> >>>>>
> >>>>>We have different ways to instantiate external HDMI encoders, and that's
> >>>>>not good. I believe everybody agrees that drm encoder slave has to go, so
> >>>>>that's already one problem solved (or rather solvable, it still requires
> >>>>>someone to do the work). We will then be left with two different methods,
> >>>>>drm bridge and non-bridge component-based instantiation. We need to
> >>>>>somehow merge the two, and I'm open to discussions on how the end result
> >>>>>should look like.
> >>>>
> >>>>I think you're idea really doesn't work - and I think your idea that
> >>>>component-based is somehow separate from other methods is wrong.
> >>>>
> >>>>Look at iMX for example - even converting all the code that could be
> >>>>a bridge to be a bridge will not get rid of its use of the component
> >>>>instantiation, because you still have other components that need to
> >>>>come from elsewhere.  The same is true of armada as well.
> >>>
> >>>Don't get me wrong, I'm certainly not against the component framework itself.
> >>>A way to bind multiple independent devices together is needed. We have a
> >>>similar framework in V4L2 called v4l2-async, and I'd like to see it moved to
> >>>the component framework at some point to unify code. Some changes to the
> >>>component framework might be needed to support needs of V4L2 that are
> >>>currently not applicable to DRM/KMS, but there's nothing major there.
> >>>
> >>>>Moreover, as I've already said, converting tda998x to a DRM bridge
> >>>>will not get rid of the encoder/connector part, because it _is_ a
> >>>>connector in the DRM sense - it provides the detection and EDID
> >>>>reading.
> >>>>
> >>>>So, what would we achieve by splitting the driver into a DRM bridge
> >>>>and DRM encoder/connector?
> >>>
> >>>Please note that DRM bridge doesn't split the DRM connector out of the bridge,
> >>>bridges instantiate drm_connector objects. In that sense they don't differ
> >>>much from the model implemented by the tda998x driver.
> >>>
> >>>I however believe that connectors should be split out DRM bridge drivers, for
> >>>the simple reason that bridges can be chained. The output of a bridge isn't
> >>>guaranteed to be a connector but can be another bridge. We managed not to have
> >>>to deal with that in a generic way so far in mainline, but we'll run into the
> >>>problem one of these days, and a solution will be needed. There's no rush
> >>>right now, and this is unrelated to converting tda998x to DRM bridge.
> >>>
> >>>>We would still need the component helper to manage the binding of
> >>>>the connector part, which would also then have to register a DRM
> >>>>bridge in addition to a DRM encoder and providing the DRM connector
> >>>>as we can't have two device drivers bound to the same i2c device.
> >>>>What we get is more complexity in the driver.
> >>>
> >>>DRM bridges indeed don't create encoders. That task is left to the display
> >>>driver. The reason is the same as above: bridges can be chained (including
> >>>with an internal encoder that is not modelled as a bridge, and that's a case
> >>>we have today), while the KMS model exposes a single encoder to userspace.
> >>>Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
> >>>Better solutions would have been to expose no encoder at all or all encoders
> >>>in the chain. We are however stuck with this model as we can't break the UABI.
> >>>For that reason a DRM encoder object doesn't represent an encoder but a chain
> >>>of encoders. As a DRM bridge models a single encoder, the DRM encoder object
> >>>must be created at a higher level, in the display driver.
> >>
> >>One more thing is that the TDA998x in its current form won't work
> >>with KMS drivers that create their own drm_encoder objects to represent
> >>their hardware's mipi DPI/RGB interfaces. For such drivers, we would
> >>want the TDA998x to tie itself to the existing encoder created by the
> >>KMS driver, rather than creating its own.
> >
> >Please show _technically_ how this would work.  I want to see code or
> >pseudo-code illustrating how a "foreign" DRM encoder could be used with
> >either dw-hdmi or tda998x, because right now I can't see any way that
> >could work.
> 
> This is something we already do with the adv7511 bridge driver on msm,
> rcar and arc (for 4.9) drivers.
> 
> I've shared pseudo code on the kms driver and encoder chip's driver
> side. I've also shared a diff that converts the tda998x driver to use
> drm_bridge(uncompiled/untested).
> 
> 1) Kms driver side:
> 
> /*
>  * Create an encoder instance. Depending on the hardware represented
>  * by the KMS driver, the encoder can ops can either have some
>  * functionality, or be nops. In the case of tilcdc, the encoder
>  * funcs would be mostly nops.
>  */
> drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs);
> drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs,
> 		 type, NULL);

How does the KMS driver know to create the encoder?

> /*
>  * Extract the bridge using DT node of the external encoder chip,
>  * i.e. tda998x
>  */
> 
> bridge = of_drm_find_bridge(encoder_chip_dev->of_node);

What if we're not using DT?  There seems to be no solution for that, so
this currently breaks non-DT armada-drm.

In the case of hardware which is:

Dove ===> TDA998x ===> display

Who provides this encoder_chip_dev's of_node?  The Dove is the CRTC.
The TDA998x is the bridge.  What's the encoder?  Do we need to make up
a ficticious DT device for that.  That will raise rightful objections,
because it means DT is no longer representative of the hardware, but
is representing the hardware in an implementation specific way - the
split of the encoder from bridge is entirely a software abstraction.

-- 
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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Archit Taneja]

On 10/22/2016 12:13 AM, Russell King - ARM Linux wrote:
> On Thu, Oct 20, 2016 at 04:56:44PM +0530, Archit Taneja wrote:
>>
>>
>> On 10/20/2016 02:45 PM, Russell King - ARM Linux wrote:
>>> On Thu, Oct 20, 2016 at 02:38:25PM +0530, Archit Taneja wrote:
>>>>
>>>>
>>>> On 10/20/2016 01:50 PM, Laurent Pinchart wrote:
>>>>> Hi Russell,
>>>>>
>>>>> On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
>>>>>> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
>>>>>>> On Wednesday 19 Oct 2016 10:11:22 Russell King - ARM Linux wrote:
>>>>>>>> In any case, I don't agree with converting it to a DRM bridge - that
>>>>>>>> will mean that we have to split the driver into two pieces, the bridge
>>>>>>>> part handling the mode set specifics, and a connector/encoder part which
>>>>>>>> handles the detection/edid stuff.
>>>>>>>>
>>>>>>>> We might as well keep the whole thing as the classical connector/encoder
>>>>>>>> rather than introducing this additional layer of complexity.
>>>>>>>
>>>>>>> We have different ways to instantiate external HDMI encoders, and that's
>>>>>>> not good. I believe everybody agrees that drm encoder slave has to go, so
>>>>>>> that's already one problem solved (or rather solvable, it still requires
>>>>>>> someone to do the work). We will then be left with two different methods,
>>>>>>> drm bridge and non-bridge component-based instantiation. We need to
>>>>>>> somehow merge the two, and I'm open to discussions on how the end result
>>>>>>> should look like.
>>>>>>
>>>>>> I think you're idea really doesn't work - and I think your idea that
>>>>>> component-based is somehow separate from other methods is wrong.
>>>>>>
>>>>>> Look at iMX for example - even converting all the code that could be
>>>>>> a bridge to be a bridge will not get rid of its use of the component
>>>>>> instantiation, because you still have other components that need to
>>>>>> come from elsewhere.  The same is true of armada as well.
>>>>>
>>>>> Don't get me wrong, I'm certainly not against the component framework itself.
>>>>> A way to bind multiple independent devices together is needed. We have a
>>>>> similar framework in V4L2 called v4l2-async, and I'd like to see it moved to
>>>>> the component framework at some point to unify code. Some changes to the
>>>>> component framework might be needed to support needs of V4L2 that are
>>>>> currently not applicable to DRM/KMS, but there's nothing major there.
>>>>>
>>>>>> Moreover, as I've already said, converting tda998x to a DRM bridge
>>>>>> will not get rid of the encoder/connector part, because it _is_ a
>>>>>> connector in the DRM sense - it provides the detection and EDID
>>>>>> reading.
>>>>>>
>>>>>> So, what would we achieve by splitting the driver into a DRM bridge
>>>>>> and DRM encoder/connector?
>>>>>
>>>>> Please note that DRM bridge doesn't split the DRM connector out of the bridge,
>>>>> bridges instantiate drm_connector objects. In that sense they don't differ
>>>>> much from the model implemented by the tda998x driver.
>>>>>
>>>>> I however believe that connectors should be split out DRM bridge drivers, for
>>>>> the simple reason that bridges can be chained. The output of a bridge isn't
>>>>> guaranteed to be a connector but can be another bridge. We managed not to have
>>>>> to deal with that in a generic way so far in mainline, but we'll run into the
>>>>> problem one of these days, and a solution will be needed. There's no rush
>>>>> right now, and this is unrelated to converting tda998x to DRM bridge.
>>>>>
>>>>>> We would still need the component helper to manage the binding of
>>>>>> the connector part, which would also then have to register a DRM
>>>>>> bridge in addition to a DRM encoder and providing the DRM connector
>>>>>> as we can't have two device drivers bound to the same i2c device.
>>>>>> What we get is more complexity in the driver.
>>>>>
>>>>> DRM bridges indeed don't create encoders. That task is left to the display
>>>>> driver. The reason is the same as above: bridges can be chained (including
>>>>> with an internal encoder that is not modelled as a bridge, and that's a case
>>>>> we have today), while the KMS model exposes a single encoder to userspace.
>>>>> Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
>>>>> Better solutions would have been to expose no encoder at all or all encoders
>>>>> in the chain. We are however stuck with this model as we can't break the UABI.
>>>>> For that reason a DRM encoder object doesn't represent an encoder but a chain
>>>>> of encoders. As a DRM bridge models a single encoder, the DRM encoder object
>>>>> must be created at a higher level, in the display driver.
>>>>
>>>> One more thing is that the TDA998x in its current form won't work
>>>> with KMS drivers that create their own drm_encoder objects to represent
>>>> their hardware's mipi DPI/RGB interfaces. For such drivers, we would
>>>> want the TDA998x to tie itself to the existing encoder created by the
>>>> KMS driver, rather than creating its own.
>>>
>>> Please show _technically_ how this would work.  I want to see code or
>>> pseudo-code illustrating how a "foreign" DRM encoder could be used with
>>> either dw-hdmi or tda998x, because right now I can't see any way that
>>> could work.
>>
>> This is something we already do with the adv7511 bridge driver on msm,
>> rcar and arc (for 4.9) drivers.
>>
>> I've shared pseudo code on the kms driver and encoder chip's driver
>> side. I've also shared a diff that converts the tda998x driver to use
>> drm_bridge(uncompiled/untested).
>>
>> 1) Kms driver side:
>>
>> /*
>>  * Create an encoder instance. Depending on the hardware represented
>>  * by the KMS driver, the encoder can ops can either have some
>>  * functionality, or be nops. In the case of tilcdc, the encoder
>>  * funcs would be mostly nops.
>>  */
>> drm_encoder_helper_add(&kms_priv->encoder, &kms_encoder_helper_funcs);
>> drm_encoder_init(kms_pirv->drm, &kms_priv->encoder, &kms_encoder_funcs,
>> 		 type, NULL);
>
> How does the KMS driver know to create the encoder?

In the DT case, the Dove CRTC would see that it is connected
to a device node which represents either a block of HW within
the SoC, or an external chip like TDA998x. The KMS driver
would be aware of its capabilities (i.e, whether it can
encode RGB to HDMI itself or not).

In cases where it can't do the encoding itself, it should
create an encoder (with nop funcs) and try to find a bridge.
Once it finds a bridge, it links the newly created encoder
to the bridge, and attach it.

In cases where we have encoding capability within the SoC,
but also have another external encoder chip, it creates
its own encoder as is (with the ops to configure the HW)
and links itself with the external encoder bridge.

To summarize, there are two cases:

- The HW represented by the bridge driver is the only encoder in
the chain: In this case, we create a 'dummy' encoder in the KMS
driver, and link it to the bridge.

- The HW represented by the bridge driver isn't the first encoder
in the chain: In this case, the KMS driver creates its encoder
as is, and gets the drm_bridge from the bridge driver, and links
the two.

>
>> /*
>>  * Extract the bridge using DT node of the external encoder chip,
>>  * i.e. tda998x
>>  */
>>
>> bridge = of_drm_find_bridge(encoder_chip_dev->of_node);
>
> What if we're not using DT?  There seems to be no solution for that, so
> this currently breaks non-DT armada-drm.

Yeah, I agree. We haven't looked at non-DT cases yet. Will try to figure
out what to do for non-DT scenarios.

>
> In the case of hardware which is:
>
> Dove ===> TDA998x ===> display
>
> Who provides this encoder_chip_dev's of_node?  The Dove is the CRTC.
> The TDA998x is the bridge.  What's the encoder?  Do we need to make up
> a ficticious DT device for that.  That will raise rightful objections,
> because it means DT is no longer representative of the hardware, but
> is representing the hardware in an implementation specific way - the
> split of the encoder from bridge is entirely a software abstraction.

The encoder_chip_dev is supposed to be the TDA998x device here. The
Dove CRTC's output port would be directly connected to the input port
of TDA998x in DT. There would be no encoder node in DT. The DT bindings
wouldn't change. Sorry about the choice of name, it made it a bit confusing.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jean-Francois Moine]

>>>> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
	(sorry, I lost your original mail)
> >>> DRM bridges indeed don't create encoders. That task is left to the display
> >>> driver. The reason is the same as above: bridges can be chained (including
> >>> with an internal encoder that is not modelled as a bridge, and that's a case
> >>> we have today), while the KMS model exposes a single encoder to userspace.
> >>> Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
> >>> Better solutions would have been to expose no encoder at all or all encoders
> >>> in the chain. We are however stuck with this model as we can't break the UABI.
> >>> For that reason a DRM encoder object doesn't represent an encoder but a chain
> >>> of encoders. As a DRM bridge models a single encoder, the DRM encoder object
> >>> must be created at a higher level, in the display driver.

I wonder why you created 'bridge's instead of simply adding links to
the encoders? (that's what ASoC did: the audio CODECs are linked)
This way, in simple cases (most cases), there would have been
	crtc -> (encoder -> connector)
instead of
	crtc -> (bridge + encoder) -> (bridge + connector) 
without any changes in the actual (encoder + connector)s.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Fri, Oct 21, 2016 at 09:04:39PM +0200, Jean-Francois Moine wrote:
> >>>> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
> 	(sorry, I lost your original mail)
> > >>> DRM bridges indeed don't create encoders. That task is left to the display
> > >>> driver. The reason is the same as above: bridges can be chained (including
> > >>> with an internal encoder that is not modelled as a bridge, and that's a case
> > >>> we have today), while the KMS model exposes a single encoder to userspace.
> > >>> Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
> > >>> Better solutions would have been to expose no encoder at all or all encoders
> > >>> in the chain. We are however stuck with this model as we can't break the UABI.
> > >>> For that reason a DRM encoder object doesn't represent an encoder but a chain
> > >>> of encoders. As a DRM bridge models a single encoder, the DRM encoder object
> > >>> must be created at a higher level, in the display driver.
> 
> I wonder why you created 'bridge's instead of simply adding links to
> the encoders? (that's what ASoC did: the audio CODECs are linked)
> This way, in simple cases (most cases), there would have been
> 	crtc -> (encoder -> connector)
> instead of
> 	crtc -> (bridge + encoder) -> (bridge + connector) 
> without any changes in the actual (encoder + connector)s.

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

	crtc -> encoder -> connector
                 `-> bridge
		     `-> bridge
		         `-> bridge

Bridges are always attached to an encoder, and there can be multiple
bridges attached to one encoder.  Bridges can't be attached to the
connector.

So, in the case of TDA998x, we end up with the TDA998x providing a
connector, because it has connector functionality, and providing a
bridge.  The encoder is left to the KMS driver, which adds additional
complexity (100+ lines) to each and every KMS driver, requiring the
KMS driver to have much more knowledge of what's attached to the
"CRTC", so it can create these encoders itself.  I still think this
is a backwards step - maybe one step forwards, two backwards.

Even if we were to provide helpers to create a dummy encoder to
work around the DRM bridge model short-comings, much of the 100+
lines is to do with working out whether or not we need to create an
encoder, and parsing the information we need to create that in a way
that existing DT doesn't break.

Then there's the fact that the bridge approach breaks non-DT at the
moment, because DRM bridge fundamentally requires DT.  This makes
DRM bridge useless for architectures which aren't DT aware, such as
x86.  So, converting drivers to be a DRM bridge effectively denies
their use on other architectures.  See drm_bridge.c, and look for
the references to bridge_list, noting that there are two: one which
adds to the bridge list, and one under #ifdef CONFIG_OF which looks
up a DT reference to a bridge.

There's another issue with TDA998x - we now have audio support in
TDA998x, and converting TDA998x to be a DRM bridge introduces some
fundamental (and as yet unsolved) races between the ASoC code and
the attachment of the DRM bridge to the DRM encoder, and the detachment
when the DRM bridge/connector gets cleaned up.  Right now, there's no
bridge callback when the encoder or drm_device goes away, so doing
stuff like:

static int tda998x_audio_get_eld(struct device *dev, void *data,
                                 uint8_t *buf, size_t len)
{
        struct tda998x_priv *priv = dev_get_drvdata(dev);
        struct drm_mode_config *config;
        struct drm_connector *connector;
        int ret = -ENODEV;

        /* FIXME: This is racy */
        if (!priv->bridge.encoder || !priv->bridge.encoder->dev)
                return ret;

        config = &priv->bridge.encoder->dev->mode_config;

        mutex_lock(&config->mutex);
        list_for_each_entry(connector, &config->connector_list, head) {
                if (priv->bridge.encoder == connector->encoder) {
                        memcpy(buf, connector->eld,
                               min(sizeof(connector->eld), len));
                        ret = 0;
                }
        }
        mutex_unlock(&config->mutex);

feels very unsafe - nothing really guarantees the validity of the
priv->bridge.encoder or priv->bridge.encoder->dev pointers.  The
config->mutex lock does nothing to solve this.  The same problem
also exists in tda998x_audio_hw_params().

Anyway, the whole bridge conversion thing is moot until every user
of the TDA998x code has been updated to cope with the lack of
drm_connector_register() inside TDA998x - see Brian Starkey's
response to this patch set.  It's also moot if it breaks armada-drm.

-- 
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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Archit Taneja]

On 10/22/2016 03:25 PM, Russell King - ARM Linux wrote:
> On Fri, Oct 21, 2016 at 09:04:39PM +0200, Jean-Francois Moine wrote:
>>>>>> On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
>> 	(sorry, I lost your original mail)
>>>>>> DRM bridges indeed don't create encoders. That task is left to the display
>>>>>> driver. The reason is the same as above: bridges can be chained (including
>>>>>> with an internal encoder that is not modelled as a bridge, and that's a case
>>>>>> we have today), while the KMS model exposes a single encoder to userspace.
>>>>>> Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
>>>>>> Better solutions would have been to expose no encoder at all or all encoders
>>>>>> in the chain. We are however stuck with this model as we can't break the UABI.
>>>>>> For that reason a DRM encoder object doesn't represent an encoder but a chain
>>>>>> of encoders. As a DRM bridge models a single encoder, the DRM encoder object
>>>>>> must be created at a higher level, in the display driver.
>>
>> I wonder why you created 'bridge's instead of simply adding links to
>> the encoders? (that's what ASoC did: the audio CODECs are linked)
>> This way, in simple cases (most cases), there would have been
>> 	crtc -> (encoder -> connector)
>> instead of
>> 	crtc -> (bridge + encoder) -> (bridge + connector)
>> without any changes in the actual (encoder + connector)s.
>
> Looking at drm_bridge_disable() and drm_bridge_enable(), the control
> model appears to be:
>
> 	crtc -> encoder -> connector
>                  `-> bridge
> 		     `-> bridge
> 		         `-> bridge
>
> Bridges are always attached to an encoder, and there can be multiple
> bridges attached to one encoder.  Bridges can't be attached to the
> connector.
>
> So, in the case of TDA998x, we end up with the TDA998x providing a
> connector, because it has connector functionality, and providing a
> bridge.  The encoder is left to the KMS driver, which adds additional
> complexity (100+ lines) to each and every KMS driver, requiring the
> KMS driver to have much more knowledge of what's attached to the
> "CRTC", so it can create these encoders itself.  I still think this
> is a backwards step - maybe one step forwards, two backwards.

The majority of KMS drivers do end up creating their own encoders (i.e,
encoders are exclusive to the HW represented by the KMS driver, not an
external chip/IP that can be used by other KMS drivers).

The additional complexity is more for KMS drivers like armada-drm,
where the encoder HW isn't provided by the main display HW altogether.

 From the initial commits that added drm_bridge, it was mainly created
to piggy-back onto an existing drm_encoder. Later, some infrastructure
was added to create independent bridge drivers that could attach to
a KMS driver. What was completely missed out when implementing 
drm_bridge was the case where the KMS driver doesn't have a drm_encoder
at all. I feel it should be fixable with additional helpers, though.
If not, I think we still aren't too late to come up with some other
way of representing encoder chains, since there still aren't too many
bridge drivers and no presence of it in user space.

>
> Even if we were to provide helpers to create a dummy encoder to
> work around the DRM bridge model short-comings, much of the 100+
> lines is to do with working out whether or not we need to create an
> encoder, and parsing the information we need to create that in a way
> that existing DT doesn't break.
>
> Then there's the fact that the bridge approach breaks non-DT at the
> moment, because DRM bridge fundamentally requires DT.  This makes
> DRM bridge useless for architectures which aren't DT aware, such as
> x86.  So, converting drivers to be a DRM bridge effectively denies
> their use on other architectures.  See drm_bridge.c, and look for
> the references to bridge_list, noting that there are two: one which
> adds to the bridge list, and one under #ifdef CONFIG_OF which looks
> up a DT reference to a bridge.

Maybe for the non-DT case, we could add a field in drm_bridge that is
populated by dev->platform_data which tells the KMS driver which
encoder it needs to bind to.

Could you point me to what the dev->platform_data retrieved by
armada_drm_probe from a board file looks like? I couldn't find
anything when I grep-ed "armada-drm"/"armada-510-drm"

I do agree that it's a step backward that we now have to search for
a corresponding bridge, which we didn't have to do when the chip
was represented as an encoder.

>
> There's another issue with TDA998x - we now have audio support in
> TDA998x, and converting TDA998x to be a DRM bridge introduces some
> fundamental (and as yet unsolved) races between the ASoC code and
> the attachment of the DRM bridge to the DRM encoder, and the detachment
> when the DRM bridge/connector gets cleaned up.  Right now, there's no
> bridge callback when the encoder or drm_device goes away, so doing
> stuff like:
>
> static int tda998x_audio_get_eld(struct device *dev, void *data,
>                                  uint8_t *buf, size_t len)
> {
>         struct tda998x_priv *priv = dev_get_drvdata(dev);
>         struct drm_mode_config *config;
>         struct drm_connector *connector;
>         int ret = -ENODEV;
>
>         /* FIXME: This is racy */
>         if (!priv->bridge.encoder || !priv->bridge.encoder->dev)
>                 return ret;
>
>         config = &priv->bridge.encoder->dev->mode_config;
>
>         mutex_lock(&config->mutex);
>         list_for_each_entry(connector, &config->connector_list, head) {
>                 if (priv->bridge.encoder == connector->encoder) {
>                         memcpy(buf, connector->eld,
>                                min(sizeof(connector->eld), len));
>                         ret = 0;
>                 }
>         }
>         mutex_unlock(&config->mutex);
>
> feels very unsafe - nothing really guarantees the validity of the
> priv->bridge.encoder or priv->bridge.encoder->dev pointers.  The
> config->mutex lock does nothing to solve this.  The same problem
> also exists in tda998x_audio_hw_params().

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

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Mon, Oct 24, 2016 at 11:58:00AM +0530, Archit Taneja wrote:
> On 10/22/2016 03:25 PM, Russell King - ARM Linux wrote:
> > On Fri, Oct 21, 2016 at 09:04:39PM +0200, Jean-Francois Moine wrote:
> > > > > > > On Wed, Oct 19, 2016 at 12:19:30PM +0300, Laurent Pinchart wrote:
> > > 	(sorry, I lost your original mail)
> > > > > > > DRM bridges indeed don't create encoders. That task is left to the display
> > > > > > > driver. The reason is the same as above: bridges can be chained (including
> > > > > > > with an internal encoder that is not modelled as a bridge, and that's a case
> > > > > > > we have today), while the KMS model exposes a single encoder to userspace.
> > > > > > > Exposing DRM encoder objects as part of the KMS UABI was probably a mistake.
> > > > > > > Better solutions would have been to expose no encoder at all or all encoders
> > > > > > > in the chain. We are however stuck with this model as we can't break the UABI.
> > > > > > > For that reason a DRM encoder object doesn't represent an encoder but a chain
> > > > > > > of encoders. As a DRM bridge models a single encoder, the DRM encoder object
> > > > > > > must be created at a higher level, in the display driver.
> > > 
> > > I wonder why you created 'bridge's instead of simply adding links to
> > > the encoders? (that's what ASoC did: the audio CODECs are linked)
> > > This way, in simple cases (most cases), there would have been
> > > 	crtc -> (encoder -> connector)
> > > instead of
> > > 	crtc -> (bridge + encoder) -> (bridge + connector)
> > > without any changes in the actual (encoder + connector)s.
> > 
> > Looking at drm_bridge_disable() and drm_bridge_enable(), the control
> > model appears to be:
> > 
> > 	crtc -> encoder -> connector
> >                  `-> bridge
> > 		     `-> bridge
> > 		         `-> bridge
> > 
> > Bridges are always attached to an encoder, and there can be multiple
> > bridges attached to one encoder.  Bridges can't be attached to the
> > connector.

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

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

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

> > So, in the case of TDA998x, we end up with the TDA998x providing a
> > connector, because it has connector functionality, and providing a
> > bridge.  The encoder is left to the KMS driver, which adds additional
> > complexity (100+ lines) to each and every KMS driver, requiring the
> > KMS driver to have much more knowledge of what's attached to the
> > "CRTC", so it can create these encoders itself.  I still think this
> > is a backwards step - maybe one step forwards, two backwards.

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

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

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

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

> > Even if we were to provide helpers to create a dummy encoder to
> > work around the DRM bridge model short-comings, much of the 100+
> > lines is to do with working out whether or not we need to create an
> > encoder, and parsing the information we need to create that in a way
> > that existing DT doesn't break.
> > 
> > Then there's the fact that the bridge approach breaks non-DT at the
> > moment, because DRM bridge fundamentally requires DT.  This makes
> > DRM bridge useless for architectures which aren't DT aware, such as
> > x86.  So, converting drivers to be a DRM bridge effectively denies
> > their use on other architectures.  See drm_bridge.c, and look for
> > the references to bridge_list, noting that there are two: one which
> > adds to the bridge list, and one under #ifdef CONFIG_OF which looks
> > up a DT reference to a bridge.

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

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

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

> > There's another issue with TDA998x - we now have audio support in
> > TDA998x, and converting TDA998x to be a DRM bridge introduces some
> > fundamental (and as yet unsolved) races between the ASoC code and
> > the attachment of the DRM bridge to the DRM encoder, and the detachment
> > when the DRM bridge/connector gets cleaned up.  Right now, there's no
> > bridge callback when the encoder or drm_device goes away, so doing
> > stuff like:
> > 
> > static int tda998x_audio_get_eld(struct device *dev, void *data,
> >                                  uint8_t *buf, size_t len)
> > {
> >         struct tda998x_priv *priv = dev_get_drvdata(dev);
> >         struct drm_mode_config *config;
> >         struct drm_connector *connector;
> >         int ret = -ENODEV;
> > 
> >         /* FIXME: This is racy */
> >         if (!priv->bridge.encoder || !priv->bridge.encoder->dev)
> >                 return ret;
> > 
> >         config = &priv->bridge.encoder->dev->mode_config;
> > 
> >         mutex_lock(&config->mutex);
> >         list_for_each_entry(connector, &config->connector_list, head) {
> >                 if (priv->bridge.encoder == connector->encoder) {
> >                         memcpy(buf, connector->eld,
> >                                min(sizeof(connector->eld), len));
> >                         ret = 0;
> >                 }
> >         }
> >         mutex_unlock(&config->mutex);
> > 
> > feels very unsafe - nothing really guarantees the validity of the
> > priv->bridge.encoder or priv->bridge.encoder->dev pointers.  The
> > config->mutex lock does nothing to solve this.  The same problem
> > also exists in tda998x_audio_hw_params().
> 
> Maybe we could ensure that the bridge attachment/detachment is
> contained within drm_encoder_init/cleanup funcs, so that their
> life is tied to the encoder drm_mode_object. It wouldn't be as
> straightforward, since the drm_bridges create connectors too.
> Will look more into this.

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

I think fundamentally the issue here is that armada still has the midlayer
callbacks, and with those this kind of stuff isn't really fixable.
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Mon, Oct 24, 2016 at 08:53:04AM +0200, Daniel Vetter wrote:
> On Mon, Oct 24, 2016 at 11:58:00AM +0530, Archit Taneja wrote:
> > On 10/22/2016 03:25 PM, Russell King - ARM Linux wrote:
> > > Looking at drm_bridge_disable() and drm_bridge_enable(), the control
> > > model appears to be:
> > > 
> > > 	crtc -> encoder -> connector
> > >                  `-> bridge
> > > 		     `-> bridge
> > > 		         `-> bridge
> > > 
> > > Bridges are always attached to an encoder, and there can be multiple
> > > bridges attached to one encoder.  Bridges can't be attached to the
> > > connector.
> 
> In helpers connectors are no-op objects. We _never_ call any connector
> callbacks when doing a modeset. Connectors are only used to probe output
> state, and as the userspace-visisble endpoint representation. Hence the
> real graph is
> 
> crtc -> encoder [ -> bridge [ -> bridge [...]]] -> connector
> 
> with the last bridge owning the connector. And that last bridge probably
> needs to store a pointer to its connector(s).

That model can't work for TDA998x if the TDA998x is followed by
another "bridge" (eg, to convert the TDMS signals to something else)
unless there's some way to tell a bridge that it isn't the last in
the chain.

However, my graph is accurate as it's reflecting the software
modelling - it reflects how the various objects are bound together in
DRM.  The DRM encoder has pointers to the DRM bridge, which has a
pointer to the next DRM bridge.  The DRM connector doesn't have any
pointers to the connector, only to the DRM encoder.  So, DRM bridges
are childs of the encoder, and the encoder (and attached encoder
bridge chain) can be selected by the DRM connector.

However, you are correct that for different "tasks" like mode setting,
or output probing, the representation is somewhat different - that's
not really what I was talking about though, and I certainly was not
talking about the userspace representation.

What I'm 100% concerned about is how this stuff looks in kernel space
and what the driver(s) should look like.

> > > So, in the case of TDA998x, we end up with the TDA998x providing a
> > > connector, because it has connector functionality, and providing a
> > > bridge.  The encoder is left to the KMS driver, which adds additional
> > > complexity (100+ lines) to each and every KMS driver, requiring the
> > > KMS driver to have much more knowledge of what's attached to the
> > > "CRTC", so it can create these encoders itself.  I still think this
> > > is a backwards step - maybe one step forwards, two backwards.
> 
> We've stubbed out everything that's in an encoder, you definitely don't
> need hundreds of lines to write one any more. If there's still a callback
> left around drm_encoder which has not yet suitable default handling, then
> that's a bug.

Sorry, but I do need exactly what I've written above, I can talk rather
definitively because I've actually got the code in front of me.  Most of
the additional lines is due to the complexity added to the KMS driver to
locate (actually for a third time) all the components in the system,
specifically parsing the DT tree to find the "encoders" (or rather the
TDA998x in this case), creating the DRM encoder objects, and binding the
TDA998x bridge.

Here's the _exact_ diffstat for the hacky conversion so far (including
something like the 10 patches I posted last weekend, which haven't had
any comments yet):

 drivers/gpu/drm/armada/armada_drv.c | 125 +++++
 drivers/gpu/drm/i2c/tda998x_drv.c   | 904 +++++++++++++++++-------------------
 2 files changed, 560 insertions(+), 469 deletions(-)

The actual bridge conversion on its own is:

 drivers/gpu/drm/armada/armada_drv.c | 125 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i2c/tda998x_drv.c   | 139 ++++++++++++++----------------------
 2 files changed, 180 insertions(+), 84 deletions(-)

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

That's not going to happen - and you know exactly why that's not going
to happen - I've tried to do it and it failed misterably with all sorts
of problems.  The idea that it can be done piecemeal as per your guide
is a falacy - it can't be.  There is no progressive way to do a
conversion.  It seems that KMS drivers need to be rewritten from
scratch, and that means there is a high risk of introducing lots of
new bugs.

I'm just not prepared to go through that - I'd much rather have a stable
kernel driver that actually works than spend the next six months rewriting
and debugging stuff just for the latest ideas about how stuff should be
done.

_OR_ there could be more help from DRM to ease the transition pain from
non-atomic to atomic KMS drivers, so that it can be done in appropriately
sized steps, so that the driver can be adequately tested to ensure that
things don't totally fall apart... you know, like imx-drm has gone from
being a stable driver to keep on falling apart now that it's been
converted to atomic modeset.

> Imo encoders should be that part which is baked into your core ip. If
> there's nothing, then you're perfectly fine with a no-op encoder.

From my point of view, the TDA998x _is_ an encoder - it takes RGB and
sync signals, and encodes them into the TDMS format for DVI or HDMI.
I guess what I call an encoder is not what DRM calls an encoder though.

What's in the Dove is effectively a pair of CRTCs, some muxes, a set of
VGA DACs and a parallel RGB bus with pixel clock and sync signals.
Apart from the VGA DACs (which aren't used in the TDA998x path) it's
pretty hard to imagine what piece of hardware could be called an
encoder.

So what does the DRM encoder represent, hardware wise in this case?
As I say, in my mind, the TDA998x _is_ the encoder.

> Maybe we
> could do a helper for creating those, if the few lines are copypasted too
> often. Then all the external IP should be bridges (and chained). And with
> chains either you need another bridge, or you're the last bridge, and then
> you're supposed to register the connector as the final endpoint.

Let me repeat: the "DRM connector" is part of the TDA998x - the TDA998x
provides the EDID reading capabilities, and the connection detection
capabilities.  It also provides the CEC communication capabilities as
well, but that's not too relevant to this discussion, apart from
illustrating that it's an all-in-one single chip solution to providing
a full HDMI source implementation.

The TDA998x is not a stand-alone "bridge" which just _encodes_ a parallel
RGB bus to TDMS signals, it's much more than that.  That's why I'm saying
we can't separate out the connector functionality from the encoder
functionality.

> > I do agree that it's a step backward that we now have to search for
> > a corresponding bridge, which we didn't have to do when the chip
> > was represented as an encoder.
> 
> You can still do the exact same thing with bridges as with encoders using
> the component framework. Should not be a step back at all.

Sorry, no you can't at the moment.  As I've already said, grep for
"bridge_list".  Read the code in drivers/gpu/drm/drm_bridge.c, and
notice that there's two places that this list is accessed:

1. inside drm_bridge_add()
2. inside of_drm_find_bridge() which is only available when CONFIG_OF
   is enabled, and requires a DT struct device_node pointer to perform
   the lookup.  struct device_node's do not exist without DT.

> > > There's another issue with TDA998x - we now have audio support in
> > > TDA998x, and converting TDA998x to be a DRM bridge introduces some
> > > fundamental (and as yet unsolved) races between the ASoC code and
> > > the attachment of the DRM bridge to the DRM encoder, and the detachment
> > > when the DRM bridge/connector gets cleaned up.  Right now, there's no
> > > bridge callback when the encoder or drm_device goes away, so doing
> > > stuff like:
> > > 
> > > static int tda998x_audio_get_eld(struct device *dev, void *data,
> > >                                  uint8_t *buf, size_t len)
> > > {
> > >         struct tda998x_priv *priv = dev_get_drvdata(dev);
> > >         struct drm_mode_config *config;
> > >         struct drm_connector *connector;
> > >         int ret = -ENODEV;
> > > 
> > >         /* FIXME: This is racy */
> > >         if (!priv->bridge.encoder || !priv->bridge.encoder->dev)
> > >                 return ret;
> > > 
> > >         config = &priv->bridge.encoder->dev->mode_config;
> > > 
> > >         mutex_lock(&config->mutex);
> > >         list_for_each_entry(connector, &config->connector_list, head) {
> > >                 if (priv->bridge.encoder == connector->encoder) {
> > >                         memcpy(buf, connector->eld,
> > >                                min(sizeof(connector->eld), len));
> > >                         ret = 0;
> > >                 }
> > >         }
> > >         mutex_unlock(&config->mutex);
> > > 
> > > feels very unsafe - nothing really guarantees the validity of the
> > > priv->bridge.encoder or priv->bridge.encoder->dev pointers.  The
> > > config->mutex lock does nothing to solve this.  The same problem
> > > also exists in tda998x_audio_hw_params().
> > 
> > Maybe we could ensure that the bridge attachment/detachment is
> > contained within drm_encoder_init/cleanup funcs, so that their
> > life is tied to the encoder drm_mode_object. It wouldn't be as
> > straightforward, since the drm_bridges create connectors too.
> > Will look more into this.
> 
> I don't see any issue with the above at all. Or well, if there is one
> there's a larger issue: You can't reach this code if you unregister your
> driver's interface _before_ you tear down anything. This is fixed by
> getting rid of the load/unload callbacks. And for additional interfaces
> there's new register/unregister callbacks on connectors (which the bridge
> also should own).

That's easy to say if you're into the "lets rewrite everything all at
the same time" mentality, which from your response I think sums up
your position on everything from atomic mode set to this problem.

Sorry, I really hate the rewrite mentality, that's not good programming
practice, especially when existing implementations work.  What's
instead required are a series of incremental steps to effect the
full outcome, especially when multiple drivers are involved.

If you look at the problems surrounding the removal of the
drm_connector_register() from TDA998x, you'll see why this is important:
it's not the drivers _with_ the mid-layer that's a problem here, but
those which were converted prematurely, or written without using the
mid-layer that are blocking the removal of drm_connector_register().

And the removal of drm_connector_register() from TDA998x blocks the
removal of the mid-layer from armada, because removing the mid-layer
_now_ causes the kernel to WARN - I know, I've tried it already:

[    1.933854] WARNING: CPU: 0 PID: 13 at /home/rmk/git/linux-cubox/lib/kobject.c:244 kobject_add_internal+0xfc/0x2d8
[    1.944286] kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0)

But... the mid-layer issue you raise is a complete red herring, the
race has absolutely nothing to do with that.

What causes the race is that during the KMS driver's probing, we get
to the point where tda998x_bind() is called.  This registers the
DRM bridge so that the KMS driver can later find and attach to the
bridge.

However, just before creating the DRM bridge, it also creates a
platform device for the audio codec side.  As soon as that platform
device is registered, ASoC is free to bind the audio subsystem and
make it available to userspace.

This means that any of the tda998x_audio_* functions are able to
be called from the point that this platform device is registered.

At this point, priv->bridge.encoder will be NULL, which means that
some of the tda998x_audio_* functions should fail due to that.

KMS driver initialisation can continue, writing the various pointers,
and that happens without locking - but at that stage, it should only
be going from NULL pointers to non-NULL pointers pointing at valid
memory.  However, there are no barriers to ensure that the various
writes occur in the expected order (we're talking about writes in the
KMS driver being visible to reads in the TDA998x audio side, possibly
by another CPU, so locking isn't the answer - I can't see any way such
a lock could be shared between TDA998x and various KMS drivers, or
even some generic dummy DRM encoder helper.  Barriers may be the
answer, we need to ensure that encoder->dev is always valid before
bridge->encoder is valid.)

However, we need to also consider the initialisation failure and
error clean up paths, assuming we have got this far - and that's
where the worry is.  drm_encoder_cleanup() memsets the entire
encoder to zero.  So, from the above, a compiler is perfectly at
liberty to re-read the priv->bridge.encoder->dev pointer between
these two statements:

	if (!priv->bridge.encoder || !priv->bridge.encoder->dev)
		return ret;

	config = &priv->bridge.encoder->dev->mode_config;

and if such a re-read co-incides with the memset() in
drm_encoder_cleanup() becoming visible, this is a possible oops
waiting to happen.

It gets worse if the KMS driver is responsible for freeing the DRM
encoder that it created to attach to the TDA998x - if it frees that
memory before tda998x_unbind() has been called, the audio subsystem
will still be visible to userspace, and creates a potential
use-after-free.

So, none of this has anything what so ever to do with "is the KMS
driver mid-layered or not" - this problem can exist irrespective of
whether I have armada mid-layered or de-mid-layered.

NB. It doesn't actually exist with armada, because armada is not used
with the audio stuff on the cubox, we feed SPDIF to the TDA998x and
let the TDA998x sort itself out, no audio codec is required there,
but the point is that the complexities here are spread between
TDA998x and associated KMS drivers - both have to be doing the
right things for there not to be any subtle bugs here, and that is
a really bad model.  As I've already said, the problem does not
exist as the driver stands in mainline today, only once it is
converted to drm bridge, and it's purely down to the way the bridge
code works.  It is solvable, provided the connector remains part of
TDA998x.

So, like everything, we need to go through a series of steps to make
these changes, and these steps need to happen in the right order,
not as one huge great big lets-change-everything-at-once kind of
approach.

It's either going to take time, feeding changes into the kernel slowly,
or it's going to need a lot of co-operation between different device
driver authors, and sharing of stable commits between different git
trees.

Right now, the drm_connector_register() thing is basically blocking
everything, and that needs to be handled in a way that's acceptable to
all parties.  The drm bridge conversion is something that can only
happen once all the ducks are properly aligned - iow,
drm_connector_register() gone, audio problems solved (eg, via the
10 patch series) and we have a way to convert TDA998x to a bridge
without requiring every KMS user of TDA998x to simultaneously grow
its own drm encoders.

-- 
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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Mon, Oct 31, 2016 at 12:09:23AM +0000, Russell King - ARM Linux wrote:
> On Mon, Oct 24, 2016 at 08:53:04AM +0200, Daniel Vetter wrote:
> > On Mon, Oct 24, 2016 at 11:58:00AM +0530, Archit Taneja wrote:
> > > On 10/22/2016 03:25 PM, Russell King - ARM Linux wrote:
> > > > Looking at drm_bridge_disable() and drm_bridge_enable(), the control
> > > > model appears to be:
> > > > 
> > > > 	crtc -> encoder -> connector
> > > >                  `-> bridge
> > > > 		     `-> bridge
> > > > 		         `-> bridge
> > > > 
> > > > Bridges are always attached to an encoder, and there can be multiple
> > > > bridges attached to one encoder.  Bridges can't be attached to the
> > > > connector.
> > 
> > In helpers connectors are no-op objects. We _never_ call any connector
> > callbacks when doing a modeset. Connectors are only used to probe output
> > state, and as the userspace-visisble endpoint representation. Hence the
> > real graph is
> > 
> > crtc -> encoder [ -> bridge [ -> bridge [...]]] -> connector
> > 
> > with the last bridge owning the connector. And that last bridge probably
> > needs to store a pointer to its connector(s).
> 
> That model can't work for TDA998x if the TDA998x is followed by
> another "bridge" (eg, to convert the TDMS signals to something else)
> unless there's some way to tell a bridge that it isn't the last in
> the chain.
> 
> However, my graph is accurate as it's reflecting the software
> modelling - it reflects how the various objects are bound together in
> DRM.  The DRM encoder has pointers to the DRM bridge, which has a
> pointer to the next DRM bridge.  The DRM connector doesn't have any
> pointers to the connector, only to the DRM encoder.  So, DRM bridges
> are childs of the encoder, and the encoder (and attached encoder
> bridge chain) can be selected by the DRM connector.

Small note: The connector -> encoder pointer is only used for legacy
modesetting drivers. In atomic we shoveled it into drm_connector_state as
as derived state of the connector->crtc link (which is what setCrtc and
atomic ioctl set).

> However, you are correct that for different "tasks" like mode setting,
> or output probing, the representation is somewhat different - that's
> not really what I was talking about though, and I certainly was not
> talking about the userspace representation.
> 
> What I'm 100% concerned about is how this stuff looks in kernel space
> and what the driver(s) should look like.

Ah, I missed that. Some shared code and pointers in generic drivers to
untangle which exact drm_bridge owns the connector would certainly be
useful. Otoh I'm not aware of any real-world chaining existing yet, I
guess that's why this is unsolved.
 
> > > > So, in the case of TDA998x, we end up with the TDA998x providing a
> > > > connector, because it has connector functionality, and providing a
> > > > bridge.  The encoder is left to the KMS driver, which adds additional
> > > > complexity (100+ lines) to each and every KMS driver, requiring the
> > > > KMS driver to have much more knowledge of what's attached to the
> > > > "CRTC", so it can create these encoders itself.  I still think this
> > > > is a backwards step - maybe one step forwards, two backwards.
> > 
> > We've stubbed out everything that's in an encoder, you definitely don't
> > need hundreds of lines to write one any more. If there's still a callback
> > left around drm_encoder which has not yet suitable default handling, then
> > that's a bug.
> 
> Sorry, but I do need exactly what I've written above, I can talk rather
> definitively because I've actually got the code in front of me.  Most of
> the additional lines is due to the complexity added to the KMS driver to
> locate (actually for a third time) all the components in the system,
> specifically parsing the DT tree to find the "encoders" (or rather the
> TDA998x in this case), creating the DRM encoder objects, and binding the
> TDA998x bridge.
> 
> Here's the _exact_ diffstat for the hacky conversion so far (including
> something like the 10 patches I posted last weekend, which haven't had
> any comments yet):
> 
>  drivers/gpu/drm/armada/armada_drv.c | 125 +++++
>  drivers/gpu/drm/i2c/tda998x_drv.c   | 904 +++++++++++++++++-------------------
>  2 files changed, 560 insertions(+), 469 deletions(-)
> 
> The actual bridge conversion on its own is:
> 
>  drivers/gpu/drm/armada/armada_drv.c | 125 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i2c/tda998x_drv.c   | 139 ++++++++++++++----------------------
>  2 files changed, 180 insertions(+), 84 deletions(-)

Hm, this doesn't look good indeed ...

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

... but I can't justify the effort really in making non-atomic drivers
look good personally. Not going to reject patches from others of course.

> That's not going to happen - and you know exactly why that's not going
> to happen - I've tried to do it and it failed misterably with all sorts
> of problems.  The idea that it can be done piecemeal as per your guide
> is a falacy - it can't be.  There is no progressive way to do a
> conversion.  It seems that KMS drivers need to be rewritten from
> scratch, and that means there is a high risk of introducing lots of
> new bugs.
> 
> I'm just not prepared to go through that - I'd much rather have a stable
> kernel driver that actually works than spend the next six months rewriting
> and debugging stuff just for the latest ideas about how stuff should be
> done.

gpus unfortunately change current idea of how stuff should work every few
years. It's part of the job ...

> _OR_ there could be more help from DRM to ease the transition pain from
> non-atomic to atomic KMS drivers, so that it can be done in appropriately
> sized steps, so that the driver can be adequately tested to ensure that
> things don't totally fall apart... you know, like imx-drm has gone from
> being a stable driver to keep on falling apart now that it's been
> converted to atomic modeset.

drm changes fast and I'm not entirely surprised that the conversion guide
isn't fully up-to-date any more. I can help with debugging issues
(preferrably on irc), but I can't magically fix bugs I'm not aware of.

But in the end it's your call to either convert to atomic, refactor the
core/helpers to need less dummy code for old modeset code or just carry
some dummy code around in your driver. Atomic is definitely the way to go
(since stuff like Android outright requires it to be useable).
 
> > Imo encoders should be that part which is baked into your core ip. If
> > there's nothing, then you're perfectly fine with a no-op encoder.
> 
> From my point of view, the TDA998x _is_ an encoder - it takes RGB and
> sync signals, and encodes them into the TDMS format for DVI or HDMI.
> I guess what I call an encoder is not what DRM calls an encoder though.
> 
> What's in the Dove is effectively a pair of CRTCs, some muxes, a set of
> VGA DACs and a parallel RGB bus with pixel clock and sync signals.
> Apart from the VGA DACs (which aren't used in the TDA998x path) it's
> pretty hard to imagine what piece of hardware could be called an
> encoder.
> 
> So what does the DRM encoder represent, hardware wise in this case?
> As I say, in my mind, the TDA998x _is_ the encoder.

Rule of thumb: If the encoder is created as an integrated part of the
overall display IP block, by the same IP company, it's probably best
represented by a drm_encoder. If otoh it's an external IP block, reused in
a bunch of places, it should be a drm_bridge. Think
s/drm_bridge/drm_non_integrated_encoder/ or similar. It would of course be
neat if the drm_encoder could be entirely no-op'ed out in that case, but
because drm_encoders are also part of the uabi (imo a design mistake) you
need to carry a dummy one around.

Other guideline: The split between drm_crtc and drm_encoder should
represent the display pipeline to outputs cross-bar (if you have one),
since that's how helpers handle different outputs. For some chips there's
a bit of generic per-output stuff, and hence it makes sense to both have
encoder code and a separate drm_bridge.
 
> > Maybe we
> > could do a helper for creating those, if the few lines are copypasted too
> > often. Then all the external IP should be bridges (and chained). And with
> > chains either you need another bridge, or you're the last bridge, and then
> > you're supposed to register the connector as the final endpoint.
> 
> Let me repeat: the "DRM connector" is part of the TDA998x - the TDA998x
> provides the EDID reading capabilities, and the connection detection
> capabilities.  It also provides the CEC communication capabilities as
> well, but that's not too relevant to this discussion, apart from
> illustrating that it's an all-in-one single chip solution to providing
> a full HDMI source implementation.
> 
> The TDA998x is not a stand-alone "bridge" which just _encodes_ a parallel
> RGB bus to TDMS signals, it's much more than that.  That's why I'm saying
> we can't separate out the connector functionality from the encoder
> functionality.

drm_bridge is meant to contain the connector, design-wise. Agreed that the
code and helpers leaves a few things to be desired in this area.

> > > I do agree that it's a step backward that we now have to search for
> > > a corresponding bridge, which we didn't have to do when the chip
> > > was represented as an encoder.
> > 
> > You can still do the exact same thing with bridges as with encoders using
> > the component framework. Should not be a step back at all.
> 
> Sorry, no you can't at the moment.  As I've already said, grep for
> "bridge_list".  Read the code in drivers/gpu/drm/drm_bridge.c, and
> notice that there's two places that this list is accessed:
> 
> 1. inside drm_bridge_add()
> 2. inside of_drm_find_bridge() which is only available when CONFIG_OF
>    is enabled, and requires a DT struct device_node pointer to perform
>    the lookup.  struct device_node's do not exist without DT.

Well, then it needs to be added. It's open-source after all ;-)

> > > > There's another issue with TDA998x - we now have audio support in
> > > > TDA998x, and converting TDA998x to be a DRM bridge introduces some
> > > > fundamental (and as yet unsolved) races between the ASoC code and
> > > > the attachment of the DRM bridge to the DRM encoder, and the detachment
> > > > when the DRM bridge/connector gets cleaned up.  Right now, there's no
> > > > bridge callback when the encoder or drm_device goes away, so doing
> > > > stuff like:
> > > > 
> > > > static int tda998x_audio_get_eld(struct device *dev, void *data,
> > > >                                  uint8_t *buf, size_t len)
> > > > {
> > > >         struct tda998x_priv *priv = dev_get_drvdata(dev);
> > > >         struct drm_mode_config *config;
> > > >         struct drm_connector *connector;
> > > >         int ret = -ENODEV;
> > > > 
> > > >         /* FIXME: This is racy */
> > > >         if (!priv->bridge.encoder || !priv->bridge.encoder->dev)
> > > >                 return ret;
> > > > 
> > > >         config = &priv->bridge.encoder->dev->mode_config;
> > > > 
> > > >         mutex_lock(&config->mutex);
> > > >         list_for_each_entry(connector, &config->connector_list, head) {
> > > >                 if (priv->bridge.encoder == connector->encoder) {
> > > >                         memcpy(buf, connector->eld,
> > > >                                min(sizeof(connector->eld), len));
> > > >                         ret = 0;
> > > >                 }
> > > >         }
> > > >         mutex_unlock(&config->mutex);
> > > > 
> > > > feels very unsafe - nothing really guarantees the validity of the
> > > > priv->bridge.encoder or priv->bridge.encoder->dev pointers.  The
> > > > config->mutex lock does nothing to solve this.  The same problem
> > > > also exists in tda998x_audio_hw_params().
> > > 
> > > Maybe we could ensure that the bridge attachment/detachment is
> > > contained within drm_encoder_init/cleanup funcs, so that their
> > > life is tied to the encoder drm_mode_object. It wouldn't be as
> > > straightforward, since the drm_bridges create connectors too.
> > > Will look more into this.
> > 
> > I don't see any issue with the above at all. Or well, if there is one
> > there's a larger issue: You can't reach this code if you unregister your
> > driver's interface _before_ you tear down anything. This is fixed by
> > getting rid of the load/unload callbacks. And for additional interfaces
> > there's new register/unregister callbacks on connectors (which the bridge
> > also should own).
> 
> That's easy to say if you're into the "lets rewrite everything all at
> the same time" mentality, which from your response I think sums up
> your position on everything from atomic mode set to this problem.
> 
> Sorry, I really hate the rewrite mentality, that's not good programming
> practice, especially when existing implementations work.  What's
> instead required are a series of incremental steps to effect the
> full outcome, especially when multiple drivers are involved.

We've been "rewriting" i915 to be atomic in small steps for 2 years now.
It works.

> If you look at the problems surrounding the removal of the
> drm_connector_register() from TDA998x, you'll see why this is important:
> it's not the drivers _with_ the mid-layer that's a problem here, but
> those which were converted prematurely, or written without using the
> mid-layer that are blocking the removal of drm_connector_register().
> 
> And the removal of drm_connector_register() from TDA998x blocks the
> removal of the mid-layer from armada, because removing the mid-layer
> _now_ causes the kernel to WARN - I know, I've tried it already:
> 
> [    1.933854] WARNING: CPU: 0 PID: 13 at /home/rmk/git/linux-cubox/lib/kobject.c:244 kobject_add_internal+0xfc/0x2d8
> [    1.944286] kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0)
> 
> But... the mid-layer issue you raise is a complete red herring, the
> race has absolutely nothing to do with that.
> 
> What causes the race is that during the KMS driver's probing, we get
> to the point where tda998x_bind() is called.  This registers the
> DRM bridge so that the KMS driver can later find and attach to the
> bridge.
> 
> However, just before creating the DRM bridge, it also creates a
> platform device for the audio codec side.  As soon as that platform
> device is registered, ASoC is free to bind the audio subsystem and
> make it available to userspace.
> 
> This means that any of the tda998x_audio_* functions are able to
> be called from the point that this platform device is registered.
> 
> At this point, priv->bridge.encoder will be NULL, which means that
> some of the tda998x_audio_* functions should fail due to that.
> 
> KMS driver initialisation can continue, writing the various pointers,
> and that happens without locking - but at that stage, it should only
> be going from NULL pointers to non-NULL pointers pointing at valid
> memory.  However, there are no barriers to ensure that the various
> writes occur in the expected order (we're talking about writes in the
> KMS driver being visible to reads in the TDA998x audio side, possibly
> by another CPU, so locking isn't the answer - I can't see any way such
> a lock could be shared between TDA998x and various KMS drivers, or
> even some generic dummy DRM encoder helper.  Barriers may be the
> answer, we need to ensure that encoder->dev is always valid before
> bridge->encoder is valid.)
> 
> However, we need to also consider the initialisation failure and
> error clean up paths, assuming we have got this far - and that's
> where the worry is.  drm_encoder_cleanup() memsets the entire
> encoder to zero.  So, from the above, a compiler is perfectly at
> liberty to re-read the priv->bridge.encoder->dev pointer between
> these two statements:
> 
> 	if (!priv->bridge.encoder || !priv->bridge.encoder->dev)
> 		return ret;
> 
> 	config = &priv->bridge.encoder->dev->mode_config;
> 
> and if such a re-read co-incides with the memset() in
> drm_encoder_cleanup() becoming visible, this is a possible oops
> waiting to happen.
> 
> It gets worse if the KMS driver is responsible for freeing the DRM
> encoder that it created to attach to the TDA998x - if it frees that
> memory before tda998x_unbind() has been called, the audio subsystem
> will still be visible to userspace, and creates a potential
> use-after-free.
> 
> So, none of this has anything what so ever to do with "is the KMS
> driver mid-layered or not" - this problem can exist irrespective of
> whether I have armada mid-layered or de-mid-layered.

Not entirely clear to me from your description, but I think if the audio
platform device registration and unregistration is put into the new
connector register_late/unregister_early callbacks, and if the load/unload
sequence is fixed to register everything as the last step/unregister as
the first, then this should be fixed. And if the bridge owns the
connector, it can set these callbacks.

If it's not fixed then I need to take another look at your code, because
fixing these kind of issues was exactly the goal with the load/unload
reorg. We have a very similar problem in i915 on connectors, but with the
backlight interfaces.

> NB. It doesn't actually exist with armada, because armada is not used
> with the audio stuff on the cubox, we feed SPDIF to the TDA998x and
> let the TDA998x sort itself out, no audio codec is required there,
> but the point is that the complexities here are spread between
> TDA998x and associated KMS drivers - both have to be doing the
> right things for there not to be any subtle bugs here, and that is
> a really bad model.  As I've already said, the problem does not
> exist as the driver stands in mainline today, only once it is
> converted to drm bridge, and it's purely down to the way the bridge
> code works.  It is solvable, provided the connector remains part of
> TDA998x.
> 
> So, like everything, we need to go through a series of steps to make
> these changes, and these steps need to happen in the right order,
> not as one huge great big lets-change-everything-at-once kind of
> approach.
> 
> It's either going to take time, feeding changes into the kernel slowly,
> or it's going to need a lot of co-operation between different device
> driver authors, and sharing of stable commits between different git
> trees.
> 
> Right now, the drm_connector_register() thing is basically blocking
> everything, and that needs to be handled in a way that's acceptable to
> all parties.  The drm bridge conversion is something that can only
> happen once all the ducks are properly aligned - iow,
> drm_connector_register() gone, audio problems solved (eg, via the
> 10 patch series) and we have a way to convert TDA998x to a bridge
> without requiring every KMS user of TDA998x to simultaneously grow
> its own drm encoders.

tbh I think I'm lost in all the actual conversion issues at hand here. I
jumped into the discussion since there seemed to be some confusion going
on at higher levels.

Aside: This should be all documented in kernel-doc somewhere. If not
please raise this, I'll try to improve the docs - (rfc) doc patches very
much welcome of course too.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Thu, Oct 20, 2016 at 11:20:05AM +0300, Laurent Pinchart wrote:
> Hi Russell,
> 
> On Wednesday 19 Oct 2016 10:35:21 Russell King - ARM Linux wrote:
> > Moreover, as I've already said, converting tda998x to a DRM bridge
> > will not get rid of the encoder/connector part, because it _is_ a
> > connector in the DRM sense - it provides the detection and EDID
> > reading.
> > 
> > So, what would we achieve by splitting the driver into a DRM bridge
> > and DRM encoder/connector?
> 
> Please note that DRM bridge doesn't split the DRM connector out of the bridge, 
> bridges instantiate drm_connector objects. In that sense they don't differ 
> much from the model implemented by the tda998x driver.

So, we what you're saying is that we from a component based DRM encoder
plus DRM connector to a component based DRM bridge plus DRM connector
and some nebulous DRM encoder provider.

How does the DRM encoder get associated with the DRM connector?  DRM
requires the presence of at least one DRM encoder for each DRM connector,
and the DRM connector needs to provide a .best_encoder method to pick
the appropriate DRM encoder.

If (as you said elsewhere in your message) that the display driver is
responsible for providing the DRM encoder in your view, how does a
bridge DRM connector get to that DRM encoder, and how does it know
which DRM encoder to pick?

> I however believe that connectors should be split out DRM bridge drivers, for 
> the simple reason that bridges can be chained. The output of a bridge isn't 
> guaranteed to be a connector but can be another bridge.

You've not said how that could be achieved with TDA998x, so I'm still
opposed to the idea.  Remember, you're the one asking for this, you
must have a view on how to achieve it if you want it to happen.

> > We can see this with what happened to the DW-HDMI driver - that still
> > needs the component helper, and it provides a DRM bridge, DRM encoder
> > and DRM connector parts.
> 
> To be precise, the DW-HDMI driver core doesn't create a DRM encoder, it's the 
> glue layers implemented as part of the Rockchip and iMX display drivers that 
> do. Nonetheless, that's a mistake, the encoder should be created by the 
> display drivers.

If we're being precise, the "glue layer" creates the DRM encoder, but
the bridge code is responsible for binding itself to the DRM encoder,
and binding the DRM encoder to its associated DRM connector.

Let's assume it's a mistake.  Please illustrate how you would solve
this mistake.

> > The only reason it made sense to split the DW-HDMI driver was due to the
> > differences between the Rockchip and Freescale implementations.  Such
> > differences do not exist for TDA998x. So even here, your idea that "DRM
> > bridge" vs "non-DRM bridge component based" doesn't work - we have "DRM
> > bridge component based" because of the problem that I'm illustrating here.
> > 
> > So, again, I ask: what's the point of needlessly splitting the code
> > between an encoder/connector and a bridge?
> 
> We need to standardize on one model. I don't care much about how the end 
> result is named, as long as it fulfils the task at hand.

I don't care about the name either, that's not what I'm asking here.

All I seem to be getting is some hand-waving "we want one model" and
"the current situation is a huge mess" (which is not a sentiment I agree
with) but with no technical response about how to achieve it.  Please do
the technical response bits and stop hand-waving.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

Before removing this, we need to refactor (at least) mali-dp and hdlcd
to move drm_dev_register() to the end of their ->bind() callback.

That could be done without moving drm_dev_unregister() to the start
of ->unbind() if you really want to nuke the drm_connector_register()
call, but to maintain symmetry (and introduce correctness) I was
putting it off until I had a chance to remove drm_vblank_cleanup()
from drm_dev_unregister() (because [2]).

Thanks,
Brian

[1] https://lists.freedesktop.org/archives/dri-devel/2016-September/119038.html
[2] https://lists.freedesktop.org/archives/dri-devel/2016-September/119268.html

On Wed, Oct 19, 2016 at 12:33:54AM +0300, Jyri Sarha wrote:
>Remove obsolete drm_connector_register() call from tda998x_bind(). All
>connectors are registered when drm_dev_register() is called by the
>master drm_device driver.
>
>Signed-off-by: Jyri Sarha <jsarha@ti.com>
>---
> drivers/gpu/drm/i2c/tda998x_drv.c | 6 ------
> 1 file changed, 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
>index 9798d40..265f854 100644
>--- a/drivers/gpu/drm/i2c/tda998x_drv.c
>+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>@@ -1656,16 +1656,10 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
> 	if (ret)
> 		goto err_connector;
>
>-	ret = drm_connector_register(&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:
>-- 
>1.9.1
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Russell King - ARM Linux]

On Wed, Oct 19, 2016 at 10:46:48AM +0100, Brian Starkey wrote:
> Hi Jyri,
> 
> I believe this will break mali-dp and hdlcd, unless something changed
> while I wasn't looking. Please see this previous thread where I did
> the same thing and then had to have it reverted: [1]
> 
> Before removing this, we need to refactor (at least) mali-dp and hdlcd
> to move drm_dev_register() to the end of their ->bind() callback.
> 
> That could be done without moving drm_dev_unregister() to the start
> of ->unbind() if you really want to nuke the drm_connector_register()
> call, but to maintain symmetry (and introduce correctness) I was
> putting it off until I had a chance to remove drm_vblank_cleanup()
> from drm_dev_unregister() (because [2]).

So what is the status of this - when is it going to happen?  Without
this happening, I can't de-midlayer armada-drm, and I can't apply
these TDA998x patches.

As armada-drm stands at the moment, it can cope with the TDA998x
driver having the drm_connector_register(), or with it eliminated.
When armada-drm is de-midlayered without changing TDA998x, the
drm_connector_register() call in TDA998x produces a warning:

WARNING: CPU: 0 PID: 13 at lib/kobject.c:244 kobject_add_internal+0xfc/0x2d8
kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0)

I suspect that you will end up with the same problem when you move
the drm_dev_register() call after component_bind_all() - and I think
the answer is... you shouldn't have de-midlayered until TDA998x had
been updated to cope with de-midlayering, iow having had the
drm_connector_register() call removed.

Given that, I don't think we can avoid breaking mali-dp and hdlcd,
except by combining the change into a single patch, changing all three
drivers simultaneously (and any other driver which uses TDA998x which
has also been de-midlayered.)

So, what I would like to see is a single patch against Linus' 4.8.0
commit fixing mali-dp, hdlcd and any other driver, together with
removing drm_connector_register() from TDA998x.  This is so the patch
can be shared between all interested parties without forcing everyone
to 4.9-rc1.  Looking at the diff between 4.8 and 4.9-rc1 for
drivers/gpu/drm/arm, that shouldn't result in any merge conflicts -
and if you want to follow on from that with 4.9-rc1 development, you
can always merge 4.9-rc1 on top of that commit.

I'm happy to take such a patch and publish it via my git tree as part
of the TDA998x development if that helps - but either way we need it
shared between all parties.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Brian Starkey]

Hi Russell,

On Sat, Oct 22, 2016 at 02:40:22PM +0100, Russell King - ARM Linux wrote:
>On Wed, Oct 19, 2016 at 10:46:48AM +0100, Brian Starkey wrote:
>> Hi Jyri,
>>
>> I believe this will break mali-dp and hdlcd, unless something changed
>> while I wasn't looking. Please see this previous thread where I did
>> the same thing and then had to have it reverted: [1]
>>
>> Before removing this, we need to refactor (at least) mali-dp and hdlcd
>> to move drm_dev_register() to the end of their ->bind() callback.
>>
>> That could be done without moving drm_dev_unregister() to the start
>> of ->unbind() if you really want to nuke the drm_connector_register()
>> call, but to maintain symmetry (and introduce correctness) I was
>> putting it off until I had a chance to remove drm_vblank_cleanup()
>> from drm_dev_unregister() (because [2]).
>
>So what is the status of this - when is it going to happen?  Without
>this happening, I can't de-midlayer armada-drm, and I can't apply
>these TDA998x patches.
>
>As armada-drm stands at the moment, it can cope with the TDA998x
>driver having the drm_connector_register(), or with it eliminated.
>When armada-drm is de-midlayered without changing TDA998x, the
>drm_connector_register() call in TDA998x produces a warning:
>
>WARNING: CPU: 0 PID: 13 at lib/kobject.c:244 kobject_add_internal+0xfc/0x2d8
>kobject_add_internal failed for card0-HDMI-A-1 (error: -2 parent: card0)
>
>I suspect that you will end up with the same problem when you move
>the drm_dev_register() call after component_bind_all() - and I think
>the answer is... you shouldn't have de-midlayered until TDA998x had
>been updated to cope with de-midlayering, iow having had the
>drm_connector_register() call removed.
>

Well technically neither hdlcd or mali-dp ever had a midlayer, but yes
it would have been nice to have never introduced this problem in the
first place, I agree.

>Given that, I don't think we can avoid breaking mali-dp and hdlcd,
>except by combining the change into a single patch, changing all three
>drivers simultaneously (and any other driver which uses TDA998x which
>has also been de-midlayered.)
>

There is a way - we can explicitly register the connectors in hdlcd
and mali-dp (drm_connector_register_all used to be exposed for that
job, afaik to work around the kind of issue we face here).

But, that would introduce some extra churn, so probably a single patch
for all three works just fine.

I couldn't spot any other drivers I think will be affected. tilcdc
isn't de-midlayered.

>So, what I would like to see is a single patch against Linus' 4.8.0
>commit fixing mali-dp, hdlcd and any other driver, together with
>removing drm_connector_register() from TDA998x.  This is so the patch
>can be shared between all interested parties without forcing everyone
>to 4.9-rc1.  Looking at the diff between 4.8 and 4.9-rc1 for
>drivers/gpu/drm/arm, that shouldn't result in any merge conflicts -
>and if you want to follow on from that with 4.9-rc1 development, you
>can always merge 4.9-rc1 on top of that commit.
>
>I'm happy to take such a patch and publish it via my git tree as part
>of the TDA998x development if that helps - but either way we need it
>shared between all parties.

OK, I will follow up to this email with that patch. I note that it
will conflict with the series you sent out yesterday (23rd October).

Hopefully that's an agreeable solution for you, and everyone else.

Thanks,
-Brian

>
>-- 
>RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
>FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
>according to speedtest.net.
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[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 <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 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(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index d83b46a..85aec8b 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -337,14 +337,10 @@ static int hdlcd_drm_bind(struct device *dev)
 	if (ret)
 		goto err_free;
 
-	ret = drm_dev_register(drm, 0);
-	if (ret)
-		goto err_unload;
-
 	ret = component_bind_all(dev, drm);
 	if (ret) {
 		DRM_ERROR("Failed to bind all components\n");
-		goto err_unregister;
+		goto err_unload;
 	}
 
 	ret = pm_runtime_set_active(dev);
@@ -371,8 +367,17 @@ static int hdlcd_drm_bind(struct device *dev)
 		goto err_fbdev;
 	}
 
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		goto err_register;
+
 	return 0;
 
+err_register:
+	if (hdlcd->fbdev) {
+		drm_fbdev_cma_fini(hdlcd->fbdev);
+		hdlcd->fbdev = NULL;
+	}
 err_fbdev:
 	drm_kms_helper_poll_fini(drm);
 	drm_mode_config_cleanup(drm);
@@ -381,8 +386,6 @@ err_vblank:
 	pm_runtime_disable(drm->dev);
 err_pm_active:
 	component_unbind_all(dev, drm);
-err_unregister:
-	drm_dev_unregister(drm);
 err_unload:
 	drm_irq_uninstall(drm);
 	of_reserved_mem_device_release(drm->dev);
@@ -398,6 +401,7 @@ static void hdlcd_drm_unbind(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct hdlcd_drm_private *hdlcd = drm->dev_private;
 
+	drm_dev_unregister(drm);
 	if (hdlcd->fbdev) {
 		drm_fbdev_cma_fini(hdlcd->fbdev);
 		hdlcd->fbdev = NULL;
@@ -411,7 +415,6 @@ static void hdlcd_drm_unbind(struct device *dev)
 	pm_runtime_disable(drm->dev);
 	of_reserved_mem_device_release(drm->dev);
 	drm_mode_config_cleanup(drm);
-	drm_dev_unregister(drm);
 	drm_dev_unref(drm);
 	drm->dev_private = NULL;
 	dev_set_drvdata(dev, NULL);
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 82171d2..79bfc13 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -354,10 +354,6 @@ static int malidp_bind(struct device *dev)
 	if (ret < 0)
 		goto init_fail;
 
-	ret = drm_dev_register(drm, 0);
-	if (ret)
-		goto register_fail;
-
 	/* Set the CRTC's port so that the encoder component can find it */
 	ep = of_graph_get_next_endpoint(dev->of_node, NULL);
 	if (!ep) {
@@ -394,8 +390,18 @@ static int malidp_bind(struct device *dev)
 	}
 
 	drm_kms_helper_poll_init(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		goto register_fail;
+
 	return 0;
 
+register_fail:
+	if (malidp->fbdev) {
+		drm_fbdev_cma_fini(malidp->fbdev);
+		malidp->fbdev = NULL;
+	}
 fbdev_fail:
 	drm_vblank_cleanup(drm);
 vblank_fail:
@@ -407,8 +413,6 @@ bind_fail:
 	of_node_put(malidp->crtc.port);
 	malidp->crtc.port = NULL;
 port_fail:
-	drm_dev_unregister(drm);
-register_fail:
 	malidp_de_planes_destroy(drm);
 	drm_mode_config_cleanup(drm);
 init_fail:
@@ -431,6 +435,7 @@ static void malidp_unbind(struct device *dev)
 	struct malidp_drm *malidp = drm->dev_private;
 	struct malidp_hw_device *hwdev = malidp->dev;
 
+	drm_dev_unregister(drm);
 	if (malidp->fbdev) {
 		drm_fbdev_cma_fini(malidp->fbdev);
 		malidp->fbdev = NULL;
@@ -442,7 +447,6 @@ static void malidp_unbind(struct device *dev)
 	component_unbind_all(dev, drm);
 	of_node_put(malidp->crtc.port);
 	malidp->crtc.port = NULL;
-	drm_dev_unregister(drm);
 	malidp_de_planes_destroy(drm);
 	drm_mode_config_cleanup(drm);
 	drm->dev_private = NULL;
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index f4315bc..6e6fca2 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -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);
 }
 
@@ -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;
-
 	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

[PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[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 <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 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(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index d83b46a..85aec8b 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -337,14 +337,10 @@ static int hdlcd_drm_bind(struct device *dev)
 	if (ret)
 		goto err_free;
 
-	ret = drm_dev_register(drm, 0);
-	if (ret)
-		goto err_unload;
-
 	ret = component_bind_all(dev, drm);
 	if (ret) {
 		DRM_ERROR("Failed to bind all components\n");
-		goto err_unregister;
+		goto err_unload;
 	}
 
 	ret = pm_runtime_set_active(dev);
@@ -371,8 +367,17 @@ static int hdlcd_drm_bind(struct device *dev)
 		goto err_fbdev;
 	}
 
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		goto err_register;
+
 	return 0;
 
+err_register:
+	if (hdlcd->fbdev) {
+		drm_fbdev_cma_fini(hdlcd->fbdev);
+		hdlcd->fbdev = NULL;
+	}
 err_fbdev:
 	drm_kms_helper_poll_fini(drm);
 	drm_mode_config_cleanup(drm);
@@ -381,8 +386,6 @@ err_vblank:
 	pm_runtime_disable(drm->dev);
 err_pm_active:
 	component_unbind_all(dev, drm);
-err_unregister:
-	drm_dev_unregister(drm);
 err_unload:
 	drm_irq_uninstall(drm);
 	of_reserved_mem_device_release(drm->dev);
@@ -398,6 +401,7 @@ static void hdlcd_drm_unbind(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct hdlcd_drm_private *hdlcd = drm->dev_private;
 
+	drm_dev_unregister(drm);
 	if (hdlcd->fbdev) {
 		drm_fbdev_cma_fini(hdlcd->fbdev);
 		hdlcd->fbdev = NULL;
@@ -411,7 +415,6 @@ static void hdlcd_drm_unbind(struct device *dev)
 	pm_runtime_disable(drm->dev);
 	of_reserved_mem_device_release(drm->dev);
 	drm_mode_config_cleanup(drm);
-	drm_dev_unregister(drm);
 	drm_dev_unref(drm);
 	drm->dev_private = NULL;
 	dev_set_drvdata(dev, NULL);
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 82171d2..79bfc13 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -354,10 +354,6 @@ static int malidp_bind(struct device *dev)
 	if (ret < 0)
 		goto init_fail;
 
-	ret = drm_dev_register(drm, 0);
-	if (ret)
-		goto register_fail;
-
 	/* Set the CRTC's port so that the encoder component can find it */
 	ep = of_graph_get_next_endpoint(dev->of_node, NULL);
 	if (!ep) {
@@ -394,8 +390,18 @@ static int malidp_bind(struct device *dev)
 	}
 
 	drm_kms_helper_poll_init(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		goto register_fail;
+
 	return 0;
 
+register_fail:
+	if (malidp->fbdev) {
+		drm_fbdev_cma_fini(malidp->fbdev);
+		malidp->fbdev = NULL;
+	}
 fbdev_fail:
 	drm_vblank_cleanup(drm);
 vblank_fail:
@@ -407,8 +413,6 @@ bind_fail:
 	of_node_put(malidp->crtc.port);
 	malidp->crtc.port = NULL;
 port_fail:
-	drm_dev_unregister(drm);
-register_fail:
 	malidp_de_planes_destroy(drm);
 	drm_mode_config_cleanup(drm);
 init_fail:
@@ -431,6 +435,7 @@ static void malidp_unbind(struct device *dev)
 	struct malidp_drm *malidp = drm->dev_private;
 	struct malidp_hw_device *hwdev = malidp->dev;
 
+	drm_dev_unregister(drm);
 	if (malidp->fbdev) {
 		drm_fbdev_cma_fini(malidp->fbdev);
 		malidp->fbdev = NULL;
@@ -442,7 +447,6 @@ static void malidp_unbind(struct device *dev)
 	component_unbind_all(dev, drm);
 	of_node_put(malidp->crtc.port);
 	malidp->crtc.port = NULL;
-	drm_dev_unregister(drm);
 	malidp_de_planes_destroy(drm);
 	drm_mode_config_cleanup(drm);
 	drm->dev_private = NULL;
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index f4315bc..6e6fca2 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -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);
 }
 
@@ -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;
-
 	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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Daniel Vetter]

On Mon, Oct 24, 2016 at 03:27:59PM +0100, Brian Starkey wrote:
> 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 <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>

> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index f4315bc..6e6fca2 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -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);
>  }
>  
> @@ -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

>  	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
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Brian Starkey]

Hi Daniel,

On Mon, Oct 24, 2016 at 04:36:27PM +0200, Daniel Vetter wrote:
>On Mon, Oct 24, 2016 at 03:27:59PM +0100, Brian Starkey wrote:
>> 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 <rmk+kernel@arm.linux.org.uk>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
>
>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
>> index f4315bc..6e6fca2 100644
>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>> @@ -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);
>>  }
>>
>> @@ -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.

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.

Cheers,
-Brian

>-Daniel
>
>>  	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
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Daniel Vetter]

On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
>>
>>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
>>> b/drivers/gpu/drm/i2c/tda998x_drv.c
>>> index f4315bc..6e6fca2 100644
>>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>>> @@ -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);
>>>  }
>>>
>>> @@ -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.
>
>
> 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.

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Daniel Vetter]

On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
>>
>>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
>>> b/drivers/gpu/drm/i2c/tda998x_drv.c
>>> index f4315bc..6e6fca2 100644
>>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>>> @@ -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);
>>>  }
>>>
>>> @@ -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.
>
>
> 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.

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Brian Starkey]

Hi Daniel,

On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
>On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
>>>
>>>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
>>>> b/drivers/gpu/drm/i2c/tda998x_drv.c
>>>> index f4315bc..6e6fca2 100644
>>>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>>>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>>>> @@ -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);
>>>>  }
>>>>
>>>> @@ -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.
>>
>>
>> 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.
>
>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.

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

Yeah, believe me I know -_-
Hindsight is always 20/20 something something

Thanks,
-Brian

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

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Daniel Vetter]

On Tue, Oct 25, 2016 at 10:52:33AM +0100, Brian Starkey wrote:
> Hi Daniel,
> 
> On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> > > > 
> > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > index f4315bc..6e6fca2 100644
> > > > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > @@ -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);
> > > > >  }
> > > > > 
> > > > > @@ -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.
> > > 
> > > 
> > > 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.
> > 
> > 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.

Ah, makes sense. Should I apply this to drm-misc so it's in a shared tree?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Daniel Vetter]

On Tue, Oct 25, 2016 at 10:52:33AM +0100, Brian Starkey wrote:
> Hi Daniel,
> 
> On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> > > > 
> > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > index f4315bc..6e6fca2 100644
> > > > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > @@ -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);
> > > > >  }
> > > > > 
> > > > > @@ -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.
> > > 
> > > 
> > > 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.
> > 
> > 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.

Ah, makes sense. Should I apply this to drm-misc so it's in a shared tree?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Brian Starkey]

On Tue, Oct 25, 2016 at 12:19:19PM +0200, Daniel Vetter wrote:
>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?

Honestly, I don't know. I didn't entirely follow what it was Russell
wanted in terms of getting this merged:

>On Sat, Oct 22, 2016 at 02:40:22PM +0100, Russell King - ARM Linux wrote:
>>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.

Looks like Jyri's series which depends on this is also dependent on
Russell's tree, so might be best to wait for him to get back online.

-Brian

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

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Brian Starkey]

On Tue, Oct 25, 2016 at 12:19:19PM +0200, Daniel Vetter wrote:
>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?

Honestly, I don't know. I didn't entirely follow what it was Russell
wanted in terms of getting this merged:

>On Sat, Oct 22, 2016 at 02:40:22PM +0100, Russell King - ARM Linux wrote:
>>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.

Looks like Jyri's series which depends on this is also dependent on
Russell's tree, so might be best to wait for him to get back online.

-Brian

>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Russell King - ARM Linux]

On Tue, Oct 25, 2016 at 12:19:19PM +0200, Daniel Vetter wrote:
> On Tue, Oct 25, 2016 at 10:52:33AM +0100, Brian Starkey wrote:
> > Hi Daniel,
> > 
> > On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> > > On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > > b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > > index f4315bc..6e6fca2 100644
> > > > > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > > > > @@ -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);
> > > > > >  }
> > > > > > 
> > > > > > @@ -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.
> > > > 
> > > > 
> > > > 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.
> > > 
> > > 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.
> 
> 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.

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

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Russell King - ARM Linux]

On Mon, Oct 31, 2016 at 09:00:08AM +0000, Russell King - ARM Linux wrote:
> 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.

... or if Brian wants to send a git pull request to us with the
patch based on v4.8, we can all merge that instead.

Brian?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Russell King - ARM Linux]

On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> >>
> >>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> >>> b/drivers/gpu/drm/i2c/tda998x_drv.c
> >>> index f4315bc..6e6fca2 100644
> >>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> >>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> >>> @@ -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);
> >>>  }
> >>>
> >>> @@ -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.
> >
> >
> > 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.
> 
> 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.

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

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

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Daniel Vetter]

On Mon, Oct 31, 2016 at 08:58:43AM +0000, Russell King - ARM Linux wrote:
> On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> > >>
> > >>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > >>> b/drivers/gpu/drm/i2c/tda998x_drv.c
> > >>> index f4315bc..6e6fca2 100644
> > >>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > >>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > >>> @@ -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);
> > >>>  }
> > >>>
> > >>> @@ -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.
> > >
> > >
> > > 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.
> > 
> > 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.
> 
> > 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).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Russell King - ARM Linux]

On Tue, Nov 08, 2016 at 10:25:52AM +0100, Daniel Vetter wrote:
> 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.

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

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Daniel Vetter]

On Tue, Nov 08, 2016 at 10:59:43AM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 10:25:52AM +0100, Daniel Vetter wrote:
> > 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.

drm-misc is just the collector for when it doesn't make sense to have a
driver or topic/feature pull request (or there isn't really a permanent
driver tree). Pull to Dave directly makes sense.
-Daniel

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

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

Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Daniel Vetter]

On Tue, Nov 08, 2016 at 10:59:43AM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 10:25:52AM +0100, Daniel Vetter wrote:
> > 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.

drm-misc is just the collector for when it doesn't make sense to have a
driver or topic/feature pull request (or there isn't really a permanent
driver tree). Pull to Dave directly makes sense.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[GIT PULL] Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Russell King - ARM Linux]

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

On Tue, Nov 08, 2016 at 10:59:43AM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 10:25:52AM +0100, Daniel Vetter wrote:
> > 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.
> 
> -- 
> 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.

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

Re: [GIT PULL] Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

[Russell King - ARM Linux]

Okay, I've queued this into my own for-next branch, along with the now
reviewed and tested set of tda998x patches that I sent out for comment
and testing.

I'm still hopeful that Dave's going to pull the initial patch at some
point... please?

On Tue, Nov 15, 2016 at 09:46:31AM +0000, Russell King - ARM Linux wrote:
> I guess Dave must have missed this as I can't see it in drm-next, so
> I'm resending the pull request.
> 
> On Tue, Nov 08, 2016 at 10:59:43AM +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 08, 2016 at 10:25:52AM +0100, Daniel Vetter wrote:
> > > 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.
> > 
> > -- 
> > 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.
> 
> -- 
> 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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 3/4] drm/tilcdc: Stop using struct drm_driver load() callback

[Jyri Sarha]

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 <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 103 ++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 231f2c7..2dca822 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -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;
 }
@@ -587,7 +602,6 @@ static const struct file_operations fops = {
 static struct drm_driver tilcdc_driver = {
 	.driver_features    = (DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET |
 			       DRIVER_PRIME | DRIVER_ATOMIC),
-	.load               = tilcdc_load,
 	.unload             = tilcdc_unload,
 	.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,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/tilcdc: Stop using struct drm_driver load() callback

[Daniel Vetter]

On Wed, Oct 19, 2016 at 12:33:55AM +0300, Jyri Sarha wrote:
> 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 <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 103 ++++++++++++++++++++----------------
>  1 file changed, 58 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 231f2c7..2dca822 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -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;
>  }
> @@ -587,7 +602,6 @@ static const struct file_operations fops = {
>  static struct drm_driver tilcdc_driver = {
>  	.driver_features    = (DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET |
>  			       DRIVER_PRIME | DRIVER_ATOMIC),
> -	.load               = tilcdc_load,
>  	.unload             = tilcdc_unload,

btw for symmetry I recommend getting rid of unload too. Either way, nice
that another driver is demidlayered.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/tilcdc: Use unload to handle initialization failures

[Jyri Sarha]

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 <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 91 ++++++++++++-------------------------
 1 file changed, 28 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 2dca822..8820133 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -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 =
 		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;
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/tilcdc: Use unload to handle initialization failures

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Wednesday 19 Oct 2016 00:33:56 Jyri Sarha wrote:
> 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.

While at it, you could replace the deprecated drm_put_dev() calls and use 
drm_dev_unregister() and drm_dev_unref() directly. Note that the former 
contains a call to drm_vblank_cleanup(), you can thus remove the direct call 
to that function. As far as I understand drm_dev_unregister() is safe to call 
after drm_mode_config_init() only but doesn't require a previous call to even 
drm_dev_register(). I'm not sure if that's guaranteed though, or if it just 
works by chance.

> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 91 +++++++++++-----------------------
>  1 file changed, 28 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 2dca822..8820133 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -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 =
>  		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;

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel