[PATCH] drm/i2c: tda998x: don't register the connector

[PATCH] drm/i2c: tda998x: don't register the connector

[Brian Starkey]

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

As drm_dev_register() now registers all of the connectors anyway,
there's no need to explicitly do it in individual drivers so remove
the calls to drm_connector_register()/drm_connector_unregister().

This allows componentised drivers to use tda998x without having racy
initialisation.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c |    8 --------
 1 file changed, 8 deletions(-)

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/i2c: tda998x: don't register the connector

[Russell King - ARM Linux]

On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
> The connector shouldn't be registered until the rest of the whole device
> is set up, so that consistent state is presented to userspace.
> 
> As drm_dev_register() now registers all of the connectors anyway,
> there's no need to explicitly do it in individual drivers so remove
> the calls to drm_connector_register()/drm_connector_unregister().
> 
> This allows componentised drivers to use tda998x without having racy
> initialisation.

Is there a corresponding patch for armada-drm so that the cubox doesn't
regress?  Has it already been merged?

-- 
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/i2c: tda998x: don't register the connector

[Brian Starkey]

Hi Russell,

On Mon, Jul 25, 2016 at 01:25:04PM +0100, Russell King - ARM Linux wrote:
>On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
>> The connector shouldn't be registered until the rest of the whole device
>> is set up, so that consistent state is presented to userspace.
>>
>> As drm_dev_register() now registers all of the connectors anyway,
>> there's no need to explicitly do it in individual drivers so remove
>> the calls to drm_connector_register()/drm_connector_unregister().
>>
>> This allows componentised drivers to use tda998x without having racy
>> initialisation.
>
>Is there a corresponding patch for armada-drm so that the cubox doesn't
>regress?  Has it already been merged?
>

A patch for armada-drm to do what?

I should perhaps have explicitly mentioned that this change depends
on e28cd4d0a223: "drm: Automatically register/unregister all
connectors", which is in drm-next.

Like my commit message says - after the above commit, all connectors
are automatically registered in drm_dev_register() - so I don't
anticipate any regression, but I don't have a cubox to test.

armada-drm seems to be doing effectively the same thing as arm/hdlcd,
which works fine after this patch with no other changes.

Let me know if I've missed something; or if you are able to test on
cubox that would be great.

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

Re: [PATCH] drm/i2c: tda998x: don't register the connector

[Daniel Vetter]

On Mon, Jul 25, 2016 at 01:54:06PM +0100, Brian Starkey wrote:
> Hi Russell,
> 
> On Mon, Jul 25, 2016 at 01:25:04PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
> > > The connector shouldn't be registered until the rest of the whole device
> > > is set up, so that consistent state is presented to userspace.
> > > 
> > > As drm_dev_register() now registers all of the connectors anyway,
> > > there's no need to explicitly do it in individual drivers so remove
> > > the calls to drm_connector_register()/drm_connector_unregister().
> > > 
> > > This allows componentised drivers to use tda998x without having racy
> > > initialisation.
> > 
> > Is there a corresponding patch for armada-drm so that the cubox doesn't
> > regress?  Has it already been merged?
> > 
> 
> A patch for armada-drm to do what?
> 
> I should perhaps have explicitly mentioned that this change depends
> on e28cd4d0a223: "drm: Automatically register/unregister all
> connectors", which is in drm-next.
> 
> Like my commit message says - after the above commit, all connectors
> are automatically registered in drm_dev_register() - so I don't
> anticipate any regression, but I don't have a cubox to test.
> 
> armada-drm seems to be doing effectively the same thing as arm/hdlcd,
> which works fine after this patch with no other changes.
> 
> Let me know if I've missed something; or if you are able to test on
> cubox that would be great.

Ack from my side on generally nuking drm_connector_register() from
everywhere except truely hotplugged connectors like dp mst. It should keep
working for everyone. Only exception is if there's a driver which calls
drm_dev_register too early (before all connectors are probed), which would
be a bug anyway.
-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/i2c: tda998x: don't register the connector

[Brian Starkey]

Hi,

On Mon, Jul 25, 2016 at 05:08:21PM +0200, Daniel Vetter wrote:
>On Mon, Jul 25, 2016 at 01:54:06PM +0100, Brian Starkey wrote:
>> Hi Russell,
>>
>> On Mon, Jul 25, 2016 at 01:25:04PM +0100, Russell King - ARM Linux wrote:
>> > On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
>> > > The connector shouldn't be registered until the rest of the whole device
>> > > is set up, so that consistent state is presented to userspace.
>> > >
>> > > As drm_dev_register() now registers all of the connectors anyway,
>> > > there's no need to explicitly do it in individual drivers so remove
>> > > the calls to drm_connector_register()/drm_connector_unregister().
>> > >
>> > > This allows componentised drivers to use tda998x without having racy
>> > > initialisation.
>> >
>> > Is there a corresponding patch for armada-drm so that the cubox doesn't
>> > regress?  Has it already been merged?
>> >
>>
>> A patch for armada-drm to do what?
>>
>> I should perhaps have explicitly mentioned that this change depends
>> on e28cd4d0a223: "drm: Automatically register/unregister all
>> connectors", which is in drm-next.
>>
>> Like my commit message says - after the above commit, all connectors
>> are automatically registered in drm_dev_register() - so I don't
>> anticipate any regression, but I don't have a cubox to test.
>>
>> armada-drm seems to be doing effectively the same thing as arm/hdlcd,
>> which works fine after this patch with no other changes.
>>
>> Let me know if I've missed something; or if you are able to test on
>> cubox that would be great.
>
>Ack from my side on generally nuking drm_connector_register() from
>everywhere except truely hotplugged connectors like dp mst. It should keep
>working for everyone. Only exception is if there's a driver which calls
>drm_dev_register too early (before all connectors are probed), which would
>be a bug anyway.

Right; the motivation for this change is to fix the init order in
HDLCD and Mali-DP (move drm_dev_register to the end), which we can't
do right now because tda998x expects the DRM device sysfs to be set
up in bind.

@Daniel: Can I take this as your Acked-by?

Should this go in via Russell's tree?

@Russell, are you happy with this change?

Thanks,
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/i2c: tda998x: don't register the connector

[Daniel Vetter]

