Message ID | 20231012123522.26045-3-ville.syrjala@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915: DPLL code cleanups | expand |
On Thu, 12 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Just include the JSL/EHL DPLL4 extra power domain in the dpll_info > struct. This way the same approach could be used by other platforms > as well (should the need arise), and we don't have to sprinkle > platform checks all over the place. > > Note that I'm perhaps slightly abusing things here as > power_domain==0 (which is actually POWER_DOMAIN_DISPLAY_CORE) now > indicates that no extra power domain is needed. I suppose using > POWER_DOMAIN_INVALID would be more correct, but then we'd have to > sprinkle that to all the other DPLLs. Cc: Imre, how bad do you think that is? Anyway, Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 30 +++++-------------- > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 6 ++++ > 2 files changed, 14 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > index b7997b096796..4e524cb8ed83 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > @@ -3838,17 +3838,8 @@ static void combo_pll_enable(struct drm_i915_private *i915, > { > i915_reg_t enable_reg = intel_combo_pll_enable_reg(i915, pll); > > - if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) && > - pll->info->id == DPLL_ID_EHL_DPLL4) { > - > - /* > - * We need to disable DC states when this DPLL is enabled. > - * This can be done by taking a reference on DPLL4 power > - * domain. > - */ > - pll->wakeref = intel_display_power_get(i915, > - POWER_DOMAIN_DC_OFF); > - } > + if (pll->info->power_domain) > + pll->wakeref = intel_display_power_get(i915, pll->info->power_domain); > > icl_pll_power_enable(i915, pll, enable_reg); > > @@ -3946,10 +3937,8 @@ static void combo_pll_disable(struct drm_i915_private *i915, > > icl_pll_disable(i915, pll, enable_reg); > > - if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) && > - pll->info->id == DPLL_ID_EHL_DPLL4) > - intel_display_power_put(i915, POWER_DOMAIN_DC_OFF, > - pll->wakeref); > + if (pll->info->power_domain) > + intel_display_power_put(i915, pll->info->power_domain, pll->wakeref); > } > > static void tbt_pll_disable(struct drm_i915_private *i915, > @@ -4041,7 +4030,8 @@ static const struct intel_dpll_mgr icl_pll_mgr = { > static const struct dpll_info ehl_plls[] = { > { .name = "DPLL 0", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL0, }, > { .name = "DPLL 1", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL1, }, > - { .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4, }, > + { .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4, > + .power_domain = POWER_DOMAIN_DC_OFF, }, > {} > }; > > @@ -4369,12 +4359,8 @@ static void readout_dpll_hw_state(struct drm_i915_private *i915, > > pll->on = intel_dpll_get_hw_state(i915, pll, &pll->state.hw_state); > > - if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) && > - pll->on && > - pll->info->id == DPLL_ID_EHL_DPLL4) { > - pll->wakeref = intel_display_power_get(i915, > - POWER_DOMAIN_DC_OFF); > - } > + if (pll->on && pll->info->power_domain) > + pll->wakeref = intel_display_power_get(i915, pll->info->power_domain); > > pll->state.pipe_mask = 0; > for_each_intel_crtc(&i915->drm, crtc) { > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > index dd4796a61751..2e7ea0d8d3ff 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > @@ -27,6 +27,7 @@ > > #include <linux/types.h> > > +#include "intel_display_power.h" > #include "intel_wakeref.h" > > #define for_each_shared_dpll(__i915, __pll, __i) \ > @@ -270,6 +271,11 @@ struct dpll_info { > */ > enum intel_dpll_id id; > > + /** > + * @power_domain: extra power domain required by the DPLL > + */ > + enum intel_display_power_domain power_domain; > + > #define INTEL_DPLL_ALWAYS_ON (1 << 0) > /** > * @flags:
On Thu, Oct 12, 2023 at 05:56:34PM +0300, Jani Nikula wrote: > On Thu, 12 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Just include the JSL/EHL DPLL4 extra power domain in the dpll_info > > struct. This way the same approach could be used by other platforms > > as well (should the need arise), and we don't have to sprinkle > > platform checks all over the place. > > > > Note that I'm perhaps slightly abusing things here as > > power_domain==0 (which is actually POWER_DOMAIN_DISPLAY_CORE) now > > indicates that no extra power domain is needed. I suppose using > > POWER_DOMAIN_INVALID would be more correct, but then we'd have to > > sprinkle that to all the other DPLLs. > > Cc: Imre, how bad do you think that is? It looks ok to me. With the original 64 bit domain mask it made sense to keep POWER_DOMAIN_INVALID at the end of the enum list, but nothing depends on the actual value, so for clarity it could be moved to be the first item. > > Anyway, > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 30 +++++-------------- > > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 6 ++++ > > 2 files changed, 14 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > index b7997b096796..4e524cb8ed83 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c > > @@ -3838,17 +3838,8 @@ static void combo_pll_enable(struct drm_i915_private *i915, > > { > > i915_reg_t enable_reg = intel_combo_pll_enable_reg(i915, pll); > > > > - if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) && > > - pll->info->id == DPLL_ID_EHL_DPLL4) { > > - > > - /* > > - * We need to disable DC states when this DPLL is enabled. > > - * This can be done by taking a reference on DPLL4 power > > - * domain. > > - */ > > - pll->wakeref = intel_display_power_get(i915, > > - POWER_DOMAIN_DC_OFF); > > - } > > + if (pll->info->power_domain) > > + pll->wakeref = intel_display_power_get(i915, pll->info->power_domain); > > > > icl_pll_power_enable(i915, pll, enable_reg); > > > > @@ -3946,10 +3937,8 @@ static void combo_pll_disable(struct drm_i915_private *i915, > > > > icl_pll_disable(i915, pll, enable_reg); > > > > - if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) && > > - pll->info->id == DPLL_ID_EHL_DPLL4) > > - intel_display_power_put(i915, POWER_DOMAIN_DC_OFF, > > - pll->wakeref); > > + if (pll->info->power_domain) > > + intel_display_power_put(i915, pll->info->power_domain, pll->wakeref); > > } > > > > static void tbt_pll_disable(struct drm_i915_private *i915, > > @@ -4041,7 +4030,8 @@ static const struct intel_dpll_mgr icl_pll_mgr = { > > static const struct dpll_info ehl_plls[] = { > > { .name = "DPLL 0", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL0, }, > > { .name = "DPLL 1", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL1, }, > > - { .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4, }, > > + { .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4, > > + .power_domain = POWER_DOMAIN_DC_OFF, }, > > {} > > }; > > > > @@ -4369,12 +4359,8 @@ static void readout_dpll_hw_state(struct drm_i915_private *i915, > > > > pll->on = intel_dpll_get_hw_state(i915, pll, &pll->state.hw_state); > > > > - if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) && > > - pll->on && > > - pll->info->id == DPLL_ID_EHL_DPLL4) { > > - pll->wakeref = intel_display_power_get(i915, > > - POWER_DOMAIN_DC_OFF); > > - } > > + if (pll->on && pll->info->power_domain) > > + pll->wakeref = intel_display_power_get(i915, pll->info->power_domain); > > > > pll->state.pipe_mask = 0; > > for_each_intel_crtc(&i915->drm, crtc) { > > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > index dd4796a61751..2e7ea0d8d3ff 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h > > @@ -27,6 +27,7 @@ > > > > #include <linux/types.h> > > > > +#include "intel_display_power.h" > > #include "intel_wakeref.h" > > > > #define for_each_shared_dpll(__i915, __pll, __i) \ > > @@ -270,6 +271,11 @@ struct dpll_info { > > */ > > enum intel_dpll_id id; > > > > + /** > > + * @power_domain: extra power domain required by the DPLL > > + */ > > + enum intel_display_power_domain power_domain; > > + > > #define INTEL_DPLL_ALWAYS_ON (1 << 0) > > /** > > * @flags: > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c index b7997b096796..4e524cb8ed83 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c @@ -3838,17 +3838,8 @@ static void combo_pll_enable(struct drm_i915_private *i915, { i915_reg_t enable_reg = intel_combo_pll_enable_reg(i915, pll); - if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) && - pll->info->id == DPLL_ID_EHL_DPLL4) { - - /* - * We need to disable DC states when this DPLL is enabled. - * This can be done by taking a reference on DPLL4 power - * domain. - */ - pll->wakeref = intel_display_power_get(i915, - POWER_DOMAIN_DC_OFF); - } + if (pll->info->power_domain) + pll->wakeref = intel_display_power_get(i915, pll->info->power_domain); icl_pll_power_enable(i915, pll, enable_reg); @@ -3946,10 +3937,8 @@ static void combo_pll_disable(struct drm_i915_private *i915, icl_pll_disable(i915, pll, enable_reg); - if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) && - pll->info->id == DPLL_ID_EHL_DPLL4) - intel_display_power_put(i915, POWER_DOMAIN_DC_OFF, - pll->wakeref); + if (pll->info->power_domain) + intel_display_power_put(i915, pll->info->power_domain, pll->wakeref); } static void tbt_pll_disable(struct drm_i915_private *i915, @@ -4041,7 +4030,8 @@ static const struct intel_dpll_mgr icl_pll_mgr = { static const struct dpll_info ehl_plls[] = { { .name = "DPLL 0", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL0, }, { .name = "DPLL 1", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL1, }, - { .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4, }, + { .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4, + .power_domain = POWER_DOMAIN_DC_OFF, }, {} }; @@ -4369,12 +4359,8 @@ static void readout_dpll_hw_state(struct drm_i915_private *i915, pll->on = intel_dpll_get_hw_state(i915, pll, &pll->state.hw_state); - if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) && - pll->on && - pll->info->id == DPLL_ID_EHL_DPLL4) { - pll->wakeref = intel_display_power_get(i915, - POWER_DOMAIN_DC_OFF); - } + if (pll->on && pll->info->power_domain) + pll->wakeref = intel_display_power_get(i915, pll->info->power_domain); pll->state.pipe_mask = 0; for_each_intel_crtc(&i915->drm, crtc) { diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h index dd4796a61751..2e7ea0d8d3ff 100644 --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h @@ -27,6 +27,7 @@ #include <linux/types.h> +#include "intel_display_power.h" #include "intel_wakeref.h" #define for_each_shared_dpll(__i915, __pll, __i) \ @@ -270,6 +271,11 @@ struct dpll_info { */ enum intel_dpll_id id; + /** + * @power_domain: extra power domain required by the DPLL + */ + enum intel_display_power_domain power_domain; + #define INTEL_DPLL_ALWAYS_ON (1 << 0) /** * @flags: