Re: [Intel-gfx] [PATCH 3/4] drm/i915/display/dg2: Sanitize CD clock

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel Open Source Graphics Center