| Message ID | 1490209125-20046-2-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 22, 2017 at 03:58:45PM -0300, Paulo Zanoni wrote: > All it does is pick the encoder and call intel_get_shared_dpll(). We > can just do this in the caller. One less indirection level during code > reading. > > As another plus, now the two callers of intel_get_shared_dpll() are > {ironlake,haswell}_crtc_compute_clock(). > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 25 ++----------------------- > drivers/gpu/drm/i915/intel_display.c | 8 +++++++- > drivers/gpu/drm/i915/intel_drv.h | 4 ++-- > 3 files changed, 11 insertions(+), 26 deletions(-) > > > To be honest I'm not super sure if this change is an improvement. I like keeping > DDI-related code in intel_ddi.c. I'll let you guys decide with the > upvote/downvote buttons in your email clients. I guess the thing that sort of ruins is the intel_ddi_get_crtc_new_encoder() call. First I thought we could nuke that by using output_types instead of encoder->type, but looks like BXT wants to disagree with us and really requires the encoder in .get_dpll() :( Oh well, we trade one ddi thing for another. Although intel_ddi_get_crtc_new_encoder() doesn't really have anything truly ddi specific in it so we could even move it elsewhere or even just inline it in .compute_clock() since it's not even called from anywhere else. I do like the fact that there's at least one less hoop to jump through when tracing through the code. So this works for me. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 08a810d..3fbe498 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -837,7 +837,8 @@ intel_ddi_get_crtc_encoder(struct intel_crtc *crtc) > return ret; > } > > -static struct intel_encoder * > +/* Finds the only possible encoder associated with the given CRTC. */ > +struct intel_encoder * > intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > @@ -1127,28 +1128,6 @@ void intel_ddi_clock_get(struct intel_encoder *encoder, > bxt_ddi_clock_get(encoder, pipe_config); > } > > -/* > - * Tries to find a *shared* PLL for the CRTC and store it in > - * intel_crtc->ddi_pll_sel. > - * > - * For private DPLLs, compute_config() should do the selection for us. This > - * function should be folded into compute_config() eventually. > - */ > -bool intel_ddi_pll_select(struct intel_crtc *intel_crtc, > - struct intel_crtc_state *crtc_state) > -{ > - struct intel_encoder *encoder = > - intel_ddi_get_crtc_new_encoder(crtc_state); > - struct intel_shared_dpll *pll; > - > - pll = intel_get_shared_dpll(intel_crtc, crtc_state, encoder); > - if (!pll) > - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", > - pipe_name(intel_crtc->pipe)); > - > - return pll != NULL; > -} > - > void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 010e5dd..9cdf9b2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8862,8 +8862,14 @@ static int haswell_crtc_compute_clock(struct intel_crtc *crtc, > struct intel_crtc_state *crtc_state) > { > if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) { > - if (!intel_ddi_pll_select(crtc, crtc_state)) > + struct intel_encoder *encoder = > + intel_ddi_get_crtc_new_encoder(crtc_state); > + > + if (!intel_get_shared_dpll(crtc, crtc_state, encoder)) { > + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", > + pipe_name(crtc->pipe)); > return -EINVAL; > + } > } > > crtc->lowfreq_avail = false; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 51228fe..ad7bdf5 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1236,8 +1236,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv, > enum transcoder cpu_transcoder); > void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state); > void intel_ddi_disable_pipe_clock(const struct intel_crtc_state *crtc_state); > -bool intel_ddi_pll_select(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state); > +struct intel_encoder * > +intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state); > void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state); > void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp); > bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector); > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 08a810d..3fbe498 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -837,7 +837,8 @@ intel_ddi_get_crtc_encoder(struct intel_crtc *crtc) return ret; } -static struct intel_encoder * +/* Finds the only possible encoder associated with the given CRTC. */ +struct intel_encoder * intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); @@ -1127,28 +1128,6 @@ void intel_ddi_clock_get(struct intel_encoder *encoder, bxt_ddi_clock_get(encoder, pipe_config); } -/* - * Tries to find a *shared* PLL for the CRTC and store it in - * intel_crtc->ddi_pll_sel. - * - * For private DPLLs, compute_config() should do the selection for us. This - * function should be folded into compute_config() eventually. - */ -bool intel_ddi_pll_select(struct intel_crtc *intel_crtc, - struct intel_crtc_state *crtc_state) -{ - struct intel_encoder *encoder = - intel_ddi_get_crtc_new_encoder(crtc_state); - struct intel_shared_dpll *pll; - - pll = intel_get_shared_dpll(intel_crtc, crtc_state, encoder); - if (!pll) - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", - pipe_name(intel_crtc->pipe)); - - return pll != NULL; -} - void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 010e5dd..9cdf9b2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8862,8 +8862,14 @@ static int haswell_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) { - if (!intel_ddi_pll_select(crtc, crtc_state)) + struct intel_encoder *encoder = + intel_ddi_get_crtc_new_encoder(crtc_state); + + if (!intel_get_shared_dpll(crtc, crtc_state, encoder)) { + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", + pipe_name(crtc->pipe)); return -EINVAL; + } } crtc->lowfreq_avail = false; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 51228fe..ad7bdf5 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1236,8 +1236,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv, enum transcoder cpu_transcoder); void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state); void intel_ddi_disable_pipe_clock(const struct intel_crtc_state *crtc_state); -bool intel_ddi_pll_select(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state); +struct intel_encoder * +intel_ddi_get_crtc_new_encoder(struct intel_crtc_state *crtc_state); void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state); void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp); bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
All it does is pick the encoder and call intel_get_shared_dpll(). We can just do this in the caller. One less indirection level during code reading. As another plus, now the two callers of intel_get_shared_dpll() are {ironlake,haswell}_crtc_compute_clock(). Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 25 ++----------------------- drivers/gpu/drm/i915/intel_display.c | 8 +++++++- drivers/gpu/drm/i915/intel_drv.h | 4 ++-- 3 files changed, 11 insertions(+), 26 deletions(-) To be honest I'm not super sure if this change is an improvement. I like keeping DDI-related code in intel_ddi.c. I'll let you guys decide with the upvote/downvote buttons in your email clients.