| Message ID | 20211118085424.685686-4-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display/dg2: Add CD clock squashing | expand |
On Thu, 18 Nov 2021, Mika Kahola <mika.kahola@intel.com> wrote: > In case of CD clock squashing the divider is always 1. We don't > need to calculate the divider in use so let's skip that for DG2. > > v2: Drop unnecessary local variable (Ville) > v3: Avoid if-else structure (Ville) > [v4: vsyrjala: Fix cd2x divider calculation (Uma), > Introduce has_cdclk_squasher()] > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 630a53d4f882..e8c58779c2a8 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -1212,6 +1212,11 @@ static void skl_cdclk_uninit_hw(struct drm_i915_private *dev_priv) > skl_set_cdclk(dev_priv, &cdclk_config, INVALID_PIPE); > } > > +static bool has_cdclk_squasher(struct drm_i915_private *i915) > +{ > + return IS_DG2(i915); > +} The obvious problem is that you use this function already in patch 2. I'm also not sure we want to start sprinkling the has_ or HAS_ query stuff all over the place in .c. files. Or if we do, we should do it in a more planned manner, not by starting to sneak these in. BR, Jani. > + > static const struct intel_cdclk_vals bxt_cdclk_table[] = { > { .refclk = 19200, .cdclk = 144000, .divider = 8, .ratio = 60 }, > { .refclk = 19200, .cdclk = 288000, .divider = 4, .ratio = 60 }, > @@ -1750,7 +1755,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv, > static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv) > { > u32 cdctl, expected; > - int cdclk, vco; > + int cdclk, clock, vco; > > intel_update_cdclk(dev_priv); > intel_dump_cdclk_config(&dev_priv->cdclk.hw, "Current CDCLK"); > @@ -1786,8 +1791,12 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv) > expected = skl_cdclk_decimal(cdclk); > > /* Figure out what CD2X divider we should be using for this cdclk */ > - expected |= bxt_cdclk_cd2x_div_sel(dev_priv, > - dev_priv->cdclk.hw.cdclk, > + if (has_cdclk_squasher(dev_priv)) > + clock = dev_priv->cdclk.hw.vco / 2; > + else > + clock = dev_priv->cdclk.hw.cdclk; > + > + expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock, > dev_priv->cdclk.hw.vco); > > /*
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Thursday, November 18, 2021 12:12 PM > To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915/display/dg2: Sanitize CD clock > > On Thu, 18 Nov 2021, Mika Kahola <mika.kahola@intel.com> wrote: > > In case of CD clock squashing the divider is always 1. We don't need > > to calculate the divider in use so let's skip that for DG2. > > > > v2: Drop unnecessary local variable (Ville) > > v3: Avoid if-else structure (Ville) > > [v4: vsyrjala: Fix cd2x divider calculation (Uma), > > Introduce has_cdclk_squasher()] > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > > b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index 630a53d4f882..e8c58779c2a8 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -1212,6 +1212,11 @@ static void skl_cdclk_uninit_hw(struct > drm_i915_private *dev_priv) > > skl_set_cdclk(dev_priv, &cdclk_config, INVALID_PIPE); } > > > > +static bool has_cdclk_squasher(struct drm_i915_private *i915) { > > + return IS_DG2(i915); > > +} > > The obvious problem is that you use this function already in patch 2. I couldn't find the original cover-letter and hence the patches might have slipped in wrong order. Thanks for pointing that out! > > I'm also not sure we want to start sprinkling the has_ or HAS_ query stuff all over > the place in .c. files. Or if we do, we should do it in a more planned manner, not > by starting to sneak these in. Well, what would be the alternative? How we should handle the cases where a feature is supported by a platform and perhaps platforms in the future? Cheers, Mika > > BR, > Jani. > > > + > > static const struct intel_cdclk_vals bxt_cdclk_table[] = { > > { .refclk = 19200, .cdclk = 144000, .divider = 8, .ratio = 60 }, > > { .refclk = 19200, .cdclk = 288000, .divider = 4, .ratio = 60 }, @@ > > -1750,7 +1755,7 @@ static void bxt_set_cdclk(struct drm_i915_private > > *dev_priv, static void bxt_sanitize_cdclk(struct drm_i915_private > > *dev_priv) { > > u32 cdctl, expected; > > - int cdclk, vco; > > + int cdclk, clock, vco; > > > > intel_update_cdclk(dev_priv); > > intel_dump_cdclk_config(&dev_priv->cdclk.hw, "Current CDCLK"); @@ > > -1786,8 +1791,12 @@ static void bxt_sanitize_cdclk(struct drm_i915_private > *dev_priv) > > expected = skl_cdclk_decimal(cdclk); > > > > /* Figure out what CD2X divider we should be using for this cdclk */ > > - expected |= bxt_cdclk_cd2x_div_sel(dev_priv, > > - dev_priv->cdclk.hw.cdclk, > > + if (has_cdclk_squasher(dev_priv)) > > + clock = dev_priv->cdclk.hw.vco / 2; > > + else > > + clock = dev_priv->cdclk.hw.cdclk; > > + > > + expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock, > > dev_priv->cdclk.hw.vco); > > > > /* > > -- > Jani Nikula, Intel Open Source Graphics Center
On Thu, 18 Nov 2021, "Kahola, Mika" <mika.kahola@intel.com> wrote: >> -----Original Message----- >> From: Jani Nikula <jani.nikula@linux.intel.com> >> Sent: Thursday, November 18, 2021 12:12 PM >> To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915/display/dg2: Sanitize CD clock >> >> On Thu, 18 Nov 2021, Mika Kahola <mika.kahola@intel.com> wrote: >> > In case of CD clock squashing the divider is always 1. We don't need >> > to calculate the divider in use so let's skip that for DG2. >> > >> > v2: Drop unnecessary local variable (Ville) >> > v3: Avoid if-else structure (Ville) >> > [v4: vsyrjala: Fix cd2x divider calculation (Uma), >> > Introduce has_cdclk_squasher()] >> > >> > Signed-off-by: Mika Kahola <mika.kahola@intel.com> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 ++++++++++++--- >> > 1 file changed, 12 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c >> > b/drivers/gpu/drm/i915/display/intel_cdclk.c >> > index 630a53d4f882..e8c58779c2a8 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c >> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >> > @@ -1212,6 +1212,11 @@ static void skl_cdclk_uninit_hw(struct >> drm_i915_private *dev_priv) >> > skl_set_cdclk(dev_priv, &cdclk_config, INVALID_PIPE); } >> > >> > +static bool has_cdclk_squasher(struct drm_i915_private *i915) { >> > + return IS_DG2(i915); >> > +} >> >> The obvious problem is that you use this function already in patch 2. > > I couldn't find the original cover-letter and hence the patches might have slipped in wrong order. Thanks for pointing that out! > >> >> I'm also not sure we want to start sprinkling the has_ or HAS_ query stuff all over >> the place in .c. files. Or if we do, we should do it in a more planned manner, not >> by starting to sneak these in. > > Well, what would be the alternative? How we should handle the cases > where a feature is supported by a platform and perhaps platforms in > the future? I mean the current approach is to add the feature check macros in i915_drv.h instead of spreading them everywhere. It may not be the best approach, and we may want to revisit that in the future, but at the moment that's where we put them. BR, Jani. > > Cheers, > Mika >> >> BR, >> Jani. >> >> > + >> > static const struct intel_cdclk_vals bxt_cdclk_table[] = { >> > { .refclk = 19200, .cdclk = 144000, .divider = 8, .ratio = 60 }, >> > { .refclk = 19200, .cdclk = 288000, .divider = 4, .ratio = 60 }, @@ >> > -1750,7 +1755,7 @@ static void bxt_set_cdclk(struct drm_i915_private >> > *dev_priv, static void bxt_sanitize_cdclk(struct drm_i915_private >> > *dev_priv) { >> > u32 cdctl, expected; >> > - int cdclk, vco; >> > + int cdclk, clock, vco; >> > >> > intel_update_cdclk(dev_priv); >> > intel_dump_cdclk_config(&dev_priv->cdclk.hw, "Current CDCLK"); @@ >> > -1786,8 +1791,12 @@ static void bxt_sanitize_cdclk(struct drm_i915_private >> *dev_priv) >> > expected = skl_cdclk_decimal(cdclk); >> > >> > /* Figure out what CD2X divider we should be using for this cdclk */ >> > - expected |= bxt_cdclk_cd2x_div_sel(dev_priv, >> > - dev_priv->cdclk.hw.cdclk, >> > + if (has_cdclk_squasher(dev_priv)) >> > + clock = dev_priv->cdclk.hw.vco / 2; >> > + else >> > + clock = dev_priv->cdclk.hw.cdclk; >> > + >> > + expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock, >> > dev_priv->cdclk.hw.vco); >> > >> > /* >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index 630a53d4f882..e8c58779c2a8 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -1212,6 +1212,11 @@ static void skl_cdclk_uninit_hw(struct drm_i915_private *dev_priv) skl_set_cdclk(dev_priv, &cdclk_config, INVALID_PIPE); } +static bool has_cdclk_squasher(struct drm_i915_private *i915) +{ + return IS_DG2(i915); +} + static const struct intel_cdclk_vals bxt_cdclk_table[] = { { .refclk = 19200, .cdclk = 144000, .divider = 8, .ratio = 60 }, { .refclk = 19200, .cdclk = 288000, .divider = 4, .ratio = 60 }, @@ -1750,7 +1755,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv, static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv) { u32 cdctl, expected; - int cdclk, vco; + int cdclk, clock, vco; intel_update_cdclk(dev_priv); intel_dump_cdclk_config(&dev_priv->cdclk.hw, "Current CDCLK"); @@ -1786,8 +1791,12 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv) expected = skl_cdclk_decimal(cdclk); /* Figure out what CD2X divider we should be using for this cdclk */ - expected |= bxt_cdclk_cd2x_div_sel(dev_priv, - dev_priv->cdclk.hw.cdclk, + if (has_cdclk_squasher(dev_priv)) + clock = dev_priv->cdclk.hw.vco / 2; + else + clock = dev_priv->cdclk.hw.cdclk; + + expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock, dev_priv->cdclk.hw.vco); /*
In case of CD clock squashing the divider is always 1. We don't need to calculate the divider in use so let's skip that for DG2. v2: Drop unnecessary local variable (Ville) v3: Avoid if-else structure (Ville) [v4: vsyrjala: Fix cd2x divider calculation (Uma), Introduce has_cdclk_squasher()] Signed-off-by: Mika Kahola <mika.kahola@intel.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)