Em Sex, 2017-02-17 às 15:49 +0200, Ville Syrjälä escreveu:
> On Fri, Feb 17, 2017 at 11:22:05AM -0200, Paulo Zanoni wrote:
> >
> > Each x_modeset_calc_cdclk() has to do the same platform checks
> > twice,
> > so extract them to a single function. This way, the platform checks
> > are all in the same place, and the platform-common code gets rid of
> > all the platform-specific checks, which IMHO makes the code easier
> > to
> > read.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++----
> > --------------
> > 1 file changed, 45 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index d505ff1..6efc5f4 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct
> > drm_atomic_state *state)
> > return max_pixel_rate;
> > }
> >
> > +static void intel_calc_cdclk(struct intel_atomic_state *state, int
> > max_pixclk,
> > + int *cdclk, int *vco)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(state-
> > >base.dev);
> > +
> > + switch (INTEL_INFO(dev_priv)->platform) {
> > + case INTEL_VALLEYVIEW:
> > + case INTEL_CHERRYVIEW:
> > + *cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> > + break;
> > + case INTEL_BROADWELL:
> > + /*
> > + * FIXME: should also account for plane ratio once
> > 64bpp pixel
> > + * formats are supported.
> > + */
> > + *cdclk = bdw_calc_cdclk(max_pixclk);
> > + break;
> > + case INTEL_SKYLAKE:
> > + case INTEL_KABYLAKE:
> > + /*
> > + * FIXME: should also account for plane ratio once
> > 64bpp pixel
> > + * formats are supported.
> > + */
> > + *vco = state->cdclk.logical.vco;
> > + if (!*vco)
> > + *vco = dev_priv->skl_preferred_vco_freq;
> > + *cdclk = skl_calc_cdclk(max_pixclk, *vco);
> > + break;
> > + case INTEL_BROXTON:
> > + *cdclk = bxt_calc_cdclk(max_pixclk);
> > + *vco = bxt_de_pll_vco(dev_priv, *cdclk);
> > + break;
> > + case INTEL_GEMINILAKE:
> > + *cdclk = glk_calc_cdclk(max_pixclk);
> > + *vco = glk_de_pll_vco(dev_priv, *cdclk);
> > + break;
> > + default:
> > + MISSING_CASE(INTEL_INFO(dev_priv)->platform);
> > + }
> > +}
>
> How about just replacing the .modeset_calc_cdclk() vfunc with a
> slightly
> lower level vfunc that just computes the cdclk/vco/whatever without
> containing the active_crtcs logic?
>
> Then we should have just
>
> intel_modeset_calc_cdclk()
> {
> .calc_cdclk(logical, max_pixclk);
>
> /*
> * maybe keep the max_cdclk check here, although it that
> * happens I think we have a bug somewhere, so perhaps
> * just convert it into a WARN, or drop entirely.
> */
>
> if (!active_crtcs)
> .calc_cdclk(actual, 0);
> else
> actual = logical;
> }
Yeah, the code above is definitely a next step to the changes I did.
I'm just not a big fan of the .calc_cdclk vfunc since it will be just 2
lines for each platform. Unless I inline them with the *real*
x_calc_cdclk() funcs we have today, but then I'll have to check their
other callers. So I'll take a look and try to submit a new patch.
>
>
> >
> > +
> > static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > @@ -1503,14 +1544,7 @@ static int vlv_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > int max_pixclk = intel_max_pixel_rate(state);
> > int cdclk;
> >
> > - /*
> > - * FIXME: Broadwell should also account for plane ratio
> > once 64bpp pixel
> > - * formats are supported.
> > - */
> > - if (IS_BROADWELL(dev_priv))
> > - cdclk = bdw_calc_cdclk(max_pixclk);
> > - else
> > - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> > + intel_calc_cdclk(intel_state, max_pixclk, &cdclk, NULL);
> >
> > if (cdclk > dev_priv->max_cdclk_freq) {
> > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds
> > max (%d kHz)\n",
> > @@ -1521,11 +1555,7 @@ static int vlv_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > intel_state->cdclk.logical.cdclk = cdclk;
> >
> > if (!intel_state->active_crtcs) {
> > - if (IS_BROADWELL(dev_priv))
> > - cdclk = bdw_calc_cdclk(0);
> > - else
> > - cdclk = vlv_calc_cdclk(dev_priv, 0);
> > -
> > + intel_calc_cdclk(intel_state, 0, &cdclk, NULL);
> > intel_state->cdclk.actual.cdclk = cdclk;
> > } else {
> > intel_state->cdclk.actual = intel_state-
> > >cdclk.logical;
> > @@ -1541,22 +1571,7 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > int max_pixclk = intel_max_pixel_rate(state);
> > int cdclk, vco;
> >
> > - /*
> > - * FIXME: Skylake/Kabylake should also account for plane
> > ratio once
> > - * 64bpp pixel formats are supported.
> > - */
> > - if (IS_GEMINILAKE(dev_priv)) {
> > - cdclk = glk_calc_cdclk(max_pixclk);
> > - vco = glk_de_pll_vco(dev_priv, cdclk);
> > - } else if (IS_BROXTON(dev_priv)) {
> > - cdclk = bxt_calc_cdclk(max_pixclk);
> > - vco = bxt_de_pll_vco(dev_priv, cdclk);
> > - } else {
> > - vco = intel_state->cdclk.logical.vco;
> > - if (!vco)
> > - vco = dev_priv->skl_preferred_vco_freq;
> > - cdclk = skl_calc_cdclk(max_pixclk, vco);
> > - }
> > + intel_calc_cdclk(intel_state, max_pixclk, &cdclk, &vco);
> >
> > if (cdclk > dev_priv->max_cdclk_freq) {
> > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds
> > max (%d kHz)\n",
> > @@ -1568,16 +1583,7 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > intel_state->cdclk.logical.cdclk = cdclk;
> >
> > if (!intel_state->active_crtcs) {
> > - if (IS_GEMINILAKE(dev_priv)) {
> > - cdclk = glk_calc_cdclk(0);
> > - vco = glk_de_pll_vco(dev_priv, cdclk);
> > - } else if (IS_BROXTON(dev_priv)) {
> > - cdclk = bxt_calc_cdclk(0);
> > - vco = bxt_de_pll_vco(dev_priv, cdclk);
> > - } else {
> > - cdclk = skl_calc_cdclk(0, vco);
> > - }
> > -
> > + intel_calc_cdclk(intel_state, 0, &cdclk, &vco);
> > intel_state->cdclk.actual.vco = vco;
> > intel_state->cdclk.actual.cdclk = cdclk;
> > } else {
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