On Mon, Aug 08, 2016 at 05:04:03PM +0100, Brian Starkey wrote:
> Hi,
> 
> On Mon, Jul 25, 2016 at 05:08:21PM +0200, Daniel Vetter wrote:
> > On Mon, Jul 25, 2016 at 01:54:06PM +0100, Brian Starkey wrote:
> > > Hi Russell,
> > > 
> > > On Mon, Jul 25, 2016 at 01:25:04PM +0100, Russell King - ARM Linux wrote:
> > > > On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
> > > > > The connector shouldn't be registered until the rest of the whole device
> > > > > is set up, so that consistent state is presented to userspace.
> > > > >
> > > > > As drm_dev_register() now registers all of the connectors anyway,
> > > > > there's no need to explicitly do it in individual drivers so remove
> > > > > the calls to drm_connector_register()/drm_connector_unregister().
> > > > >
> > > > > This allows componentised drivers to use tda998x without having racy
> > > > > initialisation.
> > > >
> > > > Is there a corresponding patch for armada-drm so that the cubox doesn't
> > > > regress?  Has it already been merged?
> > > >
> > > 
> > > A patch for armada-drm to do what?
> > > 
> > > I should perhaps have explicitly mentioned that this change depends
> > > on e28cd4d0a223: "drm: Automatically register/unregister all
> > > connectors", which is in drm-next.
> > > 
> > > Like my commit message says - after the above commit, all connectors
> > > are automatically registered in drm_dev_register() - so I don't
> > > anticipate any regression, but I don't have a cubox to test.
> > > 
> > > armada-drm seems to be doing effectively the same thing as arm/hdlcd,
> > > which works fine after this patch with no other changes.
> > > 
> > > Let me know if I've missed something; or if you are able to test on
> > > cubox that would be great.
> > 
> > Ack from my side on generally nuking drm_connector_register() from
> > everywhere except truely hotplugged connectors like dp mst. It should keep
> > working for everyone. Only exception is if there's a driver which calls
> > drm_dev_register too early (before all connectors are probed), which would
> > be a bug anyway.
> 
> Right; the motivation for this change is to fix the init order in
> HDLCD and Mali-DP (move drm_dev_register to the end), which we can't
> do right now because tda998x expects the DRM device sysfs to be set
> up in bind.
> 
> @Daniel: Can I take this as your Acked-by?

Sure.

> Should this go in via Russell's tree?

I can also throw it into drm-misc, if Russell (or someone else) can
provide a tested-by for armada. Better safe than sorry ;-)
-Daniel

> 
> @Russell, are you happy with this change?
> 
> Thanks,
> Brian
> 
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > 

-- 
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/i2c: tda998x: don't register the connector

[Russell King - ARM Linux]

On Tue, Aug 09, 2016 at 08:07:24AM +0200, Daniel Vetter wrote:
> On Mon, Aug 08, 2016 at 05:04:03PM +0100, Brian Starkey wrote:
> > Hi,
> > 
> > On Mon, Jul 25, 2016 at 05:08:21PM +0200, Daniel Vetter wrote:
> > > On Mon, Jul 25, 2016 at 01:54:06PM +0100, Brian Starkey wrote:
> > > > Hi Russell,
> > > > 
> > > > On Mon, Jul 25, 2016 at 01:25:04PM +0100, Russell King - ARM Linux wrote:
> > > > > On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
> > > > > > The connector shouldn't be registered until the rest of the whole device
> > > > > > is set up, so that consistent state is presented to userspace.
> > > > > >
> > > > > > As drm_dev_register() now registers all of the connectors anyway,
> > > > > > there's no need to explicitly do it in individual drivers so remove
> > > > > > the calls to drm_connector_register()/drm_connector_unregister().
> > > > > >
> > > > > > This allows componentised drivers to use tda998x without having racy
> > > > > > initialisation.
> > > > >
> > > > > Is there a corresponding patch for armada-drm so that the cubox doesn't
> > > > > regress?  Has it already been merged?
> > > > >
> > > > 
> > > > A patch for armada-drm to do what?
> > > > 
> > > > I should perhaps have explicitly mentioned that this change depends
> > > > on e28cd4d0a223: "drm: Automatically register/unregister all
> > > > connectors", which is in drm-next.
> > > > 
> > > > Like my commit message says - after the above commit, all connectors
> > > > are automatically registered in drm_dev_register() - so I don't
> > > > anticipate any regression, but I don't have a cubox to test.
> > > > 
> > > > armada-drm seems to be doing effectively the same thing as arm/hdlcd,
> > > > which works fine after this patch with no other changes.
> > > > 
> > > > Let me know if I've missed something; or if you are able to test on
> > > > cubox that would be great.
> > > 
> > > Ack from my side on generally nuking drm_connector_register() from
> > > everywhere except truely hotplugged connectors like dp mst. It should keep
> > > working for everyone. Only exception is if there's a driver which calls
> > > drm_dev_register too early (before all connectors are probed), which would
> > > be a bug anyway.
> > 
> > Right; the motivation for this change is to fix the init order in
> > HDLCD and Mali-DP (move drm_dev_register to the end), which we can't
> > do right now because tda998x expects the DRM device sysfs to be set
> > up in bind.
> > 
> > @Daniel: Can I take this as your Acked-by?
> 
> Sure.
> 
> > Should this go in via Russell's tree?
> 
> I can also throw it into drm-misc, if Russell (or someone else) can
> provide a tested-by for armada. Better safe than sorry ;-)

I can't provide a tested-by for armada yet, because testing it on
my cubox tree (v4.7) causes a failure.  It probably relies on some
change merged during the merge window.

-- 
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/i2c: tda998x: don't register the connector

[Brian Starkey]

Hi Russell,

Are you in a position to be able to test this now?

I believe it should work fine on anything since 4.8-rc1.
(specifically, either of commit e28cd4d0a223 or 79190ea2658a)

Thanks,
Brian

