Re: [Intel-xe] [PATCH 1/9] fixup! drm/xe/display: Implement display support

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 21 Apr 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Thu, Apr 20, 2023 at 11:49:46AM +0300, Jani Nikula wrote:
>>On Wed, 19 Apr 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>> On Wed, Apr 19, 2023 at 03:39:39PM +0300, Jani Nikula wrote:
>>>>Build the i915 display object files in i915-display sub-directory, to
>>>>increase clarity over what's coming from i915, and to simplify the rule
>>>>to build the object files.
>>>>
>>>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>---
>>>> drivers/gpu/drm/xe/Makefile | 165 +++++++++++++++++-------------------
>>>> 1 file changed, 78 insertions(+), 87 deletions(-)
>>>>
>>>>diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>>>index aceca651de57..9fa4aaad6b8f 100644
>>>>--- a/drivers/gpu/drm/xe/Makefile
>>>>+++ b/drivers/gpu/drm/xe/Makefile
>>>>@@ -105,100 +105,20 @@ subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
>>>> 	-Ddrm_i915_gem_object=xe_bo \
>>>> 	-Ddrm_i915_private=xe_device
>>>
>>> by leaving them on the same dir I was actually thinking if it would not
>>> be possible to add a display/Makefile  so these additional defines here
>>> are added as subdir-ccflags only there.
>>
>>This is my understanding, I may be wrong:
>>
>>* If you want subdir-ccflags-y to only apply within a subdirectory, you
>>  need to descend to the subdirectory using "obj-y += subdir" and have a
>>  standalone Makefile.
>>
>>* If you use "include $(src)/subdir/Makefile", subdir-ccflags-y
>>  specified there applies to the parent directory.
>>
>>* If you descend to the subdirectory using "obj-y += subdir", I don't
>>  think you can have the parent and subdirectory Makefiles add objects
>>  to the same module.
>
> true, but the snippet below does the trick for me. The problem is: it
> doesn't work due to the include dep:
>
> xe_device_types.h -> intel_display_core.h -> {the-display-world}.h

That's basically the same problem as i915_drv.h -> intel_display_core.h
in i915 land, and the solution is pretty much the same also: make the
display substruct member a pointer opaque to non-display code, and
allocate it dynamically.

That's what all the display header and include refactoring have
gradually been leading up to, but this one's just massively disruptive.


BR,
Jani.

>
> Anyway, here is what I used to remove the cflags from the !display
> sources:
>
> -------------------8<-----------------
> Subject: [PATCH] WIP cflags display removal
>
> ---
>   drivers/gpu/drm/xe/Makefile | 36 +++++++++++++++++++++---------------
>   1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index ee4a95beec20..fea31cfc3de6 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -96,24 +96,13 @@ xe-y += xe_bb.o \
>   	xe_wa.o \
>   	xe_wopcm.o
>   
> -# i915 Display compat #defines and #includes
> -subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
> -	-I$(srctree)/$(src)/display/ext \
> -	-I$(srctree)/$(src)/compat-i915-headers \
> -	-I$(srctree)/drivers/gpu/drm/xe/display/ \
> -	-I$(srctree)/drivers/gpu/drm/i915/display/ \
> -	-Ddrm_i915_gem_object=xe_bo \
> -	-Ddrm_i915_private=xe_device
> -
> -CFLAGS_i915-display/intel_fbdev.o = $(call cc-disable-warning, override-init)
> -
>   # Rule to build display code shared with i915
>   $(obj)/i915-display/%.o: $(srctree)/drivers/gpu/drm/i915/display/%.c FORCE
>   	$(call cmd,force_checksrc)
>   	$(call if_changed_rule,cc_o_c)
>   
>   # Display code specific to xe
> -xe-$(CONFIG_DRM_XE_DISPLAY) += \
> +xe-display += \
>   	xe_display.o \
>   	display/xe_fb_pin.o \
>   	display/xe_hdcp_gsc.o \
> @@ -126,7 +115,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>   	display/ext/intel_pch.o
>   
>   # Display code shared with i915
> -xe-$(CONFIG_DRM_XE_DISPLAY) += \
> +xe-display += \
>   	i915-display/icl_dsi.o \
>   	i915-display/intel_atomic.o \
>   	i915-display/intel_atomic_plane.o \
> @@ -197,15 +186,32 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>   	i915-display/skl_watermark.o
>   
>   ifeq ($(CONFIG_ACPI),y)
> -	xe-$(CONFIG_DRM_XE_DISPLAY) += \
> +	xe-display += \
>   		i915-display/intel_acpi.o \
>   		i915-display/intel_opregion.o
>   endif
>   
>   ifeq ($(CONFIG_DRM_FBDEV_EMULATION),y)
> -	xe-$(CONFIG_DRM_XE_DISPLAY) += i915-display/intel_fbdev.o
> +	xe-display += i915-display/intel_fbdev.o
>   endif
>   
> +# i915 Display compat #defines and #includes
> +xe-display-ccflags += \
> +	-I$(srctree)/$(src)/display/ext \
> +	-I$(srctree)/$(src)/compat-i915-headers \
> +	-I$(srctree)/drivers/gpu/drm/xe/display/ \
> +	-I$(srctree)/drivers/gpu/drm/i915/display/ \
> +	-Ddrm_i915_gem_object=xe_bo \
> +	-Ddrm_i915_private=xe_device
> +
> +CFLAGS_i915-display/intel_fbdev.o = $(call cc-disable-warning, override-init)
> +
> +# Apply CFAGS to all display sources
> +$(foreach d,$(xe-display), \
> +	$(eval CFLAGS_$(d) += $(xe-display-ccflags)))
> +
> +xe-$(CONFIG_DRM_XE_DISPLAY) += $(xe-display)
> +
>   obj-$(CONFIG_DRM_XE) += xe.o
>   obj-$(CONFIG_DRM_XE_KUNIT_TEST) += tests/

-- 
Jani Nikula, Intel Open Source Graphics Center