On Tue, 09 Dec 2025, Ville Syrjala <[email protected]> wrote:
> From: Ville Syrjälä <[email protected]>
>
> We use forcewake to prevent the SoC from actually entering
> PC8 while performing the PC8 disable sequence. Hide that
> behind a new parent interface to eliminate the naked
> forcewake/uncore usage from the display power code.
>
> Signed-off-by: Ville Syrjälä <[email protected]>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> .../drm/i915/display/intel_display_power.c | 8 ++---
> drivers/gpu/drm/i915/display/intel_parent.c | 10 +++++++
> drivers/gpu/drm/i915/display/intel_parent.h | 3 ++
> drivers/gpu/drm/i915/i915_display_pc8.c | 30 +++++++++++++++++++
> drivers/gpu/drm/i915/i915_display_pc8.h | 9 ++++++
> drivers/gpu/drm/i915/i915_driver.c | 2 ++
> include/drm/intel/display_parent_interface.h | 8 +++++
> 8 files changed, 67 insertions(+), 4 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/i915_display_pc8.c
> create mode 100644 drivers/gpu/drm/i915/i915_display_pc8.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 175bd99e1d0d..b57e51d626b1 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -76,6 +76,7 @@ i915-$(CONFIG_PERF_EVENTS) += \
>
> # core display adaptation
> i915-y += \
> + i915_display_pc8.o \
> i915_hdcp_gsc.o
>
> # "Graphics Technology" (aka we talk to the gpu)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 9f323c39d798..47042a4c3a30 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -1339,10 +1339,10 @@ static void hsw_restore_lcpll(struct intel_display
> *display)
> return;
>
> /*
> - * Make sure we're not on PC8 state before disabling PC8, otherwise
> - * we'll hang the machine. To prevent PC8 state, just enable force_wake.
> + * Make sure we're not on PC8 state before disabling
> + * PC8, otherwise we'll hang the machine.
> */
> - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL);
> + intel_parent_pc8_block(display);
>
> if (val & LCPLL_POWER_DOWN_ALLOW) {
> val &= ~LCPLL_POWER_DOWN_ALLOW;
> @@ -1372,7 +1372,7 @@ static void hsw_restore_lcpll(struct intel_display
> *display)
> "Switching back to LCPLL failed\n");
> }
>
> - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL);
> + intel_parent_pc8_unblock(display);
>
> intel_update_cdclk(display);
> intel_cdclk_dump_config(display, &display->cdclk.hw, "Current CDCLK");
> diff --git a/drivers/gpu/drm/i915/display/intel_parent.c
> b/drivers/gpu/drm/i915/display/intel_parent.c
> index 2ea310cc3509..9201d506c851 100644
> --- a/drivers/gpu/drm/i915/display/intel_parent.c
> +++ b/drivers/gpu/drm/i915/display/intel_parent.c
> @@ -56,6 +56,16 @@ void intel_parent_irq_synchronize(struct intel_display
> *display)
> display->parent->irq->synchronize(display->drm);
> }
>
> +void intel_parent_pc8_block(struct intel_display *display)
> +{
> + display->parent->pc8->block(display->drm);
> +}
> +
> +void intel_parent_pc8_unblock(struct intel_display *display)
> +{
> + display->parent->pc8->unblock(display->drm);
> +}
I think I'd like either:
- A substruct is mandatory, always initialized by parent, will never be
NULL, and there are no checks here.
- A substruct is optional, may be initialized by parent, may be NULL,
and there's a NULL check here.
I think it makes the interface easier to reason about. Even if I
understand that this particular interface will only be called for
platforms supported by i915.
So this should have the display->parent->pc8 != NULL check, and
"optional" mentioned in display_parent_interface.h.
> +
> bool intel_parent_rps_available(struct intel_display *display)
> {
> return display->parent->rps;
> diff --git a/drivers/gpu/drm/i915/display/intel_parent.h
> b/drivers/gpu/drm/i915/display/intel_parent.h
> index 8f91a6f75c53..974a016ab3be 100644
> --- a/drivers/gpu/drm/i915/display/intel_parent.h
> +++ b/drivers/gpu/drm/i915/display/intel_parent.h
> @@ -22,6 +22,9 @@ void intel_parent_hdcp_gsc_context_free(struct
> intel_display *display,
> bool intel_parent_irq_enabled(struct intel_display *display);
> void intel_parent_irq_synchronize(struct intel_display *display);
>
> +void intel_parent_pc8_block(struct intel_display *display);
> +void intel_parent_pc8_unblock(struct intel_display *display);
> +
> bool intel_parent_rps_available(struct intel_display *display);
> void intel_parent_rps_boost_if_not_started(struct intel_display *display,
> struct dma_fence *fence);
> void intel_parent_rps_mark_interactive(struct intel_display *display, bool
> interactive);
> diff --git a/drivers/gpu/drm/i915/i915_display_pc8.c
> b/drivers/gpu/drm/i915/i915_display_pc8.c
> new file mode 100644
> index 000000000000..443935d282e3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_display_pc8.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2025, Intel Corporation.
> + */
> +
> +#include <drm/drm_print.h>
> +#include <drm/intel/display_parent_interface.h>
> +
> +#include "i915_drv.h"
> +#include "intel_uncore.h"
For completeness, I think this should include i915_display_pc8.h. I'm a
bit surprised the compilers only warn about non-static functions without
declarations, not about non-static variables.
> +
> +static void i915_display_pc8_block(struct drm_device *drm)
> +{
> + struct intel_uncore *uncore = &to_i915(drm)->uncore;
> +
> + /* to prevent PC8 state, just enable force_wake */
> + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
> +}
> +
> +static void i915_display_pc8_unblock(struct drm_device *drm)
> +{
> + struct intel_uncore *uncore = &to_i915(drm)->uncore;
> +
> + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
> +}
> +
> +const struct intel_display_pc8_interface i915_display_pc8_interface = {
> + .block = i915_display_pc8_block,
> + .unblock = i915_display_pc8_unblock,
> +};
> diff --git a/drivers/gpu/drm/i915/i915_display_pc8.h
> b/drivers/gpu/drm/i915/i915_display_pc8.h
> new file mode 100644
> index 000000000000..717f313d2a21
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_display_pc8.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: MIT */
> +/* Copyright © 2025 Intel Corporation */
> +
> +#ifndef __I915_DISPLAY_PC8_H__
> +#define __I915_DISPLAY_PC8_H__
> +
> +extern const struct intel_display_pc8_interface i915_display_pc8_interface;
> +
> +#endif /* __I915_DISPLAY_PC8_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index d98839427ef9..723cb424b2ba 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -89,6 +89,7 @@
> #include "pxp/intel_pxp_pm.h"
>
> #include "i915_debugfs.h"
> +#include "i915_display_pc8.h"
> #include "i915_driver.h"
> #include "i915_drm_client.h"
> #include "i915_drv.h"
> @@ -761,6 +762,7 @@ static const struct intel_display_parent_interface parent
> = {
> .hdcp = &i915_display_hdcp_interface,
> .rpm = &i915_display_rpm_interface,
> .irq = &i915_display_irq_interface,
> + .pc8 = &i915_display_pc8_interface,
> .rps = &i915_display_rps_interface,
> .vgpu_active = vgpu_active,
> .has_fenced_regions = has_fenced_regions,
> diff --git a/include/drm/intel/display_parent_interface.h
> b/include/drm/intel/display_parent_interface.h
> index 61d1b22adc83..af43b213eafa 100644
> --- a/include/drm/intel/display_parent_interface.h
> +++ b/include/drm/intel/display_parent_interface.h
> @@ -41,6 +41,11 @@ struct intel_display_irq_interface {
> void (*synchronize)(struct drm_device *drm);
> };
>
> +struct intel_display_pc8_interface {
> + void (*block)(struct drm_device *drm);
> + void (*unblock)(struct drm_device *drm);
> +};
> +
> struct intel_display_rps_interface {
> void (*boost_if_not_started)(struct dma_fence *fence);
> void (*mark_interactive)(struct drm_device *drm, bool interactive);
> @@ -69,6 +74,9 @@ struct intel_display_parent_interface {
> /** @irq: IRQ interface */
> const struct intel_display_irq_interface *irq;
>
> + /** @pc8: PC8 interface */
I think this should have "Optional" in there.
> + const struct intel_display_pc8_interface *pc8;
> +
> /** @rpm: RPS interface. Optional. */
> const struct intel_display_rps_interface *rps;
--
Jani Nikula, Intel