On Tue, Aug 09, 2016 at 11:48:12PM +0100, Russell King - ARM Linux 
wrote:
>On Tue, Aug 09, 2016 at 08:07:24AM +0200, Daniel Vetter wrote:
>> On Mon, Aug 08, 2016 at 05:04:03PM +0100, Brian Starkey wrote:
>> > Hi,
>> >
>> > On Mon, Jul 25, 2016 at 05:08:21PM +0200, Daniel Vetter wrote:
>> > > On Mon, Jul 25, 2016 at 01:54:06PM +0100, Brian Starkey wrote:
>> > > > Hi Russell,
>> > > >
>> > > > On Mon, Jul 25, 2016 at 01:25:04PM +0100, Russell King - ARM Linux wrote:
>> > > > > On Mon, Jul 25, 2016 at 11:55:48AM +0100, Brian Starkey wrote:
>> > > > > > The connector shouldn't be registered until the rest of the whole device
>> > > > > > is set up, so that consistent state is presented to userspace.
>> > > > > >
>> > > > > > As drm_dev_register() now registers all of the connectors anyway,
>> > > > > > there's no need to explicitly do it in individual drivers so remove
>> > > > > > the calls to drm_connector_register()/drm_connector_unregister().
>> > > > > >
>> > > > > > This allows componentised drivers to use tda998x without having racy
>> > > > > > initialisation.
>> > > > >
>> > > > > Is there a corresponding patch for armada-drm so that the cubox doesn't
>> > > > > regress?  Has it already been merged?
>> > > > >
>> > > >
>> > > > A patch for armada-drm to do what?
>> > > >
>> > > > I should perhaps have explicitly mentioned that this change depends
>> > > > on e28cd4d0a223: "drm: Automatically register/unregister all
>> > > > connectors", which is in drm-next.
>> > > >
>> > > > Like my commit message says - after the above commit, all connectors
>> > > > are automatically registered in drm_dev_register() - so I don't
>> > > > anticipate any regression, but I don't have a cubox to test.
>> > > >
>> > > > armada-drm seems to be doing effectively the same thing as arm/hdlcd,
>> > > > which works fine after this patch with no other changes.
>> > > >
>> > > > Let me know if I've missed something; or if you are able to test on
>> > > > cubox that would be great.
>> > >
>> > > Ack from my side on generally nuking drm_connector_register() from
>> > > everywhere except truely hotplugged connectors like dp mst. It should keep
>> > > working for everyone. Only exception is if there's a driver which calls
>> > > drm_dev_register too early (before all connectors are probed), which would
>> > > be a bug anyway.
>> >
>> > Right; the motivation for this change is to fix the init order in
>> > HDLCD and Mali-DP (move drm_dev_register to the end), which we can't
>> > do right now because tda998x expects the DRM device sysfs to be set
>> > up in bind.
>> >
>> > @Daniel: Can I take this as your Acked-by?
>>
>> Sure.
>>
>> > Should this go in via Russell's tree?
>>
>> I can also throw it into drm-misc, if Russell (or someone else) can
>> provide a tested-by for armada. Better safe than sorry ;-)
>
>I can't provide a tested-by for armada yet, because testing it on
>my cubox tree (v4.7) causes a failure.  It probably relies on some
>change merged during the merge window.
>
>-- 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/i2c: tda998x: don't register the connector

[Russell King - ARM Linux]

On Wed, Sep 21, 2016 at 09:57:38AM +0100, Brian Starkey wrote:
> Hi Russell,
> 
> Are you in a position to be able to test this now?

Normally, I'd say no, because I'd normally wait for 4.8 to be out before
moving the cubox tree up.  However, as we're close to 4.8, I've merged
4.8-rc7 in (and fixed the multitude of conflicts), and manually made the
changes in your patch.  Nothing seems to have broken, so I think we're
good.

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Daniel, please take this change through the drm-misc tree as I'm unlikely
to have a branch which I can apply it to until after the merge window
opens.

-- 
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/i2c: tda998x: don't register the connector

[Brian Starkey]

Thanks Russell, it's most appreciated.

On Wed, Sep 21, 2016 at 05:28:03PM +0100, Russell King - ARM Linux wrote:
>On Wed, Sep 21, 2016 at 09:57:38AM +0100, Brian Starkey wrote:
>> Hi Russell,
>>
>> Are you in a position to be able to test this now?
>
>Normally, I'd say no, because I'd normally wait for 4.8 to be out before
>moving the cubox tree up.  However, as we're close to 4.8, I've merged
>4.8-rc7 in (and fixed the multitude of conflicts), and manually made the
>changes in your patch.  Nothing seems to have broken, so I think we're
>good.
>
>Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
>
>Daniel, please take this change through the drm-misc tree as I'm unlikely
>to have a branch which I can apply it to until after the merge window
>opens.
>

Actually, could you please hold off picking this up? We need to make
changes in mali-dp and hdlcd or this will mess up their registration.
I will send those patches later today, but better if this all goes in 
together (whenever that ends up being).

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

Re: [PATCH] drm/i2c: tda998x: don't register the connector

[Russell King - ARM Linux]

On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
> Actually, could you please hold off picking this up? We need to make
> changes in mali-dp and hdlcd or this will mess up their registration.
> I will send those patches later today, but better if this all goes in
> together (whenever that ends up being).

Sorry, but I'm annoyed with this - the impression being given was that
I was holding up this patch by not testing it on Armada, and I brought
up the issue about registration at the beginning of this.

Now we're _just_ finding out that there are drivers where removing the
connector registration in tda998x causes them to break?  It's a bit
late to be checking your own drivers when you've been chasing me...

Sorry, but it sounds like we're not ready to make this change - and as
it's the very last day that changes will appear in linux-next prior to
the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
holding off until after the merge window is over, so we can get some
testing with these other two drivers with this change in place.

-- 
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/i2c: tda998x: don't register the connector

[Sean Paul]

On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
>> Actually, could you please hold off picking this up? We need to make
>> changes in mali-dp and hdlcd or this will mess up their registration.
>> I will send those patches later today, but better if this all goes in
>> together (whenever that ends up being).
>
> Sorry, but I'm annoyed with this - the impression being given was that
> I was holding up this patch by not testing it on Armada, and I brought
> up the issue about registration at the beginning of this.
>
> Now we're _just_ finding out that there are drivers where removing the
> connector registration in tda998x causes them to break?  It's a bit
> late to be checking your own drivers when you've been chasing me...
>
> Sorry, but it sounds like we're not ready to make this change - and as
> it's the very last day that changes will appear in linux-next prior to
> the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
> holding off until after the merge window is over, so we can get some
> testing with these other two drivers with this change in place.
>

sigh. I just pushed my queue to drm-misc, which included this patch.
Sounds like I should revert?

Sean


> --
> 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/i2c: tda998x: don't register the connector

[Daniel Vetter]

On Thu, Sep 22, 2016 at 1:22 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
>>> Actually, could you please hold off picking this up? We need to make
>>> changes in mali-dp and hdlcd or this will mess up their registration.
>>> I will send those patches later today, but better if this all goes in
>>> together (whenever that ends up being).
>>
>> Sorry, but I'm annoyed with this - the impression being given was that
>> I was holding up this patch by not testing it on Armada, and I brought
>> up the issue about registration at the beginning of this.
>>
>> Now we're _just_ finding out that there are drivers where removing the
>> connector registration in tda998x causes them to break?  It's a bit
>> late to be checking your own drivers when you've been chasing me...
>>
>> Sorry, but it sounds like we're not ready to make this change - and as
>> it's the very last day that changes will appear in linux-next prior to
>> the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
>> holding off until after the merge window is over, so we can get some
>> testing with these other two drivers with this change in place.
>>
>
> sigh. I just pushed my queue to drm-misc, which included this patch.
> Sounds like I should revert?

I thought I looked at the entire situation and since we register
(since recently) all connectors in drm_dev_register() there shouldn't
be an issue for any other driver. Imo no need to revert anything here
- until someone complains with a bug report and proves me wrong ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i2c: tda998x: don't register the connector

[Brian Starkey]

On Thu, Sep 22, 2016 at 01:28:37PM +0200, Daniel Vetter wrote:
>On Thu, Sep 22, 2016 at 1:22 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>>> On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
>>>> Actually, could you please hold off picking this up? We need to make
>>>> changes in mali-dp and hdlcd or this will mess up their registration.
>>>> I will send those patches later today, but better if this all goes in
>>>> together (whenever that ends up being).
>>>
>>> Sorry, but I'm annoyed with this - the impression being given was that
>>> I was holding up this patch by not testing it on Armada, and I brought
>>> up the issue about registration at the beginning of this.
>>>

Sorry, this was poor on my part. There's no-one who will care about
Mali-DP, but I had forgotten that this would also break HDLCD.

>>> Now we're _just_ finding out that there are drivers where removing the
>>> connector registration in tda998x causes them to break?  It's a bit
>>> late to be checking your own drivers when you've been chasing me...
>>>

It's not like I didn't check our drivers, more that I should have sent
a full series for all three drivers together in the first place.

However, without patching all three drivers in the same commit, there
would always be some breakage. HDLCD and Mali-DP call
drm_dev_register() before binding the components - this was needed to
work with tda998x, which needed the device to be already registered
before its bind callback runs.

It's more proper to call drm_dev_register() as the very last thing
(i.e. after component_bind_all()) to avoid races with userspace - but
I couldn't do that without this change in tda998x first.

>>> Sorry, but it sounds like we're not ready to make this change - and as
>>> it's the very last day that changes will appear in linux-next prior to
>>> the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
>>> holding off until after the merge window is over, so we can get some
>>> testing with these other two drivers with this change in place.
>>>
>>
>> sigh. I just pushed my queue to drm-misc, which included this patch.
>> Sounds like I should revert?
>
>I thought I looked at the entire situation and since we register
>(since recently) all connectors in drm_dev_register() there shouldn't
>be an issue for any other driver. Imo no need to revert anything here
>- until someone complains with a bug report and proves me wrong ;-)

Yeah... this does indeed break HDLCD and Mali-DP. They register the
device, then bind tda998x which init's its connector. The connector
ends up in the connector_list, but not the idr.

I don't know how widely HDLCD on Juno is used (for anyone to notice).

-Brian

>-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/i2c: tda998x: don't register the connector

[Russell King - ARM Linux]

On Thu, Sep 22, 2016 at 02:38:45PM +0100, Brian Starkey wrote:
> However, without patching all three drivers in the same commit, there
> would always be some breakage. HDLCD and Mali-DP call
> drm_dev_register() before binding the components - this was needed to
> work with tda998x, which needed the device to be already registered
> before its bind callback runs.

I guess that is because drm_connector_register() relies on
drm_minor_register() having been previously run, which is rather
unfortunate.  That brings up the question about whether eliminating
the load callback is really such a wise move.

tda998x works fine either with or without the drm_connector_register()
call if it's bound within drm_dev_register(), which happens after the
minors have been registered.

> It's more proper to call drm_dev_register() as the very last thing
> (i.e. after component_bind_all()) to avoid races with userspace - but
> I couldn't do that without this change in tda998x first.

Actually, what you're saying is that you need to change tda998x in
lock-step with your drivers - if you have only the tda998x patch
applied, then you break.  If you apply the patch for your drivers,
you break.

That's really not good, and can be worked around by providing the
legacy ->load method, which just does the component_bind_all() call.
That makes you independent of the tda998x change, just like other
DRM drivers such as armada and tilcdc.

-- 
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/i2c: tda998x: don't register the connector

[Russell King - ARM Linux]

On Thu, Sep 22, 2016 at 04:22:40AM -0700, Sean Paul wrote:
> On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
> >> Actually, could you please hold off picking this up? We need to make
> >> changes in mali-dp and hdlcd or this will mess up their registration.
> >> I will send those patches later today, but better if this all goes in
> >> together (whenever that ends up being).
> >
> > Sorry, but I'm annoyed with this - the impression being given was that
> > I was holding up this patch by not testing it on Armada, and I brought
> > up the issue about registration at the beginning of this.
> >
> > Now we're _just_ finding out that there are drivers where removing the
> > connector registration in tda998x causes them to break?  It's a bit
> > late to be checking your own drivers when you've been chasing me...
> >
> > Sorry, but it sounds like we're not ready to make this change - and as
> > it's the very last day that changes will appear in linux-next prior to
> > the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
> > holding off until after the merge window is over, so we can get some
> > testing with these other two drivers with this change in place.
> >
> 
> sigh. I just pushed my queue to drm-misc, which included this patch.
> Sounds like I should revert?

Why did you do push it _without_ an ack from the maintainer of the
driver when the comments on the list were clearly indicating that it
was waiting for testing and such an ack?

There's something horribly wrong with the process here.

-- 
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/i2c: tda998x: don't register the connector

[Sean Paul]

