[PATCH 1/3] drm/i915/pps: Add helpers to lock PPS for AUX transfers
Jani Nikula
jani.nikula at linux.intel.com
Mon Mar 24 10:33:22 UTC 2025
On Fri, 21 Mar 2025, Imre Deak <imre.deak at intel.com> wrote:
> Factor out from the DP AUX transfer function the logic to lock/unlock
> the Panel Power Sequencer state and enable/disable the VDD power
> required for the AUX transfer, adding these to helpers in intel_pps.c .
> This prepares for a follow-up change making these steps dependent on the
> platform and output type.
>
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_dp_aux.c | 16 ++----------
> drivers/gpu/drm/i915/display/intel_pps.c | 29 ++++++++++++++++++++-
> drivers/gpu/drm/i915/display/intel_pps.h | 3 ++-
> 3 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> index ec27bbd70bcf0..bf5ccfa24ca0b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -272,15 +272,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> aux_domain = intel_aux_power_domain(dig_port);
>
> aux_wakeref = intel_display_power_get(display, aux_domain);
> - pps_wakeref = intel_pps_lock(intel_dp);
> -
> - /*
> - * We will be called with VDD already enabled for dpcd/edid/oui reads.
> - * In such cases we want to leave VDD enabled and it's up to upper layers
> - * to turn it off. But for eg. i2c-dev access we need to turn it on/off
> - * ourselves.
> - */
> - vdd = intel_pps_vdd_on_unlocked(intel_dp);
> + pps_wakeref = intel_pps_lock_for_aux(intel_dp, &vdd);
>
> /*
> * dp aux is extremely sensitive to irq latency, hence request the
> @@ -289,8 +281,6 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> */
> cpu_latency_qos_update_request(&intel_dp->pm_qos, 0);
>
> - intel_pps_check_power_unlocked(intel_dp);
> -
> /*
> * FIXME PSR should be disabled here to prevent
> * it using the same AUX CH simultaneously
> @@ -427,10 +417,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> out:
> cpu_latency_qos_update_request(&intel_dp->pm_qos, PM_QOS_DEFAULT_VALUE);
>
> - if (vdd)
> - intel_pps_vdd_off_unlocked(intel_dp, false);
> + intel_pps_unlock_for_aux(intel_dp, pps_wakeref, vdd);
>
> - intel_pps_unlock(intel_dp, pps_wakeref);
> intel_display_power_put_async(display, aux_domain, aux_wakeref);
> out_unlock:
> intel_digital_port_unlock(encoder);
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 617ce49931726..3c078fd53fbfa 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -571,7 +571,7 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> return intel_de_read(display, _pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD;
> }
>
> -void intel_pps_check_power_unlocked(struct intel_dp *intel_dp)
> +static void intel_pps_check_power_unlocked(struct intel_dp *intel_dp)
> {
> struct intel_display *display = to_intel_display(intel_dp);
> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> @@ -955,6 +955,33 @@ void intel_pps_vdd_off(struct intel_dp *intel_dp)
> intel_pps_vdd_off_unlocked(intel_dp, false);
> }
>
> +intel_wakeref_t intel_pps_lock_for_aux(struct intel_dp *intel_dp, bool *vdd_ref)
> +{
> + intel_wakeref_t wakeref;
> +
> + wakeref = intel_pps_lock(intel_dp);
> +
> + /*
> + * We will be called with VDD already enabled for dpcd/edid/oui reads.
> + * In such cases we want to leave VDD enabled and it's up to upper layers
> + * to turn it off. But for eg. i2c-dev access we need to turn it on/off
> + * ourselves.
> + */
> + *vdd_ref = intel_pps_vdd_on_unlocked(intel_dp);
> +
> + intel_pps_check_power_unlocked(intel_dp);
> +
> + return wakeref;
> +}
> +
> +void intel_pps_unlock_for_aux(struct intel_dp *intel_dp, intel_wakeref_t wakeref, bool vdd_ref)
> +{
> + if (vdd_ref)
> + intel_pps_vdd_off_unlocked(intel_dp, false);
> +
> + intel_pps_unlock(intel_dp, wakeref);
> +}
It took me a while to pinpoint what exactly I don't like about this
interface.
And I mean the whole intel_pps.h interface is already really difficult
to understand.
This flips the lock/unlock and vdd on/off logic inside out.
Normally you have functions for doing vdd or power or backlight, or
anything PPS really, and they're either unlocked (assuming the caller
handles PPS lock) or locked (the function itself takes the lock).
This one purports to be an interface for lock/unlock, but in reality it
also does VDD internally. And that feels really quite wrong to me.
---
These are a single-use interface that I think make intel_pps.[ch] more
difficult to understand. I'd suggest checking how you'd implement this
logic inside intel_dp_aux_xfer() *without* changing the intel_pps.[ch]
interface at all.
Okay, took a quick stab at it, and unless I'm missing something it's
super easy:
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
index ec27bbd70bcf..a5608659df59 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
@@ -247,7 +247,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
u32 aux_clock_divider;
enum intel_display_power_domain aux_domain;
intel_wakeref_t aux_wakeref;
- intel_wakeref_t pps_wakeref;
+ intel_wakeref_t pps_wakeref = NULL;
int i, ret, recv_bytes;
int try, clock = 0;
u32 status;
@@ -272,7 +272,10 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
aux_domain = intel_aux_power_domain(dig_port);
aux_wakeref = intel_display_power_get(display, aux_domain);
- pps_wakeref = intel_pps_lock(intel_dp);
+
+ if (intel_dp_is_edp(intel_dp) ||
+ (display->platform.valleyview || display->platform.cherryview))
+ pps_wakeref = intel_pps_lock(intel_dp);
/*
* We will be called with VDD already enabled for dpcd/edid/oui reads.
@@ -430,7 +433,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
if (vdd)
intel_pps_vdd_off_unlocked(intel_dp, false);
- intel_pps_unlock(intel_dp, pps_wakeref);
+ if (pps_wakeref)
+ intel_pps_unlock(intel_dp, pps_wakeref);
intel_display_power_put_async(display, aux_domain, aux_wakeref);
out_unlock:
intel_digital_port_unlock(encoder);
Please let's not make intel_pps.[ch] harder to understand.
BR,
Jani.
> +
> void intel_pps_on_unlocked(struct intel_dp *intel_dp)
> {
> struct intel_display *display = to_intel_display(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h
> index c83007152f07d..4390d05892325 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.h
> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
> @@ -31,10 +31,11 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp);
> void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync);
> void intel_pps_on_unlocked(struct intel_dp *intel_dp);
> void intel_pps_off_unlocked(struct intel_dp *intel_dp);
> -void intel_pps_check_power_unlocked(struct intel_dp *intel_dp);
>
> void intel_pps_vdd_on(struct intel_dp *intel_dp);
> void intel_pps_vdd_off(struct intel_dp *intel_dp);
> +intel_wakeref_t intel_pps_lock_for_aux(struct intel_dp *intel_dp, bool *vdd_ref);
> +void intel_pps_unlock_for_aux(struct intel_dp *intel_dp, intel_wakeref_t wakeref, bool vdd_ref);
> void intel_pps_on(struct intel_dp *intel_dp);
> void intel_pps_off(struct intel_dp *intel_dp);
> void intel_pps_vdd_off_sync(struct intel_dp *intel_dp);
--
Jani Nikula, Intel
More information about the Intel-xe
mailing list