[CI 3/6] drm/xe/pcode: add struct drm_device based interface

[Lucas De Marchi]

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.demarchi at intel.com>
>>Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>Signed-off-by: Jani Nikula <jani.nikula at 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-5ca73f2c0a56@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