Re: [Intel-xe] [PATCH 5/7] drm/xe/display: Move device info initialization to display

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

On Thu, 02 Mar 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 01 Mar 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> Instead of initializing the display info together with the platform in
>> xe_pci.c, let the display part initialize it. The small disadvantage is
>> that the description struct is not automatically mapped from the PCI ID,
>> but just doing a switch on the platform, that should already be set
>> during display initialization should be ok.
>
> Have we already given up on the idea that device info would be a pointer
> to rodata?
>
> The thing that was always problematic for i915 was that it was all too
> convenient to go fiddle with the device info runtime, based on some
> arbitrary needs.
>
> ---
>
> That said, there's really no requirement that the struct pci_device_id
> driver_data members in MODULE_DEVICE_TABLE() need to be pointers to some
> device info structures, at all. The initialization of device info could
> be done in completely different ways if we dropped that notion. The
> current macro "inheritance" in both i915 and xe is absolutely horrible.
>
> Anyway, going to block the patch, just checking on the direction here.

Sheesh, *NOT* going to. Sorry.


>
>
> BR,
> Jani.
>
>
>
>
>
>
>
>
>>
>> This allows to encapsulate the display details inside the display
>> compilation units.
>>
>> Also use __diag_push() / __diag_pop() like in xe_step to handle the few
>> places where -Woverride-init should be disabled.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/xe/Makefile     |   3 -
>>  drivers/gpu/drm/xe/xe_display.c | 114 ++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_display.h |  15 ++---
>>  drivers/gpu/drm/xe/xe_pci.c     |  90 ++-----------------------
>>  4 files changed, 128 insertions(+), 94 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 18257cd7227d..d1d255df74a1 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -24,9 +24,6 @@ subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides)
>>  subdir-ccflags-y += $(call cc-disable-warning, frame-address)
>>  subdir-ccflags-$(CONFIG_DRM_XE_WERROR) += -Werror
>>  
>> -# Fine grained warnings disable
>> -CFLAGS_xe_pci.o = $(call cc-disable-warning, override-init)
>> -
>>  subdir-ccflags-y += -I$(srctree)/$(src)
>>  
>>  # Please keep these build lists sorted!
>> diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
>> index 34bdb2d60938..76e4fb6bd67c 100644
>> --- a/drivers/gpu/drm/xe/xe_display.c
>> +++ b/drivers/gpu/drm/xe/xe_display.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/fb.h>
>>  
>>  #include <drm/drm_aperture.h>
>> +#include <drm/drm_drv.h>
>>  #include <drm/drm_managed.h>
>>  #include <drm/xe_drm.h>
>>  
>> @@ -383,4 +384,117 @@ void xe_display_pm_resume(struct xe_device *xe)
>>  	intel_power_domains_enable(xe);
>>  }
>>  
>> +/* Display info initialization */
>> +__diag_push();
>> +__diag_ignore_all("-Woverride-init", "Allow field overrides in table");
>> +
>> +#define __DISPLAY_DEFAULTS \
>> +	.pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) |			\
>> +		     BIT(PIPE_C) | BIT(PIPE_D),				\
>> +	.cpu_transcoder_mask =						\
>> +	BIT(TRANSCODER_A) | BIT(TRANSCODER_B) |				\
>> +	BIT(TRANSCODER_C) | BIT(TRANSCODER_D) |				\
>> +	BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1),			\
>> +	.pipe_offsets = {						\
>> +		[TRANSCODER_A] = PIPE_A_OFFSET,				\
>> +		[TRANSCODER_B] = PIPE_B_OFFSET,				\
>> +		[TRANSCODER_C] = PIPE_C_OFFSET,				\
>> +		[TRANSCODER_D] = PIPE_D_OFFSET,				\
>> +		[TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET,			\
>> +		[TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET,			\
>> +	},								\
>> +	.trans_offsets = {						\
>> +		[TRANSCODER_A] = TRANSCODER_A_OFFSET,			\
>> +		[TRANSCODER_B] = TRANSCODER_B_OFFSET,			\
>> +		[TRANSCODER_C] = TRANSCODER_C_OFFSET,			\
>> +		[TRANSCODER_D] = TRANSCODER_D_OFFSET,			\
>> +		[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET,		\
>> +		[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET,		\
>> +	}
>> +
>> +#define GEN12_DISPLAY \
>> +	__DISPLAY_DEFAULTS,						\
>> +	.ver = 12,							\
>> +	.abox_mask = GENMASK(2, 1),					\
>> +	.has_dmc = 1,							\
>> +	.has_dp_mst = 1,						\
>> +	.has_dsb = 0, /* FIXME: LUT load is broken with huge DSB */	\
>> +	.dbuf.size = 2048,						\
>> +	.dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2),			\
>> +	.has_dsc = 1,							\
>> +	.fbc_mask = BIT(INTEL_FBC_A),					\
>> +	.has_fpga_dbg = 1,						\
>> +	.has_hdcp = 1,							\
>> +	.has_ipc = 1,							\
>> +	.has_psr = 1,							\
>> +	.has_psr_hw_tracking = 1,					\
>> +	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262145 }
>> +
>> +#define GEN13_DISPLAY							\
>> +	__DISPLAY_DEFAULTS,						\
>> +	.ver = 13,							\
>> +	.abox_mask = GENMASK(1, 0),					\
>> +	.color = {							\
>> +		.degamma_lut_size = 128, .gamma_lut_size = 1024,	\
>> +		.degamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING |	\
>> +		DRM_COLOR_LUT_EQUAL_CHANNELS,				\
>> +	},								\
>> +	.dbuf.size = 4096,						\
>> +	.dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) |	\
>> +	BIT(DBUF_S4),							\
>> +	.has_dmc = 1,							\
>> +	.has_dp_mst = 1,						\
>> +	.has_dsb = 1,							\
>> +	.has_dsc = 1,							\
>> +	.fbc_mask = BIT(INTEL_FBC_A),					\
>> +	.has_fpga_dbg = 1,						\
>> +	.has_hdcp = 1,							\
>> +	.has_ipc = 1,							\
>> +	.has_psr = 1
>> +
>> +void xe_display_info_init(struct xe_device *xe)
>> +{
>> +	if (!xe->info.enable_display)
>> +		return;
>> +
>> +	switch (xe->info.platform) {
>> +	case XE_TIGERLAKE:
>> +	case XE_DG1:
>> +		xe->info.display = (struct xe_device_display_info) { GEN12_DISPLAY };
>> +		break;
>> +	case XE_ALDERLAKE_S:
>> +	case XE_ALDERLAKE_P:
>> +		xe->info.display = (struct xe_device_display_info) {
>> +			GEN12_DISPLAY,
>> +			.has_hti = 1,
>> +			.has_psr_hw_tracking = 0,
>> +		};
>> +		break;
>> +	case XE_DG2:
>> +		xe->info.display = (struct xe_device_display_info) {
>> +			GEN13_DISPLAY,
>> +			.cpu_transcoder_mask =
>> +				BIT(TRANSCODER_A) | BIT(TRANSCODER_B) |
>> +				BIT(TRANSCODER_C) | BIT(TRANSCODER_D),
>> +		};
>> +		break;
>> +	case XE_METEORLAKE:
>> +		xe->info.display = (struct xe_device_display_info) {
>> +			GEN13_DISPLAY,
>> +			.ver = 14,
>> +			.has_cdclk_crawl = 1,
>> +			.has_cdclk_squash = 1,
>> +		};
>> +		break;
>> +	default:
>> +		/*
>> +		 * If platform doesn't have display, enable_display should
>> +		 * had been forced to false already at this point
>> +		 */
>> +		drm_WARN_ON(&xe->drm, 1);
>> +	}
>> +}
>> +
>> +__diag_pop();
>> +
>>  #endif
>> diff --git a/drivers/gpu/drm/xe/xe_display.h b/drivers/gpu/drm/xe/xe_display.h
>> index cb5298ceed3b..d1af8e3a5c0f 100644
>> --- a/drivers/gpu/drm/xe/xe_display.h
>> +++ b/drivers/gpu/drm/xe/xe_display.h
>> @@ -6,16 +6,18 @@
>>  #ifndef _XE_DISPLAY_H_
>>  #define _XE_DISPLAY_H_
>>  
>> -#include <drm/drm_drv.h>
>> -
>>  #include "xe_device.h"
>>  
>> +struct drm_driver;
>> +
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>  
>>  int xe_display_set_driver_hooks(struct pci_dev *pdev, struct drm_driver *driver);
>>  
>>  int xe_display_create(struct xe_device *xe);
>>  
>> +void xe_display_info_init(struct xe_device *xe);
>> +
>>  int xe_display_init_nommio(struct xe_device *xe);
>>  void xe_display_fini_nommio(struct drm_device *dev, void *dummy);
>>  
>> @@ -51,6 +53,8 @@ xe_display_set_driver_hooks(struct pci_dev *pdev, struct drm_driver *driver) { r
>>  static inline int
>>  xe_display_create(struct xe_device *xe) { return 0; }
>>  
>> +static inline void xe_display_info_init(struct xe_device *xe) { }
>> +
>>  static inline int
>>  xe_display_enable(struct pci_dev *pdev, struct drm_driver *driver) { return 0; };
>>  
>> @@ -58,12 +62,7 @@ static inline int
>>  xe_display_init_nommio(struct xe_device *xe) { return 0; };
>>  static inline void xe_display_fini_nommio(struct drm_device *dev, void *dummy) {};
>>  
>> -static inline int xe_display_init_noirq(struct xe_device *xe)
>> -{
>> -	if (xe->info.display.pipe_mask != 0)
>> -		drm_warn(&xe->drm, "CONFIG_DRM_XE_DISPLAY is unset, but device is display capable\n");
>> -	return 0;
>> -}
>> +static inline int xe_display_init_noirq(struct xe_device *xe) { return 0; }
>>  
>>  static inline void
>>  xe_display_fini_noirq(struct drm_device *dev, void *dummy) {};
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 11dc0de66101..c4d9fd2e7b2b 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -16,6 +16,7 @@
>>  
>>  #include "regs/xe_regs.h"
>>  #include "xe_device.h"
>> +#include "xe_display.h"
>>  #include "xe_drv.h"
>>  #include "xe_macros.h"
>>  #include "xe_module.h"
>> @@ -62,8 +63,6 @@ struct xe_device_desc {
>>  	DEV_INFO_FOR_EACH_FLAG(DEFINE_FLAG);
>>  #undef DEFINE_FLAG
>>  
>> -	struct xe_device_display_info display;
>> -
>>  	u8 vram_flags;
>>  	u8 max_tiles;
>>  	u8 vm_max_level;
>> @@ -75,79 +74,15 @@ struct xe_device_desc {
>>  	bool has_asid;
>>  };
>>  
>> +__diag_push();
>> +__diag_ignore_all("-Woverride-init", "Allow field overrides in table");
>> +
>>  #define PLATFORM(x)		\
>>  	.platform = (x),	\
>>  	.platform_name = #x
>>  
>>  #define NOP(x)	x
>>  
>> -#define __DISPLAY_DEFAULTS \
>> -		.pipe_mask = BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D), \
>> -		.cpu_transcoder_mask = \
>> -			BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \
>> -			BIT(TRANSCODER_C) | BIT(TRANSCODER_D) | \
>> -			BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1), \
>> -		.pipe_offsets = { \
>> -			[TRANSCODER_A] = PIPE_A_OFFSET, \
>> -			[TRANSCODER_B] = PIPE_B_OFFSET, \
>> -			[TRANSCODER_C] = PIPE_C_OFFSET, \
>> -			[TRANSCODER_D] = PIPE_D_OFFSET, \
>> -			[TRANSCODER_DSI_0] = PIPE_DSI0_OFFSET, \
>> -			[TRANSCODER_DSI_1] = PIPE_DSI1_OFFSET, \
>> -		}, \
>> -		.trans_offsets = { \
>> -			[TRANSCODER_A] = TRANSCODER_A_OFFSET, \
>> -			[TRANSCODER_B] = TRANSCODER_B_OFFSET, \
>> -			[TRANSCODER_C] = TRANSCODER_C_OFFSET, \
>> -			[TRANSCODER_D] = TRANSCODER_D_OFFSET, \
>> -			[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
>> -			[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
>> -		}, \
>> -
>> -#define GEN12_DISPLAY \
>> -	.display = (struct xe_device_display_info){ \
>> -		__DISPLAY_DEFAULTS \
>> -		.ver = 12, \
>> -		.abox_mask = GENMASK(2, 1), \
>> -		.has_dmc = 1, \
>> -		.has_dp_mst = 1, \
>> -		.has_dsb = 0, /* FIXME: LUT load is broken with huge DSB */ \
>> -		.dbuf.size = 2048, \
>> -		.dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2), \
>> -		.has_dsc = 1, \
>> -		.fbc_mask = BIT(INTEL_FBC_A), \
>> -		.has_fpga_dbg = 1, \
>> -		.has_hdcp = 1, \
>> -		.has_ipc = 1, \
>> -		.has_psr = 1, \
>> -		.has_psr_hw_tracking = 1, \
>> -		.color = { .degamma_lut_size = 33, .gamma_lut_size = 262145 }, \
>> -	}
>> -
>> -#define GEN13_DISPLAY \
>> -	.display = (struct xe_device_display_info){ \
>> -		__DISPLAY_DEFAULTS \
>> -		.ver = 13,							\
>> -		.abox_mask = GENMASK(1, 0),					\
>> -		.color = {							\
>> -			.degamma_lut_size = 128, .gamma_lut_size = 1024,	\
>> -			.degamma_lut_tests = DRM_COLOR_LUT_NON_DECREASING |	\
>> -				     DRM_COLOR_LUT_EQUAL_CHANNELS,		\
>> -		},								\
>> -		.dbuf.size = 4096,						\
>> -		.dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) |	\
>> -				   BIT(DBUF_S4),				\
>> -		.has_dmc = 1,							\
>> -		.has_dp_mst = 1,						\
>> -		.has_dsb = 1,							\
>> -		.has_dsc = 1,							\
>> -		.fbc_mask = BIT(INTEL_FBC_A),					\
>> -		.has_fpga_dbg = 1,						\
>> -		.has_hdcp = 1,							\
>> -		.has_ipc = 1,							\
>> -		.has_psr = 1,							\
>> -	}
>> -
>>  /* Keep in gen based order, and chronological order within a gen */
>>  #define GEN12_FEATURES \
>>  	.require_force_probe = true, \
>> @@ -160,20 +95,15 @@ struct xe_device_desc {
>>  
>>  static const struct xe_device_desc tgl_desc = {
>>  	GEN12_FEATURES,
>> -	GEN12_DISPLAY,
>>  	PLATFORM(XE_TIGERLAKE),
>>  	.platform_engine_mask =
>>  		BIT(XE_HW_ENGINE_RCS0) | BIT(XE_HW_ENGINE_BCS0) |
>>  		BIT(XE_HW_ENGINE_VECS0) | BIT(XE_HW_ENGINE_VCS0) |
>>  		BIT(XE_HW_ENGINE_VCS2),
>> -	GEN12_DISPLAY,
>>  };
>>  
>>  static const struct xe_device_desc adl_s_desc = {
>>  	GEN12_FEATURES,
>> -	GEN12_DISPLAY,
>> -	.display.has_hti = 1,
>> -	.display.has_psr_hw_tracking = 0,
>>  	PLATFORM(XE_ALDERLAKE_S),
>>  	.platform_engine_mask =
>>  		BIT(XE_HW_ENGINE_RCS0) | BIT(XE_HW_ENGINE_BCS0) |
>> @@ -195,7 +125,6 @@ static const struct xe_device_desc adl_p_desc = {
>>  
>>  static const struct xe_device_desc dg1_desc = {
>>  	GEN12_FEATURES,
>> -	GEN12_DISPLAY,
>>  	DGFX_FEATURES,
>>  	.graphics_rel = 10,
>>  	PLATFORM(XE_DG1),
>> @@ -256,9 +185,6 @@ static const struct xe_device_desc dg2_desc = {
>>  	XE_HPM_FEATURES,
>>  
>>  	DG2_FEATURES,
>> -	GEN13_DISPLAY,
>> -	.display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) |
>> -				       BIT(TRANSCODER_C) | BIT(TRANSCODER_D),
>>  };
>>  
>>  #define PVC_ENGINES \
>> @@ -334,13 +260,10 @@ static const struct xe_device_desc mtl_desc = {
>>  	PLATFORM(XE_METEORLAKE),
>>  	.extra_gts = xelpmp_gts,
>>  	.platform_engine_mask = MTL_MAIN_ENGINES,
>> -	GEN13_DISPLAY,
>> -	.display.ver = 14,
>> -	.display.has_cdclk_crawl = 1,
>> -	.display.has_cdclk_squash = 1,
>>  };
>>  
>>  #undef PLATFORM
>> +__diag_pop();
>>  
>>  #define INTEL_VGA_DEVICE(id, info) {			\
>>  	PCI_DEVICE(PCI_VENDOR_ID_INTEL, id),		\
>> @@ -489,7 +412,6 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  	xe->info.has_asid = desc->has_asid;
>>  	xe->info.has_flat_ccs = desc->has_flat_ccs;
>>  	xe->info.has_4tile = desc->has_4tile;
>> -	xe->info.display = desc->display;
>>  	xe->info.has_range_tlb_invalidation = desc->has_range_tlb_invalidation;
>>  
>>  	spd = subplatform_get(xe, desc);
>> @@ -519,6 +441,8 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  		}
>>  	}
>>  
>> +	xe_display_info_init(xe);
>> +
>>  	drm_dbg(&xe->drm, "%s %s %04x:%04x dgfx:%d gfx100:%d media100:%d dma_m_s:%d tc:%d",
>>  		desc->platform_name, spd ? spd->name : "",
>>  		xe->info.devid, xe->info.revid,

-- 
Jani Nikula, Intel Open Source Graphics Center