2013/12/12 Damien Lespiau <damien.lespiau@xxxxxxxxx>:
> On Mon, Nov 25, 2013 at 03:27:08PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> The first piece, intel_ddi_pll_select, finds a PLL and assigns it to
>> the CRTC, but doesn't write any register. It can also fail in case it
>> doesn't find a PLL.
>>
>> The second piece, intel_ddi_pll_enable, uses the information stored by
>> intel_ddi_pll_select to actually enable the PLL by writing to its
>> register. This function can't fail. We also have some refcount sanity
>> checks here.
>>
>> The idea is that one day we'll remove all the functions that touch
>> registers from haswell_crtc_mode_set to haswell_crtc_enable, so we'll
>> call intel_ddi_pll_select at haswell_crtc_mode_set and then call
>> intel_ddi_pll_enable at haswell_crtc_enable. Since I'm already
>> touching this code, let's take care of this particular split today.
>>
>> v2: - Clock on the debug message is in KHz
>> - Add missing POSTING_READ
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
>
> Reviewed-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
>
> /*
> * As a side note, CodingStyle stipulates this is the preferred style
> * for multi-line comments. Really a small detail to keep in my in the
> * future :)
> */
I know, but our internal documentation says that "Multi-line comment
delimiters (i.e. /* and */) do not need to be on empty lines.", so I
was following this recommendation.
Thanks for the review!
>
> --
> Damien
>
>> ---
>> drivers/gpu/drm/i915/intel_ddi.c | 102 ++++++++++++++++++++++++++++-------
>> drivers/gpu/drm/i915/intel_display.c | 3 +-
>> drivers/gpu/drm/i915/intel_drv.h | 3 +-
>> 3 files changed, 88 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 731a919..4263ec3 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -696,21 +696,21 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */,
>> *n2_out = best.n2;
>> *p_out = best.p;
>> *r2_out = best.r2;
>> -
>> - DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d r2=%d\n",
>> - clock, *p_out, *n2_out, *r2_out);
>> }
>>
>> -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
>> +/* Tries to find a PLL for the CRTC. If it finds, it increases the refcount and
>> + * stores it in intel_crtc->ddi_pll_sel, so other mode sets won't be able to
>> + * steal the selected PLL. You need to call intel_ddi_pll_enable to actually
>> + * enable the PLL. */
>> +bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
>> {
>> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> + struct drm_crtc *crtc = &intel_crtc->base;
>> struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>> struct drm_encoder *encoder = &intel_encoder->base;
>> struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>> struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
>> int type = intel_encoder->type;
>> enum pipe pipe = intel_crtc->pipe;
>> - uint32_t reg, val;
>> int clock = intel_crtc->config.port_clock;
>>
>> intel_ddi_put_crtc_pll(crtc);
>> @@ -734,10 +734,8 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
>> return false;
>> }
>>
>> - /* We don't need to turn any PLL on because we'll use LCPLL. */
>> - return true;
>> -
>> } else if (type == INTEL_OUTPUT_HDMI) {
>> + uint32_t reg, val;
>> unsigned p, n2, r2;
>>
>> intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p);
>> @@ -767,6 +765,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
>> return false;
>> }
>>
>> + DRM_DEBUG_KMS("WRPLL: %dKHz refresh rate with p=%d, n2=%d r2=%d\n",
>> + clock, p, n2, r2);
>> +
>> if (reg == WRPLL_CTL1) {
>> plls->wrpll1_refcount++;
>> intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1;
>> @@ -780,29 +781,94 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
>> DRM_DEBUG_KMS("Using SPLL on pipe %c\n",
>> pipe_name(pipe));
>> plls->spll_refcount++;
>> - reg = SPLL_CTL;
>> intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>> } else {
>> DRM_ERROR("SPLL already in use\n");
>> return false;
>> }
>>
>> - WARN(I915_READ(reg) & SPLL_PLL_ENABLE,
>> - "SPLL already enabled\n");
>> -
>> - val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC;
>> -
>> } else {
>> WARN(1, "Invalid DDI encoder type %d\n", type);
>> return false;
>> }
>>
>> - I915_WRITE(reg, val);
>> - udelay(20);
>> -
>> return true;
>> }
>>
>> +/* To be called after intel_ddi_pll_select(). That one selects the PLL to be
>> + * used, this one actually enables the PLL. */
>> +void intel_ddi_pll_enable(struct intel_crtc *crtc)
>> +{
>> + struct drm_device *dev = crtc->base.dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
>> + int clock = crtc->config.port_clock;
>> + uint32_t reg, cur_val, new_val;
>> + int refcount;
>> + const char *pll_name;
>> + uint32_t enable_bit = (1 << 31);
>> + unsigned int p, n2, r2;
>> +
>> + BUILD_BUG_ON(enable_bit != SPLL_PLL_ENABLE);
>> + BUILD_BUG_ON(enable_bit != WRPLL_PLL_ENABLE);
>> +
>> + switch (crtc->ddi_pll_sel) {
>> + case PORT_CLK_SEL_LCPLL_2700:
>> + case PORT_CLK_SEL_LCPLL_1350:
>> + case PORT_CLK_SEL_LCPLL_810:
>> + /* LCPLL should always be enabled at this point of the mode set
>> + * sequence, so nothing to do. */
>> + return;
>> +
>> + case PORT_CLK_SEL_SPLL:
>> + pll_name = "SPLL";
>> + reg = SPLL_CTL;
>> + refcount = plls->spll_refcount;
>> + new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz |
>> + SPLL_PLL_SSC;
>> + break;
>> +
>> + case PORT_CLK_SEL_WRPLL1:
>> + case PORT_CLK_SEL_WRPLL2:
>> + if (crtc->ddi_pll_sel == PORT_CLK_SEL_WRPLL1) {
>> + pll_name = "WRPLL1";
>> + reg = WRPLL_CTL1;
>> + refcount = plls->wrpll1_refcount;
>> + } else {
>> + pll_name = "WRPLL2";
>> + reg = WRPLL_CTL2;
>> + refcount = plls->wrpll2_refcount;
>> + }
>> +
>> + intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p);
>> +
>> + new_val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 |
>> + WRPLL_DIVIDER_REFERENCE(r2) |
>> + WRPLL_DIVIDER_FEEDBACK(n2) | WRPLL_DIVIDER_POST(p);
>> +
>> + break;
>> +
>> + case PORT_CLK_SEL_NONE:
>> + WARN(1, "Bad selected pll: PORT_CLK_SEL_NONE\n");
>> + return;
>> + default:
>> + WARN(1, "Bad selected pll: 0x%08x\n", crtc->ddi_pll_sel);
>> + return;
>> + }
>> +
>> + cur_val = I915_READ(reg);
>> +
>> + WARN(refcount < 1, "Bad %s refcount: %d\n", pll_name, refcount);
>> + if (refcount == 1) {
>> + WARN(cur_val & enable_bit, "%s already enabled\n", pll_name);
>> + I915_WRITE(reg, new_val);
>> + POSTING_READ(reg);
>> + udelay(20);
>> + } else {
>> + WARN((cur_val & enable_bit) == 0, "%s disabled\n", pll_name);
>> + }
>> +}
>> +
>> void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
>> {
>> struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e85d838..667f641 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6880,8 +6880,9 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>> int plane = intel_crtc->plane;
>> int ret;
>>
>> - if (!intel_ddi_pll_mode_set(crtc))
>> + if (!intel_ddi_pll_select(intel_crtc))
>> return -EINVAL;
>> + intel_ddi_pll_enable(intel_crtc);
>>
>> if (intel_crtc->config.has_dp_encoder)
>> intel_dp_set_m_n(intel_crtc);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 0231281..76f52b6 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -612,7 +612,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
>> void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
>> void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
>> void intel_ddi_setup_hw_pll_state(struct drm_device *dev);
>> -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc);
>> +bool intel_ddi_pll_select(struct intel_crtc *crtc);
>> +void intel_ddi_pll_enable(struct intel_crtc *crtc);
>> void intel_ddi_put_crtc_pll(struct drm_crtc *crtc);
>> void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
>> void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