On Thu, Sep 22, 2016 at 5:09 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Thu, Sep 22, 2016 at 04:22:40AM -0700, Sean Paul wrote:
>> On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
>> >> Actually, could you please hold off picking this up? We need to make
>> >> changes in mali-dp and hdlcd or this will mess up their registration.
>> >> I will send those patches later today, but better if this all goes in
>> >> together (whenever that ends up being).
>> >
>> > Sorry, but I'm annoyed with this - the impression being given was that
>> > I was holding up this patch by not testing it on Armada, and I brought
>> > up the issue about registration at the beginning of this.
>> >
>> > Now we're _just_ finding out that there are drivers where removing the
>> > connector registration in tda998x causes them to break?  It's a bit
>> > late to be checking your own drivers when you've been chasing me...
>> >
>> > Sorry, but it sounds like we're not ready to make this change - and as
>> > it's the very last day that changes will appear in linux-next prior to
>> > the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
>> > holding off until after the merge window is over, so we can get some
>> > testing with these other two drivers with this change in place.
>> >
>>
>> sigh. I just pushed my queue to drm-misc, which included this patch.
>> Sounds like I should revert?
>
> Why did you do push it _without_ an ack from the maintainer of the
> driver when the comments on the list were clearly indicating that it
> was waiting for testing and such an ack?
>

Daniel Acked it on the list on 7/25, and you acked it yesterday asking
it to be merged to -misc. What am I missing?

Sean


> There's something horribly wrong with the process here.
>
> --
> 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/i2c: tda998x: don't register the connector

[Russell King - ARM Linux]

On Thu, Sep 22, 2016 at 05:32:35AM -0700, Sean Paul wrote:
> On Thu, Sep 22, 2016 at 5:09 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Thu, Sep 22, 2016 at 04:22:40AM -0700, Sean Paul wrote:
> >> On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
> >> <linux@armlinux.org.uk> wrote:
> >> > On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
> >> >> Actually, could you please hold off picking this up? We need to make
> >> >> changes in mali-dp and hdlcd or this will mess up their registration.
> >> >> I will send those patches later today, but better if this all goes in
> >> >> together (whenever that ends up being).
> >> >
> >> > Sorry, but I'm annoyed with this - the impression being given was that
> >> > I was holding up this patch by not testing it on Armada, and I brought
> >> > up the issue about registration at the beginning of this.
> >> >
> >> > Now we're _just_ finding out that there are drivers where removing the
> >> > connector registration in tda998x causes them to break?  It's a bit
> >> > late to be checking your own drivers when you've been chasing me...
> >> >
> >> > Sorry, but it sounds like we're not ready to make this change - and as
> >> > it's the very last day that changes will appear in linux-next prior to
> >> > the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
> >> > holding off until after the merge window is over, so we can get some
> >> > testing with these other two drivers with this change in place.
> >> >
> >>
> >> sigh. I just pushed my queue to drm-misc, which included this patch.
> >> Sounds like I should revert?
> >
> > Why did you do push it _without_ an ack from the maintainer of the
> > driver when the comments on the list were clearly indicating that it
> > was waiting for testing and such an ack?
> >
> 
> Daniel Acked it on the list on 7/25, and you acked it yesterday asking
> it to be merged to -misc. What am I missing?

Sorry, I thought you were some random person maintaining some random
tree who'd submitted a pull request to be merged into drm-misc.  If
you are in fact the drm-misc maintainer, please add yourself to the
MAINTAINERS file so that everyone knows who you are.  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/i2c: tda998x: don't register the connector

[Daniel Vetter]

On Thu, Sep 22, 2016 at 2:40 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> Sorry, I thought you were some random person maintaining some random
> tree who'd submitted a pull request to be merged into drm-misc.  If
> you are in fact the drm-misc maintainer, please add yourself to the
> MAINTAINERS file so that everyone knows who you are.  Thanks.


drm-misc has a different model than most other kernel trees. It
started out with drm-intel, and I'm going to present about it at KS.
One thing I want to fix with that discussion is some agreement on how
these trees should be handled in MAINTAINERS.
-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/i2c: tda998x: don't register the connector

[Brian Starkey]

Hi Sean,

On Thu, Sep 22, 2016 at 04:22:40AM -0700, Sean Paul wrote:
>On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
><linux@armlinux.org.uk> wrote:
>> On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
>>> Actually, could you please hold off picking this up? We need to make
>>> changes in mali-dp and hdlcd or this will mess up their registration.
>>> I will send those patches later today, but better if this all goes in
>>> together (whenever that ends up being).
>>
>> Sorry, but I'm annoyed with this - the impression being given was that
>> I was holding up this patch by not testing it on Armada, and I brought
>> up the issue about registration at the beginning of this.
>>
>> Now we're _just_ finding out that there are drivers where removing the
>> connector registration in tda998x causes them to break?  It's a bit
>> late to be checking your own drivers when you've been chasing me...
>>
>> Sorry, but it sounds like we're not ready to make this change - and as
>> it's the very last day that changes will appear in linux-next prior to
>> the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
>> holding off until after the merge window is over, so we can get some
>> testing with these other two drivers with this change in place.
>>
>
>sigh. I just pushed my queue to drm-misc, which included this patch.
>Sounds like I should revert?
>

Yes, please revert this. There's a problem in the fbdev helper code
which stops me fixing this quickly, so better to revert it.

Very sorry for the mess.

-Brian

>Sean
>
>
>> --
>> 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] drm/i2c: tda998x: don't register the connector

[Daniel Vetter]

On Thu, Sep 22, 2016 at 4:14 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> On Thu, Sep 22, 2016 at 04:22:40AM -0700, Sean Paul wrote:
>>
>> On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>>>
>>> On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
>>>>
>>>> Actually, could you please hold off picking this up? We need to make
>>>> changes in mali-dp and hdlcd or this will mess up their registration.
>>>> I will send those patches later today, but better if this all goes in
>>>> together (whenever that ends up being).
>>>
>>>
>>> Sorry, but I'm annoyed with this - the impression being given was that
>>> I was holding up this patch by not testing it on Armada, and I brought
>>> up the issue about registration at the beginning of this.
>>>
>>> Now we're _just_ finding out that there are drivers where removing the
>>> connector registration in tda998x causes them to break?  It's a bit
>>> late to be checking your own drivers when you've been chasing me...
>>>
>>> Sorry, but it sounds like we're not ready to make this change - and as
>>> it's the very last day that changes will appear in linux-next prior to
>>> the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
>>> holding off until after the merge window is over, so we can get some
>>> testing with these other two drivers with this change in place.
>>>
>>
>> sigh. I just pushed my queue to drm-misc, which included this patch.
>> Sounds like I should revert?
>>
>
> Yes, please revert this. There's a problem in the fbdev helper code
> which stops me fixing this quickly, so better to revert it.

Hm, what's the trouble wih fbdev? But yeah given this trouble I'd go
with a revert for now. For the real fix I guess we could just squash
it all in one, kinda pointless to go overboard for this.
-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

