Re: [PATCH 62/89] drm/i915/skl: Query DPLL attached to port on SKL

M, Satheeshakrishna
2014-10-01T10:52:01Z

On 9/23/2014 1:54 AM, Paulo Zanoni wrote:
> 2014-09-04 8:27 GMT-03:00 Damien Lespiau<damien.lespiau@intel.com>:
>> From: Satheeshakrishna M<satheeshakrishna.m@intel.com>
>>
>> Modify the implementation to query DPLL attached to a SKL port.
>>
>> v2: Rebase on top of the run-time PM on DPMS series (Damien)
>>
>> Signed-off-by: Satheeshakrishna M<satheeshakrishna.m@intel.com>
>> Signed-off-by: Damien Lespiau<damien.lespiau@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++++++++++++-
>> drivers/gpu/drm/i915/intel_drv.h | 5 ++++-
>> 2 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 69e023a..6e71250 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -7789,6 +7789,28 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>> return 0;
>> }
>>
>> +static void skylake_get_ddi_pll(struct drm_i915_private *dev_priv,
>> + enum port port,
>> + struct intel_crtc_config *pipe_config)
>> +{
>> + u32 temp;
>> +
>> + temp = I915_READ(DPLL_CTRL2) & DPLL_CTRL2_DDI_CLK_SEL_MASK(port);
>> + pipe_config->ddi_pll_sel = temp >> (port * 3 + 1);
> Since we're relying on the fact that ddi_pll_sel is actually "enum
> skl_dpll", I'd change the definition of the enum and explicitly
> declare SKL_DPLL0 to be "0", and SKL_DPLL1 to be 1, and so on.
Makes sense
>> +
>> + switch (pipe_config->ddi_pll_sel) {
>> + case SKL_DPLL1:
>> + pipe_config->shared_dpll = DPLL_ID_SKL_DPLL1;
>> + break;
>> + case SKL_DPLL2:
>> + pipe_config->shared_dpll = DPLL_ID_SKL_DPLL2;
>> + break;
>> + case SKL_DPLL3:
>> + pipe_config->shared_dpll = DPLL_ID_SKL_DPLL3;
>> + break;
> Please also add a WARN() to the default case, especially since there
> are 4 possible values and we're just checking 3.
ok
>> + }
>> +}
>> +
>> static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
>> enum port port,
>> struct intel_crtc_config *pipe_config)
>> @@ -7818,7 +7840,10 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>>
>> port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT;
>>
>> - haswell_get_ddi_pll(dev_priv, port, pipe_config);
>> + if (IS_SKYLAKE(dev))
> If you invert the check so that SKL is on the "else" part (as you did
> in your previous patch), you'll make sure we're ready for the next
> Gens in case they happen to be same as SKL. It's more likely they will
> be the same as SKL instead of being the same as HSW :)
>
>
> With at least the WARN added on the switch statement: Reviewed-by:
> Paulo Zanoni<paulo.r.zanoni@intel.com>
>
>> + skylake_get_ddi_pll(dev_priv, port, pipe_config);
>> + else
>> + haswell_get_ddi_pll(dev_priv, port, pipe_config);
>>
>> if (pipe_config->shared_dpll >= 0) {
>> pll = &dev_priv->shared_dplls[pipe_config->shared_dpll];
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 559b747..9558f07 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -323,7 +323,10 @@ struct intel_crtc_config {
>> /* Selected dpll when shared or DPLL_ID_PRIVATE. */
>> enum intel_dpll_id shared_dpll;
>>
>> - /* PORT_CLK_SEL for DDI ports. */
>> + /*
>> + * - PORT_CLK_SEL for DDI ports on HSW/BDW.
>> + * - enum skl_dpll on SKL
>> + */
>> uint32_t ddi_pll_sel;
>>
>> /* Actual register state of the dpll, for shared dpll cross-checking. */
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>