On Mon, Sep 19, 2022 at 12:26:19PM +0300, Jani Nikula wrote:
> On Fri, 16 Sep 2022, Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