[PATCH] Revert "drm/i2c: tda998x: don't register the connector"

[Sean Paul]

This reverts commit 6a2925ea12006911c8180a89feda6d040873ed18.

commit 6a2925ea12006911c8180a89feda6d040873ed18
Author: Brian Starkey <brian.starkey@arm.com>
Date:   Mon Jul 25 11:55:48 2016 +0100

    drm/i2c: tda998x: don't register the connector

[seanpaul]
Patch isn't fully baked, and apparently causing issues in hdlcd. Revert
until this is sorted.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 088900d..9798d40 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1584,6 +1584,7 @@ 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);
 }
 
@@ -1655,10 +1656,16 @@ 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:
@@ -1671,6 +1678,7 @@ 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);
-- 
2.8.0.rc3.226.g39d4020

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

Re: [PATCH] Revert "drm/i2c: tda998x: don't register the connector"

[liviu.dudau]

On Fri, Sep 23, 2016 at 12:18:12AM -0700, Sean Paul wrote:
> This reverts commit 6a2925ea12006911c8180a89feda6d040873ed18.
> 
> commit 6a2925ea12006911c8180a89feda6d040873ed18
> Author: Brian Starkey <brian.starkey@arm.com>
> Date:   Mon Jul 25 11:55:48 2016 +0100
> 
>     drm/i2c: tda998x: don't register the connector
> 
> [seanpaul]
> Patch isn't fully baked, and apparently causing issues in hdlcd. Revert
> until this is sorted.

I would argue that the comment is not correct, patch is fine, it is
just the dependent code is not ready to work with the patch.

Otherwise, thanks for doing this and sorry for the noise.

Best regards,
Liviu

> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 088900d..9798d40 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1584,6 +1584,7 @@ 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);
>  }
>  
> @@ -1655,10 +1656,16 @@ 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:
> @@ -1671,6 +1678,7 @@ 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);
> -- 
> 2.8.0.rc3.226.g39d4020
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Revert "drm/i2c: tda998x: don't register the connector"

[Sean Paul]

On Fri, Sep 23, 2016 at 2:32 AM,  <liviu.dudau@arm.com> wrote:
> On Fri, Sep 23, 2016 at 12:18:12AM -0700, Sean Paul wrote:
>> This reverts commit 6a2925ea12006911c8180a89feda6d040873ed18.
>>
>> commit 6a2925ea12006911c8180a89feda6d040873ed18
>> Author: Brian Starkey <brian.starkey@arm.com>
>> Date:   Mon Jul 25 11:55:48 2016 +0100
>>
>>     drm/i2c: tda998x: don't register the connector
>>
>> [seanpaul]
>> Patch isn't fully baked, and apparently causing issues in hdlcd. Revert
>> until this is sorted.
>
> I would argue that the comment is not correct, patch is fine, it is
> just the dependent code is not ready to work with the patch.
>
> Otherwise, thanks for doing this and sorry for the noise.
>

Applied to drm-misc

Sean

> Best regards,
> Liviu
>
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> ---
>>  drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
>> index 088900d..9798d40 100644
>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>> @@ -1584,6 +1584,7 @@ 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);
>>  }
>>
>> @@ -1655,10 +1656,16 @@ 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:
>> @@ -1671,6 +1678,7 @@ 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);
>> --
>> 2.8.0.rc3.226.g39d4020
>>
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i2c: tda998x: don't register the connector

[Liviu Dudau]

On Fri, Sep 23, 2016 at 09:05:50AM +0200, Daniel Vetter wrote:
> On Thu, Sep 22, 2016 at 4:14 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> > On Thu, Sep 22, 2016 at 04:22:40AM -0700, Sean Paul wrote:
> >>
> >> On Thu, Sep 22, 2016 at 3:51 AM, Russell King - ARM Linux
> >> <linux@armlinux.org.uk> wrote:
> >>>
> >>> On Thu, Sep 22, 2016 at 11:39:18AM +0100, Brian Starkey wrote:
> >>>>
> >>>> Actually, could you please hold off picking this up? We need to make
> >>>> changes in mali-dp and hdlcd or this will mess up their registration.
> >>>> I will send those patches later today, but better if this all goes in
> >>>> together (whenever that ends up being).
> >>>
> >>>
> >>> Sorry, but I'm annoyed with this - the impression being given was that
> >>> I was holding up this patch by not testing it on Armada, and I brought
> >>> up the issue about registration at the beginning of this.
> >>>
> >>> Now we're _just_ finding out that there are drivers where removing the
> >>> connector registration in tda998x causes them to break?  It's a bit
> >>> late to be checking your own drivers when you've been chasing me...
> >>>
> >>> Sorry, but it sounds like we're not ready to make this change - and as
> >>> it's the very last day that changes will appear in linux-next prior to
> >>> the merge window (assuming Linus releases 4.8 on Sunday), I'd suggest
> >>> holding off until after the merge window is over, so we can get some
> >>> testing with these other two drivers with this change in place.
> >>>
> >>
> >> sigh. I just pushed my queue to drm-misc, which included this patch.
> >> Sounds like I should revert?
> >>
> >
> > Yes, please revert this. There's a problem in the fbdev helper code
> > which stops me fixing this quickly, so better to revert it.
> 
> Hm, what's the trouble wih fbdev? But yeah given this trouble I'd go
> with a revert for now. For the real fix I guess we could just squash
> it all in one, kinda pointless to go overboard for this.
> -Daniel

rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
enabled, but we never got the call to turn off the CRTC. Brian is still
tracking through the fbdev emulation to figure out the cause for that.

Best regards,
Liviu

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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH] drm/i2c: tda998x: don't register the connector

[Daniel Vetter]

On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
> enabled, but we never got the call to turn off the CRTC. Brian is still
> tracking through the fbdev emulation to figure out the cause for that.

fbdev emulation doesn't do that for you. If you need/want to shut down
all the crtcs on driver unload, you need to do that yourself. There's
atomic helpers to do that for you that for you.
-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/i2c: tda998x: don't register the connector

[Lucas Stach]

Am Freitag, den 23.09.2016, 12:58 +0200 schrieb Daniel Vetter:
> On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau <Liviu.Dudau@arm.com>
> wrote:
> > 
> > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is
> > still
> > enabled, but we never got the call to turn off the CRTC. Brian is
> > still
> > tracking through the fbdev emulation to figure out the cause for
> > that.
> 
> fbdev emulation doesn't do that for you. If you need/want to shut
> down
> all the crtcs on driver unload, you need to do that yourself. There's
> atomic helpers to do that for you that for you.

