[PATCH 0/2] drm: add header tests

[PATCH 0/2] drm: add header tests

[Jani Nikula]

Add CONFIG_DRM_HEADER_TEST to ensure drm headers are self-contained and
pass kernel-doc. And for starters, fix one header that this catches.

Jani Nikula (2):
  drm/client: include types.h to make drm_client_event.h self-contained
  drm: ensure drm headers are self-contained and pass kernel-doc

 Kbuild                         |  1 +
 drivers/gpu/drm/Kconfig        | 11 +++++++++++
 drivers/gpu/drm/Makefile       | 18 ++++++++++++++++++
 include/Kbuild                 |  1 +
 include/drm/Makefile           | 18 ++++++++++++++++++
 include/drm/drm_client_event.h |  2 ++
 6 files changed, 51 insertions(+)
 create mode 100644 include/Kbuild
 create mode 100644 include/drm/Makefile

-- 
2.39.5

[PATCH 1/2] drm/client: include types.h to make drm_client_event.h self-contained

[Jani Nikula]

drm_client_event.h uses bool without types.h, include it.

Fixes: bf17766f1083 ("drm/client: Move suspend/resume into DRM client callbacks")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 include/drm/drm_client_event.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_client_event.h b/include/drm/drm_client_event.h
index 99863554b055..1d544d3a3228 100644
--- a/include/drm/drm_client_event.h
+++ b/include/drm/drm_client_event.h
@@ -3,6 +3,8 @@
 #ifndef _DRM_CLIENT_EVENT_H_
 #define _DRM_CLIENT_EVENT_H_
 
+#include <linux/types.h>
+
 struct drm_device;
 
 #if defined(CONFIG_DRM_CLIENT)
-- 
2.39.5

[PATCH 2/2] drm: ensure drm headers are self-contained and pass kernel-doc

[Jani Nikula]

Ensure drm headers build, are self-contained, have header guards, and
have no kernel-doc warnings, when CONFIG_DRM_HEADER_TEST=y.

The mechanism follows similar patters used in i915, xe, and usr/include.

To cover include/drm, we need to recurse there using the top level
Kbuild and the new include/Kbuild files.

v4: check for CONFIG_WERROR in addition to CONFIG_DRM_WERROR

v3: adapt to upstream build changes

v2: make DRM_HEADER_TEST depend on DRM

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 Kbuild                   |  1 +
 drivers/gpu/drm/Kconfig  | 11 +++++++++++
 drivers/gpu/drm/Makefile | 18 ++++++++++++++++++
 include/Kbuild           |  1 +
 include/drm/Makefile     | 18 ++++++++++++++++++
 5 files changed, 49 insertions(+)
 create mode 100644 include/Kbuild
 create mode 100644 include/drm/Makefile

diff --git a/Kbuild b/Kbuild
index 464b34a08f51..f327ca86990c 100644
--- a/Kbuild
+++ b/Kbuild
@@ -97,3 +97,4 @@ obj-$(CONFIG_SAMPLES)	+= samples/
 obj-$(CONFIG_NET)	+= net/
 obj-y			+= virt/
 obj-y			+= $(ARCH_DRIVERS)
+obj-$(CONFIG_DRM_HEADER_TEST)	+= include/
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index fbef3f471bd0..f9b3ebf63fa9 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -494,6 +494,17 @@ config DRM_WERROR
 
 	  If in doubt, say N.
 
+config DRM_HEADER_TEST
+	bool "Ensure DRM headers are self-contained and pass kernel-doc"
+	depends on DRM && EXPERT
+	default n
+	help
+	  Ensure the DRM subsystem headers both under drivers/gpu/drm and
+	  include/drm compile, are self-contained, have header guards, and have
+	  no kernel-doc warnings.
+
+	  If in doubt, say N.
+
 endif
 
 # Separate option because drm_panel_orientation_quirks.c is shared with fbdev
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 85af94bb907d..42901f877bf2 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -222,3 +222,21 @@ obj-y			+= solomon/
 obj-$(CONFIG_DRM_SPRD) += sprd/
 obj-$(CONFIG_DRM_LOONGSON) += loongson/
 obj-$(CONFIG_DRM_POWERVR) += imagination/
