Re: [PATCH] OMAPDSS: enable omapdss for ARCH_MULTIPLATFORM
On Thursday 07 February 2013, Tomi Valkeinen wrote: > > I would suggest only the first patch, since Linus quite specifically > > asked only for serious bug fixes. I think an allyesconfig build > > breakage is serious enough, but doing multiple patches for one > > bug should not be necessary and is much harder to justify. > > Well, as I see, the bug is omapdrm using "select", not "depends on". So > if I'd have to pick one patch, I'd send 0003. That would fix allyesconfig. > > Applying only 0003 means that omapdrm (and omapdss) won't be built on > ARCH_MULTIPLATFORM, but building omapdrm on ARCH_MULTIPLATFORM is a > feature added in this merge window, so leaving it out is not a regression. > > But I'm not sure if I'm being overly pedantic here. Patch 0001 would fix > allyesconfig, and allow building omapdrm with ARCH_MULTIPLATFORM. > However, the fixing there happens as a side effect, and so the 0001 > patch isn't even called a "fix" in its subject. That's the reason I'm a > bit reluctant with the 0001 patch as a fix. Makes sense. I suggested the same initially, but Rob preferred enabling DSS on multiplatform as well, since he wants to see broad build coverage for the DRM part. We should probably do that in 3.9, but for 3.8, just changing DRM to depend on DSS seems like the least intrusive choice. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAPDSS: enable omapdss for ARCH_MULTIPLATFORM
On 2013-02-07 13:55, Arnd Bergmann wrote: > omapdrm still has the 'select' statement in it if you only send the > first patch, so it should not be possible to disable omapdss when > it is actually needed. Yes, you're right. After adding ARCH_MULTIPLATFORM, the kconfig enables omapdss fully even if enabled by select. So select is only a problem if omapdss is enabled when omapdss's dependencies are missing. > I suggested doing only the minimum that is needed to unbreak the > allyesconfig build, which is to enable just omapdss but not omapfb > and the displays, in case they don't actually build on anything else. omapfb and the displays build also fine on all platforms. But it's true that they are not needed to fix allyesconfig. > I would suggest only the first patch, since Linus quite specifically > asked only for serious bug fixes. I think an allyesconfig build > breakage is serious enough, but doing multiple patches for one > bug should not be necessary and is much harder to justify. Well, as I see, the bug is omapdrm using "select", not "depends on". So if I'd have to pick one patch, I'd send 0003. That would fix allyesconfig. Applying only 0003 means that omapdrm (and omapdss) won't be built on ARCH_MULTIPLATFORM, but building omapdrm on ARCH_MULTIPLATFORM is a feature added in this merge window, so leaving it out is not a regression. But I'm not sure if I'm being overly pedantic here. Patch 0001 would fix allyesconfig, and allow building omapdrm with ARCH_MULTIPLATFORM. However, the fixing there happens as a side effect, and so the 0001 patch isn't even called a "fix" in its subject. That's the reason I'm a bit reluctant with the 0001 patch as a fix. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] OMAPDSS: enable omapdss for ARCH_MULTIPLATFORM
On Thursday 07 February 2013, Tomi Valkeinen wrote: > On 2013-02-06 16:29, Arnd Bergmann wrote: > > On Wednesday 06 February 2013 16:15:57 Tomi Valkeinen wrote: > > >> I have patches to add the ARCH_MULTIPLATFORM for omapdss, and to fix the > >> omap_vout and omapdrm Kconfig files. Each of them changes only one line. > >> Arnd, are you ok with queuing those patches via arm-soc/fixes? Or should > >> I send them individually to respective maintainers? > >> > >> I can send the patches properly for review, but I've also attached them > >> here so Rob can test. > > > > The second and third attachment in your mail seem to contain identical > > patches. I suggested a similar patch myself, but Rob thought his > > Hmm? They look different to me... > 0002-omap_vout-fix-the-dependency-on-OMAPDSS.patch fixes the dependency > for omap_vout, 0003-omapdrm-fix-the-dependency-on-OMAPDSS.patch for omapdrm. Right, my mistake. > > version was nicer to give better build coverage. We only need either > > Rob's patch or yours, but not both, as far as I can tell. > > I read the related posts, but I'm a bit confused here. Let me summarize > what has happened and what are the issues: > > I changed omapdss, omapfb and omap panel drivers to be platform > independent, and after that they compiled fine on OMAP and x86, and > should compile fine on any other platform as well. I thus removed the > Kconfig build dependencies for OMAP. This is what I sent for 3.8 merge > window. > > However, Linus complained that now he's getting asked if he wants to > enable omapdss driver when he's building x86 kernel. So Tony made a > patch that added the ARCH_OMAP2PLUS dependency to omapdss (and some > other omap drivers), which went in. > > Now, if I'm not mistaken, Rob then added possibility to build omapdrm on > ARCH_MULTIPLATFORM. No problem with that as such, but omapdrm's Kconfig > uses select to enable omapdss, which does not work. omapdss was not made > to work with others using "select" to enable it, but one should "depend > on" to it. > > This caused omapdss to be enabled partially when omapdrm is enabled on > ARCH_MULTIPLATFORM, causing build failure. All correct, yes. > So the real fix to the issue is the 0003 patch, which changes omapdrm to > use "depend on", not "select". However, adding only that patch will > prevent omapdrm to be built on ARCH_MULTIPLATFORM, as omapdss is not > enabled on ARCH_MULTIPLATFORM. That one can be enabled with the 0001 patch. > > Adding only 0001 patch will also "fix" the build issue, as then omapdss > is properly enabled on allyesconfig ARCH_MULTIPLATFORM build, even if > omapdrm erroneously uses select to enable omapdss. However, adding only > 0001 will still allow the user to manually break the build by disabling > omapdss (I think, I didn't test that). omapdrm still has the 'select' statement in it if you only send the first patch, so it should not be possible to disable omapdss when it is actually needed. > The difference between my 0001 patch and Rob's patch is that Rob only > enables omapdss to be built on ARCH_MULTIPLATFORM, leaving omapfb and > omap panel drivers out. Leaving omapfb is not an issue, but if the panel > drivers are left out, I don't see how omapdrm can function properly, > even if compilation works fine. I suggested doing only the minimum that is needed to unbreak the allyesconfig build, which is to enable just omapdss but not omapfb and the displays, in case they don't actually build on anything else. > > Olof can correct me, but I think we currently don't have any other > > patches queued in arm-soc for 3.8 (after Linus announced he did > > not want any of the less urgent ones), so I think it would be more > > fitting if you send one of the patches to Linus, rather than having > > an arm-soc pull request that only contains one patch in your domain. > > Ok, I'll do that. I'm still not sure if I should send only the 0001 > patch, or all three. I guess I'll go for all three if nobody objects. > > Rob, can you test the patches so we're sure they do what they are > supposed to? I would suggest only the first patch, since Linus quite specifically asked only for serious bug fixes. I think an allyesconfig build breakage is serious enough, but doing multiple patches for one bug should not be necessary and is much harder to justify. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAPDSS: enable omapdss for ARCH_MULTIPLATFORM
On 2013-02-06 16:29, Arnd Bergmann wrote: > On Wednesday 06 February 2013 16:15:57 Tomi Valkeinen wrote: >> I have patches to add the ARCH_MULTIPLATFORM for omapdss, and to fix the >> omap_vout and omapdrm Kconfig files. Each of them changes only one line. >> Arnd, are you ok with queuing those patches via arm-soc/fixes? Or should >> I send them individually to respective maintainers? >> >> I can send the patches properly for review, but I've also attached them >> here so Rob can test. > > The second and third attachment in your mail seem to contain identical > patches. I suggested a similar patch myself, but Rob thought his Hmm? They look different to me... 0002-omap_vout-fix-the-dependency-on-OMAPDSS.patch fixes the dependency for omap_vout, 0003-omapdrm-fix-the-dependency-on-OMAPDSS.patch for omapdrm. > version was nicer to give better build coverage. We only need either > Rob's patch or yours, but not both, as far as I can tell. I read the related posts, but I'm a bit confused here. Let me summarize what has happened and what are the issues: I changed omapdss, omapfb and omap panel drivers to be platform independent, and after that they compiled fine on OMAP and x86, and should compile fine on any other platform as well. I thus removed the Kconfig build dependencies for OMAP. This is what I sent for 3.8 merge window. However, Linus complained that now he's getting asked if he wants to enable omapdss driver when he's building x86 kernel. So Tony made a patch that added the ARCH_OMAP2PLUS dependency to omapdss (and some other omap drivers), which went in. Now, if I'm not mistaken, Rob then added possibility to build omapdrm on ARCH_MULTIPLATFORM. No problem with that as such, but omapdrm's Kconfig uses select to enable omapdss, which does not work. omapdss was not made to work with others using "select" to enable it, but one should "depend on" to it. This caused omapdss to be enabled partially when omapdrm is enabled on ARCH_MULTIPLATFORM, causing build failure. So the real fix to the issue is the 0003 patch, which changes omapdrm to use "depend on", not "select". However, adding only that patch will prevent omapdrm to be built on ARCH_MULTIPLATFORM, as omapdss is not enabled on ARCH_MULTIPLATFORM. That one can be enabled with the 0001 patch. Adding only 0001 patch will also "fix" the build issue, as then omapdss is properly enabled on allyesconfig ARCH_MULTIPLATFORM build, even if omapdrm erroneously uses select to enable omapdss. However, adding only 0001 will still allow the user to manually break the build by disabling omapdss (I think, I didn't test that). The difference between my 0001 patch and Rob's patch is that Rob only enables omapdss to be built on ARCH_MULTIPLATFORM, leaving omapfb and omap panel drivers out. Leaving omapfb is not an issue, but if the panel drivers are left out, I don't see how omapdrm can function properly, even if compilation works fine. > Olof can correct me, but I think we currently don't have any other > patches queued in arm-soc for 3.8 (after Linus announced he did > not want any of the less urgent ones), so I think it would be more > fitting if you send one of the patches to Linus, rather than having > an arm-soc pull request that only contains one patch in your domain. Ok, I'll do that. I'm still not sure if I should send only the 0001 patch, or all three. I guess I'll go for all three if nobody objects. Rob, can you test the patches so we're sure they do what they are supposed to? Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] OMAPDSS: enable omapdss for ARCH_MULTIPLATFORM
On Wednesday 06 February 2013 16:15:57 Tomi Valkeinen wrote: > > That would normally be me, but I've been on a long leave, and just came > back. It'll take me some time to get back on track. > > I don't think it makes sense to add ARCH_MULTIPLATFORM only for omapdss, > like this patch does. I think we should add it for omapfb and the panel > drivers also. > > I did get a report of an omapdss build-break with allyesconfig. It seems > to happen because omapdrm and omap_vout use "select" to enable omapdss, > instead of depending on omapdss. This causes an illegal config to be > created. Perhaps this is the problem that Rob mentions in his patch? Yes, that is the one. It is one of three remaining build errors we get for ARM allyesconfig, and the other two also have fixes on their way into 3.8. > Adding ARCH_MULTIPLATFORM for omapdss, as this patch does, removes > (well, hides) the above mentioned problem and allyesconfig works ok. You > can still break the config, but you need to manually select the Kconfig > options the wrong way. > > I have patches to add the ARCH_MULTIPLATFORM for omapdss, and to fix the > omap_vout and omapdrm Kconfig files. Each of them changes only one line. > Arnd, are you ok with queuing those patches via arm-soc/fixes? Or should > I send them individually to respective maintainers? > > I can send the patches properly for review, but I've also attached them > here so Rob can test. The second and third attachment in your mail seem to contain identical patches. I suggested a similar patch myself, but Rob thought his version was nicer to give better build coverage. We only need either Rob's patch or yours, but not both, as far as I can tell. Olof can correct me, but I think we currently don't have any other patches queued in arm-soc for 3.8 (after Linus announced he did not want any of the less urgent ones), so I think it would be more fitting if you send one of the patches to Linus, rather than having an arm-soc pull request that only contains one patch in your domain. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAPDSS: enable omapdss for ARCH_MULTIPLATFORM
Hi Arnd, Rob, On 2013-02-05 18:31, Arnd Bergmann wrote: > On Tuesday 22 January 2013, Arnd Bergmann wrote: >> On Tuesday 22 January 2013, Rob Clark wrote: >>> At least core omapdss does not have any build dependencies on >>> ARCH_OMAP2PLUS, and adding this dependency in the Kconfig breaks omapdrm >>> for ARCH_MULTIPLATFORM builds. >>> >>> Signed-off-by: Rob Clark >> >> Acked-by: Arnd Bergmann > > It seems nobody has applied this patch, as it has not shown up in > linux-next or upstream. Who should take care of this? If nobody feels > responsible, I can put it into arm-soc/fixes. That would normally be me, but I've been on a long leave, and just came back. It'll take me some time to get back on track. I don't think it makes sense to add ARCH_MULTIPLATFORM only for omapdss, like this patch does. I think we should add it for omapfb and the panel drivers also. I did get a report of an omapdss build-break with allyesconfig. It seems to happen because omapdrm and omap_vout use "select" to enable omapdss, instead of depending on omapdss. This causes an illegal config to be created. Perhaps this is the problem that Rob mentions in his patch? Adding ARCH_MULTIPLATFORM for omapdss, as this patch does, removes (well, hides) the above mentioned problem and allyesconfig works ok. You can still break the config, but you need to manually select the Kconfig options the wrong way. I have patches to add the ARCH_MULTIPLATFORM for omapdss, and to fix the omap_vout and omapdrm Kconfig files. Each of them changes only one line. Arnd, are you ok with queuing those patches via arm-soc/fixes? Or should I send them individually to respective maintainers? I can send the patches properly for review, but I've also attached them here so Rob can test. Tomi From b2a01a69089c965a703125e001037eff4f09af4e Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen Date: Wed, 6 Feb 2013 15:21:14 +0200 Subject: [PATCH 1/3] OMAPDSS: add support for ARCH_MULTIPLATFORM omapdss, omapfb and omap's panel drivers currently depend on ARCH_OMAP2PLUS. To make omapdss usable on multiplatform builds, add an optinal dependency to ARCH_MULTIPLATFORM. No other change is needed, as omapdss, omapfb and panel drivers already compile on non-omap platforms. Signed-off-by: Tomi Valkeinen --- drivers/video/omap2/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/omap2/Kconfig b/drivers/video/omap2/Kconfig index b07b2b0..e1cd489 100644 --- a/drivers/video/omap2/Kconfig +++ b/drivers/video/omap2/Kconfig @@ -1,7 +1,7 @@ config OMAP2_VRFB bool -if ARCH_OMAP2PLUS +if ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM source "drivers/video/omap2/dss/Kconfig" source "drivers/video/omap2/omapfb/Kconfig" -- 1.7.10.4 From 61fb1e64b5374510bfaeb2137a5af5f6ea33ba34 Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen Date: Wed, 6 Feb 2013 15:54:08 +0200 Subject: [PATCH 2/3] omap_vout: fix the dependency on OMAPDSS omap_vout uses "select" in Kconfig to enable omapdss. This doesn't work correctly, as "select" forces omapdss to be enabled in the config even if it normally could not be enabled. Instead of using select, this patch changes omap_vout to use "depend on". Signed-off-by: Tomi Valkeinen --- drivers/media/platform/omap/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/omap/Kconfig b/drivers/media/platform/omap/Kconfig index 390ab09..c102615 100644 --- a/drivers/media/platform/omap/Kconfig +++ b/drivers/media/platform/omap/Kconfig @@ -4,9 +4,9 @@ config VIDEO_OMAP2_VOUT_VRFB config VIDEO_OMAP2_VOUT tristate "OMAP2/OMAP3 V4L2-Display driver" depends on ARCH_OMAP2 || ARCH_OMAP3 + depends on OMAP2_DSS select VIDEOBUF_GEN select VIDEOBUF_DMA_CONTIG - select OMAP2_DSS select OMAP2_VRFB if ARCH_OMAP2 || ARCH_OMAP3 select VIDEO_OMAP2_VOUT_VRFB if VIDEO_OMAP2_VOUT && OMAP2_VRFB default n -- 1.7.10.4 From aca9e04648a6cef98f65ff754bf903f456c8bc5d Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen Date: Wed, 6 Feb 2013 16:03:44 +0200 Subject: [PATCH 3/3] omapdrm: fix the dependency on OMAPDSS omapdrm uses "select" in Kconfig to enable omapdss. This doesn't work correctly, as "select" forces omapdss to be enabled in the config even if it normally could not be enabled. Instead of using select, this patch changes omapdrm to use "depend on". Signed-off-by: Tomi Valkeinen --- drivers/staging/omapdrm/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/omapdrm/Kconfig b/drivers/staging/omapdrm/Kconfig index b724a41..09f65dc 100644 --- a/drivers/staging/omapdrm/Kconfig +++ b/drivers/staging/omapdrm/Kconfig @@ -3,8 +3,8 @@ config DRM_OMAP tristate "OMAP DRM" depends on DRM && !CONFIG_FB_OMAP2 depends on ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM + depends on OMAP2_DSS select DRM_KMS_HELPER - select OMAP2_DSS select FB_SYS_FILLRECT select FB_SYS_COPYAREA select FB_SYS_IMAGEBLIT -- 1.7.10.4 signature.asc Description: OpenPGP dig
Re: [PATCH] OMAPDSS: enable omapdss for ARCH_MULTIPLATFORM
On Tuesday 22 January 2013, Arnd Bergmann wrote: > On Tuesday 22 January 2013, Rob Clark wrote: > > At least core omapdss does not have any build dependencies on > > ARCH_OMAP2PLUS, and adding this dependency in the Kconfig breaks omapdrm > > for ARCH_MULTIPLATFORM builds. > > > > Signed-off-by: Rob Clark > > Acked-by: Arnd Bergmann It seems nobody has applied this patch, as it has not shown up in linux-next or upstream. Who should take care of this? If nobody feels responsible, I can put it into arm-soc/fixes. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAPDSS: enable omapdss for ARCH_MULTIPLATFORM
On Tuesday 22 January 2013, Rob Clark wrote: > At least core omapdss does not have any build dependencies on > ARCH_OMAP2PLUS, and adding this dependency in the Kconfig breaks omapdrm > for ARCH_MULTIPLATFORM builds. > > Signed-off-by: Rob Clark Acked-by: Arnd Bergmann -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/