[PATCH 1/2] drm/xe: move xe_display.[ch] under display/

[PATCH 1/2] drm/xe: move xe_display.[ch] under display/

[Jani Nikula]

All the other display related files are under display/ subdirectory,
also move xe_display.[ch] there.

Sort the build list while at it.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/xe/Makefile                   | 18 +++++++++---------
 drivers/gpu/drm/xe/{ => display}/xe_display.c |  0
 drivers/gpu/drm/xe/{ => display}/xe_display.h |  0
 3 files changed, 9 insertions(+), 9 deletions(-)
 rename drivers/gpu/drm/xe/{ => display}/xe_display.c (100%)
 rename drivers/gpu/drm/xe/{ => display}/xe_display.h (100%)

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index fe8b266a9819..e06f99def559 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -185,17 +185,17 @@ $(obj)/i915-display/%.o: $(srctree)/drivers/gpu/drm/i915/display/%.c FORCE
 
 # Display code specific to xe
 xe-$(CONFIG_DRM_XE_DISPLAY) += \
-	xe_display.o \
-	display/xe_fb_pin.o \
-	display/xe_hdcp_gsc.o \
-	display/xe_plane_initial.o \
-	display/xe_display_rps.o \
+	display/ext/i915_irq.o \
+	display/ext/i915_utils.o \
+	display/intel_fb_bo.o \
+	display/intel_fbdev_fb.o \
+	display/xe_display.o \
 	display/xe_display_misc.o \
+	display/xe_display_rps.o \
 	display/xe_dsb_buffer.o \
-	display/intel_fbdev_fb.o \
-	display/intel_fb_bo.o \
-	display/ext/i915_irq.o \
-	display/ext/i915_utils.o
+	display/xe_fb_pin.o \
+	display/xe_hdcp_gsc.o \
+	display/xe_plane_initial.o
 
 # SOC code shared with i915
 xe-$(CONFIG_DRM_XE_DISPLAY) += \
diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
similarity index 100%
rename from drivers/gpu/drm/xe/xe_display.c
rename to drivers/gpu/drm/xe/display/xe_display.c
diff --git a/drivers/gpu/drm/xe/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
similarity index 100%
rename from drivers/gpu/drm/xe/xe_display.h
rename to drivers/gpu/drm/xe/display/xe_display.h
-- 
2.39.2

[PATCH 2/2] drm/xe: drop display/ subdir from include directories

[Jani Nikula]

There are very few places that need to include anything from under
display/. Require the display/ prefix in #include directives, and drop
the subdirectory from the header search path.

Sort the include lists while at it.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/xe/Makefile    | 1 -
 drivers/gpu/drm/xe/xe_device.c | 6 +++---
 drivers/gpu/drm/xe/xe_irq.c    | 2 +-
 drivers/gpu/drm/xe/xe_pci.c    | 2 +-
 drivers/gpu/drm/xe/xe_pm.c     | 2 +-
 5 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index e06f99def559..789d310cb5bc 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -165,7 +165,6 @@ xe-$(CONFIG_DRM_XE_KUNIT_TEST) += \
 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
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index ab417f4f7d2a..eea0b259ae85 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -15,20 +15,21 @@
 #include <drm/drm_print.h>
 #include <drm/xe_drm.h>
 
+#include "display/xe_display.h"
 #include "regs/xe_gt_regs.h"
 #include "regs/xe_regs.h"
 #include "xe_bo.h"
 #include "xe_debugfs.h"
-#include "xe_display.h"
 #include "xe_dma_buf.h"
 #include "xe_drm_client.h"
 #include "xe_drv.h"
-#include "xe_exec_queue.h"
 #include "xe_exec.h"
+#include "xe_exec_queue.h"
 #include "xe_ggtt.h"
 #include "xe_gsc_proxy.h"
 #include "xe_gt.h"
 #include "xe_gt_mcr.h"
+#include "xe_hwmon.h"
 #include "xe_irq.h"
 #include "xe_memirq.h"
 #include "xe_mmio.h"
@@ -43,7 +44,6 @@
 #include "xe_ttm_sys_mgr.h"
 #include "xe_vm.h"
 #include "xe_wait_user_fence.h"
-#include "xe_hwmon.h"
 
 #ifdef CONFIG_LOCKDEP
 struct lockdep_map xe_device_mem_access_lockdep_map = {
diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
index 2fd8cc26fc9f..d31e87de5a1c 100644
--- a/drivers/gpu/drm/xe/xe_irq.c
+++ b/drivers/gpu/drm/xe/xe_irq.c
@@ -9,10 +9,10 @@
 
 #include <drm/drm_managed.h>
 
+#include "display/xe_display.h"
 #include "regs/xe_gt_regs.h"
 #include "regs/xe_regs.h"
 #include "xe_device.h"
-#include "xe_display.h"
 #include "xe_drv.h"
 #include "xe_gsc_proxy.h"
 #include "xe_gt.h"
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index df6b3b31f419..09c50471331c 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -15,9 +15,9 @@
 #include <drm/drm_drv.h>
 #include <drm/xe_pciids.h>
 
+#include "display/xe_display.h"
 #include "regs/xe_gt_regs.h"
 #include "xe_device.h"
-#include "xe_display.h"
 #include "xe_drv.h"
 #include "xe_gt.h"
 #include "xe_macros.h"
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index d5f219796d7e..4d3984ae4ff1 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -10,11 +10,11 @@
 #include <drm/drm_managed.h>
 #include <drm/ttm/ttm_placement.h>
 
+#include "display/xe_display.h"
 #include "xe_bo.h"
 #include "xe_bo_evict.h"
 #include "xe_device.h"
 #include "xe_device_sysfs.h"
-#include "xe_display.h"
 #include "xe_ggtt.h"
 #include "xe_gt.h"
 #include "xe_guc.h"
-- 
2.39.2

Re: [PATCH 2/2] drm/xe: drop display/ subdir from include directories

[Rodrigo Vivi]

On Mon, Jan 22, 2024 at 12:14:28PM +0200, Jani Nikula wrote:
> There are very few places that need to include anything from under
> display/. Require the display/ prefix in #include directives, and drop
> the subdirectory from the header search path.
> 
> Sort the include lists while at it.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/xe/Makefile    | 1 -
>  drivers/gpu/drm/xe/xe_device.c | 6 +++---
>  drivers/gpu/drm/xe/xe_irq.c    | 2 +-
>  drivers/gpu/drm/xe/xe_pci.c    | 2 +-
>  drivers/gpu/drm/xe/xe_pm.c     | 2 +-
>  5 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index e06f99def559..789d310cb5bc 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -165,7 +165,6 @@ xe-$(CONFIG_DRM_XE_KUNIT_TEST) += \
>  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
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index ab417f4f7d2a..eea0b259ae85 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -15,20 +15,21 @@
>  #include <drm/drm_print.h>
>  #include <drm/xe_drm.h>
>  
> +#include "display/xe_display.h"
>  #include "regs/xe_gt_regs.h"
>  #include "regs/xe_regs.h"
>  #include "xe_bo.h"
>  #include "xe_debugfs.h"
> -#include "xe_display.h"
>  #include "xe_dma_buf.h"
>  #include "xe_drm_client.h"
>  #include "xe_drv.h"
> -#include "xe_exec_queue.h"
>  #include "xe_exec.h"
> +#include "xe_exec_queue.h"
>  #include "xe_ggtt.h"
>  #include "xe_gsc_proxy.h"
>  #include "xe_gt.h"
>  #include "xe_gt_mcr.h"
> +#include "xe_hwmon.h"
>  #include "xe_irq.h"
>  #include "xe_memirq.h"
>  #include "xe_mmio.h"
> @@ -43,7 +44,6 @@
>  #include "xe_ttm_sys_mgr.h"
>  #include "xe_vm.h"
>  #include "xe_wait_user_fence.h"
> -#include "xe_hwmon.h"
>  
>  #ifdef CONFIG_LOCKDEP
>  struct lockdep_map xe_device_mem_access_lockdep_map = {
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index 2fd8cc26fc9f..d31e87de5a1c 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -9,10 +9,10 @@
>  
>  #include <drm/drm_managed.h>
>  
> +#include "display/xe_display.h"
>  #include "regs/xe_gt_regs.h"
>  #include "regs/xe_regs.h"
>  #include "xe_device.h"
> -#include "xe_display.h"
>  #include "xe_drv.h"
>  #include "xe_gsc_proxy.h"
>  #include "xe_gt.h"
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index df6b3b31f419..09c50471331c 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -15,9 +15,9 @@
>  #include <drm/drm_drv.h>
>  #include <drm/xe_pciids.h>
>  
> +#include "display/xe_display.h"
>  #include "regs/xe_gt_regs.h"
>  #include "xe_device.h"
> -#include "xe_display.h"
>  #include "xe_drv.h"
>  #include "xe_gt.h"
>  #include "xe_macros.h"
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index d5f219796d7e..4d3984ae4ff1 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -10,11 +10,11 @@
>  #include <drm/drm_managed.h>
>  #include <drm/ttm/ttm_placement.h>
>  
> +#include "display/xe_display.h"
>  #include "xe_bo.h"
>  #include "xe_bo_evict.h"
>  #include "xe_device.h"
>  #include "xe_device_sysfs.h"
> -#include "xe_display.h"
>  #include "xe_ggtt.h"
>  #include "xe_gt.h"
>  #include "xe_guc.h"
> -- 
> 2.39.2
> 

Re: [PATCH 1/2] drm/xe: move xe_display.[ch] under display/

[Rodrigo Vivi]

On Mon, Jan 22, 2024 at 12:14:27PM +0200, Jani Nikula wrote:
> All the other display related files are under display/ subdirectory,
> also move xe_display.[ch] there.
> 
> Sort the build list while at it.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/xe/Makefile                   | 18 +++++++++---------
>  drivers/gpu/drm/xe/{ => display}/xe_display.c |  0
>  drivers/gpu/drm/xe/{ => display}/xe_display.h |  0
>  3 files changed, 9 insertions(+), 9 deletions(-)
>  rename drivers/gpu/drm/xe/{ => display}/xe_display.c (100%)
>  rename drivers/gpu/drm/xe/{ => display}/xe_display.h (100%)
> 
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index fe8b266a9819..e06f99def559 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -185,17 +185,17 @@ $(obj)/i915-display/%.o: $(srctree)/drivers/gpu/drm/i915/display/%.c FORCE
>  
>  # Display code specific to xe
>  xe-$(CONFIG_DRM_XE_DISPLAY) += \
> -	xe_display.o \
> -	display/xe_fb_pin.o \
> -	display/xe_hdcp_gsc.o \
> -	display/xe_plane_initial.o \
> -	display/xe_display_rps.o \
> +	display/ext/i915_irq.o \
> +	display/ext/i915_utils.o \
> +	display/intel_fb_bo.o \
> +	display/intel_fbdev_fb.o \
> +	display/xe_display.o \
>  	display/xe_display_misc.o \
> +	display/xe_display_rps.o \
>  	display/xe_dsb_buffer.o \
> -	display/intel_fbdev_fb.o \
> -	display/intel_fb_bo.o \
> -	display/ext/i915_irq.o \
> -	display/ext/i915_utils.o
> +	display/xe_fb_pin.o \
> +	display/xe_hdcp_gsc.o \
> +	display/xe_plane_initial.o
>  
>  # SOC code shared with i915
>  xe-$(CONFIG_DRM_XE_DISPLAY) += \
> diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> similarity index 100%
> rename from drivers/gpu/drm/xe/xe_display.c
> rename to drivers/gpu/drm/xe/display/xe_display.c
> diff --git a/drivers/gpu/drm/xe/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
> similarity index 100%
> rename from drivers/gpu/drm/xe/xe_display.h
> rename to drivers/gpu/drm/xe/display/xe_display.h
> -- 
> 2.39.2
> 

Re: [PATCH 2/2] drm/xe: drop display/ subdir from include directories

[Lucas De Marchi]

On Mon, Jan 22, 2024 at 12:14:28PM +0200, Jani Nikula wrote:
>There are very few places that need to include anything from under
>display/. Require the display/ prefix in #include directives, and drop
>the subdirectory from the header search path.
>
>Sort the include lists while at it.
>
>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>---
> drivers/gpu/drm/xe/Makefile    | 1 -
> drivers/gpu/drm/xe/xe_device.c | 6 +++---
> drivers/gpu/drm/xe/xe_irq.c    | 2 +-
> drivers/gpu/drm/xe/xe_pci.c    | 2 +-
> drivers/gpu/drm/xe/xe_pm.c     | 2 +-
> 5 files changed, 6 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>index e06f99def559..789d310cb5bc 100644
>--- a/drivers/gpu/drm/xe/Makefile
>+++ b/drivers/gpu/drm/xe/Makefile
>@@ -165,7 +165,6 @@ xe-$(CONFIG_DRM_XE_KUNIT_TEST) += \
> 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
>diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>index ab417f4f7d2a..eea0b259ae85 100644
>--- a/drivers/gpu/drm/xe/xe_device.c
>+++ b/drivers/gpu/drm/xe/xe_device.c
>@@ -15,20 +15,21 @@
> #include <drm/drm_print.h>
> #include <drm/xe_drm.h>
>
>+#include "display/xe_display.h"
> #include "regs/xe_gt_regs.h"
> #include "regs/xe_regs.h"
> #include "xe_bo.h"
> #include "xe_debugfs.h"
>-#include "xe_display.h"
> #include "xe_dma_buf.h"
> #include "xe_drm_client.h"
> #include "xe_drv.h"
>-#include "xe_exec_queue.h"
> #include "xe_exec.h"
>+#include "xe_exec_queue.h"
> #include "xe_ggtt.h"
> #include "xe_gsc_proxy.h"
> #include "xe_gt.h"
> #include "xe_gt_mcr.h"
>+#include "xe_hwmon.h"
> #include "xe_irq.h"
> #include "xe_memirq.h"
> #include "xe_mmio.h"
>@@ -43,7 +44,6 @@
> #include "xe_ttm_sys_mgr.h"
> #include "xe_vm.h"
> #include "xe_wait_user_fence.h"
>-#include "xe_hwmon.h"
>
> #ifdef CONFIG_LOCKDEP
> struct lockdep_map xe_device_mem_access_lockdep_map = {
>diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>index 2fd8cc26fc9f..d31e87de5a1c 100644
>--- a/drivers/gpu/drm/xe/xe_irq.c
>+++ b/drivers/gpu/drm/xe/xe_irq.c
>@@ -9,10 +9,10 @@
>
> #include <drm/drm_managed.h>
>
>+#include "display/xe_display.h"
> #include "regs/xe_gt_regs.h"
> #include "regs/xe_regs.h"
> #include "xe_device.h"
>-#include "xe_display.h"
> #include "xe_drv.h"
> #include "xe_gsc_proxy.h"
> #include "xe_gt.h"
>diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>index df6b3b31f419..09c50471331c 100644
>--- a/drivers/gpu/drm/xe/xe_pci.c
>+++ b/drivers/gpu/drm/xe/xe_pci.c
>@@ -15,9 +15,9 @@
> #include <drm/drm_drv.h>
> #include <drm/xe_pciids.h>
>
>+#include "display/xe_display.h"
> #include "regs/xe_gt_regs.h"
> #include "xe_device.h"
>-#include "xe_display.h"
> #include "xe_drv.h"
> #include "xe_gt.h"
> #include "xe_macros.h"
>diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>index d5f219796d7e..4d3984ae4ff1 100644
>--- a/drivers/gpu/drm/xe/xe_pm.c
>+++ b/drivers/gpu/drm/xe/xe_pm.c
>@@ -10,11 +10,11 @@
> #include <drm/drm_managed.h>
> #include <drm/ttm/ttm_placement.h>
>
>+#include "display/xe_display.h"
> #include "xe_bo.h"
> #include "xe_bo_evict.h"
> #include "xe_device.h"
> #include "xe_device_sysfs.h"
>-#include "xe_display.h"

the downside of patch 1 is now that core xe code can include any of the
display/ headers, but only xe_display.h is acceptable to keep the
interface sane. Or are you thinking about changing the interface?

Lucas De Marchi

> #include "xe_ggtt.h"
> #include "xe_gt.h"
> #include "xe_guc.h"
>-- 
>2.39.2
>

Re: [PATCH 2/2] drm/xe: drop display/ subdir from include directories

[Jani Nikula]

On Mon, 22 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> the downside of patch 1 is now that core xe code can include any of the
> display/ headers, but only xe_display.h is acceptable to keep the
> interface sane.

I'd say xe_display.h remains the only interface towards xe core, I don't
think this patch changes that, and its location doesn't really give any
guarantees. It's been a matter of sticking display/ in the include
anyway, and enforcing that is a matter of maintainer vigilance.

> Or are you thinking about changing the interface?

I agree the interface should be in one file only, but changing the
interface is an orthogonal matter (I have no plans atm).

BR,
Jani.


-- 
Jani Nikula, Intel

Re: Re: [PATCH 2/2] drm/xe: drop display/ subdir from include directories

[Lucas De Marchi]

On Mon, Jan 22, 2024 at 07:39:56PM +0200, Jani Nikula wrote:
>On Mon, 22 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> the downside of patch 1 is now that core xe code can include any of the
>> display/ headers, but only xe_display.h is acceptable to keep the
>> interface sane.
>
>I'd say xe_display.h remains the only interface towards xe core, I don't
>think this patch changes that, and its location doesn't really give any
>guarantees. It's been a matter of sticking display/ in the include
>anyway, and enforcing that is a matter of maintainer vigilance.

I just thought the previous split was slightly better: No code in xe
should include display/ and should rather use the xe_display.[hc]
interface.

Now with #include "display/xe_display.h" spread throughout the code,
this could serve as example for people to start including stuff they
shouldn't.

I'm not entirely opposed, so if you and others agree, please go ahead.

Lucas De Mrachi

>
>> Or are you thinking about changing the interface?
>
>I agree the interface should be in one file only, but changing the
>interface is an orthogonal matter (I have no plans atm).
>
>BR,
>Jani.
>
>
>-- 
>Jani Nikula, Intel

Re: Re: [PATCH 2/2] drm/xe: drop display/ subdir from include directories

[Jani Nikula]

On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Mon, Jan 22, 2024 at 07:39:56PM +0200, Jani Nikula wrote:
>>On Mon, 22 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>> the downside of patch 1 is now that core xe code can include any of the
>>> display/ headers, but only xe_display.h is acceptable to keep the
>>> interface sane.
>>
>>I'd say xe_display.h remains the only interface towards xe core, I don't
>>think this patch changes that, and its location doesn't really give any
>>guarantees. It's been a matter of sticking display/ in the include
>>anyway, and enforcing that is a matter of maintainer vigilance.
>
> I just thought the previous split was slightly better: No code in xe
> should include display/ and should rather use the xe_display.[hc]
> interface.
>
> Now with #include "display/xe_display.h" spread throughout the code,
> this could serve as example for people to start including stuff they
> shouldn't.
>
> I'm not entirely opposed, so if you and others agree, please go ahead.

Well, at the moment you can include *anything* from under display/
*without* the prefix, because it's all in the include path. (Although
that will fail for DRM_XE_DISPLAY=n, but does CI even build that combo
regularly?)

I'm fine with dropping this too. I just think it makes it easier to
check that nothing outside of display/ does any displayish things.

Your call.


BR,
Jani.

>
> Lucas De Mrachi
>
>>
>>> Or are you thinking about changing the interface?
>>
>>I agree the interface should be in one file only, but changing the
>>interface is an orthogonal matter (I have no plans atm).
>>
>>BR,
>>Jani.
>>
>>
>>-- 
>>Jani Nikula, Intel

-- 
Jani Nikula, Intel

Re: Re: [PATCH 2/2] drm/xe: drop display/ subdir from include directories

[Jani Nikula]

On Tue, 23 Jan 2024, Jani Nikula <jani.nikula@intel.com> wrote:
> On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> On Mon, Jan 22, 2024 at 07:39:56PM +0200, Jani Nikula wrote:
>>>On Mon, 22 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>>> the downside of patch 1 is now that core xe code can include any of the
>>>> display/ headers, but only xe_display.h is acceptable to keep the
>>>> interface sane.
>>>
>>>I'd say xe_display.h remains the only interface towards xe core, I don't
>>>think this patch changes that, and its location doesn't really give any
>>>guarantees. It's been a matter of sticking display/ in the include
>>>anyway, and enforcing that is a matter of maintainer vigilance.
>>
>> I just thought the previous split was slightly better: No code in xe
>> should include display/ and should rather use the xe_display.[hc]
>> interface.
>>
>> Now with #include "display/xe_display.h" spread throughout the code,
>> this could serve as example for people to start including stuff they
>> shouldn't.
>>
>> I'm not entirely opposed, so if you and others agree, please go ahead.
>
> Well, at the moment you can include *anything* from under display/
> *without* the prefix, because it's all in the include path. (Although
> that will fail for DRM_XE_DISPLAY=n, but does CI even build that combo
> regularly?)
>
> I'm fine with dropping this too. I just think it makes it easier to
> check that nothing outside of display/ does any displayish things.
>
> Your call.

I've held off on pushing this. What's the verdict?

BR,
Jani.


-- 
Jani Nikula, Intel

Re: Re: Re: [PATCH 2/2] drm/xe: drop display/ subdir from include directories

[Lucas De Marchi]

On Tue, Jan 30, 2024 at 11:19:45AM +0200, Jani Nikula wrote:
>On Tue, 23 Jan 2024, Jani Nikula <jani.nikula@intel.com> wrote:
>> On Tue, 23 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>> On Mon, Jan 22, 2024 at 07:39:56PM +0200, Jani Nikula wrote:
>>>>On Mon, 22 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>>>> the downside of patch 1 is now that core xe code can include any of the
>>>>> display/ headers, but only xe_display.h is acceptable to keep the
>>>>> interface sane.
>>>>
>>>>I'd say xe_display.h remains the only interface towards xe core, I don't
>>>>think this patch changes that, and its location doesn't really give any
>>>>guarantees. It's been a matter of sticking display/ in the include
>>>>anyway, and enforcing that is a matter of maintainer vigilance.
>>>
>>> I just thought the previous split was slightly better: No code in xe
>>> should include display/ and should rather use the xe_display.[hc]
>>> interface.
>>>
>>> Now with #include "display/xe_display.h" spread throughout the code,
>>> this could serve as example for people to start including stuff they
>>> shouldn't.
>>>
>>> I'm not entirely opposed, so if you and others agree, please go ahead.
>>
>> Well, at the moment you can include *anything* from under display/
>> *without* the prefix, because it's all in the include path. (Although
>> that will fail for DRM_XE_DISPLAY=n, but does CI even build that combo
>> regularly?)
>>
>> I'm fine with dropping this too. I just think it makes it easier to
>> check that nothing outside of display/ does any displayish things.
>>
>> Your call.
>
>I've held off on pushing this. What's the verdict?

Let's push this. If it doesn't work out due to people including other
headers, we may think of a way to block that without reverting.

Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>
>BR,
>Jani.
>
>
>-- 
>Jani Nikula, Intel

Re: Re: Re: [PATCH 2/2] drm/xe: drop display/ subdir from include directories

[Jani Nikula]

On Tue, 30 Jan 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Let's push this. If it doesn't work out due to people including other
> headers, we may think of a way to block that without reverting.
>
> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>

Thanks, pushed.

BR,
Jani.


-- 
Jani Nikula, Intel