From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/display: add intel_display_device_present()
Date: Fri, 5 Sep 2025 14:27:42 +0300 [thread overview]
Message-ID: <aLrJLuvHzyW-kjT-@intel.com> (raw)
In-Reply-To: <20250903090408.3492875-1-jani.nikula@intel.com>
On Wed, Sep 03, 2025 at 12:04:08PM +0300, Jani Nikula wrote:
> Add a proper function for display && HAS_DISPLAY(display) to hide
> indirect struct intel_display access via the macro from a number of
> places outside of display. This makes struct intel_display * an opaque
> pointer in these places. All HAS_DISPLAY() usage is now constrained
> within display.
Looks all right. Though a lot of code where this gets used really
shouldn't exist outside the display code in the first place. But
cleaning all that up is going to take some effort.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> .../drm/i915/display/intel_display_device.c | 5 +++
> .../drm/i915/display/intel_display_device.h | 1 +
> .../i915/gem/selftests/i915_gem_client_blt.c | 4 +--
> drivers/gpu/drm/i915/i915_driver.c | 18 +++++-----
> drivers/gpu/drm/i915/i915_switcheroo.c | 7 ++--
> drivers/gpu/drm/i915/soc/intel_dram.c | 2 +-
> drivers/gpu/drm/xe/display/xe_display.c | 33 ++++++++-----------
> 7 files changed, 36 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> index 65f0efc35bb7..a002bc6ce7b0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> @@ -1944,6 +1944,11 @@ void intel_display_device_info_print(const struct intel_display_device_info *inf
> drm_printf(p, "rawclk rate: %u kHz\n", runtime->rawclk_freq);
> }
>
> +bool intel_display_device_present(struct intel_display *display)
> +{
> + return display && HAS_DISPLAY(display);
> +}
> +
> /*
> * Assuming the device has display hardware, should it be enabled?
> *
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 6e87b763fe7c..f329f1beafef 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -306,6 +306,7 @@ struct intel_display_device_info {
> } color;
> };
>
> +bool intel_display_device_present(struct intel_display *display);
> bool intel_display_device_enabled(struct intel_display *display);
> struct intel_display *intel_display_device_probe(struct pci_dev *pdev);
> void intel_display_device_remove(struct intel_display *display);
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
> index e747f5ed195e..539c620364e3 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
> @@ -5,7 +5,7 @@
>
> #include "i915_selftest.h"
>
> -#include "display/intel_display_core.h"
> +#include "display/intel_display_device.h"
> #include "gt/intel_context.h"
> #include "gt/intel_engine_regs.h"
> #include "gt/intel_engine_user.h"
> @@ -122,7 +122,7 @@ static bool fastblit_supports_x_tiling(const struct drm_i915_private *i915)
> if (GRAPHICS_VER_FULL(i915) < IP_VER(12, 55))
> return false;
>
> - return HAS_DISPLAY(display);
> + return intel_display_device_present(display);
> }
>
> static bool fast_blit_ok(const struct blit_buffer *buf)
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 70f042ce8705..a28c3710c4d5 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -51,13 +51,15 @@
> #include "display/intel_bw.h"
> #include "display/intel_cdclk.h"
> #include "display/intel_crtc.h"
> -#include "display/intel_display_core.h"
> +#include "display/intel_display_device.h"
> #include "display/intel_display_driver.h"
> +#include "display/intel_display_power.h"
> #include "display/intel_dmc.h"
> #include "display/intel_dp.h"
> #include "display/intel_dpt.h"
> #include "display/intel_encoder.h"
> #include "display/intel_fbdev.h"
> +#include "display/intel_gmbus.h"
> #include "display/intel_hotplug.h"
> #include "display/intel_opregion.h"
> #include "display/intel_overlay.h"
> @@ -977,7 +979,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
> intel_power_domains_disable(display);
>
> drm_client_dev_suspend(&i915->drm, false);
> - if (HAS_DISPLAY(display)) {
> + if (intel_display_device_present(display)) {
> drm_kms_helper_poll_disable(&i915->drm);
> intel_display_driver_disable_user_access(display);
>
> @@ -989,7 +991,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
> intel_irq_suspend(i915);
> intel_hpd_cancel_work(display);
>
> - if (HAS_DISPLAY(display))
> + if (intel_display_device_present(display))
> intel_display_driver_suspend_access(display);
>
> intel_encoder_suspend_all(display);
> @@ -1060,7 +1062,7 @@ static int i915_drm_suspend(struct drm_device *dev)
> * properly. */
> intel_power_domains_disable(display);
> drm_client_dev_suspend(dev, false);
> - if (HAS_DISPLAY(display)) {
> + if (intel_display_device_present(display)) {
> drm_kms_helper_poll_disable(dev);
> intel_display_driver_disable_user_access(display);
> }
> @@ -1072,7 +1074,7 @@ static int i915_drm_suspend(struct drm_device *dev)
> intel_irq_suspend(dev_priv);
> intel_hpd_cancel_work(display);
>
> - if (HAS_DISPLAY(display))
> + if (intel_display_device_present(display))
> intel_display_driver_suspend_access(display);
>
> intel_encoder_suspend_all(display);
> @@ -1219,7 +1221,7 @@ static int i915_drm_resume(struct drm_device *dev)
> */
> intel_irq_resume(dev_priv);
>
> - if (HAS_DISPLAY(display))
> + if (intel_display_device_present(display))
> drm_mode_config_reset(dev);
>
> i915_gem_resume(dev_priv);
> @@ -1228,14 +1230,14 @@ static int i915_drm_resume(struct drm_device *dev)
>
> intel_clock_gating_init(dev_priv);
>
> - if (HAS_DISPLAY(display))
> + if (intel_display_device_present(display))
> intel_display_driver_resume_access(display);
>
> intel_hpd_init(display);
>
> intel_display_driver_resume(display);
>
> - if (HAS_DISPLAY(display)) {
> + if (intel_display_device_present(display)) {
> intel_display_driver_enable_user_access(display);
> drm_kms_helper_poll_enable(dev);
> }
> diff --git a/drivers/gpu/drm/i915/i915_switcheroo.c b/drivers/gpu/drm/i915/i915_switcheroo.c
> index 3a95a55b2e87..d5b6d8ab31a2 100644
> --- a/drivers/gpu/drm/i915/i915_switcheroo.c
> +++ b/drivers/gpu/drm/i915/i915_switcheroo.c
> @@ -5,7 +5,7 @@
>
> #include <linux/vga_switcheroo.h>
>
> -#include "display/intel_display_core.h"
> +#include "display/intel_display_device.h"
>
> #include "i915_driver.h"
> #include "i915_drv.h"
> @@ -22,7 +22,7 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev,
> dev_err(&pdev->dev, "DRM not initialized, aborting switch.\n");
> return;
> }
> - if (!HAS_DISPLAY(display)) {
> + if (!intel_display_device_present(display)) {
> dev_err(&pdev->dev, "Device state not initialized, aborting switch.\n");
> return;
> }
> @@ -52,7 +52,8 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
> * locking inversion with the driver load path. And the access here is
> * completely racy anyway. So don't bother with locking for now.
> */
> - return i915 && HAS_DISPLAY(display) && atomic_read(&i915->drm.open_count) == 0;
> + return i915 && intel_display_device_present(display) &&
> + atomic_read(&i915->drm.open_count) == 0;
> }
>
> static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
> diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c
> index 3eb748ab44d9..00d3d834a83d 100644
> --- a/drivers/gpu/drm/i915/soc/intel_dram.c
> +++ b/drivers/gpu/drm/i915/soc/intel_dram.c
> @@ -725,7 +725,7 @@ int intel_dram_detect(struct drm_i915_private *i915)
> struct dram_info *dram_info;
> int ret;
>
> - if (IS_DG2(i915) || !HAS_DISPLAY(display))
> + if (IS_DG2(i915) || !intel_display_device_present(display))
> return 0;
>
> dram_info = drmm_kzalloc(&i915->drm, sizeof(*dram_info), GFP_KERNEL);
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index 8b68d70db6c8..19e691fccf8c 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -20,7 +20,7 @@
> #include "intel_audio.h"
> #include "intel_bw.h"
> #include "intel_display.h"
> -#include "intel_display_core.h"
> +#include "intel_display_device.h"
> #include "intel_display_driver.h"
> #include "intel_display_irq.h"
> #include "intel_display_types.h"
> @@ -37,13 +37,6 @@
>
> /* Xe device functions */
>
> -static bool has_display(struct xe_device *xe)
> -{
> - struct intel_display *display = xe->display;
> -
> - return HAS_DISPLAY(display);
> -}
> -
> /**
> * xe_display_driver_probe_defer - Detect if we need to wait for other drivers
> * early on
> @@ -290,7 +283,7 @@ static void xe_display_enable_d3cold(struct xe_device *xe)
>
> intel_dmc_suspend(display);
>
> - if (has_display(xe))
> + if (intel_display_device_present(display))
> intel_hpd_poll_enable(display);
> }
>
> @@ -303,14 +296,14 @@ static void xe_display_disable_d3cold(struct xe_device *xe)
>
> intel_dmc_resume(display);
>
> - if (has_display(xe))
> + if (intel_display_device_present(display))
> drm_mode_config_reset(&xe->drm);
>
> intel_display_driver_init_hw(display);
>
> intel_hpd_init(display);
>
> - if (has_display(xe))
> + if (intel_display_device_present(display))
> intel_hpd_poll_disable(display);
>
> intel_opregion_resume(display);
> @@ -333,7 +326,7 @@ void xe_display_pm_suspend(struct xe_device *xe)
> intel_power_domains_disable(display);
> drm_client_dev_suspend(&xe->drm, false);
>
> - if (has_display(xe)) {
> + if (intel_display_device_present(display)) {
> drm_kms_helper_poll_disable(&xe->drm);
> intel_display_driver_disable_user_access(display);
> intel_display_driver_suspend(display);
> @@ -345,7 +338,7 @@ void xe_display_pm_suspend(struct xe_device *xe)
>
> intel_hpd_cancel_work(display);
>
> - if (has_display(xe)) {
> + if (intel_display_device_present(display)) {
> intel_display_driver_suspend_access(display);
> intel_encoder_suspend_all(display);
> }
> @@ -365,7 +358,7 @@ void xe_display_pm_shutdown(struct xe_device *xe)
> intel_power_domains_disable(display);
> drm_client_dev_suspend(&xe->drm, false);
>
> - if (has_display(xe)) {
> + if (intel_display_device_present(display)) {
> drm_kms_helper_poll_disable(&xe->drm);
> intel_display_driver_disable_user_access(display);
> intel_display_driver_suspend(display);
> @@ -376,7 +369,7 @@ void xe_display_pm_shutdown(struct xe_device *xe)
> intel_encoder_block_all_hpds(display);
> intel_hpd_cancel_work(display);
>
> - if (has_display(xe))
> + if (intel_display_device_present(display))
> intel_display_driver_suspend_access(display);
>
> intel_encoder_suspend_all(display);
> @@ -465,25 +458,25 @@ void xe_display_pm_resume(struct xe_device *xe)
>
> intel_dmc_resume(display);
>
> - if (has_display(xe))
> + if (intel_display_device_present(display))
> drm_mode_config_reset(&xe->drm);
>
> intel_display_driver_init_hw(display);
>
> - if (has_display(xe))
> + if (intel_display_device_present(display))
> intel_display_driver_resume_access(display);
>
> intel_hpd_init(display);
>
> intel_encoder_unblock_all_hpds(display);
>
> - if (has_display(xe)) {
> + if (intel_display_device_present(display)) {
> intel_display_driver_resume(display);
> drm_kms_helper_poll_enable(&xe->drm);
> intel_display_driver_enable_user_access(display);
> }
>
> - if (has_display(xe))
> + if (intel_display_device_present(display))
> intel_hpd_poll_disable(display);
>
> intel_opregion_resume(display);
> @@ -548,7 +541,7 @@ int xe_display_probe(struct xe_device *xe)
>
> xe->display = display;
>
> - if (has_display(xe))
> + if (intel_display_device_present(display))
> return 0;
>
> no_display:
> --
> 2.47.2
--
Ville Syrjälä
Intel
next prev parent reply other threads:[~2025-09-05 11:27 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-03 9:04 [PATCH] drm/i915/display: add intel_display_device_present() Jani Nikula
2025-09-03 9:32 ` ✓ CI.KUnit: success for drm/i915/display: add intel_display_device_present() (rev3) Patchwork
2025-09-03 14:10 ` ✗ i915.CI.BAT: failure for drm/i915/display: add intel_display_device_present() (rev2) Patchwork
2025-09-03 16:19 ` ✗ Xe.CI.Full: failure for drm/i915/display: add intel_display_device_present() (rev3) Patchwork
2025-09-05 11:27 ` Ville Syrjälä [this message]
2025-09-09 8:22 ` [PATCH] drm/i915/display: add intel_display_device_present() Jani Nikula
2025-09-08 18:41 ` ✓ i915.CI.BAT: success for drm/i915/display: add intel_display_device_present() (rev3) Patchwork
2025-09-09 3:49 ` ✓ i915.CI.Full: " Patchwork
-- strict thread matches above, loose matches on Subject: below --
2025-05-27 11:46 [PATCH] drm/i915/display: add intel_display_device_present() Jani Nikula
2025-05-27 19:14 ` Rodrigo Vivi
2025-05-30 9:38 ` Jani Nikula
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aLrJLuvHzyW-kjT-@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.