+
+# Ensure drm headers are self-contained and pass kernel-doc
+hdrtest-files := \
+	$(shell cd $(src) && find . -maxdepth 1 -name 'drm_*.h') \
+	$(shell cd $(src) && find display lib -name '*.h')
+
+always-$(CONFIG_DRM_HEADER_TEST) += \
+	$(patsubst %.h,%.hdrtest, $(hdrtest-files))
+
+# Include the header twice to detect missing include guard.
+quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
+      cmd_hdrtest = \
+		$(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
+		$(srctree)/scripts/kernel-doc -none $(if $(CONFIG_WERROR)$(CONFIG_DRM_WERROR),-Werror) $<; \
+		touch $@
+
+$(obj)/%.hdrtest: $(src)/%.h FORCE
+	$(call if_changed_dep,hdrtest)
diff --git a/include/Kbuild b/include/Kbuild
new file mode 100644
index 000000000000..5e76a599e2dd
--- /dev/null
+++ b/include/Kbuild
@@ -0,0 +1 @@
+obj-$(CONFIG_DRM_HEADER_TEST)	+= drm/
diff --git a/include/drm/Makefile b/include/drm/Makefile
new file mode 100644
index 000000000000..a7bd15d2803e
--- /dev/null
+++ b/include/drm/Makefile
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0
+
+# Ensure drm headers are self-contained and pass kernel-doc
+hdrtest-files := \
+	$(shell cd $(src) && find * -name '*.h' 2>/dev/null)
+
+always-$(CONFIG_DRM_HEADER_TEST) += \
+	$(patsubst %.h,%.hdrtest, $(hdrtest-files))
+
+# Include the header twice to detect missing include guard.
+quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
+      cmd_hdrtest = \
+		$(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
+		$(srctree)/scripts/kernel-doc -none $(if $(CONFIG_WERROR)$(CONFIG_DRM_WERROR),-Werror) $<; \
+		touch $@
+
+$(obj)/%.hdrtest: $(src)/%.h FORCE
+	$(call if_changed_dep,hdrtest)
-- 
2.39.5

Re: [PATCH 2/2] drm: ensure drm headers are self-contained and pass kernel-doc

[Masahiro Yamada]

+CC: Linus

On Wed, Jan 22, 2025 at 11:41 PM Jani Nikula <jani.nikula@intel.com> wrote:
>
> Ensure drm headers build, are self-contained, have header guards, and
> have no kernel-doc warnings, when CONFIG_DRM_HEADER_TEST=y.
>
> The mechanism follows similar patters used in i915, xe, and usr/include.
>
> To cover include/drm, we need to recurse there using the top level
> Kbuild and the new include/Kbuild files.

NACK.

I replied here:
https://lore.kernel.org/all/CAK7LNARJgqADxnOXAX49XzDFD4zT=7i8yTB0o=EmNtxmScq8jA@mail.gmail.com/T/#u


I CCed Linus to avoid him accidentally pulling this.
He disliked this misfeature.




>
> v4: check for CONFIG_WERROR in addition to CONFIG_DRM_WERROR
>
> v3: adapt to upstream build changes
>
> v2: make DRM_HEADER_TEST depend on DRM
>
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  Kbuild                   |  1 +
>  drivers/gpu/drm/Kconfig  | 11 +++++++++++
>  drivers/gpu/drm/Makefile | 18 ++++++++++++++++++
>  include/Kbuild           |  1 +
>  include/drm/Makefile     | 18 ++++++++++++++++++
>  5 files changed, 49 insertions(+)
>  create mode 100644 include/Kbuild
>  create mode 100644 include/drm/Makefile
>
> diff --git a/Kbuild b/Kbuild
> index 464b34a08f51..f327ca86990c 100644
> --- a/Kbuild
> +++ b/Kbuild
> @@ -97,3 +97,4 @@ obj-$(CONFIG_SAMPLES) += samples/
>  obj-$(CONFIG_NET)      += net/
>  obj-y                  += virt/
>  obj-y                  += $(ARCH_DRIVERS)
> +obj-$(CONFIG_DRM_HEADER_TEST)  += include/
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index fbef3f471bd0..f9b3ebf63fa9 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -494,6 +494,17 @@ config DRM_WERROR
>
>           If in doubt, say N.
>
> +config DRM_HEADER_TEST
> +       bool "Ensure DRM headers are self-contained and pass kernel-doc"
> +       depends on DRM && EXPERT
> +       default n
> +       help
> +         Ensure the DRM subsystem headers both under drivers/gpu/drm and
> +         include/drm compile, are self-contained, have header guards, and have
> +         no kernel-doc warnings.
> +
> +         If in doubt, say N.
> +
>  endif
>
>  # Separate option because drm_panel_orientation_quirks.c is shared with fbdev
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 85af94bb907d..42901f877bf2 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -222,3 +222,21 @@ obj-y                      += solomon/
>  obj-$(CONFIG_DRM_SPRD) += sprd/
>  obj-$(CONFIG_DRM_LOONGSON) += loongson/
>  obj-$(CONFIG_DRM_POWERVR) += imagination/
> +
> +# Ensure drm headers are self-contained and pass kernel-doc
> +hdrtest-files := \
> +       $(shell cd $(src) && find . -maxdepth 1 -name 'drm_*.h') \
> +       $(shell cd $(src) && find display lib -name '*.h')
> +
> +always-$(CONFIG_DRM_HEADER_TEST) += \
> +       $(patsubst %.h,%.hdrtest, $(hdrtest-files))
> +
> +# Include the header twice to detect missing include guard.
> +quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
> +      cmd_hdrtest = \
> +               $(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
> +               $(srctree)/scripts/kernel-doc -none $(if $(CONFIG_WERROR)$(CONFIG_DRM_WERROR),-Werror) $<; \
> +               touch $@
> +
> +$(obj)/%.hdrtest: $(src)/%.h FORCE
> +       $(call if_changed_dep,hdrtest)
> diff --git a/include/Kbuild b/include/Kbuild
> new file mode 100644
> index 000000000000..5e76a599e2dd
> --- /dev/null
> +++ b/include/Kbuild
> @@ -0,0 +1 @@
> +obj-$(CONFIG_DRM_HEADER_TEST)  += drm/
> diff --git a/include/drm/Makefile b/include/drm/Makefile
> new file mode 100644
> index 000000000000..a7bd15d2803e
> --- /dev/null
> +++ b/include/drm/Makefile
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# Ensure drm headers are self-contained and pass kernel-doc
> +hdrtest-files := \
> +       $(shell cd $(src) && find * -name '*.h' 2>/dev/null)
> +
> +always-$(CONFIG_DRM_HEADER_TEST) += \
> +       $(patsubst %.h,%.hdrtest, $(hdrtest-files))
> +
> +# Include the header twice to detect missing include guard.
> +quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
> +      cmd_hdrtest = \
> +               $(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
> +               $(srctree)/scripts/kernel-doc -none $(if $(CONFIG_WERROR)$(CONFIG_DRM_WERROR),-Werror) $<; \
> +               touch $@
> +
> +$(obj)/%.hdrtest: $(src)/%.h FORCE
> +       $(call if_changed_dep,hdrtest)
> --
> 2.39.5
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] drm: ensure drm headers are self-contained and pass kernel-doc

[Jani Nikula]

On Mon, 03 Mar 2025, Masahiro Yamada <masahiroy@kernel.org> wrote:
> +CC: Linus
>
> On Wed, Jan 22, 2025 at 11:41 PM Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> Ensure drm headers build, are self-contained, have header guards, and
>> have no kernel-doc warnings, when CONFIG_DRM_HEADER_TEST=y.
>>
>> The mechanism follows similar patters used in i915, xe, and usr/include.
>>
>> To cover include/drm, we need to recurse there using the top level
>> Kbuild and the new include/Kbuild files.
>
> NACK.
>
> I replied here:
> https://lore.kernel.org/all/CAK7LNARJgqADxnOXAX49XzDFD4zT=7i8yTB0o=EmNtxmScq8jA@mail.gmail.com/T/#u

I really don't find it fair to completely ignore several pings over an
extended period of time, and then show up to NAK after the patches have
been merged.

> I CCed Linus to avoid him accidentally pulling this.
> He disliked this misfeature.

I believe being able to statically check the headers at build time, both
by the developers and CI, depending on a config option, makes for a more
pleasant development experience.

We've had this in i915 and xe for a long time, and we avoid a lot of
build breakage due to missing includes e.g. while refactoring, and we
don't get reports about kernel-doc issues either. Because they all fail
at build, and we catch the issues pre-merge. We skip a whole class of
merge->dammit->fix cycles with this.

All of the drm headers are clean and pass. We don't add any exception
lists. It's not enabled by default.

I can appreciate this might not be the best approach for all of
include/linux, but for include/drm, I think it's definitely a win.

And one of the underlying goals is to make for minimal headers with
minimal includes and minimal dependencies, preferring forward
declarations over includes, splitting functionality by header, etc. It's
just that doing that often leads to broken headers, unless you actually
build test them... and here we are.


BR,
Jani.


>
>
>
>
>>
>> v4: check for CONFIG_WERROR in addition to CONFIG_DRM_WERROR
>>
>> v3: adapt to upstream build changes
>>
>> v2: make DRM_HEADER_TEST depend on DRM
>>
>> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  Kbuild                   |  1 +
>>  drivers/gpu/drm/Kconfig  | 11 +++++++++++
>>  drivers/gpu/drm/Makefile | 18 ++++++++++++++++++
>>  include/Kbuild           |  1 +
>>  include/drm/Makefile     | 18 ++++++++++++++++++
>>  5 files changed, 49 insertions(+)
>>  create mode 100644 include/Kbuild
>>  create mode 100644 include/drm/Makefile
>>
>> diff --git a/Kbuild b/Kbuild
>> index 464b34a08f51..f327ca86990c 100644
>> --- a/Kbuild
>> +++ b/Kbuild
>> @@ -97,3 +97,4 @@ obj-$(CONFIG_SAMPLES) += samples/
>>  obj-$(CONFIG_NET)      += net/
>>  obj-y                  += virt/
>>  obj-y                  += $(ARCH_DRIVERS)
>> +obj-$(CONFIG_DRM_HEADER_TEST)  += include/
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index fbef3f471bd0..f9b3ebf63fa9 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -494,6 +494,17 @@ config DRM_WERROR
>>
>>           If in doubt, say N.
>>
>> +config DRM_HEADER_TEST
>> +       bool "Ensure DRM headers are self-contained and pass kernel-doc"
>> +       depends on DRM && EXPERT
>> +       default n
>> +       help
>> +         Ensure the DRM subsystem headers both under drivers/gpu/drm and
>> +         include/drm compile, are self-contained, have header guards, and have
>> +         no kernel-doc warnings.
>> +
>> +         If in doubt, say N.
>> +
>>  endif
>>
>>  # Separate option because drm_panel_orientation_quirks.c is shared with fbdev
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 85af94bb907d..42901f877bf2 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -222,3 +222,21 @@ obj-y                      += solomon/
>>  obj-$(CONFIG_DRM_SPRD) += sprd/
>>  obj-$(CONFIG_DRM_LOONGSON) += loongson/
>>  obj-$(CONFIG_DRM_POWERVR) += imagination/
>> +
>> +# Ensure drm headers are self-contained and pass kernel-doc
>> +hdrtest-files := \
>> +       $(shell cd $(src) && find . -maxdepth 1 -name 'drm_*.h') \
>> +       $(shell cd $(src) && find display lib -name '*.h')
>> +
>> +always-$(CONFIG_DRM_HEADER_TEST) += \
>> +       $(patsubst %.h,%.hdrtest, $(hdrtest-files))
>> +
>> +# Include the header twice to detect missing include guard.
>> +quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
>> +      cmd_hdrtest = \
>> +               $(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
>> +               $(srctree)/scripts/kernel-doc -none $(if $(CONFIG_WERROR)$(CONFIG_DRM_WERROR),-Werror) $<; \
>> +               touch $@
>> +
>> +$(obj)/%.hdrtest: $(src)/%.h FORCE
>> +       $(call if_changed_dep,hdrtest)
>> diff --git a/include/Kbuild b/include/Kbuild
>> new file mode 100644
>> index 000000000000..5e76a599e2dd
>> --- /dev/null
>> +++ b/include/Kbuild
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_DRM_HEADER_TEST)  += drm/
>> diff --git a/include/drm/Makefile b/include/drm/Makefile
>> new file mode 100644
>> index 000000000000..a7bd15d2803e
>> --- /dev/null
>> +++ b/include/drm/Makefile
>> @@ -0,0 +1,18 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +# Ensure drm headers are self-contained and pass kernel-doc
>> +hdrtest-files := \
>> +       $(shell cd $(src) && find * -name '*.h' 2>/dev/null)
>> +
>> +always-$(CONFIG_DRM_HEADER_TEST) += \
>> +       $(patsubst %.h,%.hdrtest, $(hdrtest-files))
>> +
>> +# Include the header twice to detect missing include guard.
>> +quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
>> +      cmd_hdrtest = \
>> +               $(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
>> +               $(srctree)/scripts/kernel-doc -none $(if $(CONFIG_WERROR)$(CONFIG_DRM_WERROR),-Werror) $<; \
>> +               touch $@
>> +
>> +$(obj)/%.hdrtest: $(src)/%.h FORCE
>> +       $(call if_changed_dep,hdrtest)
>> --
>> 2.39.5
>>

-- 
Jani Nikula, Intel

Re: [PATCH 2/2] drm: ensure drm headers are self-contained and pass kernel-doc

[Masahiro Yamada]

On Mon, Mar 3, 2025 at 7:02 PM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Mon, 03 Mar 2025, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > +CC: Linus
> >
> > On Wed, Jan 22, 2025 at 11:41 PM Jani Nikula <jani.nikula@intel.com> wrote:
> >>
> >> Ensure drm headers build, are self-contained, have header guards, and
> >> have no kernel-doc warnings, when CONFIG_DRM_HEADER_TEST=y.
> >>
> >> The mechanism follows similar patters used in i915, xe, and usr/include.
> >>
> >> To cover include/drm, we need to recurse there using the top level
> >> Kbuild and the new include/Kbuild files.
> >
> > NACK.
> >
> > I replied here:
> > https://lore.kernel.org/all/CAK7LNARJgqADxnOXAX49XzDFD4zT=7i8yTB0o=EmNtxmScq8jA@mail.gmail.com/T/#u
>
> I really don't find it fair to completely ignore several pings over an
> extended period of time, and then show up to NAK after the patches have
> been merged.


Sorry, I didn't mean to ignore it - I simply didn't notice it.

I regularly check linux-kbuild and linux-kernel MLs (though I still miss
responding to many emails).
However, I don't check the drm ML at all.
I need to reconsider my email filtering rules, but in reality,
I can't respond to all emails in time.

I believe you are re-adding something Linus was negative about:
https://lore.kernel.org/all/87a7982hwc.fsf@intel.com/




> > I CCed Linus to avoid him accidentally pulling this.
> > He disliked this misfeature.
>
> I believe being able to statically check the headers at build time, both
> by the developers and CI, depending on a config option, makes for a more
> pleasant development experience.
>
> We've had this in i915 and xe for a long time, and we avoid a lot of
> build breakage due to missing includes e.g. while refactoring, and we
> don't get reports about kernel-doc issues either. Because they all fail
> at build, and we catch the issues pre-merge. We skip a whole class of
> merge->dammit->fix cycles with this.
>
> All of the drm headers are clean and pass. We don't add any exception
> lists. It's not enabled by default.

I'm not a big fan of the header tests in i915 and xe.
However, you've built a fence and you are dong what you want
in driver-local Makefiles, so I can't avoid them.


>
> I can appreciate this might not be the best approach for all of
> include/linux, but for include/drm, I think it's definitely a win.
>
> And one of the underlying goals is to make for minimal headers with
> minimal includes and minimal dependencies, preferring forward
> declarations over includes, splitting functionality by header, etc. It's
> just that doing that often leads to broken headers, unless you actually
> build test them... and here we are.


What I learned from my last attempt is that we cannot avoid
false positives without adding a lot of exceptions.

We can never be certain whether you are making DRM headers
self-contained for valid reasons or for hypothetical, invalid ones.







>
> BR,
> Jani.
>
>
> >
> >
> >
> >
> >>
> >> v4: check for CONFIG_WERROR in addition to CONFIG_DRM_WERROR
> >>
> >> v3: adapt to upstream build changes
> >>
> >> v2: make DRM_HEADER_TEST depend on DRM
> >>
> >> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> >> Cc: David Airlie <airlied@gmail.com>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Maxime Ripard <mripard@kernel.org>
> >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  Kbuild                   |  1 +
> >>  drivers/gpu/drm/Kconfig  | 11 +++++++++++
> >>  drivers/gpu/drm/Makefile | 18 ++++++++++++++++++
> >>  include/Kbuild           |  1 +
> >>  include/drm/Makefile     | 18 ++++++++++++++++++
> >>  5 files changed, 49 insertions(+)
> >>  create mode 100644 include/Kbuild
> >>  create mode 100644 include/drm/Makefile
> >>
> >> diff --git a/Kbuild b/Kbuild
> >> index 464b34a08f51..f327ca86990c 100644
> >> --- a/Kbuild
> >> +++ b/Kbuild
> >> @@ -97,3 +97,4 @@ obj-$(CONFIG_SAMPLES) += samples/
> >>  obj-$(CONFIG_NET)      += net/
> >>  obj-y                  += virt/
> >>  obj-y                  += $(ARCH_DRIVERS)
> >> +obj-$(CONFIG_DRM_HEADER_TEST)  += include/
> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >> index fbef3f471bd0..f9b3ebf63fa9 100644
> >> --- a/drivers/gpu/drm/Kconfig
> >> +++ b/drivers/gpu/drm/Kconfig
> >> @@ -494,6 +494,17 @@ config DRM_WERROR
> >>
> >>           If in doubt, say N.
> >>
> >> +config DRM_HEADER_TEST
> >> +       bool "Ensure DRM headers are self-contained and pass kernel-doc"
> >> +       depends on DRM && EXPERT
> >> +       default n
> >> +       help
> >> +         Ensure the DRM subsystem headers both under drivers/gpu/drm and
> >> +         include/drm compile, are self-contained, have header guards, and have
> >> +         no kernel-doc warnings.
> >> +
> >> +         If in doubt, say N.
> >> +
> >>  endif
> >>
> >>  # Separate option because drm_panel_orientation_quirks.c is shared with fbdev
> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >> index 85af94bb907d..42901f877bf2 100644
> >> --- a/drivers/gpu/drm/Makefile
> >> +++ b/drivers/gpu/drm/Makefile
> >> @@ -222,3 +222,21 @@ obj-y                      += solomon/
> >>  obj-$(CONFIG_DRM_SPRD) += sprd/
> >>  obj-$(CONFIG_DRM_LOONGSON) += loongson/
> >>  obj-$(CONFIG_DRM_POWERVR) += imagination/
> >> +
> >> +# Ensure drm headers are self-contained and pass kernel-doc
> >> +hdrtest-files := \
> >> +       $(shell cd $(src) && find . -maxdepth 1 -name 'drm_*.h') \
> >> +       $(shell cd $(src) && find display lib -name '*.h')
> >> +
> >> +always-$(CONFIG_DRM_HEADER_TEST) += \
> >> +       $(patsubst %.h,%.hdrtest, $(hdrtest-files))
> >> +
> >> +# Include the header twice to detect missing include guard.
> >> +quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
> >> +      cmd_hdrtest = \
> >> +               $(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
> >> +               $(srctree)/scripts/kernel-doc -none $(if $(CONFIG_WERROR)$(CONFIG_DRM_WERROR),-Werror) $<; \
> >> +               touch $@
> >> +
> >> +$(obj)/%.hdrtest: $(src)/%.h FORCE
> >> +       $(call if_changed_dep,hdrtest)
> >> diff --git a/include/Kbuild b/include/Kbuild
> >> new file mode 100644
> >> index 000000000000..5e76a599e2dd
> >> --- /dev/null
> >> +++ b/include/Kbuild
> >> @@ -0,0 +1 @@
> >> +obj-$(CONFIG_DRM_HEADER_TEST)  += drm/
> >> diff --git a/include/drm/Makefile b/include/drm/Makefile
> >> new file mode 100644
> >> index 000000000000..a7bd15d2803e
> >> --- /dev/null
> >> +++ b/include/drm/Makefile
> >> @@ -0,0 +1,18 @@
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +
> >> +# Ensure drm headers are self-contained and pass kernel-doc
> >> +hdrtest-files := \
> >> +       $(shell cd $(src) && find * -name '*.h' 2>/dev/null)
> >> +
> >> +always-$(CONFIG_DRM_HEADER_TEST) += \
> >> +       $(patsubst %.h,%.hdrtest, $(hdrtest-files))
> >> +
> >> +# Include the header twice to detect missing include guard.
> >> +quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
> >> +      cmd_hdrtest = \
> >> +               $(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
> >> +               $(srctree)/scripts/kernel-doc -none $(if $(CONFIG_WERROR)$(CONFIG_DRM_WERROR),-Werror) $<; \
> >> +               touch $@
> >> +
> >> +$(obj)/%.hdrtest: $(src)/%.h FORCE
> >> +       $(call if_changed_dep,hdrtest)
> >> --
> >> 2.39.5
> >>
>
> --
> Jani Nikula, Intel



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] drm: ensure drm headers are self-contained and pass kernel-doc

[Jani Nikula]

On Mon, 03 Mar 2025, Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Mon, Mar 3, 2025 at 7:02 PM Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> On Mon, 03 Mar 2025, Masahiro Yamada <masahiroy@kernel.org> wrote:
>> And one of the underlying goals is to make for minimal headers with
>> minimal includes and minimal dependencies, preferring forward
>> declarations over includes, splitting functionality by header, etc. It's
>> just that doing that often leads to broken headers, unless you actually
>> build test them... and here we are.
>
>
> What I learned from my last attempt is that we cannot avoid
> false positives without adding a lot of exceptions.

All of the drm core, xe and i915 headers build fine without
exceptions. There are no false positives. (*)

> We can never be certain whether you are making DRM headers
> self-contained for valid reasons or for hypothetical, invalid ones.

Please enlighten me. What are hypothetical, invalid reasons for making
headers self-contained?

IMO headers should almost invariably be self-contained, instead of
putting the burden on their users to include other headers to make it
work. It's a PITA in a project the size of the kernel, or even just the
drm subsystem, to track these cases when you modify includes in either
users or the headers being included.

The exception to this are headers that are not to be included directly
by users, but rather by other headers as an implementation detail. There
may be such cases in include/linux, but not under include/drm.


BR,
Jani.


(*) Fine, there's one *intentional* special case in i915.

-- 
Jani Nikula, Intel

Re: [PATCH 2/2] drm: ensure drm headers are self-contained and pass kernel-doc

[Masahiro Yamada]

On Mon, Mar 3, 2025 at 10:53 PM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Mon, 03 Mar 2025, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > On Mon, Mar 3, 2025 at 7:02 PM Jani Nikula <jani.nikula@intel.com> wrote:
> >>
> >> On Mon, 03 Mar 2025, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >> And one of the underlying goals is to make for minimal headers with
> >> minimal includes and minimal dependencies, preferring forward
> >> declarations over includes, splitting functionality by header, etc. It's
> >> just that doing that often leads to broken headers, unless you actually
> >> build test them... and here we are.
> >
> >
> > What I learned from my last attempt is that we cannot avoid
> > false positives without adding a lot of exceptions.
>
> All of the drm core, xe and i915 headers build fine without
> exceptions. There are no false positives. (*)
>
> > We can never be certain whether you are making DRM headers
> > self-contained for valid reasons or for hypothetical, invalid ones.
>
> Please enlighten me. What are hypothetical, invalid reasons for making
> headers self-contained?

See this thread:
https://lore.kernel.org/all/20190718130835.GA28520@lst.de/

When CONFIG_BLOCK=n, it does not make sense to
ensure <linux/iomap.h> is self-contained.
This is just one example.

I am pretty sure I observed more false-positives
in header compile tests.

>
> IMO headers should almost invariably be self-contained, instead of
> putting the burden on their users to include other headers to make it
> work. It's a PITA in a project the size of the kernel, or even just the
> drm subsystem, to track these cases when you modify includes in either
> users or the headers being included.
>
> The exception to this are headers that are not to be included directly
> by users, but rather by other headers as an implementation detail. There
> may be such cases in include/linux, but not under include/drm.

This results in a false check for include/linux/.

I don’t see much sense in doing this exceptionally for include/drm/
after we've learned that it doesn't work globally.





>
> BR,
> Jani.
>
>
> (*) Fine, there's one *intentional* special case in i915.
>
> --
> Jani Nikula, Intel



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] drm: ensure drm headers are self-contained and pass kernel-doc

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

On Wed, Mar 05, 2025 at 03:05:25AM +0900, Masahiro Yamada wrote:
> > IMO headers should almost invariably be self-contained, instead of
> > putting the burden on their users to include other headers to make it
> > work. It's a PITA in a project the size of the kernel, or even just the
> > drm subsystem, to track these cases when you modify includes in either
> > users or the headers being included.
> >
> > The exception to this are headers that are not to be included directly
> > by users, but rather by other headers as an implementation detail. There
> > may be such cases in include/linux, but not under include/drm.
> 
> This results in a false check for include/linux/.
> 
> I don’t see much sense in doing this exceptionally for include/drm/
> after we've learned that it doesn't work globally.

As far as I'm concerned, I find this extremely helpful for DRM. If only
to ensure that the huge amount of work that went into cleaning up our
headers doesn't get lost.

Nobody here claims that it should be enabled globally, just that it
should be enabled for DRM. We already have plenty of exceptions like
that for compiler flags, checkpatch, contribution process, etc. so I'm
not sure why those would be ok, but additional checks limited to a
subsystem wouldn't.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 2/2] drm: ensure drm headers are self-contained and pass kernel-doc

[Masahiro Yamada]

On Wed, Mar 5, 2025 at 10:59 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Wed, Mar 05, 2025 at 03:05:25AM +0900, Masahiro Yamada wrote:
> > > IMO headers should almost invariably be self-contained, instead of
> > > putting the burden on their users to include other headers to make it
> > > work. It's a PITA in a project the size of the kernel, or even just the
> > > drm subsystem, to track these cases when you modify includes in either
> > > users or the headers being included.
> > >
> > > The exception to this are headers that are not to be included directly
> > > by users, but rather by other headers as an implementation detail. There
> > > may be such cases in include/linux, but not under include/drm.
> >
> > This results in a false check for include/linux/.
> >
> > I don’t see much sense in doing this exceptionally for include/drm/
> > after we've learned that it doesn't work globally.
>
> As far as I'm concerned, I find this extremely helpful for DRM. If only
> to ensure that the huge amount of work that went into cleaning up our
> headers doesn't get lost.
>
> Nobody here claims that it should be enabled globally, just that it
> should be enabled for DRM. We already have plenty of exceptions like
> that for compiler flags, checkpatch, contribution process, etc. so I'm
> not sure why those would be ok, but additional checks limited to a
> subsystem wouldn't.
>
> Maxime

Because we learned this feature is broken.
It was broken under include/linux/, so it will be broken under include/drm/ too.

Headers are included conditionally.
There is no need to make them self-contained in all cases
by compile-testing every header detected by the 'find' command.

I am very negative about this patch.
I hope the upstream maintainers and Linus will not pull this.





--
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] drm: ensure drm headers are self-contained and pass kernel-doc

[Jani Nikula]

On Sun, 09 Mar 2025, Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Wed, Mar 5, 2025 at 10:59 PM Maxime Ripard <mripard@kernel.org> wrote:
>> As far as I'm concerned, I find this extremely helpful for DRM. If only
>> to ensure that the huge amount of work that went into cleaning up our
>> headers doesn't get lost.
>>
>> Nobody here claims that it should be enabled globally, just that it
>> should be enabled for DRM. We already have plenty of exceptions like
>> that for compiler flags, checkpatch, contribution process, etc. so I'm
>> not sure why those would be ok, but additional checks limited to a
>> subsystem wouldn't.
>>
>> Maxime
>
> Because we learned this feature is broken.
> It was broken under include/linux/, so it will be broken under include/drm/ too.

I don't think that's a valid conclusion.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH 0/2] drm: add header tests

[Simona Vetter]

On Wed, Jan 22, 2025 at 04:41:32PM +0200, Jani Nikula wrote:
> Add CONFIG_DRM_HEADER_TEST to ensure drm headers are self-contained and
> pass kernel-doc. And for starters, fix one header that this catches.
> 
> Jani Nikula (2):
>   drm/client: include types.h to make drm_client_event.h self-contained
>   drm: ensure drm headers are self-contained and pass kernel-doc

I guess we should give this another shot. On the series:

Acked-by: Simona Vetter <simona.vetter@ffwll.ch>


> 
>  Kbuild                         |  1 +
>  drivers/gpu/drm/Kconfig        | 11 +++++++++++
>  drivers/gpu/drm/Makefile       | 18 ++++++++++++++++++
>  include/Kbuild                 |  1 +
>  include/drm/Makefile           | 18 ++++++++++++++++++
>  include/drm/drm_client_event.h |  2 ++
>  6 files changed, 51 insertions(+)
>  create mode 100644 include/Kbuild
>  create mode 100644 include/drm/Makefile
> 
> -- 
> 2.39.5
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/2] drm: add header tests

[Jani Nikula]

On Thu, 23 Jan 2025, Simona Vetter <simona.vetter@ffwll.ch> wrote:
> On Wed, Jan 22, 2025 at 04:41:32PM +0200, Jani Nikula wrote:
>> Add CONFIG_DRM_HEADER_TEST to ensure drm headers are self-contained and
>> pass kernel-doc. And for starters, fix one header that this catches.
>> 
>> Jani Nikula (2):
>>   drm/client: include types.h to make drm_client_event.h self-contained
>>   drm: ensure drm headers are self-contained and pass kernel-doc
>
> I guess we should give this another shot. On the series:
>
> Acked-by: Simona Vetter <simona.vetter@ffwll.ch>

Thanks, pushed to drm-misc-next.

BR,
Jani.

>
>
>> 
>>  Kbuild                         |  1 +
>>  drivers/gpu/drm/Kconfig        | 11 +++++++++++
>>  drivers/gpu/drm/Makefile       | 18 ++++++++++++++++++
>>  include/Kbuild                 |  1 +
>>  include/drm/Makefile           | 18 ++++++++++++++++++
>>  include/drm/drm_client_event.h |  2 ++
>>  6 files changed, 51 insertions(+)
>>  create mode 100644 include/Kbuild
>>  create mode 100644 include/drm/Makefile
>> 
>> -- 
>> 2.39.5
>> 

-- 
Jani Nikula, Intel