Re: [Intel-gfx] [PATCH 3/4] drm/i915: Stop requiring PLL index == PLL ID
On Thu, Sep 22, 2022 at 06:46:30PM +0300, Jani Nikula wrote:
> On Wed, 21 Sep 2022, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > There's no good reason to keep around this PLL index == PLL ID
> > footgun. Get rid of it.
> >
> > Both i915->shared_dplls[] and state->shared_dpll[] are indexed
> > by the same thing now, which is just the index we get at
> > initialization from dpll_mgr->dpll_info[]. The rest is all about
> > PLL IDs now.
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 64 +--
> > .../gpu/drm/i915/display/intel_pch_refclk.c | 5 +-
> > 2 files changed, 47 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 f900c4c73cc6..fb09029cc4aa 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -110,7 +110,7 @@ static void
> > intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
> > struct intel_shared_dpll_state *shared_dpll)
> > {
> > - enum intel_dpll_id i;
> > + int i;
> >
> > /* Copy shared dpll state */
> > for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> > @@ -137,6 +137,13 @@ intel_atomic_get_shared_dpll_state(struct
> > drm_atomic_state *s)
> > return state->shared_dpll;
> > }
> >
> > +static int
> > +intel_shared_dpll_idx(struct drm_i915_private *i915,
> > + const struct intel_shared_dpll *pll)
> > +{
> > + return pll - >display.dpll.shared_dplls[0];
> > +}
>
> I liked getting rid of this magic in the previous patch, and would not
> like to have it brought back!
>
> I'm thinking
>
> static int
> intel_shared_dpll_idx(struct drm_i915_private *i915, enum intel_dpll_id id)
>
> which would loop over shared_dplls[] and return the index, similar to
> the function below. Feels more robust.
Dunno if it's more robust, but I guess it does decouple us
a bit more from the array storage of the actual plls. We
could even do this exactly like eg. drm_crtc, ie. introduce
pll->index for the atomic state indexing but could use a
linked list to keep the actual plls. Though that would
again mean more kmallocs() that can fail, so I don't think
I'll go quite that far, at least not yet. Though I suppose
I could introduce pll->index already...
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Stop requiring PLL index == PLL ID
On Wed, 21 Sep 2022, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> There's no good reason to keep around this PLL index == PLL ID
> footgun. Get rid of it.
>
> Both i915->shared_dplls[] and state->shared_dpll[] are indexed
> by the same thing now, which is just the index we get at
> initialization from dpll_mgr->dpll_info[]. The rest is all about
> PLL IDs now.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 64 +--
> .../gpu/drm/i915/display/intel_pch_refclk.c | 5 +-
> 2 files changed, 47 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 f900c4c73cc6..fb09029cc4aa 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -110,7 +110,7 @@ static void
> intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
> struct intel_shared_dpll_state *shared_dpll)
> {
> - enum intel_dpll_id i;
> + int i;
>
> /* Copy shared dpll state */
> for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> @@ -137,6 +137,13 @@ intel_atomic_get_shared_dpll_state(struct
> drm_atomic_state *s)
> return state->shared_dpll;
> }
>
> +static int
> +intel_shared_dpll_idx(struct drm_i915_private *i915,
> + const struct intel_shared_dpll *pll)
> +{
> + return pll - >display.dpll.shared_dplls[0];
> +}
I liked getting rid of this magic in the previous patch, and would not
like to have it brought back!
I'm thinking
static int
intel_shared_dpll_idx(struct drm_i915_private *i915, enum intel_dpll_id id)
which would loop over shared_dplls[] and return the index, similar to
the function below. Feels more robust.
Otherwise LGTM.
BR,
Jani.
> +
> /**
> * intel_get_shared_dpll_by_id - get a DPLL given its id
> * @dev_priv: i915 device instance
> @@ -149,7 +156,17 @@ struct intel_shared_dpll *
> intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
> enum intel_dpll_id id)
> {
> - return _priv->display.dpll.shared_dplls[id];
> + int i;
> +
> + for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> + struct intel_shared_dpll *pll =
> _priv->display.dpll.shared_dplls[i];
> +
> + if (pll->info->id == id)
> + return pll;
> + }
> +
> + MISSING_CASE(id);
> + return NULL;
> }
>
> /* For ILK+ */
> @@ -305,16 +322,23 @@ intel_find_shared_dpll(struct intel_atomic_state *state,
> unsigned long dpll_mask)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - struct intel_shared_dpll *pll, *unused_pll = NULL;
> struct intel_shared_dpll_state *shared_dpll;
> - enum intel_dpll_id i;
> + struct intel_shared_dpll *unused_pll = NULL;
> + enum intel_dpll_id id;
>
> shared_dpll = intel_atomic_get_shared_dpll_state(>base);
>
> drm_WARN_ON(_priv->drm, dpll_mask & ~(BIT(I915_NUM_PLLS) - 1));
>
> - for_each_set_bit(i, _mask, I915_NUM_PLLS) {
> - pll = _priv->display.dpll.shared_dplls[i];
> + for_each_set_bit(id, _mask, I915_NUM_PLLS) {
> + struct intel_shared_dpll *pll;
> + int i;
> +
> + pll = intel_get_shared_dpll_by_id(dev_priv, id);
> + if (!pll)
> + continue;
> +
> + i = intel_shared_dpll_idx(dev_priv, pll);
>
> /* Only want to check enabled timings first */
> if (shared_dpll[i].pipe_mask == 0) {
> @@ -355,27 +379,29 @@ intel_reference_shared_dpll(struct intel_atomic_state
> *state,
> {
> struct drm_i915_private *i915 = to_i915(state->base.dev);
> struct intel_shared_dpll_state *shared_dpll;
> - const enum intel_dpll_id id = pll->info->id;
> + int i = intel_shared_dpll_idx(i915, pll);
>
> shared_dpll = intel_atomic_get_shared_dpll_state(>base);
>
> - if (shared_dpll[id].pipe_mask == 0)
> - shared_dpll[id].hw_state = *pll_state;
> + if (shared_dpll[i].pipe_mask == 0)
> + shared_dpll[i].hw_state = *pll_state;
>
> drm_dbg(>drm, "using %s for pipe %c\n", pll->info->name,
> pipe_name(crtc->pipe));
>
> - shared_dpll[id].pipe_mask |= BIT(crtc->pipe);
> + shared_dpll[i].pipe_mask |= BIT(crtc->pipe);
> }
>
> static void intel_unreference_shared_dpll(struct intel_atomic_state *state,
> const struct intel_crtc *crtc,
> const struct intel_shared_dpll *pll)
> {
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> struct intel_shared_dpll_state *shared_dpll;
> + int i = intel_shared_dpll_idx(i915, pll);
>
> shared_dpll = intel_atomic_get_shared_dpll_state(>base);
> -
[Intel-gfx] [PATCH 3/4] drm/i915: Stop requiring PLL index == PLL ID
From: Ville Syrjälä
There's no good reason to keep around this PLL index == PLL ID
footgun. Get rid of it.
Both i915->shared_dplls[] and state->shared_dpll[] are indexed
by the same thing now, which is just the index we get at
initialization from dpll_mgr->dpll_info[]. The rest is all about
PLL IDs now.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 64 +--
.../gpu/drm/i915/display/intel_pch_refclk.c | 5 +-
2 files changed, 47 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 f900c4c73cc6..fb09029cc4aa 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -110,7 +110,7 @@ static void
intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
struct intel_shared_dpll_state *shared_dpll)
{
- enum intel_dpll_id i;
+ int i;
/* Copy shared dpll state */
for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
@@ -137,6 +137,13 @@ intel_atomic_get_shared_dpll_state(struct drm_atomic_state
*s)
return state->shared_dpll;
}
+static int
+intel_shared_dpll_idx(struct drm_i915_private *i915,
+ const struct intel_shared_dpll *pll)
+{
+ return pll - >display.dpll.shared_dplls[0];
+}
+
/**
* intel_get_shared_dpll_by_id - get a DPLL given its id
* @dev_priv: i915 device instance
@@ -149,7 +156,17 @@ struct intel_shared_dpll *
intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
enum intel_dpll_id id)
{
- return _priv->display.dpll.shared_dplls[id];
+ int i;
+
+ for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
+ struct intel_shared_dpll *pll =
_priv->display.dpll.shared_dplls[i];
+
+ if (pll->info->id == id)
+ return pll;
+ }
+
+ MISSING_CASE(id);
+ return NULL;
}
/* For ILK+ */
@@ -305,16 +322,23 @@ intel_find_shared_dpll(struct intel_atomic_state *state,
unsigned long dpll_mask)
{
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
- struct intel_shared_dpll *pll, *unused_pll = NULL;
struct intel_shared_dpll_state *shared_dpll;
- enum intel_dpll_id i;
+ struct intel_shared_dpll *unused_pll = NULL;
+ enum intel_dpll_id id;
shared_dpll = intel_atomic_get_shared_dpll_state(>base);
drm_WARN_ON(_priv->drm, dpll_mask & ~(BIT(I915_NUM_PLLS) - 1));
- for_each_set_bit(i, _mask, I915_NUM_PLLS) {
- pll = _priv->display.dpll.shared_dplls[i];
+ for_each_set_bit(id, _mask, I915_NUM_PLLS) {
+ struct intel_shared_dpll *pll;
+ int i;
+
+ pll = intel_get_shared_dpll_by_id(dev_priv, id);
+ if (!pll)
+ continue;
+
+ i = intel_shared_dpll_idx(dev_priv, pll);
/* Only want to check enabled timings first */
if (shared_dpll[i].pipe_mask == 0) {
@@ -355,27 +379,29 @@ intel_reference_shared_dpll(struct intel_atomic_state
*state,
{
struct drm_i915_private *i915 = to_i915(state->base.dev);
struct intel_shared_dpll_state *shared_dpll;
- const enum intel_dpll_id id = pll->info->id;
+ int i = intel_shared_dpll_idx(i915, pll);
shared_dpll = intel_atomic_get_shared_dpll_state(>base);
- if (shared_dpll[id].pipe_mask == 0)
- shared_dpll[id].hw_state = *pll_state;
+ if (shared_dpll[i].pipe_mask == 0)
+ shared_dpll[i].hw_state = *pll_state;
drm_dbg(>drm, "using %s for pipe %c\n", pll->info->name,
pipe_name(crtc->pipe));
- shared_dpll[id].pipe_mask |= BIT(crtc->pipe);
+ shared_dpll[i].pipe_mask |= BIT(crtc->pipe);
}
static void intel_unreference_shared_dpll(struct intel_atomic_state *state,
const struct intel_crtc *crtc,
const struct intel_shared_dpll *pll)
{
+ struct drm_i915_private *i915 = to_i915(state->base.dev);
struct intel_shared_dpll_state *shared_dpll;
+ int i = intel_shared_dpll_idx(i915, pll);
shared_dpll = intel_atomic_get_shared_dpll_state(>base);
- shared_dpll[pll->info->id].pipe_mask &= ~BIT(crtc->pipe);
+ shared_dpll[i].pipe_mask &= ~BIT(crtc->pipe);
}
static void intel_put_dpll(struct intel_atomic_state *state,
@@ -409,14 +435,13 @@ void intel_shared_dpll_swap_state(struct
intel_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
struct intel_shared_dpll_state *shared_dpll = state->shared_dpll;
- enum intel_dpll_id i;
+ int i;
if (!state->dpll_set)
return;
for (i = 0; i <