Which reminds me that my patch (drm/atomic-helper: add unlocked disable
all outputs helper) to add such a helper wasn't applied. Probably my
own fault by being non-responsive to Seans question.

Regards,
Lucas

Re: [PATCH] drm/i2c: tda998x: don't register the connector

[Brian Starkey]

On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote:
>On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>> rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
>> enabled, but we never got the call to turn off the CRTC. Brian is still
>> tracking through the fbdev emulation to figure out the cause for that.
>
>fbdev emulation doesn't do that for you. If you need/want to shut down
>all the crtcs on driver unload, you need to do that yourself. There's
>atomic helpers to do that for you that for you.

The problem is a sort-of circular dependency between ->lastclose (at
least the common implementation of it), unregister and disabling
fbdev.

I want to move drm_dev_unregister() to be the first thing we do at
rmmod-time. However we need to disable fbdev first, otherwise
->lastclose restores the fbdev mode, guaranteeing that vsync is turned
on for drm_vsync_cleanup() to then WARN on.

There's a slightly different (perceived) problem - the one that Liviu
mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway.
You say it's not the fbdev helpers' responsibility to teardown their
modeset, but regardless I have nowhere to disable the CRTC if I want
to do drm_dev_unregister() first; and if the CRTC isn't disabled
there's always a chance of hitting the same vsync WARN even without
fbdev.

We *could* add an ->unload and disable everything there, but as that's
deprecated I'm guessing there should be another way.
Perhaps we should track ->firstopen/->lastclose pairs so we can detect
that ->lastclose is being called from unregister and use it to
disable everything in that case.

drm_vblank_cleanup() seems to have been carried over to unregister
from drm_put_dev(), but drm_dev_register() doesn't call
drm_vblank_init() so it seems a little strange to have it there.
I can see other drivers I'd expect to hit the same WARN but I don't
have HW to test it on.

Thanks,
Brian

>-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/i2c: tda998x: don't register the connector

[Daniel Vetter]

On Fri, Sep 23, 2016 at 01:52:49PM +0100, Brian Starkey wrote:
> On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote:
> > On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
> > > enabled, but we never got the call to turn off the CRTC. Brian is still
> > > tracking through the fbdev emulation to figure out the cause for that.
> > 
> > fbdev emulation doesn't do that for you. If you need/want to shut down
> > all the crtcs on driver unload, you need to do that yourself. There's
> > atomic helpers to do that for you that for you.
> 
> The problem is a sort-of circular dependency between ->lastclose (at
> least the common implementation of it), unregister and disabling
> fbdev.
> 
> I want to move drm_dev_unregister() to be the first thing we do at
> rmmod-time. However we need to disable fbdev first, otherwise
> ->lastclose restores the fbdev mode, guaranteeing that vsync is turned
> on for drm_vsync_cleanup() to then WARN on.
> 
> There's a slightly different (perceived) problem - the one that Liviu
> mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway.
> You say it's not the fbdev helpers' responsibility to teardown their
> modeset, but regardless I have nowhere to disable the CRTC if I want
> to do drm_dev_unregister() first; and if the CRTC isn't disabled
> there's always a chance of hitting the same vsync WARN even without
> fbdev.

Just disable all crtc in a suitable place (after drm_dev_unregister,
before you tear down fbdev).
> 
> We *could* add an ->unload and disable everything there, but as that's
> deprecated I'm guessing there should be another way.
> Perhaps we should track ->firstopen/->lastclose pairs so we can detect
> that ->lastclose is being called from unregister and use it to
> disable everything in that case.

Hm, maybe we should simply not call ->lastclose for kms drivers. That is
kinda only a hack for ums/dri1 drivers.

But even with that gone you might still unload while fbdev is enabled, so
this won't fix it all.

> drm_vblank_cleanup() seems to have been carried over to unregister
> from drm_put_dev(), but drm_dev_register() doesn't call
> drm_vblank_init() so it seems a little strange to have it there.
> I can see other drivers I'd expect to hit the same WARN but I don't
> have HW to test it on.

Oops. That call to drm_vblank_cleanup() really shouldn't be in there. We
should push it into all callers instead I think.
-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/i2c: tda998x: don't register the connector

[Russell King - ARM Linux]

On Fri, Sep 23, 2016 at 03:13:15PM +0200, Daniel Vetter wrote:
> Hm, maybe we should simply not call ->lastclose for kms drivers. That is
> kinda only a hack for ums/dri1 drivers.

Are you sure about that - isn't it needed so that the fbdev mode gets
restored when the last DRM user exits, so that the VT consoles becomes
functional again?

I ended up needing a call to drm_fb_helper_restore_fbdev_mode_unlocked()
in Armada's ->lastclose to ensure that VT consoles worked after Xorg was
shutdown.

-- 
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/i2c: tda998x: don't register the connector

[Daniel Vetter]

On Fri, Sep 23, 2016 at 03:05:03PM +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 03:13:15PM +0200, Daniel Vetter wrote:
> > Hm, maybe we should simply not call ->lastclose for kms drivers. That is
> > kinda only a hack for ums/dri1 drivers.
> 
> Are you sure about that - isn't it needed so that the fbdev mode gets
> restored when the last DRM user exits, so that the VT consoles becomes
> functional again?
> 
> I ended up needing a call to drm_fb_helper_restore_fbdev_mode_unlocked()
> in Armada's ->lastclose to ensure that VT consoles worked after Xorg was
> shutdown.

I meant not call it on driver unload. When closing the last drm fd we
indeed need it to restore the fbdev vt. That use-case definitely needs to
stay for kms drivers.
-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/i2c: tda998x: don't register the connector

[Brian Starkey]

