[Intel-gfx] [PATCH 28/67] drm/i915/cnl: Implement .get_display_clock_speed() for CNL

[Ville Syrjälä]

On Wed, Jun 07, 2017 at 01:59:05PM +0300, Ville Syrjälä wrote:
> On Tue, Jun 06, 2017 at 02:56:23PM -0700, Rodrigo Vivi wrote:
> > When addressing Imre's comments I noticed:
> > 
> > error: ‘cnl_set_cdclk’ defined but not used [-Werror=unused-function]
> > static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> >              ^
> > cc1: all warnings being treated as errors
> > 
> > Ville, since the original is yours I'd like your advise on how to proceed.
> > 1. squash both
> > 2. swap the patches and create a temporary emply cnl_set_cdclk
> > 3. ?
> 
> I would just ignore that since it gets fixed in the following patches.
> I know adding unused stuff isn't really looked upon favorably but 
> squashing it with the display init patch would make that patch less
> focused and quite large. I guess you could squash it with the
> dynamic cdclk change patch, but then you'd have to reorder the display
> init patch to be after that and that order doesn't make as much sense
> since you would then be adding optional extra functionality before the
> basic functionality in in place.

If you really want to avoid the warning then I guess adding
__attribute__((unused)) and subsequently removing it would do the trick.

> 
> > 
> > Thanks,
> > Rodrigo.
> > 
> > On Mon, Jun 5, 2017 at 1:07 PM, Imre Deak <imre.deak at intel.com> wrote:
> > > On Mon, Jun 05, 2017 at 09:28:52PM +0300, Vivi, Rodrigo wrote:
> > >> On Mon, 2017-06-05 at 21:21 +0300, Imre Deak wrote:
> > >> > On Mon, Jun 05, 2017 at 09:04:26PM +0300, Ville Syrjälä wrote:
> > >> > > On Mon, Jun 05, 2017 at 05:59:02PM +0000, Vivi, Rodrigo wrote:
> > >> > > > On Fri, 2017-06-02 at 21:06 +0300, Imre Deak wrote:
> > >> > > > > > [...]
> > >> > > > > > +static void cnl_cdclk_pll_update(struct drm_i915_private *dev_priv,
> > >> > > > > > +                            struct intel_cdclk_state *cdclk_state)
> > >> > > > > > +{
> > >> > > > > > +   u32 val;
> > >> > > > > > +
> > >> > > > > > +   if (I915_READ(SKL_DSSM) & CNL_DSSM_CDCLK_PLL_REFCLK_24MHz)
> > >> > > > > > +           cdclk_state->ref = 24000;
> > >> > > > > > +   else
> > >> > > > > > +           cdclk_state->ref = 19200;
> > >> > > > > > +
> > >> > > > > > +   cdclk_state->vco = 0;
> > >> > > > > > +
> > >> > > > > > +   val = I915_READ(BXT_DE_PLL_ENABLE);
> > >> > > > > > +   if ((val & BXT_DE_PLL_PLL_ENABLE) == 0)
> > >> > > > > > +           return;
> > >> > > > > > +
> > >> > > > > > +   if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0))
> > >> > > > > > +           return;
> > >> > > > > > +
> > >> > > > > > +   cdclk_state->vco = (val & CNL_CDCLK_PLL_RATIO_MASK) * cdclk_state->ref;
> > >> > > > > > +}
> > >> > > > > > +
> > >> > > > > > +static void cnl_get_cdclk(struct drm_i915_private *dev_priv,
> > >> > > > > > +                    struct intel_cdclk_state *cdclk_state)
> > >> > > > > > +{
> > >> > > > > > +   u32 divider;
> > >> > > > > > +   int div;
> > >> > > > > > +
> > >> > > > > > +   cnl_cdclk_pll_update(dev_priv, cdclk_state);
> > >> > > > >
> > >> > > > > The other platforms set cdclk to the ref clock here, not sure
> > >> > > > > if it's ok to leave it uninited. With that change it looks ok:
> > >> > > >
> > >> > > > Not sure how to address this here...
> > >> > > > I see bxt and skl using the cdclk_state here...
> > >> > >
> > >> > > Assuming refclk is the bypass clock then just doing what the earlier
> > >> > > platforms do would be correct.
> > >> >
> > >> > Yes, according to Bspec the CDCLK freq is the refclock freq when the PLL
> > >> > is disabled.
> > >>
> > >> So, do I need to change anything?
> > >
> > > Yes, add the following after cnl_cdclk_pll_update() as done on other
> > > gen9+ platforms:
> > >
> > > cdclk_state->cdclk = cdclk_state->ref;
> > >
> > > --Imre
> > >
> > >>
> > >> >
> > >> > > IIRC there was some platform where the bypass clock wasn't the refclk,
> > >> > > but that was perhaps some future thing. Either way, whatever the
> > >> > > bypass clock is we will want the readout to correctly reflect it when
> > >> > > the PLL is off.
> > >>
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > 
> > -- 
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC