| Message ID | 1403014631-18072-10-git-send-email-guido@vanguardiasur.com.ar |
|---|---|
| State | New |
| Headers | show |
On 06/17/2014 09:17 AM, Guido Martínez wrote: > Use module_init instead of late_initcall, as is the norm for modular > drivers. > > module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b > ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall, > but it does not explain why. Tests show it's working properly with > module_init. > If I recall, the late_initcall stuff was done to try and make sure the tda998x/i2c subsystem came up before tilcdc. However it didn't always work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try and ensure the ordering. This might be why changing back to module_init is fine (and I agree with your assessment from my testing). > Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar> > --- > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > index 2c860c4..6be623b 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c > @@ -629,7 +629,7 @@ static void __exit tilcdc_drm_fini(void) > tilcdc_tfp410_fini(); > } > > -late_initcall(tilcdc_drm_init); > +module_init(tilcdc_drm_init); > module_exit(tilcdc_drm_fini); > > MODULE_AUTHOR("Rob Clark <robdclark@gmail.com"); >
On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote: > On 06/17/2014 09:17 AM, Guido Martínez wrote: >> Use module_init instead of late_initcall, as is the norm for modular >> drivers. >> >> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b >> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall, >> but it does not explain why. Tests show it's working properly with >> module_init. >> > > If I recall, the late_initcall stuff was done to try and make sure the > tda998x/i2c subsystem came up before tilcdc. However it didn't always > work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try > and ensure the ordering. This might be why changing back to module_init > is fine (and I agree with your assessment from my testing). There's a solution to that... I have patches which convert the tda998x driver to also register into the component helpers as well as remaining as a drm slave device. This makes it possible to transition tilcdc to use the component helper to solve the initialisation ordering. I'll (re-)post them later today.
On Wed, Jun 25, 2014 at 02:00:42PM +0100, Russell King - ARM Linux wrote: > On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote: > > On 06/17/2014 09:17 AM, Guido Martínez wrote: > >> Use module_init instead of late_initcall, as is the norm for modular > >> drivers. > >> > >> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b > >> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall, > >> but it does not explain why. Tests show it's working properly with > >> module_init. > >> > > > > If I recall, the late_initcall stuff was done to try and make sure the > > tda998x/i2c subsystem came up before tilcdc. However it didn't always > > work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try > > and ensure the ordering. This might be why changing back to module_init > > is fine (and I agree with your assessment from my testing). > > There's a solution to that... I have patches which convert the tda998x > driver to also register into the component helpers as well as remaining > as a drm slave device. This makes it possible to transition tilcdc to > use the component helper to solve the initialisation ordering. > > I'll (re-)post them later today. Except Daniel Mack will not be receiving any copies of them: zonque@gmail.com SMTP error from remote mail server after end of data: host gmail-smtp-in.l.google.com [2a00:1450:400c:c03::1a]: 550-5.7.1 [2001:4d48:ad52:3201:214:fdff:fe10:1be6 12] Our system has 550-5.7.1 detected that this message is likely unsolicited mail. To reduce the 550-5.7.1 amount of spam sent to Gmail, this message has been blocked. Please 550-5.7.1 visit 550-5.7.1 http://support.google.com/mail/bin/answer.py?hl=en&answer=188131 for 550 5.7.1 more information. fs8si6627678wib.99 - gsmtp Google's anti-ham filter seems to be working perfectly, allowing spam through and blocking real messages... as usual. And as usual, google provides no support for this crap. Gmail has a history of increasingly blocking legitimate email in their alleged anti-spam fight. Don't use gmail.
(Ccing Guido back) Hello Russell, Darren, On 25 Jun 02:00 PM, Russell King - ARM Linux wrote: > On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote: > > On 06/17/2014 09:17 AM, Guido Martínez wrote: > >> Use module_init instead of late_initcall, as is the norm for modular > >> drivers. > >> > >> module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b > >> ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall, > >> but it does not explain why. Tests show it's working properly with > >> module_init. > >> > > > > If I recall, the late_initcall stuff was done to try and make sure the > > tda998x/i2c subsystem came up before tilcdc. That doesn't make any sense. Using late_initcall for the tilcdc DRM driver would make the tilcdc DRM get probed before any other regular module_init driver, including the tda998x encoder. > > However it didn't always > > work so we added commit: 39de6194131c155901f96686a063212656d80c2e to try > > and ensure the ordering. This might be why changing back to module_init > > is fine (and I agree with your assessment from my testing). That commit is adds a proper probe deferal mechanism in case the i2c is not available. OMAP is special because it has its own nasty initcall to probe the i2c busses. However, that should be fixed and then i2c would be always probed *after* the DRM, as per the ordering in drivers/Makefile. > > There's a solution to that... A solution to *what* ? > I have patches which convert the tda998x > driver to also register into the component helpers as well as remaining > as a drm slave device. This makes it possible to transition tilcdc to > use the component helper to solve the initialisation ordering. > AFAIK, there's no issue with the initialisation ordering, except that the DRM driver is currently probed before the TDA998x encoder and so the probe is defered. Unless I'm missing something that's very easy to fix, you just need to change the link order to guarantee that i2c encoders are probed *before* the DRM drivers that require them. I have just posted a very simple patch to fix it, but it seems Thierry wasn't happy with it, not sure why yet [1]. [1] http://www.spinics.net/lists/dri-devel/msg61987.html
On Wed, Jun 25, 2014 at 11:32:46AM -0300, Ezequiel García wrote: > (Ccing Guido back) > > Hello Russell, Darren, > > On 25 Jun 02:00 PM, Russell King - ARM Linux wrote: > > On Tue, Jun 24, 2014 at 05:04:36PM -0500, Darren Etheridge wrote: > > > If I recall, the late_initcall stuff was done to try and make sure the > > > tda998x/i2c subsystem came up before tilcdc. > > That doesn't make any sense. Using late_initcall for the tilcdc DRM > driver would make the tilcdc DRM get probed before any other regular > module_init driver, including the tda998x encoder. A module_init() is a device_initcall(), which is at level 6. A late_initcall() is at level 7. Level 6 initcalls are run before level 7 initcalls. The tda998x is a module_init(), so the tda998x gets initialised *before* tilcdc's late_initcall(). Now, if you build everything as a module, then you have no initialisation ordering, and you can't rely on any kind of order. Initialisation functions can even run in parallel on different CPUs due to modules being loaded from userspace in a multi-threaded way. So anything which relies on a certain initcall ordering is fundamentally broken if it can be modular. > > There's a solution to that... > > A solution to *what* ? Maybe if you left the context above that line in place, you might understand that my "that" refers to what was discussed in that context. That being the initialisation ordering. Convention and proper Internet etiquette is to trim the quoted text and place relies below the paragraph to which they refer, the quoted paragraph giving the context to the reply.
Hi Russell, On 25 Jun 03:46 PM, Russell King - ARM Linux wrote: > > > > That doesn't make any sense. Using late_initcall for the tilcdc DRM > > driver would make the tilcdc DRM get probed before any other regular > > module_init driver, including the tda998x encoder. > > A module_init() is a device_initcall(), which is at level 6. A > late_initcall() is at level 7. Level 6 initcalls are run before level > 7 initcalls. The tda998x is a module_init(), so the tda998x gets > initialised *before* tilcdc's late_initcall(). > Oh, thanks a lot for correcting me. > Now, if you build everything as a module, then you have no initialisation > ordering, and you can't rely on any kind of order. Initialisation > functions can even run in parallel on different CPUs due to modules being > loaded from userspace in a multi-threaded way. So anything which relies > on a certain initcall ordering is fundamentally broken if it can be > modular. > OK, but is there any outstanding issue with the tilcdc initialization, either when compiled as built-in or as modules, *after* this patchset? Of course, this driver only worked as built-in (see the horrible bugs we are fixing here, that prevented unloading and re-loading it). This series fixes all such bugs, and also adds the required changes to allow to build everything as a module. Again, I can't see any issues that remain to be fixed after this patchset. When compiled as built-in, the tilcdc DRM will be defered and once the tda998x is ready, the tilcdc DRM will load properly. When compiled as module you can load any of them in any order. If the tda998x module is not loaded by the time you load the tilcdc module, the former will get loaded. Hm... now that I think about this. We still get into trouble if the tda998x is built as module, but the tilcdc is built-in. Shouldn't we just forbid that? Either both built-ins or both as modules? > > > There's a solution to that... > > > > A solution to *what* ? > > Maybe if you left the context above that line in place, you might > understand that my "that" refers to what was discussed in that > context. That being the initialisation ordering. > > Convention and proper Internet etiquette is to trim the quoted text > and place relies below the paragraph to which they refer, the quoted > paragraph giving the context to the reply. > Yeah, sorry about that.
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 2c860c4..6be623b 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -629,7 +629,7 @@ static void __exit tilcdc_drm_fini(void) tilcdc_tfp410_fini(); } -late_initcall(tilcdc_drm_init); +module_init(tilcdc_drm_init); module_exit(tilcdc_drm_fini); MODULE_AUTHOR("Rob Clark <robdclark@gmail.com");
Use module_init instead of late_initcall, as is the norm for modular drivers. module_init was used until 6e8de0bd6a51fdeebd5d975c4fcc426f730b339b ("drm/tilcdc: add encoder slave (v2)") changed it to a late_initcall, but it does not explain why. Tests show it's working properly with module_init. Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar> --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)