| Message ID | c6ef815da57085bf7e98753463e551905f5d2706.1524245455.git.mchehab@s-opensource.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday, April 20, 2018 01:42:51 PM Mauro Carvalho Chehab wrote: > Add stubs for omapfb_dss.h, in the case it is included by > some driver when CONFIG_FB_OMAP2 is not defined, with can > happen on ARM when DRM_OMAP is not 'n'. > > That allows building such driver(s) with COMPILE_TEST. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> This patch should be dropped (together with patch #6/7) as it was superseded by a better solution suggested by Laurent: https://patchwork.kernel.org/patch/10325193/ ACK-ed by Tomi: https://www.spinics.net/lists/dri-devel/msg171918.html and already merged by you (commit 7378f1149884 "media: omap2: omapfb: allow building it with COMPILE_TEST").. > --- > include/video/omapfb_dss.h | 54 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 52 insertions(+), 2 deletions(-) Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
Em Mon, 23 Apr 2018 14:47:28 +0200 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> escreveu: > On Friday, April 20, 2018 01:42:51 PM Mauro Carvalho Chehab wrote: > > Add stubs for omapfb_dss.h, in the case it is included by > > some driver when CONFIG_FB_OMAP2 is not defined, with can > > happen on ARM when DRM_OMAP is not 'n'. > > > > That allows building such driver(s) with COMPILE_TEST. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > This patch should be dropped (together with patch #6/7) as it was > superseded by a better solution suggested by Laurent: > > https://patchwork.kernel.org/patch/10325193/ > > ACK-ed by Tomi: > > https://www.spinics.net/lists/dri-devel/msg171918.html > > and already merged by you (commit 7378f1149884 "media: omap2: > omapfb: allow building it with COMPILE_TEST").. I "ressurected" this patch due to patch 6/7. The problem with the solution already acked/merged is that it works *only* if you don't try to build for ARM. At the moment you want to build a FB_OMAP2-dependent driver on ARM with allyesc onfig, DRM_OMAP will be true, and FB_OMAP2 will be disabled: menuconfig FB_OMAP2 tristate "OMAP2+ frame buffer support" depends on FB depends on DRM_OMAP = n One solution might be to change the depends on to: depends on (DRM_OMAP = n || COMPILE_TEST) But someone pointed me that the above check was added to avoid building duplicated symbols. So, the above would cause build failures. So, in order to build for ARM with DRM_OMAP selected (allyesconfig, allmodconfig), we have the following alternatives: 1) apply patch 5/7; 2) make sure that FB_OMAP2 and DRM_OMAP won't declare the same non-static symbols; 3) redesign FB_OMAP2 to work with DRM_OMAP built. I suspect that (1) is easier. Thanks, Mauro
On Monday, April 23, 2018 02:47:28 PM Bartlomiej Zolnierkiewicz wrote: > On Friday, April 20, 2018 01:42:51 PM Mauro Carvalho Chehab wrote: > > Add stubs for omapfb_dss.h, in the case it is included by > > some driver when CONFIG_FB_OMAP2 is not defined, with can > > happen on ARM when DRM_OMAP is not 'n'. > > > > That allows building such driver(s) with COMPILE_TEST. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > This patch should be dropped (together with patch #6/7) as it was > superseded by a better solution suggested by Laurent: > > https://patchwork.kernel.org/patch/10325193/ > > ACK-ed by Tomi: > > https://www.spinics.net/lists/dri-devel/msg171918.html > > and already merged by you (commit 7378f1149884 "media: omap2: > omapfb: allow building it with COMPILE_TEST").. Hmm, I see now while this patch is still included: menuconfig FB_OMAP2 tristate "OMAP2+ frame buffer support" depends on FB depends on DRM_OMAP = n Ideally we should be able to build both drivers in the same kernel (especially as modules). I was hoping that it could be fixed easily but then I discovered the root source of the problem: drivers/gpu/drm/omapdrm/dss/display.o: In function `omapdss_unregister_display': display.c:(.text+0x2c): multiple definition of `omapdss_unregister_display' drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198): first defined here ... I need some more time to think about this.. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 23/04/18 16:56, Bartlomiej Zolnierkiewicz wrote: > Ideally we should be able to build both drivers in the same kernel > (especially as modules). > > I was hoping that it could be fixed easily but then I discovered > the root source of the problem: > > drivers/gpu/drm/omapdrm/dss/display.o: In function `omapdss_unregister_display': > display.c:(.text+0x2c): multiple definition of `omapdss_unregister_display' > drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198): first defined here The main problem is that omapdrm and omapfb are two different drivers for the same HW. You need to pick one, even if we would change those functions and fix the link issue. At some point in time we could compile both as modules (but not built-in), but the only use for that was development/testing and in the end made our life more difficult. So, now you must fully disable one of them to enable the other. And, actually, we even have boot-time code, not included in the module itself, which gets enabled when omapdrm is enabled. While it's of course good to support COMPILE_TEST, if using COMPILE_TEST with omapfb is problematic, I'm not sure if it's worth to spend time on that. We should be moving away from omapfb to omapdrm. Tomi
Em Mon, 23 Apr 2018 15:56:53 +0200 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> escreveu: > On Monday, April 23, 2018 02:47:28 PM Bartlomiej Zolnierkiewicz wrote: > > On Friday, April 20, 2018 01:42:51 PM Mauro Carvalho Chehab wrote: > > > Add stubs for omapfb_dss.h, in the case it is included by > > > some driver when CONFIG_FB_OMAP2 is not defined, with can > > > happen on ARM when DRM_OMAP is not 'n'. > > > > > > That allows building such driver(s) with COMPILE_TEST. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > > > This patch should be dropped (together with patch #6/7) as it was > > superseded by a better solution suggested by Laurent: > > > > https://patchwork.kernel.org/patch/10325193/ > > > > ACK-ed by Tomi: > > > > https://www.spinics.net/lists/dri-devel/msg171918.html > > > > and already merged by you (commit 7378f1149884 "media: omap2: > > omapfb: allow building it with COMPILE_TEST").. > > Hmm, I see now while this patch is still included: > > menuconfig FB_OMAP2 > tristate "OMAP2+ frame buffer support" > depends on FB > depends on DRM_OMAP = n > > Ideally we should be able to build both drivers in the same kernel > (especially as modules). > > I was hoping that it could be fixed easily but then I discovered > the root source of the problem: > > drivers/gpu/drm/omapdrm/dss/display.o: In function `omapdss_unregister_display': > display.c:(.text+0x2c): multiple definition of `omapdss_unregister_display' > drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198): first defined here > ... Yes, and declared on two different places: drivers/gpu/drm/omapdrm/dss/omapdss.h:void omapdss_unregister_display(struct omap_dss_device *dssdev); include/video/omapfb_dss.h:void omapdss_unregister_display(struct omap_dss_device *dssdev); one alternative would be to give different names to it, and a common header for both. At such header, it could be doing something like: static inline void omapdss_unregister_display(struct omap_dss_device *dssdev) { #if enabled(CONFIG_DRM_OMAP) omapdss_unregister_display_drm(struct omap_dss_device *dssdev); #else omapdss_unregister_display_fb(struct omap_dss_device *dssdev); ##endif } Yet, after a very quick check, it seems that nowadays only the media omap driver uses the symbols at FB_OMAP: $ git grep omapfb_dss.h drivers/media/platform/omap/omap_vout.c:#include <video/omapfb_dss.h> drivers/media/platform/omap/omap_voutdef.h:#include <video/omapfb_dss.h> drivers/media/platform/omap/omap_voutlib.c:#include <video/omapfb_dss.h> So, perhaps just renaming the common symbols and changing FB_OMAP2 to: menuconfig FB_OMAP2 tristate "OMAP2+ frame buffer support" depends on FB depends on (DRM_OMAP = n) || COMPILE_TEST would be enough to allow to build both on ARM. > I need some more time to think about this.. > > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics Thanks, Mauro
Em Mon, 23 Apr 2018 17:11:14 +0300 Tomi Valkeinen <tomi.valkeinen@ti.com> escreveu: > On 23/04/18 16:56, Bartlomiej Zolnierkiewicz wrote: > > > Ideally we should be able to build both drivers in the same kernel > > (especially as modules). > > > > I was hoping that it could be fixed easily but then I discovered > > the root source of the problem: > > > > drivers/gpu/drm/omapdrm/dss/display.o: In function `omapdss_unregister_display': > > display.c:(.text+0x2c): multiple definition of `omapdss_unregister_display' > > drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198): first defined here > > The main problem is that omapdrm and omapfb are two different drivers > for the same HW. You need to pick one, even if we would change those > functions and fix the link issue. > > At some point in time we could compile both as modules (but not > built-in), but the only use for that was development/testing and in the > end made our life more difficult. So, now you must fully disable one of > them to enable the other. And, actually, we even have boot-time code, > not included in the module itself, which gets enabled when omapdrm is > enabled. > > While it's of course good to support COMPILE_TEST, if using COMPILE_TEST > with omapfb is problematic, I'm not sure if it's worth to spend time on > that. We should be moving away from omapfb to omapdrm. Yeah, moving away from omapfb sounds the best alternative. As it seems that there's just one driver currently depending on it, I guess it shouldn't be that hard to do such change from Kernel's view, but I may be wrong, as I've no clue what this would mean to userspace. Thanks, Mauro
Hi Mauro, On Monday, 23 April 2018 17:22:27 EEST Mauro Carvalho Chehab wrote: > Em Mon, 23 Apr 2018 15:56:53 +0200 Bartlomiej Zolnierkiewicz escreveu: > > On Monday, April 23, 2018 02:47:28 PM Bartlomiej Zolnierkiewicz wrote: > >> On Friday, April 20, 2018 01:42:51 PM Mauro Carvalho Chehab wrote: > >>> Add stubs for omapfb_dss.h, in the case it is included by > >>> some driver when CONFIG_FB_OMAP2 is not defined, with can > >>> happen on ARM when DRM_OMAP is not 'n'. > >>> > >>> That allows building such driver(s) with COMPILE_TEST. > >>> > >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > >> > >> This patch should be dropped (together with patch #6/7) as it was > >> superseded by a better solution suggested by Laurent: > >> > >> https://patchwork.kernel.org/patch/10325193/ > >> > >> ACK-ed by Tomi: > >> > >> https://www.spinics.net/lists/dri-devel/msg171918.html > >> > >> and already merged by you (commit 7378f1149884 "media: omap2: > >> omapfb: allow building it with COMPILE_TEST").. > > > > Hmm, I see now while this patch is still included: > > > > menuconfig FB_OMAP2 > > tristate "OMAP2+ frame buffer support" > > depends on FB > > depends on DRM_OMAP = n > > > > Ideally we should be able to build both drivers in the same kernel > > (especially as modules). > > > > I was hoping that it could be fixed easily but then I discovered > > the root source of the problem: > > > > drivers/gpu/drm/omapdrm/dss/display.o: In function > > `omapdss_unregister_display': display.c:(.text+0x2c): multiple definition > > of `omapdss_unregister_display' > > drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198): > > first defined here ... > > Yes, and declared on two different places: > > drivers/gpu/drm/omapdrm/dss/omapdss.h:void omapdss_unregister_display(struct > omap_dss_device *dssdev); include/video/omapfb_dss.h:void > omapdss_unregister_display(struct omap_dss_device *dssdev); > > one alternative would be to give different names to it, and a common > header for both. > > At such header, it could be doing something like: > > static inline void omapdss_unregister_display(struct omap_dss_device > *dssdev) { > #if enabled(CONFIG_DRM_OMAP) > omapdss_unregister_display_drm(struct omap_dss_device *dssdev); > #else > omapdss_unregister_display_fb(struct omap_dss_device *dssdev); > ##endif > } > > Yet, after a very quick check, it seems that nowadays only the > media omap driver uses the symbols at FB_OMAP: > > $ git grep omapfb_dss.h > drivers/media/platform/omap/omap_vout.c:#include <video/omapfb_dss.h> > drivers/media/platform/omap/omap_voutdef.h:#include <video/omapfb_dss.h> > drivers/media/platform/omap/omap_voutlib.c:#include <video/omapfb_dss.h> > > So, perhaps just renaming the common symbols and changing FB_OMAP2 to: > > menuconfig FB_OMAP2 > tristate "OMAP2+ frame buffer support" > depends on FB > depends on (DRM_OMAP = n) || COMPILE_TEST > > would be enough to allow to build both on ARM. I don't think it's worth it renaming the common symbols. They will change over time as omapdrm is under heavy rework, and it's painful enough without having to handle cross-tree changes. Let's just live with the fact that both drivers can't be compiled at the same time, given that omapfb is deprecated. > > I need some more time to think about this..
Em Mon, 23 Apr 2018 22:48:06 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro, > > On Monday, 23 April 2018 17:22:27 EEST Mauro Carvalho Chehab wrote: > > Em Mon, 23 Apr 2018 15:56:53 +0200 Bartlomiej Zolnierkiewicz escreveu: > > > On Monday, April 23, 2018 02:47:28 PM Bartlomiej Zolnierkiewicz wrote: > > >> On Friday, April 20, 2018 01:42:51 PM Mauro Carvalho Chehab wrote: > > >>> Add stubs for omapfb_dss.h, in the case it is included by > > >>> some driver when CONFIG_FB_OMAP2 is not defined, with can > > >>> happen on ARM when DRM_OMAP is not 'n'. > > >>> > > >>> That allows building such driver(s) with COMPILE_TEST. > > >>> > > >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > >> > > >> This patch should be dropped (together with patch #6/7) as it was > > >> superseded by a better solution suggested by Laurent: > > >> > > >> https://patchwork.kernel.org/patch/10325193/ > > >> > > >> ACK-ed by Tomi: > > >> > > >> https://www.spinics.net/lists/dri-devel/msg171918.html > > >> > > >> and already merged by you (commit 7378f1149884 "media: omap2: > > >> omapfb: allow building it with COMPILE_TEST").. > > > > > > Hmm, I see now while this patch is still included: > > > > > > menuconfig FB_OMAP2 > > > tristate "OMAP2+ frame buffer support" > > > depends on FB > > > depends on DRM_OMAP = n > > > > > > Ideally we should be able to build both drivers in the same kernel > > > (especially as modules). > > > > > > I was hoping that it could be fixed easily but then I discovered > > > the root source of the problem: > > > > > > drivers/gpu/drm/omapdrm/dss/display.o: In function > > > `omapdss_unregister_display': display.c:(.text+0x2c): multiple definition > > > of `omapdss_unregister_display' > > > drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198): > > > first defined here ... > > > > Yes, and declared on two different places: > > > > drivers/gpu/drm/omapdrm/dss/omapdss.h:void omapdss_unregister_display(struct > > omap_dss_device *dssdev); include/video/omapfb_dss.h:void > > omapdss_unregister_display(struct omap_dss_device *dssdev); > > > > one alternative would be to give different names to it, and a common > > header for both. > > > > At such header, it could be doing something like: > > > > static inline void omapdss_unregister_display(struct omap_dss_device > > *dssdev) { > > #if enabled(CONFIG_DRM_OMAP) > > omapdss_unregister_display_drm(struct omap_dss_device *dssdev); > > #else > > omapdss_unregister_display_fb(struct omap_dss_device *dssdev); > > ##endif > > } > > > > Yet, after a very quick check, it seems that nowadays only the > > media omap driver uses the symbols at FB_OMAP: > > > > $ git grep omapfb_dss.h > > drivers/media/platform/omap/omap_vout.c:#include <video/omapfb_dss.h> > > drivers/media/platform/omap/omap_voutdef.h:#include <video/omapfb_dss.h> > > drivers/media/platform/omap/omap_voutlib.c:#include <video/omapfb_dss.h> > > > > So, perhaps just renaming the common symbols and changing FB_OMAP2 to: > > > > menuconfig FB_OMAP2 > > tristate "OMAP2+ frame buffer support" > > depends on FB > > depends on (DRM_OMAP = n) || COMPILE_TEST > > > > would be enough to allow to build both on ARM. > > I don't think it's worth it renaming the common symbols. They will change over > time as omapdrm is under heavy rework, and it's painful enough without having > to handle cross-tree changes. It could just rename the namespace-conflicting FB_OMAP2 functions, keeping the DRM ones as-is. > Let's just live with the fact that both drivers > can't be compiled at the same time, given that omapfb is deprecated. IMO, a driver that it is deprecated, being in a state where it conflicts with a non-deprecated driver that is under heavy rework is a very good candidate to go to drivers/staging or even to /dev/null. Thanks, Mauro
On Monday, 23 April 2018 23:09:55 EEST Mauro Carvalho Chehab wrote: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > > On Monday, 23 April 2018 17:22:27 EEST Mauro Carvalho Chehab wrote: > > > Em Mon, 23 Apr 2018 15:56:53 +0200 Bartlomiej Zolnierkiewicz escreveu: > > > > On Monday, April 23, 2018 02:47:28 PM Bartlomiej Zolnierkiewicz wrote: > > > >> On Friday, April 20, 2018 01:42:51 PM Mauro Carvalho Chehab wrote: > > > >>> Add stubs for omapfb_dss.h, in the case it is included by > > > >>> some driver when CONFIG_FB_OMAP2 is not defined, with can > > > >>> happen on ARM when DRM_OMAP is not 'n'. > > > >>> > > > >>> That allows building such driver(s) with COMPILE_TEST. > > > >>> > > > >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > > >> > > > >> This patch should be dropped (together with patch #6/7) as it was > > > >> superseded by a better solution suggested by Laurent: > > > >> > > > >> https://patchwork.kernel.org/patch/10325193/ > > > >> > > > >> ACK-ed by Tomi: > > > >> > > > >> https://www.spinics.net/lists/dri-devel/msg171918.html > > > >> > > > >> and already merged by you (commit 7378f1149884 "media: omap2: > > > >> omapfb: allow building it with COMPILE_TEST").. > > > > > > > > Hmm, I see now while this patch is still included: > > > > > > > > menuconfig FB_OMAP2 > > > > > > > > tristate "OMAP2+ frame buffer support" > > > > depends on FB > > > > depends on DRM_OMAP = n > > > > > > > > Ideally we should be able to build both drivers in the same kernel > > > > (especially as modules). > > > > > > > > I was hoping that it could be fixed easily but then I discovered > > > > the root source of the problem: > > > > > > > > drivers/gpu/drm/omapdrm/dss/display.o: In function > > > > `omapdss_unregister_display': display.c:(.text+0x2c): multiple > > > > definition > > > > of `omapdss_unregister_display' > > > > drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198) > > > > : > > > > first defined here ... > > > > > > Yes, and declared on two different places: > > > > > > drivers/gpu/drm/omapdrm/dss/omapdss.h:void > > > omapdss_unregister_display(struct omap_dss_device *dssdev); > > > include/video/omapfb_dss.h:void > > > omapdss_unregister_display(struct omap_dss_device *dssdev); > > > > > > one alternative would be to give different names to it, and a common > > > header for both. > > > > > > At such header, it could be doing something like: > > > > > > static inline void omapdss_unregister_display(struct omap_dss_device > > > *dssdev) { > > > #if enabled(CONFIG_DRM_OMAP) > > > > > > omapdss_unregister_display_drm(struct omap_dss_device *dssdev); > > > > > > #else > > > > > > omapdss_unregister_display_fb(struct omap_dss_device *dssdev); > > > > > > ##endif > > > } > > > > > > Yet, after a very quick check, it seems that nowadays only the > > > media omap driver uses the symbols at FB_OMAP: > > > > > > $ git grep omapfb_dss.h > > > drivers/media/platform/omap/omap_vout.c:#include <video/omapfb_dss.h> > > > drivers/media/platform/omap/omap_voutdef.h:#include <video/omapfb_dss.h> > > > drivers/media/platform/omap/omap_voutlib.c:#include <video/omapfb_dss.h> > > > > > > So, perhaps just renaming the common symbols and changing FB_OMAP2 to: > > > menuconfig FB_OMAP2 > > > > > > tristate "OMAP2+ frame buffer support" > > > depends on FB > > > depends on (DRM_OMAP = n) || COMPILE_TEST > > > > > > would be enough to allow to build both on ARM. > > > > I don't think it's worth it renaming the common symbols. They will change > > over time as omapdrm is under heavy rework, and it's painful enough > > without having to handle cross-tree changes. > > It could just rename the namespace-conflicting FB_OMAP2 functions, > keeping the DRM ones as-is. > > > Let's just live with the fact that both drivers > > can't be compiled at the same time, given that omapfb is deprecated. > > IMO, a driver that it is deprecated, being in a state where it > conflicts with a non-deprecated driver that is under heavy rework > is a very good candidate to go to drivers/staging or even to /dev/null. It's on its way, but slowly as we need to take userspace into account. Tomi should have more insight on a possible schedule for removal of omapfb.
On 23/04/18 23:09, Mauro Carvalho Chehab wrote: >> I don't think it's worth it renaming the common symbols. They will change over >> time as omapdrm is under heavy rework, and it's painful enough without having >> to handle cross-tree changes. > > It could just rename the namespace-conflicting FB_OMAP2 functions, > keeping the DRM ones as-is. Yes, I'm fine with renaming omapfb functions if that helps. But still, if omapdrm is enabled in the kernel as module or built-in, omapfb will not work. So even if we get them to compile and link, it'll break at runtime one way or another. >> Let's just live with the fact that both drivers >> can't be compiled at the same time, given that omapfb is deprecated. > > IMO, a driver that it is deprecated, being in a state where it > conflicts with a non-deprecated driver that is under heavy rework > is a very good candidate to go to drivers/staging or even to /dev/null. The problem is that it supports old devices which are not supported by omapdrm. But both omapfb and omapdrm support many of the same devices. Tomi
Hi Tomi, On Wednesday, 25 April 2018 09:24:14 EEST Tomi Valkeinen wrote: > On 23/04/18 23:09, Mauro Carvalho Chehab wrote: > >> I don't think it's worth it renaming the common symbols. They will change > >> over time as omapdrm is under heavy rework, and it's painful enough > >> without having to handle cross-tree changes. > > > > It could just rename the namespace-conflicting FB_OMAP2 functions, > > keeping the DRM ones as-is. > > Yes, I'm fine with renaming omapfb functions if that helps. But still, > if omapdrm is enabled in the kernel as module or built-in, omapfb will > not work. So even if we get them to compile and link, it'll break at > runtime one way or another. > > >> Let's just live with the fact that both drivers > >> can't be compiled at the same time, given that omapfb is deprecated. > > > > IMO, a driver that it is deprecated, being in a state where it > > conflicts with a non-deprecated driver that is under heavy rework > > is a very good candidate to go to drivers/staging or even to /dev/null. > > The problem is that it supports old devices which are not supported by > omapdrm. But both omapfb and omapdrm support many of the same devices. Could we trim down omapfb to remove support for the devices supported by omapdrm ?
On 25/04/18 12:03, Laurent Pinchart wrote: > Could we trim down omapfb to remove support for the devices supported by > omapdrm ? I was thinking about just that. But, of course, it's not quite straightforward either. We've got DSI manual update functionality in OMAP3-OMAP5 SoCs, which covers a lot of devices. And VRFB on OMAP2/3. Those need omapfb. Tomi
Hi Tomi, On Wednesday, 25 April 2018 12:33:53 EEST Tomi Valkeinen wrote: > On 25/04/18 12:03, Laurent Pinchart wrote: > > Could we trim down omapfb to remove support for the devices supported by > > omapdrm ? > > I was thinking about just that. But, of course, it's not quite > straightforward either. > > We've got DSI manual update functionality in OMAP3-OMAP5 SoCs, which > covers a lot of devices. Sebastian is working on getting that feature in omapdrm, isn't he ? > And VRFB on OMAP2/3. And that's something I'd really like to have in omapdrm too. > Those need omapfb.
On 25/04/18 13:02, Laurent Pinchart wrote: > Hi Tomi, > > On Wednesday, 25 April 2018 12:33:53 EEST Tomi Valkeinen wrote: >> On 25/04/18 12:03, Laurent Pinchart wrote: >>> Could we trim down omapfb to remove support for the devices supported by >>> omapdrm ? >> >> I was thinking about just that. But, of course, it's not quite >> straightforward either. >> >> We've got DSI manual update functionality in OMAP3-OMAP5 SoCs, which >> covers a lot of devices. > > Sebastian is working on getting that feature in omapdrm, isn't he ? Yes, and I keep pushing it forward because of the restructuring you're doing =) (feel free to comment on that thread). But agreed, it's getting better. When we have manual update support, I think the biggest missing feature is then in omapdrm. >> And VRFB on OMAP2/3. > > And that's something I'd really like to have in omapdrm too. Considering how much headache TILER has given, I'm not exactly looking forward to it ;). If we get manual update and VRFB, I think we are more or less covered on the supported HW features. It'll still break userspace apps which use omapfb, though. Unless we also port the omapfb specific IOCTLs and the sysfs files, which I believe we should not. Tomi
Hi Tomi, On Wednesday, 25 April 2018 13:10:43 EEST Tomi Valkeinen wrote: > On 25/04/18 13:02, Laurent Pinchart wrote: > > On Wednesday, 25 April 2018 12:33:53 EEST Tomi Valkeinen wrote: > >> On 25/04/18 12:03, Laurent Pinchart wrote: > >>> Could we trim down omapfb to remove support for the devices supported by > >>> omapdrm ? > >> > >> I was thinking about just that. But, of course, it's not quite > >> straightforward either. > >> > >> We've got DSI manual update functionality in OMAP3-OMAP5 SoCs, which > >> covers a lot of devices. > > > > Sebastian is working on getting that feature in omapdrm, isn't he ? > > Yes, and I keep pushing it forward because of the restructuring you're > doing =) (feel free to comment on that thread). But agreed, it's getting > better. When we have manual update support, I think the biggest missing > feature is then in omapdrm. > > >> And VRFB on OMAP2/3. > > > > And that's something I'd really like to have in omapdrm too. > > Considering how much headache TILER has given, I'm not exactly looking > forward to it ;). > > If we get manual update and VRFB, I think we are more or less covered on > the supported HW features. It'll still break userspace apps which use > omapfb, though. Unless we also port the omapfb specific IOCTLs and the > sysfs files, which I believe we should not. I agree with you, we shouldn't. We'll need a grace period before removing omapfb, if we ever do so.
On Monday, April 23, 2018 10:55:57 AM Mauro Carvalho Chehab wrote: > Em Mon, 23 Apr 2018 14:47:28 +0200 > Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> escreveu: > > > On Friday, April 20, 2018 01:42:51 PM Mauro Carvalho Chehab wrote: > > > Add stubs for omapfb_dss.h, in the case it is included by > > > some driver when CONFIG_FB_OMAP2 is not defined, with can > > > happen on ARM when DRM_OMAP is not 'n'. > > > > > > That allows building such driver(s) with COMPILE_TEST. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > > > This patch should be dropped (together with patch #6/7) as it was > > superseded by a better solution suggested by Laurent: > > > > https://patchwork.kernel.org/patch/10325193/ > > > > ACK-ed by Tomi: > > > > https://www.spinics.net/lists/dri-devel/msg171918.html > > > > and already merged by you (commit 7378f1149884 "media: omap2: > > omapfb: allow building it with COMPILE_TEST").. > > I "ressurected" this patch due to patch 6/7. > > The problem with the solution already acked/merged is that > it works *only* if you don't try to build for ARM. > > At the moment you want to build a FB_OMAP2-dependent driver > on ARM with allyesc onfig, DRM_OMAP will be true, and FB_OMAP2 > will be disabled: > > menuconfig FB_OMAP2 > tristate "OMAP2+ frame buffer support" > depends on FB > depends on DRM_OMAP = n > > One solution might be to change the depends on to: > depends on (DRM_OMAP = n || COMPILE_TEST) > > But someone pointed me that the above check was added to avoid building > duplicated symbols. So, the above would cause build failures. > > So, in order to build for ARM with DRM_OMAP selected (allyesconfig, > allmodconfig), we have the following alternatives: > > 1) apply patch 5/7; > 2) make sure that FB_OMAP2 and DRM_OMAP won't declare the > same non-static symbols; > 3) redesign FB_OMAP2 to work with DRM_OMAP built. > > I suspect that (1) is easier. I agree. You can merge this patch through your tree with: Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Monday, April 23, 2018 05:11:14 PM Tomi Valkeinen wrote: > On 23/04/18 16:56, Bartlomiej Zolnierkiewicz wrote: > > > Ideally we should be able to build both drivers in the same kernel > > (especially as modules). > > > > I was hoping that it could be fixed easily but then I discovered > > the root source of the problem: > > > > drivers/gpu/drm/omapdrm/dss/display.o: In function `omapdss_unregister_display': > > display.c:(.text+0x2c): multiple definition of `omapdss_unregister_display' > > drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198): first defined here > > The main problem is that omapdrm and omapfb are two different drivers > for the same HW. You need to pick one, even if we would change those > functions and fix the link issue. With proper resource allocation in both drivers this shouldn't be a problem - the one which allocates resources first will be used (+ we can initialize omapdrm first in case it is built-in). This is how similar situations are handled in other kernel subsystems. It seems that the real root problem is commit f76ee892a99e ("omapfb: copy omapdss & displays for omapfb") from Dec 2015 which resulted in duplication of ~30 KLOC of code. The code in question seems to be both fbdev & drm independent: " * omapdss, located in drivers/video/fbdev/omap2/dss/. This is a driver for the display subsystem IPs used on OMAP (and related) SoCs. It offers only a kernel internal API, and does not implement anything for fbdev or drm. * omapdss panels and encoders, located in drivers/video/fbdev/omap2/displays-new/. These are panel and external encoder drivers, which use APIs offered by omapdss driver. These also don't implement anything for fbdev or drm. " While I understand some motives behind this change I'm not overall happy with it.. > At some point in time we could compile both as modules (but not > built-in), but the only use for that was development/testing and in the > end made our life more difficult. So, now you must fully disable one of > them to enable the other. And, actually, we even have boot-time code, > not included in the module itself, which gets enabled when omapdrm is > enabled. Do you mean some code in arch/arm/mach-omap2/ or something else? > While it's of course good to support COMPILE_TEST, if using COMPILE_TEST > with omapfb is problematic, I'm not sure if it's worth to spend time on > that. We should be moving away from omapfb to omapdrm. Is there some approximate schedule for omapfb removal available? Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On 25/04/18 14:13, Bartlomiej Zolnierkiewicz wrote: > > On Monday, April 23, 2018 05:11:14 PM Tomi Valkeinen wrote: >> On 23/04/18 16:56, Bartlomiej Zolnierkiewicz wrote: >> >>> Ideally we should be able to build both drivers in the same kernel >>> (especially as modules). >>> >>> I was hoping that it could be fixed easily but then I discovered >>> the root source of the problem: >>> >>> drivers/gpu/drm/omapdrm/dss/display.o: In function `omapdss_unregister_display': >>> display.c:(.text+0x2c): multiple definition of `omapdss_unregister_display' >>> drivers/video/fbdev/omap2/omapfb/dss/display.o:display.c:(.text+0x198): first defined here >> >> The main problem is that omapdrm and omapfb are two different drivers >> for the same HW. You need to pick one, even if we would change those >> functions and fix the link issue. > > With proper resource allocation in both drivers this shouldn't be > a problem - the one which allocates resources first will be used > (+ we can initialize omapdrm first in case it is built-in). This is > how similar situations are handled in other kernel subsystems. We have boot time code, which is always built-in, for both drivers which adjusts device tree data. I imagine those could easily conflict. Maybe there's something else too. And it's not only about the main omapfb or omapdrm driver. We have drivers for the encoders and panels. Those would conflict too. I guess we could have the case where omapdrm probes, and then a panel driver from omapfb probes. Actually, many of the panel and encoder drivers probably have same symbols too, which would prevent linking. > It seems that the real root problem is commit f76ee892a99e ("omapfb: > copy omapdss & displays for omapfb") from Dec 2015 which resulted in > duplication of ~30 KLOC of code. The code in question seems to be > both fbdev & drm independent: > > " > * omapdss, located in drivers/video/fbdev/omap2/dss/. This is a driver for the > display subsystem IPs used on OMAP (and related) SoCs. It offers only a > kernel internal API, and does not implement anything for fbdev or drm. > > * omapdss panels and encoders, located in > drivers/video/fbdev/omap2/displays-new/. These are panel and external encoder > drivers, which use APIs offered by omapdss driver. These also don't implement > anything for fbdev or drm. > " > > While I understand some motives behind this change I'm not overall > happy with it.. Neither was I, but I have to say it was a game-changer for omapdrm development. Doing anything new on omapdrm meant trying to get it to work on omapfb too (in the same commit, so cross-tree), and many changes were just too complex to even try. After that commit we were free to really start restructuring the code to fit the DRM world. >> At some point in time we could compile both as modules (but not >> built-in), but the only use for that was development/testing and in the >> end made our life more difficult. So, now you must fully disable one of >> them to enable the other. And, actually, we even have boot-time code, >> not included in the module itself, which gets enabled when omapdrm is >> enabled. > > Do you mean some code in arch/arm/mach-omap2/ or something else? That and the omapdss-boot-init.c we have for both omapfb and omapdrm. >> While it's of course good to support COMPILE_TEST, if using COMPILE_TEST >> with omapfb is problematic, I'm not sure if it's worth to spend time on >> that. We should be moving away from omapfb to omapdrm. > > Is there some approximate schedule for omapfb removal available? Unfortunatey not. omapfb still has support for some legacy devices that omapdrm doesn't support. Tomi
Em Wed, 25 Apr 2018 12:47:34 +0200 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> escreveu: > On Monday, April 23, 2018 10:55:57 AM Mauro Carvalho Chehab wrote: > > Em Mon, 23 Apr 2018 14:47:28 +0200 > > Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> escreveu: > > > > > On Friday, April 20, 2018 01:42:51 PM Mauro Carvalho Chehab wrote: > > > > Add stubs for omapfb_dss.h, in the case it is included by > > > > some driver when CONFIG_FB_OMAP2 is not defined, with can > > > > happen on ARM when DRM_OMAP is not 'n'. > > > > > > > > That allows building such driver(s) with COMPILE_TEST. > > > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > > > > > This patch should be dropped (together with patch #6/7) as it was > > > superseded by a better solution suggested by Laurent: > > > > > > https://patchwork.kernel.org/patch/10325193/ > > > > > > ACK-ed by Tomi: > > > > > > https://www.spinics.net/lists/dri-devel/msg171918.html > > > > > > and already merged by you (commit 7378f1149884 "media: omap2: > > > omapfb: allow building it with COMPILE_TEST").. > > > > I "ressurected" this patch due to patch 6/7. > > > > The problem with the solution already acked/merged is that > > it works *only* if you don't try to build for ARM. > > > > At the moment you want to build a FB_OMAP2-dependent driver > > on ARM with allyesc onfig, DRM_OMAP will be true, and FB_OMAP2 > > will be disabled: > > > > menuconfig FB_OMAP2 > > tristate "OMAP2+ frame buffer support" > > depends on FB > > depends on DRM_OMAP = n > > > > One solution might be to change the depends on to: > > depends on (DRM_OMAP = n || COMPILE_TEST) > > > > But someone pointed me that the above check was added to avoid building > > duplicated symbols. So, the above would cause build failures. > > > > So, in order to build for ARM with DRM_OMAP selected (allyesconfig, > > allmodconfig), we have the following alternatives: > > > > 1) apply patch 5/7; > > 2) make sure that FB_OMAP2 and DRM_OMAP won't declare the > > same non-static symbols; > > 3) redesign FB_OMAP2 to work with DRM_OMAP built. > > > > I suspect that (1) is easier. > > I agree. > > You can merge this patch through your tree with: > > Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Thanks, I'll merge it. It would still be really cool if Tony or someone else could find a better way for omap3isp driver to not depend on it. Regards, Maur Thanks, Mauro
diff --git a/include/video/omapfb_dss.h b/include/video/omapfb_dss.h index 1d38901d599d..e9775144ff3b 100644 --- a/include/video/omapfb_dss.h +++ b/include/video/omapfb_dss.h @@ -774,6 +774,12 @@ struct omap_dss_driver { const struct hdmi_avi_infoframe *avi); }; +#define for_each_dss_dev(d) while ((d = omap_dss_get_next_device(d)) != NULL) + +typedef void (*omap_dispc_isr_t) (void *arg, u32 mask); + +#ifdef CONFIG_FB_OMAP2 + enum omapdss_version omapdss_get_version(void); bool omapdss_is_initialized(void); @@ -785,7 +791,6 @@ void omapdss_unregister_display(struct omap_dss_device *dssdev); struct omap_dss_device *omap_dss_get_device(struct omap_dss_device *dssdev); void omap_dss_put_device(struct omap_dss_device *dssdev); -#define for_each_dss_dev(d) while ((d = omap_dss_get_next_device(d)) != NULL) struct omap_dss_device *omap_dss_get_next_device(struct omap_dss_device *from); struct omap_dss_device *omap_dss_find_device(void *data, int (*match)(struct omap_dss_device *dssdev, void *data)); @@ -826,7 +831,6 @@ int omapdss_default_get_recommended_bpp(struct omap_dss_device *dssdev); void omapdss_default_get_timings(struct omap_dss_device *dssdev, struct omap_video_timings *timings); -typedef void (*omap_dispc_isr_t) (void *arg, u32 mask); int omap_dispc_register_isr(omap_dispc_isr_t isr, void *arg, u32 mask); int omap_dispc_unregister_isr(omap_dispc_isr_t isr, void *arg, u32 mask); @@ -856,5 +860,51 @@ omapdss_of_get_first_endpoint(const struct device_node *parent); struct omap_dss_device * omapdss_of_find_source_for_first_ep(struct device_node *node); +#else + +static inline enum omapdss_version omapdss_get_version(void) +{ return OMAPDSS_VER_UNKNOWN; }; + +static inline bool omapdss_is_initialized(void) +{ return false; }; + +static inline int omap_dispc_register_isr(omap_dispc_isr_t isr, + void *arg, u32 mask) +{ return 0; }; + +static inline int omap_dispc_unregister_isr(omap_dispc_isr_t isr, + void *arg, u32 mask) +{ return 0; }; + +static inline struct omap_dss_device +*omap_dss_get_device(struct omap_dss_device *dssdev) +{ return NULL; }; + +static inline struct omap_dss_device +*omap_dss_get_next_device(struct omap_dss_device *from) +{return NULL; }; + +static inline void omap_dss_put_device(struct omap_dss_device *dssdev) {}; + +static inline int omapdss_compat_init(void) +{ return 0; }; + +static inline void omapdss_compat_uninit(void) {}; + +static inline int omap_dss_get_num_overlay_managers(void) +{ return 0; }; + +static inline struct omap_overlay_manager *omap_dss_get_overlay_manager(int num) +{ return NULL; }; + +static inline int omap_dss_get_num_overlays(void) +{ return 0; }; + +static inline struct omap_overlay *omap_dss_get_overlay(int num) +{ return NULL; }; + + +#endif /* FB_OMAP2 */ + #endif /* __OMAPFB_DSS_H */
Add stubs for omapfb_dss.h, in the case it is included by some driver when CONFIG_FB_OMAP2 is not defined, with can happen on ARM when DRM_OMAP is not 'n'. That allows building such driver(s) with COMPILE_TEST. Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> --- include/video/omapfb_dss.h | 54 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-)