On Thu, Jun 26, 2025 at 01:56:23PM -0500, Lucas De Marchi wrote:
On Mon, Jun 23, 2025 at 02:43:46PM +0300, Jani Nikula wrote:
In preparation for dropping the dependency on struct intel_uncore or
struct xe_tile from display code, add a struct drm_device based
interface to pcode.
Cc: Lucas De Marchi <lucas.demar...@intel.com>
Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com>
Signed-off-by: Jani Nikula <jani.nik...@intel.com>
---
So now we define intel_pcode_read() in both xe.ko and i915.ko.
And intel_pcode is only called from display or
drivers/gpu/drm/i915/soc/intel_dram.c (which afair xe is concerned is
built under XE_DISPLAY only).
We used to allow both i915 and xe as built-in as long as XE_DISPLAY is
not set, but with this patch this is now broken.
I think we have a few possible ways to handle it.
1) Revert. See
https://lore.kernel.org/intel-xe/3667a992-a24b-4e49-aab2-5ca73f2c0...@infradead.org/
nah... too much
2) Move the common symbols to a separate module. We can name the module
xe-i915-common or intel-display or something else. Then we keep moving
symbols there until we can move the entire display. From the module
point of view it's just another dependency that will get loaded.
However, looking at the implementation, they are actually helpers that
depend on the driver backing that device so it's not very
straightforward at this point.
3) Forbid DRM_XE=y && DRM_I915=y (rather than based on DRM_XE_DISPLAY)
I have another patch applied so this doesn't apply as is, but should be
something like this:
diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 2e198536e59a0..529e6792d2497 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -4,6 +4,9 @@ config DRM_XE
depends on DRM && PCI
depends on KUNIT || !KUNIT
depends on INTEL_VSEC || !INTEL_VSEC
+ # currently not allowed as there would be duplicate display symbols.
+ # TODO: drop once display is in its own module
+ depends on m || DRM_I915!=y
depends on X86_PLATFORM_DEVICES || !(X86 && ACPI)
select INTERVAL_TREE
# we need shmfs for the swappable backing store, and in particular
@@ -53,7 +56,7 @@ config DRM_XE
config DRM_XE_DISPLAY
bool "Enable display support"
- depends on DRM_XE && DRM_XE=m && HAS_IOPORT
+ depends on DRM_XE && HAS_IOPORT
select FB_IOMEM_HELPERS if DRM_FBDEV_EMULATION
select I2C
select I2C_ALGOBIT
4) ifdef the helpers based on XE_DISPLAY... because if XE_DISPLAY is
set, then XE can't be built-in.
This was easy too:
diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
index 87323ad0cbbb2..08aee78ff08ed 100644
--- a/drivers/gpu/drm/xe/xe_pcode.c
+++ b/drivers/gpu/drm/xe/xe_pcode.c
@@ -337,7 +337,10 @@ int xe_pcode_probe_early(struct xe_device *xe)
}
ALLOW_ERROR_INJECTION(xe_pcode_probe_early, ERRNO); /* See xe_pci_probe */
-/* Helpers with drm device */
+
+/* Helpers with drm device. These should only be called by the display side */
+#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
+
int intel_pcode_read(struct drm_device *drm, u32 mbox, u32 *val, u32 *val1)
{
struct xe_device *xe = to_xe_device(drm);
@@ -362,3 +365,5 @@ int intel_pcode_request(struct drm_device *drm, u32 mbox,
u32 request,
return xe_pcode_request(tile, mbox, request, reply_mask, reply, timeout_base_ms);
}
+
+#endif
Let me know what you think
Lucas De Marchi
I have (3) ready based on an earlier patch and (4) is pretty easy. But
I'd prefer (2) to move things forward. Or maybe you already have
something else? Thoughts?
thanks
Lucas De Marchi
