drm: Hack around CONFIG_AGP=m build failures

Commit Message

Jani Nikula Oct. 1, 2015, 8:19 a.m. UTC

On Thu, 01 Oct 2015, Daniel Vetter <[e-mail]> wrote:
> Surprisingly kbuild can't cope with tristates in the
> <module>-$(CONFIG_FOO) pattern. This patch hacks up a solution.

Given that it's two distinct Makefile variables (foo-y and foo-m) being
assigned to, I don't really find this surprising. Maybe this could be
made to work as a convenience, but there might be other, more surpising
consequences.

> Reported-by: Stephen Rothwell <[e-mail]>
> Reported-by: Ville Syrjälä <[e-mail]>
> Cc: Stephen Rothwell <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Michal Marek <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  drivers/gpu/drm/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index f458d6e33655..e814517513ce 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -21,6 +21,8 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>  drm-$(CONFIG_OF) += drm_of.o
>  drm-$(CONFIG_AGP) += drm_agpsupport.o
>  
> +drm-y += $(drm-m)

I know what you're doing here, but somehow this looks wrong to me. A bit
of git grep also doesn't find convincing arguments to back this up.

I'd solve this with a bit of fairly straightforward indirection in the
Kconfig to keep the Makefile itself neat. I think it also better
documents the intention here.

Patch follows.

BR,
Jani.


From efbc9fb6f6284e81a4b9e7599b3233b656cec452 Mon Sep 17 00:00:00 2001
From: Jani Nikula <[e-mail]>
Date: Thu, 1 Oct 2015 10:53:22 +0300
Subject: [PATCH] drm: fix CONFIG_AGP=m build failures
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Cc: Jani Nikula <[e-mail]>

If CONFIG_AGP is enabled as a module, drm_agpsupport.o is not linked
into drm, causing:

ERROR: "drm_agp_unbind_ioctl" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "drm_agp_init" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "drm_agp_alloc_ioctl" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "drm_agp_clear" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "drm_agp_info_ioctl" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "drm_agp_enable_ioctl" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "drm_agp_release_ioctl" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "drm_agp_bind_ioctl" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "drm_agp_acquire_ioctl" [drivers/gpu/drm/drm.ko] undefined!
ERROR: "drm_agp_free_ioctl" [drivers/gpu/drm/drm.ko] undefined!

Regression from
commit a7fb8a23c1afa607ec8ce9f61df645f37c529434
Author: Daniel Vetter <[e-mail]>
Date:   Wed Sep 9 16:45:52 2015 +0200

    drm: Remove __OS_HAS_AGP

Add an intermediate bool CONFIG_DRM_AGP, selected if CONFIG_AGP is
enabled either as module or built-in.

Reported-by: Stephen Rothwell <[e-mail]>
Reported-by: Ville Syrjälä <[e-mail]>
Cc: Stephen Rothwell <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Michal Marek <[e-mail]>
Cc: [e-mail]
Cc: Daniel Vetter <[e-mail]>
Signed-off-by: Jani Nikula <[e-mail]>
---
 drivers/gpu/drm/Kconfig  | 4 ++++
 drivers/gpu/drm/Makefile | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Michal Marek Oct. 1, 2015, 9:58 a.m. UTC | #1

On 2015-10-01 10:19, Jani Nikula wrote:
> On Thu, 01 Oct 2015, Daniel Vetter <[e-mail]> wrote:
>> Surprisingly kbuild can't cope with tristates in the
>> <module>-$(CONFIG_FOO) pattern. This patch hacks up a solution.
> 
> Given that it's two distinct Makefile variables (foo-y and foo-m) being
> assigned to, I don't really find this surprising. Maybe this could be
> made to work as a convenience, but there might be other, more surpising
> consequences.

I actually think that kbuild should be able to handle this. The likely
reason why it is not doing it right now is that in an ideal world,
modules are modules can be built out of tree against just the kernel and
their static dependencies. In real world, we sometimes have features in
modules that are enabled if other modules are enabled. I'll post a patch
later. We also have lots of tests fo CONFIG_FOO || CONFIG_FOO_MODULE in
built-in code, which is a similar case.

Michal

Daniel Vetter Oct. 1, 2015, 10:17 a.m. UTC | #2

On Thu, Oct 01, 2015 at 11:58:32AM +0200, Michal Marek wrote:
> On 2015-10-01 10:19, Jani Nikula wrote:
> > On Thu, 01 Oct 2015, Daniel Vetter <[e-mail]> wrote:
> >> Surprisingly kbuild can't cope with tristates in the
> >> <module>-$(CONFIG_FOO) pattern. This patch hacks up a solution.
> > 
> > Given that it's two distinct Makefile variables (foo-y and foo-m) being
> > assigned to, I don't really find this surprising. Maybe this could be
> > made to work as a convenience, but there might be other, more surpising
> > consequences.
> 
> I actually think that kbuild should be able to handle this. The likely
> reason why it is not doing it right now is that in an ideal world,
> modules are modules can be built out of tree against just the kernel and
> their static dependencies. In real world, we sometimes have features in
> modules that are enabled if other modules are enabled. I'll post a patch
> later. We also have lots of tests fo CONFIG_FOO || CONFIG_FOO_MODULE in
> built-in code, which is a similar case.

Cool. I'll keep this hack in drm-misc then. Please cc me on the proper
solution so I know when I can revert it again.
-Daniel

Michal Marek Oct. 1, 2015, 2:50 p.m. UTC | #3

On 2015-10-01 12:17, Daniel Vetter wrote:
> On Thu, Oct 01, 2015 at 11:58:32AM +0200, Michal Marek wrote:
>> On 2015-10-01 10:19, Jani Nikula wrote:
>>> On Thu, 01 Oct 2015, Daniel Vetter <[e-mail]> wrote:
>>>> Surprisingly kbuild can't cope with tristates in the
>>>> <module>-$(CONFIG_FOO) pattern. This patch hacks up a solution.
>>>
>>> Given that it's two distinct Makefile variables (foo-y and foo-m) being
>>> assigned to, I don't really find this surprising. Maybe this could be
>>> made to work as a convenience, but there might be other, more surpising
>>> consequences.
>>
>> I actually think that kbuild should be able to handle this. The likely
>> reason why it is not doing it right now is that in an ideal world,
>> modules are modules can be built out of tree against just the kernel and
>> their static dependencies. In real world, we sometimes have features in
>> modules that are enabled if other modules are enabled. I'll post a patch
>> later. We also have lots of tests fo CONFIG_FOO || CONFIG_FOO_MODULE in
>> built-in code, which is a similar case.
> 
> Cool. I'll keep this hack in drm-misc then. Please cc me on the proper
> solution so I know when I can revert it again.

It's not as trivial as it seemed, because there are at least three
Makefiles that rely on the current behavior:

init/Makefile
drivers/misc/ibmasm/Makefile
fs/logfs/Makefile

While ibmasm and logfs can and probably should be fixed to work with
modular 8250 and mtd, respectively, init/Makefile would need a
workaround to only pick up do_mounts_rd.o and do_mounts_md.o if the
respective block drivers are built-in. So we would be trading one hack
for another.

Michal

Michal Marek Oct. 1, 2015, 2:57 p.m. UTC | #4

On 2015-10-01 16:50, Michal Marek wrote:
> On 2015-10-01 12:17, Daniel Vetter wrote:
>> On Thu, Oct 01, 2015 at 11:58:32AM +0200, Michal Marek wrote:
>>> On 2015-10-01 10:19, Jani Nikula wrote:
>>>> On Thu, 01 Oct 2015, Daniel Vetter <[e-mail]> wrote:
>>>>> Surprisingly kbuild can't cope with tristates in the
>>>>> <module>-$(CONFIG_FOO) pattern. This patch hacks up a solution.
>>>>
>>>> Given that it's two distinct Makefile variables (foo-y and foo-m) being
>>>> assigned to, I don't really find this surprising. Maybe this could be
>>>> made to work as a convenience, but there might be other, more surpising
>>>> consequences.
>>>
>>> I actually think that kbuild should be able to handle this. The likely
>>> reason why it is not doing it right now is that in an ideal world,
>>> modules are modules can be built out of tree against just the kernel and
>>> their static dependencies. In real world, we sometimes have features in
>>> modules that are enabled if other modules are enabled. I'll post a patch
>>> later. We also have lots of tests fo CONFIG_FOO || CONFIG_FOO_MODULE in
>>> built-in code, which is a similar case.
>>
>> Cool. I'll keep this hack in drm-misc then. Please cc me on the proper
>> solution so I know when I can revert it again.
> 
> It's not as trivial as it seemed, because there are at least three
> Makefiles that rely on the current behavior:
> 
> init/Makefile
> drivers/misc/ibmasm/Makefile
> fs/logfs/Makefile

drivers/usb/chipidea is also on the list.

Michal

Patch

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df2eed8..aecbec030aa4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -7,6 +7,7 @@ 
 menuconfig DRM
 	tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
 	depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && MMU && HAS_DMA
+	select DRM_AGP if AGP
 	select HDMI
 	select FB_CMDLINE
 	select I2C
@@ -21,6 +22,9 @@  menuconfig DRM
 	  details.  You should also select and configure AGP
 	  (/dev/agpgart) support if it is available for your platform.
 
+config DRM_AGP
+	bool
+
 config DRM_MIPI_DSI
 	bool
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index f458d6e33655..c04d5a33fcac 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -19,7 +19,7 @@  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 drm-$(CONFIG_PCI) += ati_pcigart.o
 drm-$(CONFIG_DRM_PANEL) += drm_panel.o
 drm-$(CONFIG_OF) += drm_of.o
-drm-$(CONFIG_AGP) += drm_agpsupport.o
+drm-$(CONFIG_DRM_AGP) += drm_agpsupport.o
 
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o