[PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Arnd Bergmann]

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>
---
I posted this in July, the patch is still required and should work
on its own.
---
 drivers/gpu/drm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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
-- 
2.29.2

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Kees Cook]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Daniel Vetter]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Jani Nikula]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Jani Nikula]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Arnd Bergmann]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Javier Martinez Canillas]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Arnd Bergmann]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Javier Martinez Canillas]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Jani Nikula]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Arnd Bergmann]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Javier Martinez Canillas]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Jani Nikula]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Javier Martinez Canillas]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Jani Nikula]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Javier Martinez Canillas]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Daniel Vetter]

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

Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Arnd Bergmann]

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