Re: [Intel-gfx] [PATCH] drm/i915: Bump GLK CDCLK frequency when driving multiple pipes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 02, 2023 at 04:07:58PM +0200, Jani Nikula wrote:
> On Tue, 31 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > On GLK CDCLK frequency needs to be at least 2*96 MHz when accessing
> > the audio hardware. Currently we bump the CDCLK frequency up
> > temporarily (if not high enough already) whenever audio hardware
> > is being accessed, and drop it back down afterwards.
> >
> > With a single active pipe this works just fine as we can switch
> > between all the valid CDCLK frequencies by changing the cd2x
> > divider, which doesn't require a full modeset. However with
> > multiple active pipes the cd2x divider trick no longer works,
> > and thus we end up blinking all displays off and back on.
> >
> > To avoid this let's just bump the CDCLK frequency to >=2*96MHz
> > whenever multiple pipes are active. The downside is slightly
> > higher power consumption, but that seems like an acceptable
> > tradeoff. With a single active pipe we can stick to the current
> > more optiomal (from power comsumption POV) behaviour.
> >
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9599
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 6d7ba4d0f130..3ddf4201e224 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2750,6 +2750,18 @@ static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state)
> >  	for_each_pipe(dev_priv, pipe)
> >  		min_cdclk = max(cdclk_state->min_cdclk[pipe], min_cdclk);
> >  
> > +	/*
> > +	 * Avoid glk_force_audio_cdclk() causing excessive screen
> > +	 * blinking when multiple pipes are active by making sure
> > +	 * cdclk frequency is always high enough for audio. With a
> > +	 * single active pipe we can always change CDCLK frequency
> > +	 * by changing the cd2x divider (see glk_cdclk_table[]) and
> > +	 * thus a full modeset won't be needed then.
> > +	 */
> > +	if (DISPLAY_VER(dev_priv) == 10 && cdclk_state->active_pipes &&
> > +	    !is_power_of_2(cdclk_state->active_pipes))
> > +		min_cdclk = max(2 * 96000, min_cdclk);
> 
> For consistency with glk_force_audio_cdclk(), maybe
> s/DISPLAY_VER(dev_priv) == 10/IS_GEMINILAKE()/?

I suppose. Though we do use both check for glk, even in the rest
of the cdclk code.

> 
> There's a bit of duplication with the frequencies, but I guess the
> comment is enough to alleviate the concern.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks.

> 
> 
> > +
> >  	if (min_cdclk > dev_priv->display.cdclk.max_cdclk_freq) {
> >  		drm_dbg_kms(&dev_priv->drm,
> >  			    "required cdclk (%d kHz) exceeds max (%d kHz)\n",
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel