| Message ID | 1358788568-11137-16-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/21/2013 11:16 AM, Arnd Bergmann wrote: > The omapdrm driver is incorrectly flagged to allow building > on non-omap platforms, when ARCH_MULTIPLATFORM is set. > > This does not work, because it unconditionally selects > the OMAP2_DSS symbol that only works on OMAP. > The problem was introduced in 5e3b087499 "staging: > drm/omap: add support for ARCH_MULTIPLATFORM", which this > patch partly reverts. Are you sure OMAP2_DSS requires ARCH_OMAP2PLUS? I don't see this, and it at least used to not depend on ARCH_OMAP2PLUS. If it does now, I think the correct fix would be to remove the dependency in OMAP2_DSS. I don't think removing ARCH_MULTIPLATFORM support in omapdrm is the correct solution. > Without this patch, building allyesconfig results in: > > warning: (VIDEO_OMAP2_VOUT && DRM_OMAP) selects OMAP2_DSS which has unmet direct dependencies (HAS_IOMEM && ARCH_OMAP2PLUS) > warning: (VIDEO_OMAP2_VOUT && DRM_OMAP) selects OMAP2_DSS which has unmet direct dependencies (HAS_IOMEM && ARCH_OMAP2PLUS) > drivers/video/omap2/dss/dss.c: In function 'dss_calc_clock_div': > drivers/video/omap2/dss/dss.c:572:20: error: 'CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK' undeclared (first use in this function) > drivers/video/omap2/dss/dss.c:572:20: note: each undeclared identifier is reported only once for each function it appears in > drivers/staging/omapdrm/omap_connector.c: In function 'omap_connector_dpms': > drivers/staging/omapdrm/omap_connector.c:116:8: error: 'OMAP_DSS_DISPLAY_SUSPENDED' undeclared (first use in this function) > drivers/staging/omapdrm/omap_connector.c:116:8: note: each undeclared identifier is reported only once for each function it appears in This was an unrelated build break which should be fixed in latest master after 'staging: drm/omap: use omapdss low level API' BR, -R > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Cc: Rob Clark <rob@ti.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > 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..81a7cba 100644 > --- a/drivers/staging/omapdrm/Kconfig > +++ b/drivers/staging/omapdrm/Kconfig > @@ -2,7 +2,7 @@ > config DRM_OMAP > tristate "OMAP DRM" > depends on DRM && !CONFIG_FB_OMAP2 > - depends on ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM > + depends on ARCH_OMAP2PLUS > select DRM_KMS_HELPER > select OMAP2_DSS > select FB_SYS_FILLRECT
On 01/21/2013 11:26 AM, Rob Clark wrote: > On 01/21/2013 11:16 AM, Arnd Bergmann wrote: >> The omapdrm driver is incorrectly flagged to allow building >> on non-omap platforms, when ARCH_MULTIPLATFORM is set. >> >> This does not work, because it unconditionally selects >> the OMAP2_DSS symbol that only works on OMAP. >> The problem was introduced in 5e3b087499 "staging: >> drm/omap: add support for ARCH_MULTIPLATFORM", which this >> patch partly reverts. > > Are you sure OMAP2_DSS requires ARCH_OMAP2PLUS? I don't see this, and > it at least used to not depend on ARCH_OMAP2PLUS. If it does now, I > think the correct fix would be to remove the dependency in OMAP2_DSS. > I don't think removing ARCH_MULTIPLATFORM support in omapdrm is the > correct solution. hmm, yes, it does appear that OMAP2_DSS depends on ARCH_OMAP2PLUS.. let me have a quick look at this, fixing omapdss would be a better solution.. BR, -R > > >> Without this patch, building allyesconfig results in: >> >> warning: (VIDEO_OMAP2_VOUT && DRM_OMAP) selects OMAP2_DSS which has >> unmet direct dependencies (HAS_IOMEM && ARCH_OMAP2PLUS) >> warning: (VIDEO_OMAP2_VOUT && DRM_OMAP) selects OMAP2_DSS which has >> unmet direct dependencies (HAS_IOMEM && ARCH_OMAP2PLUS) >> drivers/video/omap2/dss/dss.c: In function 'dss_calc_clock_div': >> drivers/video/omap2/dss/dss.c:572:20: error: >> 'CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK' undeclared (first use in this >> function) >> drivers/video/omap2/dss/dss.c:572:20: note: each undeclared >> identifier is reported only once for each function it appears in >> drivers/staging/omapdrm/omap_connector.c: In function >> 'omap_connector_dpms': >> drivers/staging/omapdrm/omap_connector.c:116:8: error: >> 'OMAP_DSS_DISPLAY_SUSPENDED' undeclared (first use in this function) >> drivers/staging/omapdrm/omap_connector.c:116:8: note: each undeclared >> identifier is reported only once for each function it appears in > > > This was an unrelated build break which should be fixed in latest > master after 'staging: drm/omap: use omapdss low level API' > > BR, > -R > >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> Cc: Rob Clark <rob@ti.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> --- >> 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..81a7cba 100644 >> --- a/drivers/staging/omapdrm/Kconfig >> +++ b/drivers/staging/omapdrm/Kconfig >> @@ -2,7 +2,7 @@ >> config DRM_OMAP >> tristate "OMAP DRM" >> depends on DRM && !CONFIG_FB_OMAP2 >> - depends on ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM >> + depends on ARCH_OMAP2PLUS >> select DRM_KMS_HELPER >> select OMAP2_DSS >> select FB_SYS_FILLRECT >
On Monday 21 January 2013, Rob Clark wrote: > Are you sure OMAP2_DSS requires ARCH_OMAP2PLUS? I don't see this, and > it at least used to not depend on ARCH_OMAP2PLUS. If it does now, I > think the correct fix would be to remove the dependency in OMAP2_DSS. I > don't think removing ARCH_MULTIPLATFORM support in omapdrm is the > correct solution. At least it says so in drivers/video/omap2/Kconfig, which contains if ARCH_OMAP2PLUS source drivers/video/omap2/dss/Kconfig endif We can probably change this, but until we do, we should not select OMAP2_DSS from something that doesn't also depend on ARCH_OMAP2PLUS. > > drivers/video/omap2/dss/dss.c: In function 'dss_calc_clock_div': > > drivers/video/omap2/dss/dss.c:572:20: error: 'CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK' undeclared (first use in this function) > > drivers/video/omap2/dss/dss.c:572:20: note: each undeclared identifier is reported only once for each function it appears in > > drivers/staging/omapdrm/omap_connector.c: In function 'omap_connector_dpms': > > drivers/staging/omapdrm/omap_connector.c:116:8: error: 'OMAP_DSS_DISPLAY_SUSPENDED' undeclared (first use in this function) > > drivers/staging/omapdrm/omap_connector.c:116:8: note: each undeclared identifier is reported only once for each function it appears in > > > This was an unrelated build break which should be fixed in latest master > after 'staging: drm/omap: use omapdss low level API' Ok, it seems the message is stale then, I created the patch some time ago, but only today wrote rest of the explanation for the changeset text. With all the other patches from my series applied, allyesconfig still gives me drivers/video/omap2/dss/dss.c:572:20: error: 'CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK' undeclared because that symbol is only defined when OMAP2_DSS is enabled rather than selected. Changing drivers/video/omap2/Kconfig to not depend on OMAP seems to work as well, but that seems a little intrusive for v3.8, because it would let a lot of people build that code that have no use for it. Arnd
On 01/21/2013 11:41 AM, Arnd Bergmann wrote: > On Monday 21 January 2013, Rob Clark wrote: >> Are you sure OMAP2_DSS requires ARCH_OMAP2PLUS? I don't see this, and >> it at least used to not depend on ARCH_OMAP2PLUS. If it does now, I >> think the correct fix would be to remove the dependency in OMAP2_DSS. I >> don't think removing ARCH_MULTIPLATFORM support in omapdrm is the >> correct solution. > At least it says so in drivers/video/omap2/Kconfig, which contains > > if ARCH_OMAP2PLUS > source drivers/video/omap2/dss/Kconfig > endif ahh, ok, I see.. the if ARCH_OMAP2PLUS bit looks like it came in recently (770b6cb) what about changing this to 'if ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM'? BR, -R > We can probably change this, but until we do, we should not select > OMAP2_DSS from something that doesn't also depend on ARCH_OMAP2PLUS. > >>> drivers/video/omap2/dss/dss.c: In function 'dss_calc_clock_div': >>> drivers/video/omap2/dss/dss.c:572:20: error: 'CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK' undeclared (first use in this function) >>> drivers/video/omap2/dss/dss.c:572:20: note: each undeclared identifier is reported only once for each function it appears in >>> drivers/staging/omapdrm/omap_connector.c: In function 'omap_connector_dpms': >>> drivers/staging/omapdrm/omap_connector.c:116:8: error: 'OMAP_DSS_DISPLAY_SUSPENDED' undeclared (first use in this function) >>> drivers/staging/omapdrm/omap_connector.c:116:8: note: each undeclared identifier is reported only once for each function it appears in >> >> This was an unrelated build break which should be fixed in latest master >> after 'staging: drm/omap: use omapdss low level API' > Ok, it seems the message is stale then, I created the patch some time ago, but > only today wrote rest of the explanation for the changeset text. > > With all the other patches from my series applied, allyesconfig still gives me > > drivers/video/omap2/dss/dss.c:572:20: error: 'CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK' undeclared > > because that symbol is only defined when OMAP2_DSS is enabled rather than > selected. Changing drivers/video/omap2/Kconfig to not depend on OMAP seems > to work as well, but that seems a little intrusive for v3.8, because it would > let a lot of people build that code that have no use for it. > > Arnd
On Monday 21 January 2013, Rob Clark wrote: > ahh, ok, I see.. the if ARCH_OMAP2PLUS bit looks like it came in > recently (770b6cb) > > what about changing this to 'if ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM'? Fine with me in general, but the patch I posted would be the more conservative choice for v3.8. Arnd
On Mon, Jan 21, 2013 at 12:39:31PM -0600, Rob Clark wrote: > On 01/21/2013 11:41 AM, Arnd Bergmann wrote: > >On Monday 21 January 2013, Rob Clark wrote: > >>Are you sure OMAP2_DSS requires ARCH_OMAP2PLUS? I don't see this, and > >>it at least used to not depend on ARCH_OMAP2PLUS. If it does now, I > >>think the correct fix would be to remove the dependency in OMAP2_DSS. I > >>don't think removing ARCH_MULTIPLATFORM support in omapdrm is the > >>correct solution. > >At least it says so in drivers/video/omap2/Kconfig, which contains > > > >if ARCH_OMAP2PLUS > >source drivers/video/omap2/dss/Kconfig > >endif > > ahh, ok, I see.. the if ARCH_OMAP2PLUS bit looks like it came in > recently (770b6cb) > > what about changing this to 'if ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM'? That's what Arnd's patch did. totally confused, greg k-h
On 01/22/2013 10:53 AM, Greg Kroah-Hartman wrote: > On Mon, Jan 21, 2013 at 12:39:31PM -0600, Rob Clark wrote: >> On 01/21/2013 11:41 AM, Arnd Bergmann wrote: >>> On Monday 21 January 2013, Rob Clark wrote: >>>> Are you sure OMAP2_DSS requires ARCH_OMAP2PLUS? I don't see this, and >>>> it at least used to not depend on ARCH_OMAP2PLUS. If it does now, I >>>> think the correct fix would be to remove the dependency in OMAP2_DSS. I >>>> don't think removing ARCH_MULTIPLATFORM support in omapdrm is the >>>> correct solution. >>> At least it says so in drivers/video/omap2/Kconfig, which contains >>> >>> if ARCH_OMAP2PLUS >>> source drivers/video/omap2/dss/Kconfig >>> endif >> ahh, ok, I see.. the if ARCH_OMAP2PLUS bit looks like it came in >> recently (770b6cb) >> >> what about changing this to 'if ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM'? > That's what Arnd's patch did. sorry, I was talking about in drivers/video/omap2/Kconfig Ie. I'd prefer to re-enable omapdss on multi-plat rather than disabling omapdrm. With changes in drm core, it is a bit of a pain to compile test all the arm drivers by doing N different builds, so we've been trying to get to the point of all arm drm drivers supporting multi-plat BR, -R > totally confused, > > greg k-h
On Tue, Jan 22, 2013 at 10:57:44AM -0600, Rob Clark wrote: > On 01/22/2013 10:53 AM, Greg Kroah-Hartman wrote: > >On Mon, Jan 21, 2013 at 12:39:31PM -0600, Rob Clark wrote: > >>On 01/21/2013 11:41 AM, Arnd Bergmann wrote: > >>>On Monday 21 January 2013, Rob Clark wrote: > >>>>Are you sure OMAP2_DSS requires ARCH_OMAP2PLUS? I don't see this, and > >>>>it at least used to not depend on ARCH_OMAP2PLUS. If it does now, I > >>>>think the correct fix would be to remove the dependency in OMAP2_DSS. I > >>>>don't think removing ARCH_MULTIPLATFORM support in omapdrm is the > >>>>correct solution. > >>>At least it says so in drivers/video/omap2/Kconfig, which contains > >>> > >>>if ARCH_OMAP2PLUS > >>>source drivers/video/omap2/dss/Kconfig > >>>endif > >>ahh, ok, I see.. the if ARCH_OMAP2PLUS bit looks like it came in > >>recently (770b6cb) > >> > >>what about changing this to 'if ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM'? > >That's what Arnd's patch did. > > sorry, I was talking about in drivers/video/omap2/Kconfig Ah, ok. > Ie. I'd prefer to re-enable omapdss on multi-plat rather than > disabling omapdrm. With changes in drm core, it is a bit of a pain > to compile test all the arm drivers by doing N different builds, so > we've been trying to get to the point of all arm drm drivers > supporting multi-plat Ok, I'll let you and Arnd fight it out and drop this patch from my to-apply queue for now... greg k-h
On Tuesday 22 January 2013, Greg Kroah-Hartman wrote: > > Ie. I'd prefer to re-enable omapdss on multi-plat rather than > > disabling omapdrm. With changes in drm core, it is a bit of a pain > > to compile test all the arm drivers by doing N different builds, so > > we've been trying to get to the point of all arm drm drivers > > supporting multi-plat > > Ok, I'll let you and Arnd fight it out and drop this patch from my > to-apply queue for now... If Rob thinks there is no danger in allowing omap2_dss to be built on all platforms, and Tomi has no objections, I'm fine with that, too. In general, that is the right solution, I was just trying to be conservative for the 3.8 cycle. Arnd
On 01/22/2013 11:47 AM, Arnd Bergmann wrote: > On Tuesday 22 January 2013, Greg Kroah-Hartman wrote: >>> Ie. I'd prefer to re-enable omapdss on multi-plat rather than >>> disabling omapdrm. With changes in drm core, it is a bit of a pain >>> to compile test all the arm drivers by doing N different builds, so >>> we've been trying to get to the point of all arm drm drivers >>> supporting multi-plat >> Ok, I'll let you and Arnd fight it out and drop this patch from my >> to-apply queue for now... > If Rob thinks there is no danger in allowing omap2_dss to be built > on all platforms, and Tomi has no objections, I'm fine with that, too. > In general, that is the right solution, I was just trying to be > conservative for the 3.8 cycle. I think it should be safe.. or at least it built fine for multi-plat in the recent past and shouldn't really do anything if there is no omapdss platform device. Do you want me to make a patch or are you already doing this? BR, -R > Arnd
On Tuesday 22 January 2013, Rob Clark wrote: > I think it should be safe.. or at least it built fine for multi-plat in > the recent past and shouldn't really do anything if there is no omapdss > platform device. > > Do you want me to make a patch or are you already doing this? Please make one. Feel free to reuse my changeset description if needed. Arnd
diff --git a/drivers/staging/omapdrm/Kconfig b/drivers/staging/omapdrm/Kconfig index b724a41..81a7cba 100644 --- a/drivers/staging/omapdrm/Kconfig +++ b/drivers/staging/omapdrm/Kconfig @@ -2,7 +2,7 @@ config DRM_OMAP tristate "OMAP DRM" depends on DRM && !CONFIG_FB_OMAP2 - depends on ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM + depends on ARCH_OMAP2PLUS select DRM_KMS_HELPER select OMAP2_DSS select FB_SYS_FILLRECT
The omapdrm driver is incorrectly flagged to allow building on non-omap platforms, when ARCH_MULTIPLATFORM is set. This does not work, because it unconditionally selects the OMAP2_DSS symbol that only works on OMAP. The problem was introduced in 5e3b087499 "staging: drm/omap: add support for ARCH_MULTIPLATFORM", which this patch partly reverts. Without this patch, building allyesconfig results in: warning: (VIDEO_OMAP2_VOUT && DRM_OMAP) selects OMAP2_DSS which has unmet direct dependencies (HAS_IOMEM && ARCH_OMAP2PLUS) warning: (VIDEO_OMAP2_VOUT && DRM_OMAP) selects OMAP2_DSS which has unmet direct dependencies (HAS_IOMEM && ARCH_OMAP2PLUS) drivers/video/omap2/dss/dss.c: In function 'dss_calc_clock_div': drivers/video/omap2/dss/dss.c:572:20: error: 'CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK' undeclared (first use in this function) drivers/video/omap2/dss/dss.c:572:20: note: each undeclared identifier is reported only once for each function it appears in drivers/staging/omapdrm/omap_connector.c: In function 'omap_connector_dpms': drivers/staging/omapdrm/omap_connector.c:116:8: error: 'OMAP_DSS_DISPLAY_SUSPENDED' undeclared (first use in this function) drivers/staging/omapdrm/omap_connector.c:116:8: note: each undeclared identifier is reported only once for each function it appears in Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: Rob Clark <rob@ti.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/staging/omapdrm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)