[PATCH v2] drm/i915/display: Pcode sets the CD clock frequency voltage levels

[Kahola, Mika]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Tuesday, 12 November 2024 22.09
> To: Kahola, Mika <mika.kahola at intel.com>
> Cc: intel-xe at lists.freedesktop.org; ville.syrjala at linux.intel.com
> Subject: Re: [PATCH v2] drm/i915/display: Pcode sets the CD clock frequency
> voltage levels
> 
> On Tue, Nov 12, 2024 at 03:24:11PM +0200, Mika Kahola wrote:
> > On xe3lpd the CD clock frequency is set by the driver without setting
> > voltage levels. Previously, we had 4 levels of voltages to choose
> > from. However, having only 4 levels is rather coarse and we may end up
> > consuming more power than necessary. Therefore, these are now removed
> > and choosing the correct voltage level is now handed over to Pcode
> > firmware which is able to set voltage level with higher granularity.
> >
> > v2: Leave voltage level calculcations untouched as it returns
> >     always 0. Drop only voltage level nofication to pcode (Ville)
> 
> Can't we just drop this patch completely?  Since we always leave the voltage level
> as "0" on Xe3 and beyond, the existing macro does exactly the same thing as the
> new one you're adding here (OR'ing in an extra 0 has no effect on the resulting
> value).
We can drop this patch. As per chat with Ville this voltage level was the only thing that we could drop from pcode notification. We still have cd clock calculation logic in place even though we wouldn't need to do that. To me this looks like a dead code since voltage level is not used by the pcode firmware. However, dropping even voltage level calculations would yield if-else checks all over the driver which is not good either.

All in all, let's drop this patch for simplicity.

Thanks!

-Mika-

> 
> 
> Matt
> 
> >
> > Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 5 ++++-
> >  drivers/gpu/drm/i915/i915_reg.h            | 3 +++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 5a4c8c2410ae..325100cd3130 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2497,7 +2497,10 @@ static void intel_pcode_notify(struct intel_display
> *display,
> >  	if (!IS_DG2(i915))
> >  		return;
> >
> > -	update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk,
> active_pipe_count, voltage_level);
> > +	if (DISPLAY_VER(display) == 30)
> > +		update_mask = DISPLAY30_TO_PCODE_UPDATE_MASK(cdclk,
> active_pipe_count);
> > +	else
> > +		update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk,
> > +active_pipe_count, voltage_level);
> >
> >  	if (cdclk_update_valid)
> >  		update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID; diff --git
> > a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 261f388b49c7..8e4aa5584adb 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3539,6 +3539,9 @@
> >  		((DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
> >  		(DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
> >  		(DISPLAY_TO_PCODE_VOLTAGE(voltage_level)))
> > +#define   DISPLAY30_TO_PCODE_UPDATE_MASK(cdclk, num_pipes) \
> > +		((DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
> > +		(DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)))
> >  #define   ICL_PCODE_SAGV_DE_MEM_SS_CONFIG	0xe
> >  #define     ICL_PCODE_REP_QGV_MASK		REG_GENMASK(1, 0)
> >  #define     ICL_PCODE_REP_QGV_SAFE
> 	REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
> > --
> > 2.43.0
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation