| Message ID | 19beb306efff2e4380d7eed18f6262361ffb6ece.1725458428.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pps: hide VLV/CHV PPS pipe stuff inside intel_pps.c | expand |
On Wed, Sep 04, 2024 at 05:02:33PM +0300, Jani Nikula wrote: > Add intel_pps_dp_power_down() and move the VLV/CHV active pipe clear > there from intel_dp_link_down(), hiding the PPS pipe details inside PPS > code. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/g4x_dp.c | 9 +-------- > drivers/gpu/drm/i915/display/intel_pps.c | 16 ++++++++++++++++ > drivers/gpu/drm/i915/display/intel_pps.h | 1 + > 3 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c > index 1f9812223263..98405fbd0e04 100644 > --- a/drivers/gpu/drm/i915/display/g4x_dp.c > +++ b/drivers/gpu/drm/i915/display/g4x_dp.c > @@ -475,14 +475,7 @@ intel_dp_link_down(struct intel_encoder *encoder, > intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true); > } > > - msleep(intel_dp->pps.panel_power_down_delay); > - > - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > - intel_wakeref_t wakeref; > - > - with_intel_pps_lock(intel_dp, wakeref) > - intel_dp->pps.active_pipe = INVALID_PIPE; > - } > + intel_pps_dp_power_down(intel_dp); > } > > static void g4x_dp_audio_enable(struct intel_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c > index 9e54acfea992..a7f7e5e1f3aa 100644 > --- a/drivers/gpu/drm/i915/display/intel_pps.c > +++ b/drivers/gpu/drm/i915/display/intel_pps.c > @@ -1599,6 +1599,22 @@ static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd > (intel_de_read(display, regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK)); > } > > +/* Call on all DP, not just eDP */ > +void intel_pps_dp_power_down(struct intel_dp *intel_dp) The name seems to imply this powers down something, which it doesn't. > +{ > + struct intel_display *display = to_intel_display(intel_dp); > + struct drm_i915_private *i915 = to_i915(display->drm); > + > + msleep(intel_dp->pps.panel_power_down_delay); I can't actually figure out why this msleep() even exists. We already waited for the power down delay (by waiting for the pps state machine) when we turned off the panel power, so this seems completely redundant. The whole pre-ddi modeset sequence is a bit weird because the port enable/disable essentially happens on the wrong side of panel power enable/disable. But I guess that's just how the hw works and the PPS somehow makes sure things happen in the right order. And I suppose the magic PPS register write protect thing even prevents doing it in the opposite order (although IIRC we always disable the write protect mechanism). > + > + if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) { > + intel_wakeref_t wakeref; > + > + with_intel_pps_lock(intel_dp, wakeref) > + intel_dp->pps.active_pipe = INVALID_PIPE; > + } This part is basically the counterpart to vlv_pps_init(), so the function naming should probably reflect that somehow. vlv_pps_port_{enable,disable}() or something like that? > +} > + > /* Call on all DP, not just eDP */ > void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp) > { > diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h > index 8dbea05f498d..42f0377a93a8 100644 > --- a/drivers/gpu/drm/i915/display/intel_pps.h > +++ b/drivers/gpu/drm/i915/display/intel_pps.h > @@ -45,6 +45,7 @@ void intel_pps_init_late(struct intel_dp *intel_dp); > > void intel_pps_dp_init(struct intel_dp *intel_dp); > void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp); > +void intel_pps_dp_power_down(struct intel_dp *intel_dp); > void intel_pps_reset_all(struct intel_display *display); > > void vlv_pps_init(struct intel_encoder *encoder, > -- > 2.39.2
On Wed, 04 Sep 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Sep 04, 2024 at 05:02:33PM +0300, Jani Nikula wrote: >> Add intel_pps_dp_power_down() and move the VLV/CHV active pipe clear >> there from intel_dp_link_down(), hiding the PPS pipe details inside PPS >> code. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/display/g4x_dp.c | 9 +-------- >> drivers/gpu/drm/i915/display/intel_pps.c | 16 ++++++++++++++++ >> drivers/gpu/drm/i915/display/intel_pps.h | 1 + >> 3 files changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c >> index 1f9812223263..98405fbd0e04 100644 >> --- a/drivers/gpu/drm/i915/display/g4x_dp.c >> +++ b/drivers/gpu/drm/i915/display/g4x_dp.c >> @@ -475,14 +475,7 @@ intel_dp_link_down(struct intel_encoder *encoder, >> intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true); >> } >> >> - msleep(intel_dp->pps.panel_power_down_delay); >> - >> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { >> - intel_wakeref_t wakeref; >> - >> - with_intel_pps_lock(intel_dp, wakeref) >> - intel_dp->pps.active_pipe = INVALID_PIPE; >> - } >> + intel_pps_dp_power_down(intel_dp); >> } >> >> static void g4x_dp_audio_enable(struct intel_encoder *encoder, >> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c >> index 9e54acfea992..a7f7e5e1f3aa 100644 >> --- a/drivers/gpu/drm/i915/display/intel_pps.c >> +++ b/drivers/gpu/drm/i915/display/intel_pps.c >> @@ -1599,6 +1599,22 @@ static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd >> (intel_de_read(display, regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK)); >> } >> >> +/* Call on all DP, not just eDP */ >> +void intel_pps_dp_power_down(struct intel_dp *intel_dp) > > The name seems to imply this powers down something, which it doesn't. Agreed. > >> +{ >> + struct intel_display *display = to_intel_display(intel_dp); >> + struct drm_i915_private *i915 = to_i915(display->drm); >> + >> + msleep(intel_dp->pps.panel_power_down_delay); > > I can't actually figure out why this msleep() even exists. We already > waited for the power down delay (by waiting for the pps state machine) > when we turned off the panel power, so this seems completely redundant. > > The whole pre-ddi modeset sequence is a bit weird because the port > enable/disable essentially happens on the wrong side of panel power > enable/disable. But I guess that's just how the hw works and the PPS > somehow makes sure things happen in the right order. And I suppose > the magic PPS register write protect thing even prevents doing it > in the opposite order (although IIRC we always disable the write > protect mechanism). > >> + >> + if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) { >> + intel_wakeref_t wakeref; >> + >> + with_intel_pps_lock(intel_dp, wakeref) >> + intel_dp->pps.active_pipe = INVALID_PIPE; >> + } > > This part is basically the counterpart to vlv_pps_init(), > so the function naming should probably reflect that somehow. > vlv_pps_port_{enable,disable}() or something like that? I ended up doing things a bit differently across the entire series, properly isolating pps_pipe/active_pipe to VLV/CHV code only, and adding functions for that stuff. I left the above msleep() where it is now. Didn't dare touch it yet, and it should be a separate change to remove it. New version of the whole series at [1]. BR, Jani. [1] https://lore.kernel.org/all/cover.1725883885.git.jani.nikula@intel.com/ > >> +} >> + >> /* Call on all DP, not just eDP */ >> void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp) >> { >> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h >> index 8dbea05f498d..42f0377a93a8 100644 >> --- a/drivers/gpu/drm/i915/display/intel_pps.h >> +++ b/drivers/gpu/drm/i915/display/intel_pps.h >> @@ -45,6 +45,7 @@ void intel_pps_init_late(struct intel_dp *intel_dp); >> >> void intel_pps_dp_init(struct intel_dp *intel_dp); >> void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp); >> +void intel_pps_dp_power_down(struct intel_dp *intel_dp); >> void intel_pps_reset_all(struct intel_display *display); >> >> void vlv_pps_init(struct intel_encoder *encoder, >> -- >> 2.39.2
diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c b/drivers/gpu/drm/i915/display/g4x_dp.c index 1f9812223263..98405fbd0e04 100644 --- a/drivers/gpu/drm/i915/display/g4x_dp.c +++ b/drivers/gpu/drm/i915/display/g4x_dp.c @@ -475,14 +475,7 @@ intel_dp_link_down(struct intel_encoder *encoder, intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true); } - msleep(intel_dp->pps.panel_power_down_delay); - - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { - intel_wakeref_t wakeref; - - with_intel_pps_lock(intel_dp, wakeref) - intel_dp->pps.active_pipe = INVALID_PIPE; - } + intel_pps_dp_power_down(intel_dp); } static void g4x_dp_audio_enable(struct intel_encoder *encoder, diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c index 9e54acfea992..a7f7e5e1f3aa 100644 --- a/drivers/gpu/drm/i915/display/intel_pps.c +++ b/drivers/gpu/drm/i915/display/intel_pps.c @@ -1599,6 +1599,22 @@ static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd (intel_de_read(display, regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK)); } +/* Call on all DP, not just eDP */ +void intel_pps_dp_power_down(struct intel_dp *intel_dp) +{ + struct intel_display *display = to_intel_display(intel_dp); + struct drm_i915_private *i915 = to_i915(display->drm); + + msleep(intel_dp->pps.panel_power_down_delay); + + if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) { + intel_wakeref_t wakeref; + + with_intel_pps_lock(intel_dp, wakeref) + intel_dp->pps.active_pipe = INVALID_PIPE; + } +} + /* Call on all DP, not just eDP */ void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp) { diff --git a/drivers/gpu/drm/i915/display/intel_pps.h b/drivers/gpu/drm/i915/display/intel_pps.h index 8dbea05f498d..42f0377a93a8 100644 --- a/drivers/gpu/drm/i915/display/intel_pps.h +++ b/drivers/gpu/drm/i915/display/intel_pps.h @@ -45,6 +45,7 @@ void intel_pps_init_late(struct intel_dp *intel_dp); void intel_pps_dp_init(struct intel_dp *intel_dp); void intel_pps_dp_encoder_reset(struct intel_dp *intel_dp); +void intel_pps_dp_power_down(struct intel_dp *intel_dp); void intel_pps_reset_all(struct intel_display *display); void vlv_pps_init(struct intel_encoder *encoder,
Add intel_pps_dp_power_down() and move the VLV/CHV active pipe clear there from intel_dp_link_down(), hiding the PPS pipe details inside PPS code. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/display/g4x_dp.c | 9 +-------- drivers/gpu/drm/i915/display/intel_pps.c | 16 ++++++++++++++++ drivers/gpu/drm/i915/display/intel_pps.h | 1 + 3 files changed, 18 insertions(+), 8 deletions(-)