[Intel-gfx] [PATCH 2/6] drm/i915/display: add cdclk action struct to cdclk_config

[Navare, Manasi]

On Mon, Sep 19, 2022 at 12:26:19PM +0300, Jani Nikula wrote:
> On Fri, 16 Sep 2022, Anusha Srivatsa <anusha.srivatsa at intel.com> wrote:
> > The struct has the action to be performed - squash, crawl
> > or modeset and the corresponding cdclk which is the desired
> > cdclk. This is the structure that gets populated during
> > atomic check once it is determined what the cdclk change looks
> > like
> >
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.h | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > index c674879a84a5..3869f93e8ad2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > @@ -11,13 +11,27 @@
> >  #include "intel_display.h"
> >  #include "intel_global_state.h"
> >  
> > +#define	MAX_CDCLK_ACTIONS	1
> 
> Okay, this review is just nitpicks, but they'll need to get fixed
> eventually so here goes.
> 
> No tab after #define.
> 
> > +
> >  struct drm_i915_private;
> >  struct intel_atomic_state;
> >  struct intel_crtc_state;
> >  
> > +enum cdclk_sequence {
> 
> Needs to be named intel_ something.

Agree here

> 
> > +	CDCLK_INVALID_ACTION = -1,
> > +
> > +	CDCLK_SQUASH_ONLY = 0,
> > +	CDCLK_CRAWL_ONLY,
> > +	CDCLK_LEGACY,
> > +};
> > +
> >  struct intel_cdclk_config {
> >  	unsigned int cdclk, vco, ref, bypass;
> >  	u8 voltage_level;
> > +	struct cdclk_step {
> 
> Needs to be named intel_ something.
> 
> Since this is used independently, I'd prefer it to be defined outside of
> struct intel_cdclk_config.

I think the point of having it as part of intel_cdclk_config is that
because we already pass cdclk_config to set_cdclk where these actions
are going to get used.

Manasi

> 
> > +		enum cdclk_sequence action;
> > +		u32 cdclk;
> > +	} steps[MAX_CDCLK_ACTIONS];
> >  };
> >  
> >  struct intel_cdclk_state {
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center