On Fri, Sep 23, 2016 at 03:13:15PM +0200, Daniel Vetter wrote:
>On Fri, Sep 23, 2016 at 01:52:49PM +0100, Brian Starkey wrote:
>> On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote:
>> > On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>> > > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
>> > > enabled, but we never got the call to turn off the CRTC. Brian is still
>> > > tracking through the fbdev emulation to figure out the cause for that.
>> >
>> > fbdev emulation doesn't do that for you. If you need/want to shut down
>> > all the crtcs on driver unload, you need to do that yourself. There's
>> > atomic helpers to do that for you that for you.
>>
>> The problem is a sort-of circular dependency between ->lastclose (at
>> least the common implementation of it), unregister and disabling
>> fbdev.
>>
>> I want to move drm_dev_unregister() to be the first thing we do at
>> rmmod-time. However we need to disable fbdev first, otherwise
>> ->lastclose restores the fbdev mode, guaranteeing that vsync is turned
>> on for drm_vsync_cleanup() to then WARN on.
>>
>> There's a slightly different (perceived) problem - the one that Liviu
>> mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway.
>> You say it's not the fbdev helpers' responsibility to teardown their
>> modeset, but regardless I have nowhere to disable the CRTC if I want
>> to do drm_dev_unregister() first; and if the CRTC isn't disabled
>> there's always a chance of hitting the same vsync WARN even without
>> fbdev.
>
>Just disable all crtc in a suitable place (after drm_dev_unregister,
>before you tear down fbdev).

I think this is predicated on first removing the drm_vblank_cleanup
call.

>>
>> We *could* add an ->unload and disable everything there, but as that's
>> deprecated I'm guessing there should be another way.
>> Perhaps we should track ->firstopen/->lastclose pairs so we can detect
>> that ->lastclose is being called from unregister and use it to
>> disable everything in that case.
>
>Hm, maybe we should simply not call ->lastclose for kms drivers. That is
>kinda only a hack for ums/dri1 drivers.
>

To be clear (and in response to Russell's question) - you mean
only the call to ->lastclose in drm_dev_unregister, not in general?

>But even with that gone you might still unload while fbdev is 
>enabled, so
>this won't fix it all.
>

Yeah it will be tidier, but I don't think it actually fixes anything.

>> drm_vblank_cleanup() seems to have been carried over to unregister
>> from drm_put_dev(), but drm_dev_register() doesn't call
>> drm_vblank_init() so it seems a little strange to have it there.
>> I can see other drivers I'd expect to hit the same WARN but I don't
>> have HW to test it on.
>
>Oops. That call to drm_vblank_cleanup() really shouldn't be in there. We
>should push it into all callers instead I think.

OK so two things to do - remove drm_vblank_cleanup() from
drm_dev_unregister(), and then do the teardown like so:

	drm_dev_unregister();
	drm_crtc_force_disable_all(); // or atomic equivalent
	fbdev_teardown();
	...

Seems good to me. Are there any ordering constraints you're aware of
for drm_vblank_cleanup()? Or you think just putting it after
drm_dev_unregister() should be OK?

Thanks,
-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/i2c: tda998x: don't register the connector

[Daniel Vetter]

On Fri, Sep 23, 2016 at 03:42:52PM +0100, Brian Starkey wrote:
> On Fri, Sep 23, 2016 at 03:13:15PM +0200, Daniel Vetter wrote:
> > On Fri, Sep 23, 2016 at 01:52:49PM +0100, Brian Starkey wrote:
> > > On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote:
> > > > On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > > > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
> > > > > enabled, but we never got the call to turn off the CRTC. Brian is still
> > > > > tracking through the fbdev emulation to figure out the cause for that.
> > > >
> > > > fbdev emulation doesn't do that for you. If you need/want to shut down
> > > > all the crtcs on driver unload, you need to do that yourself. There's
> > > > atomic helpers to do that for you that for you.
> > > 
> > > The problem is a sort-of circular dependency between ->lastclose (at
> > > least the common implementation of it), unregister and disabling
> > > fbdev.
> > > 
> > > I want to move drm_dev_unregister() to be the first thing we do at
> > > rmmod-time. However we need to disable fbdev first, otherwise
> > > ->lastclose restores the fbdev mode, guaranteeing that vsync is turned
> > > on for drm_vsync_cleanup() to then WARN on.
> > > 
> > > There's a slightly different (perceived) problem - the one that Liviu
> > > mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway.
> > > You say it's not the fbdev helpers' responsibility to teardown their
> > > modeset, but regardless I have nowhere to disable the CRTC if I want
> > > to do drm_dev_unregister() first; and if the CRTC isn't disabled
> > > there's always a chance of hitting the same vsync WARN even without
> > > fbdev.
> > 
> > Just disable all crtc in a suitable place (after drm_dev_unregister,
> > before you tear down fbdev).
> 
> I think this is predicated on first removing the drm_vblank_cleanup
> call.
> 
> > > 
> > > We *could* add an ->unload and disable everything there, but as that's
> > > deprecated I'm guessing there should be another way.
> > > Perhaps we should track ->firstopen/->lastclose pairs so we can detect
> > > that ->lastclose is being called from unregister and use it to
> > > disable everything in that case.
> > 
> > Hm, maybe we should simply not call ->lastclose for kms drivers. That is
> > kinda only a hack for ums/dri1 drivers.
> > 
> 
> To be clear (and in response to Russell's question) - you mean
> only the call to ->lastclose in drm_dev_unregister, not in general?

Yup. But I also don't think it's needed, since drm_vblanke_cleanup seems
to be the root-cause offender.

> > But even with that gone you might still unload while fbdev is enabled,
> > so
> > this won't fix it all.
> > 
> 
> Yeah it will be tidier, but I don't think it actually fixes anything.
> 
> > > drm_vblank_cleanup() seems to have been carried over to unregister
> > > from drm_put_dev(), but drm_dev_register() doesn't call
> > > drm_vblank_init() so it seems a little strange to have it there.
> > > I can see other drivers I'd expect to hit the same WARN but I don't
> > > have HW to test it on.
> > 
> > Oops. That call to drm_vblank_cleanup() really shouldn't be in there. We
> > should push it into all callers instead I think.
> 
> OK so two things to do - remove drm_vblank_cleanup() from
> drm_dev_unregister(), and then do the teardown like so:
> 
> 	drm_dev_unregister();
> 	drm_crtc_force_disable_all(); // or atomic equivalent
> 	fbdev_teardown();
> 	...
> 
> Seems good to me. Are there any ordering constraints you're aware of
> for drm_vblank_cleanup()? Or you think just putting it after
> drm_dev_unregister() should be OK?

Since that's where it's currently is, mechanically pushing it right after
it for each caller should be fine. Driver maintainers can then move it to
an even more suitable place on their own ;-)

Also placing drm_vblank_cleanup in drm_dev_put() makes it even more
obvious that drivers really shouldn't use that function any more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch