[Intel-xe] [PATCH 5/7] drm/xe/display: Move device info initialization to display
Jani Nikula
jani.nikula at linux.intel.com
Thu Mar 2 14:43:35 UTC 2023
On Wed, 01 Mar 2023, Lucas De Marchi <lucas.demarchi at 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.
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 at 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
More information about the Intel-xe
mailing list