Re: [PATCH 8/8] drm/i915: try to use WR PLL 2

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Paulo Zanoni <przanoni@gmail.com>, intel-gfx@lists.freedesktop.org
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 8/8] drm/i915: try to use WR PLL 2
Date: Thu, 09 Aug 2012 14:32:30 +0300	[thread overview]
Message-ID: <87ipcse1wx.fsf@intel.com> (raw)
In-Reply-To: <1344446134-3704-9-git-send-email-przanoni@gmail.com>

On Wed, 08 Aug 2012, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> The current situation is: we use WR PLL1 for everything, so if we have
> 2 HDMI outputs they will share the same PLL. As a consequence, when
> you set a mode on HDMI2, HDMI1 will change its refresh rate. If the
> modes are too different, setting a mode on HDMI2 may make the HDMI1
> screen blank (with one of those error messages from your monitor).
>
> So now we at least try to use WR PLL 2. This will improve the case
> where we have 2 HDMI outputs and don't keep crazily changing ports,
> but it's still not the perfect solution:
>
> - We need to select PORT_CLK_SEL_NONE when we stop using a port, so we
>   will be able to reuse its PLL.
> - We need to move the whole DDI PLL selection code to a place where we
>   can properly fail and return an error code, possibly undoing the
>   mode set. Right now, instead of failing we're hijacking WR PLL 2.
>
> This patch is not the perfect solution, but it's at least better than
> the current one. Future patches will fix the remaining problems.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 41 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index db242cf..80b9902 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -659,7 +659,10 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>  	int port = intel_hdmi->ddi_port;
>  	int pipe = intel_crtc->pipe;
> -	int p, n2, r2;
> +	uint32_t pll_reg[] = {WRPLL_CTL1, WRPLL_CTL2};
> +	uint32_t pll_sel[] = {PORT_CLK_SEL_WRPLL1, PORT_CLK_SEL_WRPLL2};
> +	int p, n2, r2, pll;
> +	bool used;
>  	u32 temp, i;
>  
>  	/* On Haswell, we need to enable the clocks and prepare DDI function to
> @@ -667,6 +670,35 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
>  	 */
>  	DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
>  
> +	for (pll = 0; pll < 2; pll++) {

Personally I'd prefer to use ARRAY_SIZE() even for small arrays like
this. YMMV.

> +		used = false;
> +		for (i = PORT_A; i <= PORT_E; i++) {
> +			if (i == port)
> +				continue;
> +
> +			if (I915_READ(PORT_CLK_SEL(i)) == pll_sel[pll]) {
> +				used = true;
> +				break;
> +			}
> +		}
> +		if (!used)
> +			break;
> +	}

I wonder if the logic (esp. the use of the "used" variable) could be
simplified by putting the inner for loop into a function. But this is
just bikeshedding, really.

> +	if (pll == 2) {
> +		/* FIXME: just claiming failure and returning from this function
> +		 * won't help us at all. So instead we hijack WR PLL 2 and hope
> +		 * we don't break the other output (if the refresh rates are
> +		 * similar we may survive). We should definitely move the PLL
> +		 * code to somewhere else, where we may be able to properly
> +		 * fail. Also, we should write code to select PORT_CLK_SEL_NONE
> +		 * when we stop using the port, so other ports will be able to
> +		 * reuse the WR PLL. */
> +		DRM_ERROR("No WR PLL available\n");
> +		pll = 1;
> +	}
> +
> +	DRM_DEBUG_KMS("Using WR PLL %d\n", pll+1);
> +
>  	for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
>  		if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
>  			break;
> @@ -688,9 +720,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
>  		I915_WRITE(LCPLL_CTL,
>  				temp & ~LCPLL_PLL_DISABLE);
>  
> -	/* Configure WR PLL 1, program the correct divider values for
> -	 * the desired frequency and wait for warmup */
> -	I915_WRITE(WRPLL_CTL1,
> +	I915_WRITE(pll_reg[pll],
>  			WRPLL_PLL_ENABLE |
>  			WRPLL_PLL_SELECT_LCPLL_2700 |
>  			WRPLL_DIVIDER_REFERENCE(r2) |
> @@ -702,8 +732,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
>  	/* Use WRPLL1 clock to drive the output to the port, and tell the pipe to use
>  	 * this port for connection.
>  	 */
> -	I915_WRITE(PORT_CLK_SEL(port),
> -			PORT_CLK_SEL_WRPLL1);
> +	I915_WRITE(PORT_CLK_SEL(port), pll_sel[pll]);
>  	I915_WRITE(PIPE_CLK_SEL(pipe),
>  			PIPE_CLK_SEL_PORT(port));
>  
> -- 
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx