| Message ID | 20250321145626.94101-2-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915: Fix DP MST SB message timeouts due to PPS delays | expand |
On Fri, 21 Mar 2025, Imre Deak <imre.deak@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@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);
On Mon, Mar 24, 2025 at 12:33:22PM +0200, Jani Nikula wrote: > On Fri, 21 Mar 2025, Imre Deak <imre.deak@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@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). The PPS and VDD handling steps are dependent (PPS must be locked for enabling VDD) and both are skipped for the same reason during AUX transfers. So I thought it makes sense to move these to a separate function and skip both based on the same platform/output type check. > 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: I still think it'd be better to have a separate function for both locking PPS and enabling VDD for the reason I described above, that is to clarify that the PPS state must be locked to enable VDD. I guess the above could be done separately later in any case, so I can inline the fix as you suggest. > 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
On Mon, 24 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: > On Mon, Mar 24, 2025 at 12:33:22PM +0200, Jani Nikula wrote: >> On Fri, 21 Mar 2025, Imre Deak <imre.deak@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@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). > > The PPS and VDD handling steps are dependent (PPS must be locked for > enabling VDD) and both are skipped for the same reason during AUX > transfers. So I thought it makes sense to move these to a separate > function and skip both based on the same platform/output type check. On the contrary, I think the reasons are different. VDD is only needed for eDP. The PPS must be locked for VDD change (IOW for eDP) and for VLV/CHV pipe based PPS. But these two cases are independent. >> 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: > > I still think it'd be better to have a separate function for both > locking PPS and enabling VDD for the reason I described above, that is > to clarify that the PPS state must be locked to enable VDD. But there's no requirement that they must be done at the same time. The PPS lock could be held for a much longer period or for other things than just VDD. And in this case, the PPS lock may indeed protect *other* things than just VDD. Adding the separate function ties these unrelated cases together for IMO not good enough reason. intel_pps_vdd_on_unlocked() does check that it's called with the PPS lock held. But I realize it needs to be relaxed a bit like this: diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c index 617ce4993172..c883e872c9c8 100644 --- a/drivers/gpu/drm/i915/display/intel_pps.c +++ b/drivers/gpu/drm/i915/display/intel_pps.c @@ -744,11 +744,11 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp) i915_reg_t pp_stat_reg, pp_ctrl_reg; bool need_to_disable = !intel_dp->pps.want_panel_vdd; - lockdep_assert_held(&display->pps.mutex); - if (!intel_dp_is_edp(intel_dp)) return false; + lockdep_assert_held(&display->pps.mutex); + cancel_delayed_work(&intel_dp->pps.panel_vdd_work); intel_dp->pps.want_panel_vdd = true; @@ -925,11 +925,11 @@ void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync) { struct intel_display *display = to_intel_display(intel_dp); - lockdep_assert_held(&display->pps.mutex); - if (!intel_dp_is_edp(intel_dp)) return; + lockdep_assert_held(&display->pps.mutex); + INTEL_DISPLAY_STATE_WARN(display, !intel_dp->pps.want_panel_vdd, "[ENCODER:%d:%s] %s VDD not forced on", dp_to_dig_port(intel_dp)->base.base.base.id, > I guess the above could be done separately later in any case, so I can > inline the fix as you suggest. > >> 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
On Mon, Mar 24, 2025 at 02:28:35PM +0200, Jani Nikula wrote: > On Mon, 24 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: > > On Mon, Mar 24, 2025 at 12:33:22PM +0200, Jani Nikula wrote: > >> On Fri, 21 Mar 2025, Imre Deak <imre.deak@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@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). > > > > The PPS and VDD handling steps are dependent (PPS must be locked for > > enabling VDD) and both are skipped for the same reason during AUX > > transfers. So I thought it makes sense to move these to a separate > > function and skip both based on the same platform/output type check. > > On the contrary, I think the reasons are different. > > VDD is only needed for eDP. > > The PPS must be locked for VDD change (IOW for eDP) and for VLV/CHV pipe > based PPS. But these two cases are independent. The case requiring VDD (eDP) is a subset of the cases requring PPS to be locked (eDP or VLV/CHV). These are not independent cases. > >> 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: > > > > I still think it'd be better to have a separate function for both > > locking PPS and enabling VDD for the reason I described above, that is > > to clarify that the PPS state must be locked to enable VDD. > > But there's no requirement that they must be done at the same time. There is also no reason not do them at the same time for AUX. A benefit of doing that would be to clarify the dependency of VDD on PPS and also simplify intel_dp_aux_xfer(). > The PPS lock could be held for a much longer period or for other > things than just VDD. And in this case, the PPS lock may indeed > protect *other* things than just VDD. Adding the separate function > ties these unrelated cases together for IMO not good enough reason. > intel_pps_vdd_on_unlocked() does check that it's called with the PPS > lock held. > > But I realize it needs to be relaxed a bit like this: Yes, noticed this too. It was one reason I opted for skipping PPS locking / VDD enabling from one spot. > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c > index 617ce4993172..c883e872c9c8 100644 > --- a/drivers/gpu/drm/i915/display/intel_pps.c > +++ b/drivers/gpu/drm/i915/display/intel_pps.c > @@ -744,11 +744,11 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp) > i915_reg_t pp_stat_reg, pp_ctrl_reg; > bool need_to_disable = !intel_dp->pps.want_panel_vdd; > > - lockdep_assert_held(&display->pps.mutex); > - > if (!intel_dp_is_edp(intel_dp)) > return false; > > + lockdep_assert_held(&display->pps.mutex); > + > cancel_delayed_work(&intel_dp->pps.panel_vdd_work); > intel_dp->pps.want_panel_vdd = true; > > @@ -925,11 +925,11 @@ void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync) > { > struct intel_display *display = to_intel_display(intel_dp); > > - lockdep_assert_held(&display->pps.mutex); > - > if (!intel_dp_is_edp(intel_dp)) > return; > > + lockdep_assert_held(&display->pps.mutex); > + > INTEL_DISPLAY_STATE_WARN(display, !intel_dp->pps.want_panel_vdd, > "[ENCODER:%d:%s] %s VDD not forced on", > dp_to_dig_port(intel_dp)->base.base.base.id, > > > > I guess the above could be done separately later in any case, so I can > > inline the fix as you suggest. > > > >> 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 > > -- > Jani Nikula, Intel
On Mon, 24 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: > On Mon, Mar 24, 2025 at 02:28:35PM +0200, Jani Nikula wrote: >> On Mon, 24 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: >> > On Mon, Mar 24, 2025 at 12:33:22PM +0200, Jani Nikula wrote: >> >> On Fri, 21 Mar 2025, Imre Deak <imre.deak@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@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). >> > >> > The PPS and VDD handling steps are dependent (PPS must be locked for >> > enabling VDD) and both are skipped for the same reason during AUX >> > transfers. So I thought it makes sense to move these to a separate >> > function and skip both based on the same platform/output type check. >> >> On the contrary, I think the reasons are different. >> >> VDD is only needed for eDP. >> >> The PPS must be locked for VDD change (IOW for eDP) and for VLV/CHV pipe >> based PPS. But these two cases are independent. > > The case requiring VDD (eDP) is a subset of the cases requring PPS to be > locked (eDP or VLV/CHV). These are not independent cases. Logically, they are. VLV/CHV requires the PPS lock also for non-eDP. It's not a subset. BR, Jani. > >> >> 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: >> > >> > I still think it'd be better to have a separate function for both >> > locking PPS and enabling VDD for the reason I described above, that is >> > to clarify that the PPS state must be locked to enable VDD. >> >> But there's no requirement that they must be done at the same time. > > There is also no reason not do them at the same time for AUX. A benefit > of doing that would be to clarify the dependency of VDD on PPS and also > simplify intel_dp_aux_xfer(). > >> The PPS lock could be held for a much longer period or for other >> things than just VDD. And in this case, the PPS lock may indeed >> protect *other* things than just VDD. Adding the separate function >> ties these unrelated cases together for IMO not good enough reason. >> intel_pps_vdd_on_unlocked() does check that it's called with the PPS >> lock held. >> >> But I realize it needs to be relaxed a bit like this: > > Yes, noticed this too. It was one reason I opted for skipping PPS > locking / VDD enabling from one spot. > >> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c >> index 617ce4993172..c883e872c9c8 100644 >> --- a/drivers/gpu/drm/i915/display/intel_pps.c >> +++ b/drivers/gpu/drm/i915/display/intel_pps.c >> @@ -744,11 +744,11 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp) >> i915_reg_t pp_stat_reg, pp_ctrl_reg; >> bool need_to_disable = !intel_dp->pps.want_panel_vdd; >> >> - lockdep_assert_held(&display->pps.mutex); >> - >> if (!intel_dp_is_edp(intel_dp)) >> return false; >> >> + lockdep_assert_held(&display->pps.mutex); >> + >> cancel_delayed_work(&intel_dp->pps.panel_vdd_work); >> intel_dp->pps.want_panel_vdd = true; >> >> @@ -925,11 +925,11 @@ void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync) >> { >> struct intel_display *display = to_intel_display(intel_dp); >> >> - lockdep_assert_held(&display->pps.mutex); >> - >> if (!intel_dp_is_edp(intel_dp)) >> return; >> >> + lockdep_assert_held(&display->pps.mutex); >> + >> INTEL_DISPLAY_STATE_WARN(display, !intel_dp->pps.want_panel_vdd, >> "[ENCODER:%d:%s] %s VDD not forced on", >> dp_to_dig_port(intel_dp)->base.base.base.id, >> >> >> > I guess the above could be done separately later in any case, so I can >> > inline the fix as you suggest. >> > >> >> 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 >> >> -- >> Jani Nikula, Intel
On Mon, Mar 24, 2025 at 03:59:50PM +0200, Jani Nikula wrote: > On Mon, 24 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: > > On Mon, Mar 24, 2025 at 02:28:35PM +0200, Jani Nikula wrote: > >> On Mon, 24 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: > >> > On Mon, Mar 24, 2025 at 12:33:22PM +0200, Jani Nikula wrote: > >> >> On Fri, 21 Mar 2025, Imre Deak <imre.deak@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@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). > >> > > >> > The PPS and VDD handling steps are dependent (PPS must be locked for > >> > enabling VDD) and both are skipped for the same reason during AUX > >> > transfers. So I thought it makes sense to move these to a separate > >> > function and skip both based on the same platform/output type check. > >> > >> On the contrary, I think the reasons are different. > >> > >> VDD is only needed for eDP. > >> > >> The PPS must be locked for VDD change (IOW for eDP) and for VLV/CHV pipe > >> based PPS. But these two cases are independent. > > > > The case requiring VDD (eDP) is a subset of the cases requring PPS to be > > locked (eDP or VLV/CHV). These are not independent cases. > > Logically, they are. VLV/CHV requires the PPS lock also for > non-eDP. Yes, that is what I meant. 1. Cases needing PPS lock: eDP or non-eDP on VLV/CHV. 2. Case needing VDD: eDP. 2. is a subset of 1. > It's not a subset. > > BR, > Jani. > > > > > > >> >> 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: > >> > > >> > I still think it'd be better to have a separate function for both > >> > locking PPS and enabling VDD for the reason I described above, that is > >> > to clarify that the PPS state must be locked to enable VDD. > >> > >> But there's no requirement that they must be done at the same time. > > > > There is also no reason not do them at the same time for AUX. A benefit > > of doing that would be to clarify the dependency of VDD on PPS and also > > simplify intel_dp_aux_xfer(). > > > >> The PPS lock could be held for a much longer period or for other > >> things than just VDD. And in this case, the PPS lock may indeed > >> protect *other* things than just VDD. Adding the separate function > >> ties these unrelated cases together for IMO not good enough reason. > >> intel_pps_vdd_on_unlocked() does check that it's called with the PPS > >> lock held. > >> > >> But I realize it needs to be relaxed a bit like this: > > > > Yes, noticed this too. It was one reason I opted for skipping PPS > > locking / VDD enabling from one spot. > > > >> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c > >> index 617ce4993172..c883e872c9c8 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_pps.c > >> +++ b/drivers/gpu/drm/i915/display/intel_pps.c > >> @@ -744,11 +744,11 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp) > >> i915_reg_t pp_stat_reg, pp_ctrl_reg; > >> bool need_to_disable = !intel_dp->pps.want_panel_vdd; > >> > >> - lockdep_assert_held(&display->pps.mutex); > >> - > >> if (!intel_dp_is_edp(intel_dp)) > >> return false; > >> > >> + lockdep_assert_held(&display->pps.mutex); > >> + > >> cancel_delayed_work(&intel_dp->pps.panel_vdd_work); > >> intel_dp->pps.want_panel_vdd = true; > >> > >> @@ -925,11 +925,11 @@ void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync) > >> { > >> struct intel_display *display = to_intel_display(intel_dp); > >> > >> - lockdep_assert_held(&display->pps.mutex); > >> - > >> if (!intel_dp_is_edp(intel_dp)) > >> return; > >> > >> + lockdep_assert_held(&display->pps.mutex); > >> + > >> INTEL_DISPLAY_STATE_WARN(display, !intel_dp->pps.want_panel_vdd, > >> "[ENCODER:%d:%s] %s VDD not forced on", > >> dp_to_dig_port(intel_dp)->base.base.base.id, > >> > >> > >> > I guess the above could be done separately later in any case, so I can > >> > inline the fix as you suggest. > >> > > >> >> 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 > >> > >> -- > >> Jani Nikula, Intel > > -- > Jani Nikula, Intel
On Mon, 24 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: > On Mon, Mar 24, 2025 at 03:59:50PM +0200, Jani Nikula wrote: >> On Mon, 24 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: >> > On Mon, Mar 24, 2025 at 02:28:35PM +0200, Jani Nikula wrote: >> >> On Mon, 24 Mar 2025, Imre Deak <imre.deak@intel.com> wrote: >> >> > On Mon, Mar 24, 2025 at 12:33:22PM +0200, Jani Nikula wrote: >> >> >> On Fri, 21 Mar 2025, Imre Deak <imre.deak@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@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). >> >> > >> >> > The PPS and VDD handling steps are dependent (PPS must be locked for >> >> > enabling VDD) and both are skipped for the same reason during AUX >> >> > transfers. So I thought it makes sense to move these to a separate >> >> > function and skip both based on the same platform/output type check. >> >> >> >> On the contrary, I think the reasons are different. >> >> >> >> VDD is only needed for eDP. >> >> >> >> The PPS must be locked for VDD change (IOW for eDP) and for VLV/CHV pipe >> >> based PPS. But these two cases are independent. >> > >> > The case requiring VDD (eDP) is a subset of the cases requring PPS to be >> > locked (eDP or VLV/CHV). These are not independent cases. >> >> Logically, they are. VLV/CHV requires the PPS lock also for >> non-eDP. > > Yes, that is what I meant. > > 1. Cases needing PPS lock: eDP or non-eDP on VLV/CHV. > 2. Case needing VDD: eDP. > > 2. is a subset of 1. I see what you mean but I disagree that they should be tied together this way. I think they should remain separate as they are, and I think it's easier to understand that way. BR, Jani. > >> It's not a subset. > >> >> BR, >> Jani. >> >> >> >> > >> >> >> 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: >> >> > >> >> > I still think it'd be better to have a separate function for both >> >> > locking PPS and enabling VDD for the reason I described above, that is >> >> > to clarify that the PPS state must be locked to enable VDD. >> >> >> >> But there's no requirement that they must be done at the same time. >> > >> > There is also no reason not do them at the same time for AUX. A benefit >> > of doing that would be to clarify the dependency of VDD on PPS and also >> > simplify intel_dp_aux_xfer(). >> > >> >> The PPS lock could be held for a much longer period or for other >> >> things than just VDD. And in this case, the PPS lock may indeed >> >> protect *other* things than just VDD. Adding the separate function >> >> ties these unrelated cases together for IMO not good enough reason. >> >> intel_pps_vdd_on_unlocked() does check that it's called with the PPS >> >> lock held. >> >> >> >> But I realize it needs to be relaxed a bit like this: >> > >> > Yes, noticed this too. It was one reason I opted for skipping PPS >> > locking / VDD enabling from one spot. >> > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c >> >> index 617ce4993172..c883e872c9c8 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_pps.c >> >> +++ b/drivers/gpu/drm/i915/display/intel_pps.c >> >> @@ -744,11 +744,11 @@ bool intel_pps_vdd_on_unlocked(struct intel_dp *intel_dp) >> >> i915_reg_t pp_stat_reg, pp_ctrl_reg; >> >> bool need_to_disable = !intel_dp->pps.want_panel_vdd; >> >> >> >> - lockdep_assert_held(&display->pps.mutex); >> >> - >> >> if (!intel_dp_is_edp(intel_dp)) >> >> return false; >> >> >> >> + lockdep_assert_held(&display->pps.mutex); >> >> + >> >> cancel_delayed_work(&intel_dp->pps.panel_vdd_work); >> >> intel_dp->pps.want_panel_vdd = true; >> >> >> >> @@ -925,11 +925,11 @@ void intel_pps_vdd_off_unlocked(struct intel_dp *intel_dp, bool sync) >> >> { >> >> struct intel_display *display = to_intel_display(intel_dp); >> >> >> >> - lockdep_assert_held(&display->pps.mutex); >> >> - >> >> if (!intel_dp_is_edp(intel_dp)) >> >> return; >> >> >> >> + lockdep_assert_held(&display->pps.mutex); >> >> + >> >> INTEL_DISPLAY_STATE_WARN(display, !intel_dp->pps.want_panel_vdd, >> >> "[ENCODER:%d:%s] %s VDD not forced on", >> >> dp_to_dig_port(intel_dp)->base.base.base.id, >> >> >> >> >> >> > I guess the above could be done separately later in any case, so I can >> >> > inline the fix as you suggest. >> >> > >> >> >> 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 >> >> >> >> -- >> >> Jani Nikula, Intel >> >> -- >> Jani Nikula, Intel
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); +} + 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);
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@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(-)