Re: [Intel-gfx] [PATCH] drm/i915: Select DPLL's via mask
On Tue, Oct 08, 2019 at 07:28:45PM +0300, Ville Syrjälä wrote:
On Tue, Oct 08, 2019 at 09:12:52AM -0700, Matt Roper wrote:
This slightly simplifies the EHL DPLL4 handling and also gives us more
flexibility in the future in case we need to skip the use of specific
PLL's (e.g., due to hardware workarounds and such).
Cc: Lucas De Marchi
Signed-off-by: Matt Roper
---
drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 42 +--
1 file changed, 20 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 5e9e84c94a15..14e040658b12 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -247,8 +247,7 @@ static struct intel_shared_dpll *
intel_find_shared_dpll(struct intel_atomic_state *state,
const struct intel_crtc *crtc,
const struct intel_dpll_hw_state *pll_state,
- enum intel_dpll_id range_min,
- enum intel_dpll_id range_max)
+ unsigned long dpll_mask)
I don't like seeing lone longs hanging around since they
change meaning between 32bit and 64bit builds. Always makes
me suspicious.
{
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
struct intel_shared_dpll *pll, *unused_pll = NULL;
@@ -257,7 +256,9 @@ intel_find_shared_dpll(struct intel_atomic_state *state,
shared_dpll = intel_atomic_get_shared_dpll_state(>base);
- for (i = range_min; i <= range_max; i++) {
+ WARN_ON(dpll_mask & ~(BIT(I915_NUM_PLLS) - 1));
+
+ for_each_set_bit(i, _mask, I915_NUM_PLLS) {
But I guess this guy demands one :(
We are generally safe when the limit is much lower than the word sizes
we care about. I915_NUM_PLLS is currently 9 and I don't think it will go
near 32.
@@ -2975,7 +2975,7 @@ static bool icl_get_combo_phy_dpll(struct
intel_atomic_state *state,
_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
enum port port = encoder->port;
- bool has_dpll4 = false;
+ unsigned dpll_mask;
for consistency this should be unsigned long too.
Lucas De Marchi
if (!icl_calc_dpll_state(crtc_state, encoder, _dpll->hw_state)) {
DRM_DEBUG_KMS("Could not calculate combo PHY PLL state.\n");
@@ -2984,13 +2984,13 @@ static bool icl_get_combo_phy_dpll(struct
intel_atomic_state *state,
}
if (IS_ELKHARTLAKE(dev_priv) && port != PORT_A)
- has_dpll4 = true;
+ dpll_mask = GENMASK(DPLL_ID_EHL_DPLL4, DPLL_ID_ICL_DPLL0);
+ else
+ dpll_mask = GENMASK(DPLL_ID_ICL_DPLL1, DPLL_ID_ICL_DPLL0);
port_dpll->pll = intel_find_shared_dpll(state, crtc,
_dpll->hw_state,
- DPLL_ID_ICL_DPLL0,
- has_dpll4 ? DPLL_ID_EHL_DPLL4
- : DPLL_ID_ICL_DPLL1);
+ dpll_mask);
if (!port_dpll->pll) {
DRM_DEBUG_KMS("No combo PHY PLL found for [ENCODER:%d:%s]\n",
encoder->base.base.id, encoder->base.name);
@@ -3023,8 +3023,7 @@ static bool icl_get_tc_phy_dplls(struct
intel_atomic_state *state,
port_dpll->pll = intel_find_shared_dpll(state, crtc,
_dpll->hw_state,
- DPLL_ID_ICL_TBTPLL,
- DPLL_ID_ICL_TBTPLL);
+ BIT(DPLL_ID_ICL_TBTPLL));
if (!port_dpll->pll) {
DRM_DEBUG_KMS("No TBT-ALT PLL found\n");
return false;
@@ -3043,8 +3042,7 @@ static bool icl_get_tc_phy_dplls(struct
intel_atomic_state *state,
encoder->port));
port_dpll->pll = intel_find_shared_dpll(state, crtc,
_dpll->hw_state,
- dpll_id,
- dpll_id);
+ BIT(dpll_id));
if (!port_dpll->pll) {
DRM_DEBUG_KMS("No MG PHY PLL found\n");
goto err_unreference_tbt_pll;
--
2.21.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Select DPLL's via mask
On Tue, Oct 08, 2019 at 09:12:52AM -0700, Matt Roper wrote:
> This slightly simplifies the EHL DPLL4 handling and also gives us more
> flexibility in the future in case we need to skip the use of specific
> PLL's (e.g., due to hardware workarounds and such).
>
> Cc: Lucas De Marchi
> Signed-off-by: Matt Roper
> ---
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 42 +--
> 1 file changed, 20 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 5e9e84c94a15..14e040658b12 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -247,8 +247,7 @@ static struct intel_shared_dpll *
> intel_find_shared_dpll(struct intel_atomic_state *state,
> const struct intel_crtc *crtc,
> const struct intel_dpll_hw_state *pll_state,
> -enum intel_dpll_id range_min,
> -enum intel_dpll_id range_max)
> +unsigned long dpll_mask)
I don't like seeing lone longs hanging around since they
change meaning between 32bit and 64bit builds. Always makes
me suspicious.
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> struct intel_shared_dpll *pll, *unused_pll = NULL;
> @@ -257,7 +256,9 @@ intel_find_shared_dpll(struct intel_atomic_state *state,
>
> shared_dpll = intel_atomic_get_shared_dpll_state(>base);
>
> - for (i = range_min; i <= range_max; i++) {
> + WARN_ON(dpll_mask & ~(BIT(I915_NUM_PLLS) - 1));
> +
> + for_each_set_bit(i, _mask, I915_NUM_PLLS) {
But I guess this guy demands one :(
> pll = _priv->shared_dplls[i];
>
> /* Only want to check enabled timings first */
> @@ -464,8 +465,8 @@ static bool ibx_get_dpll(struct intel_atomic_state *state,
> } else {
> pll = intel_find_shared_dpll(state, crtc,
>_state->dpll_hw_state,
> - DPLL_ID_PCH_PLL_A,
> - DPLL_ID_PCH_PLL_B);
> + GENMASK(DPLL_ID_PCH_PLL_B,
> + DPLL_ID_PCH_PLL_A));
I wonder if it wouldn't be better to always do BIT(A)|BIT(B)...
so that we won't get bitten if someone ever inserts new DPLL
IDs in the middle of the enum?
Anyways, seems at least as good as the current thing:
Reviewed-by: Ville Syrjälä
> }
>
> if (!pll)
> @@ -814,7 +815,7 @@ hsw_ddi_hdmi_get_dpll(struct intel_atomic_state *state,
>
> pll = intel_find_shared_dpll(state, crtc,
>_state->dpll_hw_state,
> - DPLL_ID_WRPLL1, DPLL_ID_WRPLL2);
> + GENMASK(DPLL_ID_WRPLL2, DPLL_ID_WRPLL1));
>
> if (!pll)
> return NULL;
> @@ -877,7 +878,7 @@ static bool hsw_get_dpll(struct intel_atomic_state *state,
>
> pll = intel_find_shared_dpll(state, crtc,
>_state->dpll_hw_state,
> - DPLL_ID_SPLL, DPLL_ID_SPLL);
> + BIT(DPLL_ID_SPLL));
> } else {
> return false;
> }
> @@ -1447,13 +1448,12 @@ static bool skl_get_dpll(struct intel_atomic_state
> *state,
> if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP))
> pll = intel_find_shared_dpll(state, crtc,
>_state->dpll_hw_state,
> - DPLL_ID_SKL_DPLL0,
> - DPLL_ID_SKL_DPLL0);
> + BIT(DPLL_ID_SKL_DPLL0));
> else
> pll = intel_find_shared_dpll(state, crtc,
>_state->dpll_hw_state,
> - DPLL_ID_SKL_DPLL1,
> - DPLL_ID_SKL_DPLL3);
> + GENMASK(DPLL_ID_SKL_DPLL3,
> + DPLL_ID_SKL_DPLL1));
> if (!pll)
> return false;
>
> @@ -2401,8 +2401,8 @@ static bool cnl_get_dpll(struct intel_atomic_state
> *state,
>
> pll = intel_find_shared_dpll(state, crtc,
>_state->dpll_hw_state,
> - DPLL_ID_SKL_DPLL0,
> - DPLL_ID_SKL_DPLL2);
> + GENMASK(DPLL_ID_SKL_DPLL2,
> + DPLL_ID_SKL_DPLL0));
> if (!pll) {
> DRM_DEBUG_KMS("No PLL selected\n");
> return false;
> @@ -2975,7 +2975,7 @@ static bool icl_get_combo_phy_dpll(struct
> intel_atomic_state *state,
>
Re: [Intel-gfx] [PATCH] drm/i915: Select DPLL's via mask
On Tue, Oct 08, 2019 at 09:12:52AM -0700, Matt Roper wrote:
This slightly simplifies the EHL DPLL4 handling and also gives us more
flexibility in the future in case we need to skip the use of specific
PLL's (e.g., due to hardware workarounds and such).
Cc: Lucas De Marchi
Signed-off-by: Matt Roper
Reviewed-by: Lucas De Marchi
Lucas De Marchi
---
drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 42 +--
1 file changed, 20 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 5e9e84c94a15..14e040658b12 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -247,8 +247,7 @@ static struct intel_shared_dpll *
intel_find_shared_dpll(struct intel_atomic_state *state,
const struct intel_crtc *crtc,
const struct intel_dpll_hw_state *pll_state,
- enum intel_dpll_id range_min,
- enum intel_dpll_id range_max)
+ unsigned long dpll_mask)
{
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
struct intel_shared_dpll *pll, *unused_pll = NULL;
@@ -257,7 +256,9 @@ intel_find_shared_dpll(struct intel_atomic_state *state,
shared_dpll = intel_atomic_get_shared_dpll_state(>base);
- for (i = range_min; i <= range_max; i++) {
+ WARN_ON(dpll_mask & ~(BIT(I915_NUM_PLLS) - 1));
+
+ for_each_set_bit(i, _mask, I915_NUM_PLLS) {
pll = _priv->shared_dplls[i];
/* Only want to check enabled timings first */
@@ -464,8 +465,8 @@ static bool ibx_get_dpll(struct intel_atomic_state *state,
} else {
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_PCH_PLL_A,
-DPLL_ID_PCH_PLL_B);
+GENMASK(DPLL_ID_PCH_PLL_B,
+DPLL_ID_PCH_PLL_A));
}
if (!pll)
@@ -814,7 +815,7 @@ hsw_ddi_hdmi_get_dpll(struct intel_atomic_state *state,
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_WRPLL1, DPLL_ID_WRPLL2);
+GENMASK(DPLL_ID_WRPLL2, DPLL_ID_WRPLL1));
if (!pll)
return NULL;
@@ -877,7 +878,7 @@ static bool hsw_get_dpll(struct intel_atomic_state *state,
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_SPLL, DPLL_ID_SPLL);
+BIT(DPLL_ID_SPLL));
} else {
return false;
}
@@ -1447,13 +1448,12 @@ static bool skl_get_dpll(struct intel_atomic_state
*state,
if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP))
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_SKL_DPLL0,
-DPLL_ID_SKL_DPLL0);
+BIT(DPLL_ID_SKL_DPLL0));
else
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_SKL_DPLL1,
-DPLL_ID_SKL_DPLL3);
+GENMASK(DPLL_ID_SKL_DPLL3,
+ DPLL_ID_SKL_DPLL1));
if (!pll)
return false;
@@ -2401,8 +2401,8 @@ static bool cnl_get_dpll(struct intel_atomic_state *state,
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_SKL_DPLL0,
-DPLL_ID_SKL_DPLL2);
+GENMASK(DPLL_ID_SKL_DPLL2,
+DPLL_ID_SKL_DPLL0));
if (!pll) {
DRM_DEBUG_KMS("No PLL selected\n");
return false;
@@ -2975,7 +2975,7 @@ static bool icl_get_combo_phy_dpll(struct
intel_atomic_state *state,
_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
enum port port = encoder->port;
- bool has_dpll4 = false;
+ unsigned dpll_mask;
if (!icl_calc_dpll_state(crtc_state, encoder, _dpll->hw_state)) {
DRM_DEBUG_KMS("Could not calculate combo PHY PLL state.\n");
@@ -2984,13 +2984,13 @@ static bool icl_get_combo_phy_dpll(struct
[Intel-gfx] [PATCH] drm/i915: Select DPLL's via mask
This slightly simplifies the EHL DPLL4 handling and also gives us more
flexibility in the future in case we need to skip the use of specific
PLL's (e.g., due to hardware workarounds and such).
Cc: Lucas De Marchi
Signed-off-by: Matt Roper
---
drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 42 +--
1 file changed, 20 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 5e9e84c94a15..14e040658b12 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -247,8 +247,7 @@ static struct intel_shared_dpll *
intel_find_shared_dpll(struct intel_atomic_state *state,
const struct intel_crtc *crtc,
const struct intel_dpll_hw_state *pll_state,
- enum intel_dpll_id range_min,
- enum intel_dpll_id range_max)
+ unsigned long dpll_mask)
{
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
struct intel_shared_dpll *pll, *unused_pll = NULL;
@@ -257,7 +256,9 @@ intel_find_shared_dpll(struct intel_atomic_state *state,
shared_dpll = intel_atomic_get_shared_dpll_state(>base);
- for (i = range_min; i <= range_max; i++) {
+ WARN_ON(dpll_mask & ~(BIT(I915_NUM_PLLS) - 1));
+
+ for_each_set_bit(i, _mask, I915_NUM_PLLS) {
pll = _priv->shared_dplls[i];
/* Only want to check enabled timings first */
@@ -464,8 +465,8 @@ static bool ibx_get_dpll(struct intel_atomic_state *state,
} else {
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_PCH_PLL_A,
-DPLL_ID_PCH_PLL_B);
+GENMASK(DPLL_ID_PCH_PLL_B,
+DPLL_ID_PCH_PLL_A));
}
if (!pll)
@@ -814,7 +815,7 @@ hsw_ddi_hdmi_get_dpll(struct intel_atomic_state *state,
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_WRPLL1, DPLL_ID_WRPLL2);
+GENMASK(DPLL_ID_WRPLL2, DPLL_ID_WRPLL1));
if (!pll)
return NULL;
@@ -877,7 +878,7 @@ static bool hsw_get_dpll(struct intel_atomic_state *state,
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_SPLL, DPLL_ID_SPLL);
+BIT(DPLL_ID_SPLL));
} else {
return false;
}
@@ -1447,13 +1448,12 @@ static bool skl_get_dpll(struct intel_atomic_state
*state,
if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP))
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_SKL_DPLL0,
-DPLL_ID_SKL_DPLL0);
+BIT(DPLL_ID_SKL_DPLL0));
else
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_SKL_DPLL1,
-DPLL_ID_SKL_DPLL3);
+GENMASK(DPLL_ID_SKL_DPLL3,
+ DPLL_ID_SKL_DPLL1));
if (!pll)
return false;
@@ -2401,8 +2401,8 @@ static bool cnl_get_dpll(struct intel_atomic_state *state,
pll = intel_find_shared_dpll(state, crtc,
_state->dpll_hw_state,
-DPLL_ID_SKL_DPLL0,
-DPLL_ID_SKL_DPLL2);
+GENMASK(DPLL_ID_SKL_DPLL2,
+DPLL_ID_SKL_DPLL0));
if (!pll) {
DRM_DEBUG_KMS("No PLL selected\n");
return false;
@@ -2975,7 +2975,7 @@ static bool icl_get_combo_phy_dpll(struct
intel_atomic_state *state,
_state->icl_port_dplls[ICL_PORT_DPLL_DEFAULT];
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
enum port port = encoder->port;
- bool has_dpll4 = false;
+ unsigned dpll_mask;
if (!icl_calc_dpll_state(crtc_state, encoder, _dpll->hw_state)) {
DRM_DEBUG_KMS("Could not calculate combo PHY PLL state.\n");
@@ -2984,13 +2984,13 @@ static bool icl_get_combo_phy_dpll(struct
intel_atomic_state *state,
}
if (IS_ELKHARTLAKE(dev_priv) && port != PORT_A)
-
