| Message ID | 20210927142816.2069269-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] drm: fb_helper: fix CONFIG_FB dependency | expand |
On Mon, Sep 27, 2021 at 04:28:02PM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > With CONFIG_FB=m and CONFIG_DRM=y, we get a link error in the fb helper: > > aarch64-linux-ld: drivers/gpu/drm/drm_fb_helper.o: in function `drm_fb_helper_alloc_fbi': > (.text+0x10cc): undefined reference to `framebuffer_alloc' > > Tighten the dependency so it is only allowed in the case that DRM can > link against FB. > > Fixes: f611b1e7624c ("drm: Avoid circular dependencies for CONFIG_FB") > Link: https://lore.kernel.org/all/20210721152211.2706171-1-arnd@kernel.org/ > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Thanks for fixing this! Reviewed-by: Kees Cook <keescook@chromium.org> -- Kees Cook
On Mon, Sep 27, 2021 at 09:23:45AM -0700, Kees Cook wrote: > On Mon, Sep 27, 2021 at 04:28:02PM +0200, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > With CONFIG_FB=m and CONFIG_DRM=y, we get a link error in the fb helper: > > > > aarch64-linux-ld: drivers/gpu/drm/drm_fb_helper.o: in function `drm_fb_helper_alloc_fbi': > > (.text+0x10cc): undefined reference to `framebuffer_alloc' > > > > Tighten the dependency so it is only allowed in the case that DRM can > > link against FB. > > > > Fixes: f611b1e7624c ("drm: Avoid circular dependencies for CONFIG_FB") > > Link: https://lore.kernel.org/all/20210721152211.2706171-1-arnd@kernel.org/ > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Thanks for fixing this! > > Reviewed-by: Kees Cook <keescook@chromium.org> Stuffed into drm-misc-next. Arnd, I guess I still can't volunteer you for commit rights so I don't have to bother with this anymore? It's nice to be lazy and comfy :-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Thu, 30 Sep 2021, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Sep 27, 2021 at 09:23:45AM -0700, Kees Cook wrote: >> On Mon, Sep 27, 2021 at 04:28:02PM +0200, Arnd Bergmann wrote: >> > From: Arnd Bergmann <arnd@arndb.de> >> > >> > With CONFIG_FB=m and CONFIG_DRM=y, we get a link error in the fb helper: >> > >> > aarch64-linux-ld: drivers/gpu/drm/drm_fb_helper.o: in function `drm_fb_helper_alloc_fbi': >> > (.text+0x10cc): undefined reference to `framebuffer_alloc' >> > >> > Tighten the dependency so it is only allowed in the case that DRM can >> > link against FB. >> > >> > Fixes: f611b1e7624c ("drm: Avoid circular dependencies for CONFIG_FB") >> > Link: https://lore.kernel.org/all/20210721152211.2706171-1-arnd@kernel.org/ >> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> >> Thanks for fixing this! >> >> Reviewed-by: Kees Cook <keescook@chromium.org> > > Stuffed into drm-misc-next. The problem is, I don't think the patch is semantically correct. drm_fb_helper.o is not part of drm.ko, it's part of drm_kms_helper.ko. This adds some sort of indirect dependency via DRM which might work, maybe by coincidence, maybe not - but it's certainly not obvious. The likely culprit is, again, the overuse of select, and in this case select DRM_KMS_HELPER. And DRM_KMS_HELPER should depend on FB if DRM_FBDEV_EMULATION=y. That's the problem. All of the drm Kconfigs could use an overhaul to be semantically correct, but that's a hill nobody wants to die on. Instead we keep piling up tweaks to paper over the issues, ad infinitum. (And this ties to a previous comment I had about the organization of files under drm/, a hundred files in one big lump that belong to different modules, and it's not helping people figure out the dependencies.) BR, Jani. PS. I was brought here via [1] which is another complicated "fix" to the same problem. [1] https://lore.kernel.org/r/20211027072044.4105113-1-javierm@redhat.com -- Jani Nikula, Intel Open Source Graphics Center
On Wed, 27 Oct 2021, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 30 Sep 2021, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Mon, Sep 27, 2021 at 09:23:45AM -0700, Kees Cook wrote: >>> On Mon, Sep 27, 2021 at 04:28:02PM +0200, Arnd Bergmann wrote: >>> > From: Arnd Bergmann <arnd@arndb.de> >>> > >>> > With CONFIG_FB=m and CONFIG_DRM=y, we get a link error in the fb helper: >>> > >>> > aarch64-linux-ld: drivers/gpu/drm/drm_fb_helper.o: in function `drm_fb_helper_alloc_fbi': >>> > (.text+0x10cc): undefined reference to `framebuffer_alloc' >>> > >>> > Tighten the dependency so it is only allowed in the case that DRM can >>> > link against FB. >>> > >>> > Fixes: f611b1e7624c ("drm: Avoid circular dependencies for CONFIG_FB") >>> > Link: https://lore.kernel.org/all/20210721152211.2706171-1-arnd@kernel.org/ >>> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> >>> Thanks for fixing this! >>> >>> Reviewed-by: Kees Cook <keescook@chromium.org> >> >> Stuffed into drm-misc-next. > > The problem is, I don't think the patch is semantically correct. > > drm_fb_helper.o is not part of drm.ko, it's part of > drm_kms_helper.ko. This adds some sort of indirect dependency via DRM > which might work, maybe by coincidence, maybe not - but it's certainly > not obvious. > > The likely culprit is, again, the overuse of select, and in this case > select DRM_KMS_HELPER. And DRM_KMS_HELPER should depend on FB if > DRM_FBDEV_EMULATION=y. That's the problem. Almost all of the recurring Kconfig related dependency issues would go away by following Documentation/kbuild/kconfig-language.rst: Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over. If the kconfig parser had a lint mode to issue a warning for all of those uses, and someone persistent enough followed through with fixing them, we'd all be better off. Oh, and maybe the menuconfig tools also need better ways to recursively enable config options with dependencies, because one of the reasons people like select is the convenience of just enabling a config option, and it selects everything that's needed (albeit with the occasional dependency issues). With dependencies, you need to start with the leaf dependencies and work your way up to what you need, and it's not easy. BR, Jani. > > All of the drm Kconfigs could use an overhaul to be semantically > correct, but that's a hill nobody wants to die on. Instead we keep > piling up tweaks to paper over the issues, ad infinitum. > > (And this ties to a previous comment I had about the organization of > files under drm/, a hundred files in one big lump that belong to > different modules, and it's not helping people figure out the > dependencies.) > > > BR, > Jani. > > > PS. I was brought here via [1] which is another complicated "fix" to the > same problem. > > > [1] https://lore.kernel.org/r/20211027072044.4105113-1-javierm@redhat.com -- Jani Nikula, Intel Open Source Graphics Center
On Wed, Oct 27, 2021 at 1:47 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 30 Sep 2021, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Sep 27, 2021 at 09:23:45AM -0700, Kees Cook wrote: > >> On Mon, Sep 27, 2021 at 04:28:02PM +0200, Arnd Bergmann wrote: > >> > From: Arnd Bergmann <arnd@arndb.de> > >> > > >> > With CONFIG_FB=m and CONFIG_DRM=y, we get a link error in the fb helper: > >> > > >> > aarch64-linux-ld: drivers/gpu/drm/drm_fb_helper.o: in function `drm_fb_helper_alloc_fbi': > >> > (.text+0x10cc): undefined reference to `framebuffer_alloc' > >> > > >> > Tighten the dependency so it is only allowed in the case that DRM can > >> > link against FB. > >> > > >> > Fixes: f611b1e7624c ("drm: Avoid circular dependencies for CONFIG_FB") > >> > Link: https://lore.kernel.org/all/20210721152211.2706171-1-arnd@kernel.org/ > >> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > >> > >> Thanks for fixing this! > >> > >> Reviewed-by: Kees Cook <keescook@chromium.org> > > > > Stuffed into drm-misc-next. > > The problem is, I don't think the patch is semantically correct. > > drm_fb_helper.o is not part of drm.ko, it's part of > drm_kms_helper.ko. This adds some sort of indirect dependency via DRM > which might work, maybe by coincidence, maybe not - but it's certainly > not obvious. Right, how about this change on top? --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -117,9 +117,8 @@ config DRM_DEBUG_MODESET_LOCK config DRM_FBDEV_EMULATION bool "Enable legacy fbdev support for your modesetting driver" - depends on DRM - depends on FB=y || FB=DRM - select DRM_KMS_HELPER + depends on DRM_KMS_HELPER + depends on FB=y || FB=DRM_KMS_HELPER select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT That would probably make it work for DRM=y, FB=m, DRM_KMS_HELPER=m, but it needs more randconfig testing, which I can help with. > The likely culprit is, again, the overuse of select, and in this case > select DRM_KMS_HELPER. And DRM_KMS_HELPER should depend on FB if > DRM_FBDEV_EMULATION=y. That's the problem. This is something we can't easily express in Kconfig, as we can't add the dependency to a symbol that only gets selected by other drivers, which is why the dependency has to be in the user-visible symbol, in this case DRM_FBDEV_EMULATION. > All of the drm Kconfigs could use an overhaul to be semantically > correct, but that's a hill nobody wants to die on. Instead we keep > piling up tweaks to paper over the issues, ad infinitum. Yes, that is a big issue, though we have similar problems with drivers/media and net/. On a related note, I did manage to sort out the backlight dependency issue (intel_panel.c:(.text+0x2f58): undefined reference to `backlight_device_register'), but haven't sent that one again yet, but I can if you like. This one changes DRM_I915 and all of drivers/video/fbdev from 'select BACKLIGHT_CLASS_DEVICE' to 'depends on', which I think moves everything into broadly the right direction. Let me know if you would like me to send those now, or have a look at the top 3 patches in [1] if you are interested. This has passed a few thousand randconfig builds and should not depend on additional patches. Arnd [1] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=randconfig-5.16-next
On 10/27/21 14:18, Arnd Bergmann wrote: > On Wed, Oct 27, 2021 at 1:47 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: [snip] >> drm_fb_helper.o is not part of drm.ko, it's part of >> drm_kms_helper.ko. This adds some sort of indirect dependency via DRM >> which might work, maybe by coincidence, maybe not - but it's certainly >> not obvious. Indeed, you are correct that's not semantically correct. > > Right, how about this change on top? > > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -117,9 +117,8 @@ config DRM_DEBUG_MODESET_LOCK > > config DRM_FBDEV_EMULATION > bool "Enable legacy fbdev support for your modesetting driver" > - depends on DRM > - depends on FB=y || FB=DRM > - select DRM_KMS_HELPER > + depends on DRM_KMS_HELPER > + depends on FB=y || FB=DRM_KMS_HELPER > select FB_CFB_FILLRECT > select FB_CFB_COPYAREA > select FB_CFB_IMAGEBLIT > > That would probably make it work for DRM=y, FB=m, DRM_KMS_HELPER=m, > but it needs more randconfig testing, which I can help with. > >> The likely culprit is, again, the overuse of select, and in this case >> select DRM_KMS_HELPER. And DRM_KMS_HELPER should depend on FB if >> DRM_FBDEV_EMULATION=y. That's the problem. > > This is something we can't easily express in Kconfig, as we can't add the > dependency to a symbol that only gets selected by other drivers, which > is why the dependency has to be in the user-visible symbol, > in this case DRM_FBDEV_EMULATION. > Why the dependency has to be in a user-visible symbol? What could be the problem with having something like: diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index cea777ae7fb9..f80b404946ca 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -82,6 +82,7 @@ config DRM_DEBUG_SELFTEST config DRM_KMS_HELPER tristate depends on DRM + depends on (DRM_FBDEV_EMULATION && FB) || !DRM_FBDEV_EMULATION help CRTC helpers for KMS drivers. @@ -104,7 +105,6 @@ config DRM_FBDEV_EMULATION bool "Enable legacy fbdev support for your modesetting driver" depends on DRM depends on FB - select DRM_KMS_HELPER select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
On Wed, Oct 27, 2021 at 2:38 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > > > > This is something we can't easily express in Kconfig, as we can't add the > > dependency to a symbol that only gets selected by other drivers, which > > is why the dependency has to be in the user-visible symbol, > > in this case DRM_FBDEV_EMULATION. > > > > Why the dependency has to be in a user-visible symbol? What could be the > problem with having something like: > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index cea777ae7fb9..f80b404946ca 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -82,6 +82,7 @@ config DRM_DEBUG_SELFTEST > config DRM_KMS_HELPER > tristate > depends on DRM > + depends on (DRM_FBDEV_EMULATION && FB) || !DRM_FBDEV_EMULATION > help > CRTC helpers for KMS drivers. > > @@ -104,7 +105,6 @@ config DRM_FBDEV_EMULATION > bool "Enable legacy fbdev support for your modesetting driver" > depends on DRM > depends on FB > - select DRM_KMS_HELPER > select FB_CFB_FILLRECT > select FB_CFB_COPYAREA > select FB_CFB_IMAGEBLIT This fails because of all the other drivers that try to 'select DRM_KMS_HELPER'. Kconfig will now complain about a symbol that gets selected while its dependencies are not met. To work around that, every single driver that has 'selects DRM_KMS_HELPER' would now have to also list 'depends on (DRM_FBDEV_EMULATION && FB) || !DRM_FBDEV_EMULATION'. Arnd
On Wed, 27 Oct 2021, Javier Martinez Canillas <javierm@redhat.com> wrote: > On 10/27/21 14:18, Arnd Bergmann wrote: >> On Wed, Oct 27, 2021 at 1:47 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > [snip] > >>> drm_fb_helper.o is not part of drm.ko, it's part of >>> drm_kms_helper.ko. This adds some sort of indirect dependency via DRM >>> which might work, maybe by coincidence, maybe not - but it's certainly >>> not obvious. > > Indeed, you are correct that's not semantically correct. > >> >> Right, how about this change on top? >> >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -117,9 +117,8 @@ config DRM_DEBUG_MODESET_LOCK >> >> config DRM_FBDEV_EMULATION >> bool "Enable legacy fbdev support for your modesetting driver" >> - depends on DRM >> - depends on FB=y || FB=DRM >> - select DRM_KMS_HELPER >> + depends on DRM_KMS_HELPER >> + depends on FB=y || FB=DRM_KMS_HELPER >> select FB_CFB_FILLRECT >> select FB_CFB_COPYAREA >> select FB_CFB_IMAGEBLIT >> >> That would probably make it work for DRM=y, FB=m, DRM_KMS_HELPER=m, >> but it needs more randconfig testing, which I can help with. >> >>> The likely culprit is, again, the overuse of select, and in this case >>> select DRM_KMS_HELPER. And DRM_KMS_HELPER should depend on FB if >>> DRM_FBDEV_EMULATION=y. That's the problem. >> >> This is something we can't easily express in Kconfig, as we can't add the >> dependency to a symbol that only gets selected by other drivers, which >> is why the dependency has to be in the user-visible symbol, >> in this case DRM_FBDEV_EMULATION. >> > > Why the dependency has to be in a user-visible symbol? What could be the > problem with having something like: > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index cea777ae7fb9..f80b404946ca 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -82,6 +82,7 @@ config DRM_DEBUG_SELFTEST > config DRM_KMS_HELPER > tristate > depends on DRM > + depends on (DRM_FBDEV_EMULATION && FB) || !DRM_FBDEV_EMULATION To me, this seems like the right solution. Depend on FB if DRM_FBDEV_EMULATION is enabled. That's exactly what the relationship is. BR, Jani. > help > CRTC helpers for KMS drivers. > > @@ -104,7 +105,6 @@ config DRM_FBDEV_EMULATION > bool "Enable legacy fbdev support for your modesetting driver" > depends on DRM > depends on FB > - select DRM_KMS_HELPER > select FB_CFB_FILLRECT > select FB_CFB_COPYAREA > select FB_CFB_IMAGEBLIT > > Best regards, -- Jani Nikula, Intel Open Source Graphics Center
On 10/27/21 14:52, Arnd Bergmann wrote: [snip] >> >> @@ -104,7 +105,6 @@ config DRM_FBDEV_EMULATION >> bool "Enable legacy fbdev support for your modesetting driver" >> depends on DRM >> depends on FB >> - select DRM_KMS_HELPER >> select FB_CFB_FILLRECT >> select FB_CFB_COPYAREA >> select FB_CFB_IMAGEBLIT > > This fails because of all the other drivers that try to 'select DRM_KMS_HELPER'. > Kconfig will now complain about a symbol that gets selected while its > dependencies > are not met. > > To work around that, every single driver that has 'selects DRM_KMS_HELPER' would > now have to also list 'depends on (DRM_FBDEV_EMULATION && FB) || > !DRM_FBDEV_EMULATION'. > Ah, I see now. Thanks a lot for the explanation. > Arnd > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
On Wed, 27 Oct 2021, Arnd Bergmann <arnd@kernel.org> wrote: > On a related note, I did manage to sort out the backlight dependency issue > (intel_panel.c:(.text+0x2f58): undefined reference to > `backlight_device_register'), > but haven't sent that one again yet, but I can if you like. This one changes > DRM_I915 and all of drivers/video/fbdev from 'select BACKLIGHT_CLASS_DEVICE' > to 'depends on', which I think moves everything into broadly the right > direction. > > Let me know if you would like me to send those now, or have a look at the > top 3 patches in [1] if you are interested. This has passed a few > thousand randconfig > builds and should not depend on additional patches. FWIW, Acked-by: Jani Nikula <jani.nikula@intel.com> on the patches. I think I've sent patches before to do the same change from "select" to "depends on", but they went nowhere. IIRC the opposition was that people wanted to be able to find and enable their driver in menuconfig without first having to enable BACKLIGHT_CLASS_DEVICE. BR, Jani. > > Arnd > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=randconfig-5.16-next -- Jani Nikula, Intel Open Source Graphics Center
On Wed, 27 Oct 2021, Arnd Bergmann <arnd@kernel.org> wrote: > On Wed, Oct 27, 2021 at 2:38 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> > >> > This is something we can't easily express in Kconfig, as we can't add the >> > dependency to a symbol that only gets selected by other drivers, which >> > is why the dependency has to be in the user-visible symbol, >> > in this case DRM_FBDEV_EMULATION. >> > >> >> Why the dependency has to be in a user-visible symbol? What could be the >> problem with having something like: >> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >> index cea777ae7fb9..f80b404946ca 100644 >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -82,6 +82,7 @@ config DRM_DEBUG_SELFTEST >> config DRM_KMS_HELPER >> tristate >> depends on DRM >> + depends on (DRM_FBDEV_EMULATION && FB) || !DRM_FBDEV_EMULATION >> help >> CRTC helpers for KMS drivers. >> >> @@ -104,7 +105,6 @@ config DRM_FBDEV_EMULATION >> bool "Enable legacy fbdev support for your modesetting driver" >> depends on DRM >> depends on FB >> - select DRM_KMS_HELPER >> select FB_CFB_FILLRECT >> select FB_CFB_COPYAREA >> select FB_CFB_IMAGEBLIT > > This fails because of all the other drivers that try to 'select DRM_KMS_HELPER'. > Kconfig will now complain about a symbol that gets selected while its > dependencies > are not met. > > To work around that, every single driver that has 'selects DRM_KMS_HELPER' would > now have to also list 'depends on (DRM_FBDEV_EMULATION && FB) || > !DRM_FBDEV_EMULATION'. So the fix would be that nobody selects DRM_KMS_HELPER... BR, Jani. > > Arnd -- Jani Nikula, Intel Open Source Graphics Center
On 10/27/21 14:55, Jani Nikula wrote: [snip] >> Why the dependency has to be in a user-visible symbol? What could be the >> problem with having something like: >> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >> index cea777ae7fb9..f80b404946ca 100644 >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -82,6 +82,7 @@ config DRM_DEBUG_SELFTEST >> config DRM_KMS_HELPER >> tristate >> depends on DRM >> + depends on (DRM_FBDEV_EMULATION && FB) || !DRM_FBDEV_EMULATION > > To me, this seems like the right solution. Depend on FB if > DRM_FBDEV_EMULATION is enabled. That's exactly what the relationship is. > The problem as Arnd explained is that then this relationship will have to be expressed in all the Kconfig symbols that select DRM_KMS_HELPER. Otherwise the symbol will happily select the wrong state and even when a warning is printed by Kconfig, it will just set an invalid configuration. For example with CONFIG_FB=m (that led to the linker errors if the symbol is also not CONFIG_DRM_KMS_HELPER=m) and CONFIG_SIMPLEDRM=y (that selects CONFIG_DRM_KMS_HELPER), this would cause the following unmet dependencies: $ make prepare modules_prepare WARNING: unmet direct dependencies detected for DRM_KMS_HELPER Depends on [m]: HAS_IOMEM [=y] && DRM [=y] && (DRM_FBDEV_EMULATION [=y] && FB [=m] || !DRM_FBDEV_EMULATION [=y]) Selected by [y]: - DRM_SIMPLEDRM [=y] && HAS_IOMEM [=y] && DRM [=y] Selected by [m]: - DRM_I915 [=m] && HAS_IOMEM [=y] && DRM [=y] && X86 [=y] && PCI [=y] - DRM_VIRTIO_GPU [=m] && HAS_IOMEM [=y] && DRM [=y] && VIRTIO_MENU [=y] && MMU [=y] so CONFIG_DRM_KMS_HELPER will wrongly set to =y which will cause the issue. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
On 10/27/21 14:18, Arnd Bergmann wrote: [snip] > Right, how about this change on top? > > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -117,9 +117,8 @@ config DRM_DEBUG_MODESET_LOCK > > config DRM_FBDEV_EMULATION > bool "Enable legacy fbdev support for your modesetting driver" > - depends on DRM > - depends on FB=y || FB=DRM > - select DRM_KMS_HELPER > + depends on DRM_KMS_HELPER > + depends on FB=y || FB=DRM_KMS_HELPER > select FB_CFB_FILLRECT > select FB_CFB_COPYAREA > select FB_CFB_IMAGEBLIT > > That would probably make it work for DRM=y, FB=m, DRM_KMS_HELPER=m, > but it needs more randconfig testing, which I can help with. Looks good to me as well. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
On Wed, Oct 27, 2021 at 3:06 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Wed, 27 Oct 2021, Arnd Bergmann <arnd@kernel.org> wrote: > > This fails because of all the other drivers that try to 'select DRM_KMS_HELPER'. > > Kconfig will now complain about a symbol that gets selected while its > > dependencies > > are not met. > > > > To work around that, every single driver that has 'selects DRM_KMS_HELPER' would > > now have to also list 'depends on (DRM_FBDEV_EMULATION && FB) || > > !DRM_FBDEV_EMULATION'. > > So the fix would be that nobody selects DRM_KMS_HELPER... That's not going to help in this case, the way the helper functions work is that you select them as needed, and you avoid the other dependencies. This part works fine. We could probably get rid of this symbol by just making it an unconditional part of drm.ko, as almost every driver ends up using it anyway. Arguably, this would make the end result worse, as you'd again get drm.ko itself to link against the old framebuffer code. What I'm not sure about is whether drivers/video/fbdev/core/fb.ko could be split up into smaller parts so DRM_FBDEV_EMULATION could only depend on a set of common code without the bits that are needed for the classic fbdev drivers. Arnd
On 10/27/21 15:25, Arnd Bergmann wrote: [snip] > That's not going to help in this case, the way the helper functions work is that > you select them as needed, and you avoid the other dependencies. This part > works fine. > > We could probably get rid of this symbol by just making it an unconditional > part of drm.ko, as almost every driver ends up using it anyway. > > Arguably, this would make the end result worse, as you'd again get drm.ko > itself to link against the old framebuffer code. > > What I'm not sure about is whether drivers/video/fbdev/core/fb.ko could > be split up into smaller parts so DRM_FBDEV_EMULATION could > only depend on a set of common code without the bits that are needed > for the classic fbdev drivers. > I attempted to do something like that but the changes were nacked: https://patchwork.kernel.org/project/linux-fbdev/list/?series=538227 > Arnd > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
On Wed, Oct 27, 2021 at 03:19:34PM +0200, Javier Martinez Canillas wrote: > On 10/27/21 14:18, Arnd Bergmann wrote: > > [snip] > > > Right, how about this change on top? > > > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -117,9 +117,8 @@ config DRM_DEBUG_MODESET_LOCK > > > > config DRM_FBDEV_EMULATION > > bool "Enable legacy fbdev support for your modesetting driver" > > - depends on DRM > > - depends on FB=y || FB=DRM > > - select DRM_KMS_HELPER > > + depends on DRM_KMS_HELPER > > + depends on FB=y || FB=DRM_KMS_HELPER > > select FB_CFB_FILLRECT > > select FB_CFB_COPYAREA > > select FB_CFB_IMAGEBLIT > > > > That would probably make it work for DRM=y, FB=m, DRM_KMS_HELPER=m, > > but it needs more randconfig testing, which I can help with. > > Looks good to me as well. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Is the mess I created sorted now, or something for me to do? I'm terribly burried in stuff :-/ -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Thu, Oct 28, 2021 at 5:24 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Oct 27, 2021 at 03:19:34PM +0200, Javier Martinez Canillas wrote: > > On 10/27/21 14:18, Arnd Bergmann wrote: > > > > [snip] > > > > > Right, how about this change on top? > > > > > > --- a/drivers/gpu/drm/Kconfig > > > +++ b/drivers/gpu/drm/Kconfig > > > @@ -117,9 +117,8 @@ config DRM_DEBUG_MODESET_LOCK > > > > > > config DRM_FBDEV_EMULATION > > > bool "Enable legacy fbdev support for your modesetting driver" > > > - depends on DRM > > > - depends on FB=y || FB=DRM > > > - select DRM_KMS_HELPER > > > + depends on DRM_KMS_HELPER > > > + depends on FB=y || FB=DRM_KMS_HELPER > > > select FB_CFB_FILLRECT > > > select FB_CFB_COPYAREA > > > select FB_CFB_IMAGEBLIT > > > > > > That would probably make it work for DRM=y, FB=m, DRM_KMS_HELPER=m, > > > but it needs more randconfig testing, which I can help with. > > > > Looks good to me as well. > > > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > Is the mess I created sorted now, or something for me to do? I'm terribly > burried in stuff :-/ I have done a few days worth of build testing with that patch applied, and did not see any other problems. I've written a proper description and submitted it as https://lore.kernel.org/all/20211029120307.1407047-1-arnd@kernel.org/T The version you have in linux-next works correctly, but this patch on top is an improvement. Arnd
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index cea777ae7fb9..9199f53861ca 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -103,7 +103,7 @@ config DRM_DEBUG_DP_MST_TOPOLOGY_REFS config DRM_FBDEV_EMULATION bool "Enable legacy fbdev support for your modesetting driver" depends on DRM - depends on FB + depends on FB=y || FB=DRM select DRM_KMS_HELPER select FB_CFB_FILLRECT select FB_CFB_COPYAREA