| Message ID | 20220917004404.414981-3-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce struct cdclk_step | expand |
On Fri, 16 Sep 2022, Anusha Srivatsa <anusha.srivatsa@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@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. > + 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. > + enum cdclk_sequence action; > + u32 cdclk; > + } steps[MAX_CDCLK_ACTIONS]; > }; > > struct intel_cdclk_state {
On Mon, Sep 19, 2022 at 12:26:19PM +0300, Jani Nikula wrote: > On Fri, 16 Sep 2022, Anusha Srivatsa <anusha.srivatsa@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@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
> -----Original Message----- > From: Navare, Manasi D <manasi.d.navare@intel.com> > Sent: Monday, September 19, 2022 12:33 PM > To: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 2/6] drm/i915/display: add cdclk action struct > to cdclk_config > > On Mon, Sep 19, 2022 at 12:26:19PM +0300, Jani Nikula wrote: > > On Fri, 16 Sep 2022, Anusha Srivatsa <anusha.srivatsa@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@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 Agree with all the above. Will make the suitable changes. > > > > > + 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. Yes. That is correct. This eventually gets used in bxt_set_cdclk() and we are already passing cdclk_config there. Having this new struct embedded in cdclk_config makes the fields - action and cdclk to be accessible without having to change the function signature of set_cdclk() Anusha > Manasi > > > > > > + enum cdclk_sequence action; > > > + u32 cdclk; > > > + } steps[MAX_CDCLK_ACTIONS]; > > > }; > > > > > > struct intel_cdclk_state { > > > > -- > > Jani Nikula, Intel Open Source Graphics Center
On Mon, 19 Sep 2022, "Srivatsa, Anusha" <anusha.srivatsa@intel.com> wrote: >> -----Original Message----- >> From: Navare, Manasi D <manasi.d.navare@intel.com> >> Sent: Monday, September 19, 2022 12:33 PM >> To: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- >> gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH 2/6] drm/i915/display: add cdclk action struct >> to cdclk_config >> >> On Mon, Sep 19, 2022 at 12:26:19PM +0300, Jani Nikula wrote: >> > On Fri, 16 Sep 2022, Anusha Srivatsa <anusha.srivatsa@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@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 > > Agree with all the above. Will make the suitable changes. > >> > >> > > + 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. > > Yes. That is correct. This eventually gets used in bxt_set_cdclk() and > we are already passing cdclk_config there. Having this new struct > embedded in cdclk_config makes the fields - action and cdclk to be > accessible without having to change the function signature of > set_cdclk() I referred to defining the *type* outside of struct intel_cdclk_config. Contrast struct foo { struct bar { ... } baz; }; with struct bar { ... }; struct foo { struct bar baz; }; when you actually use struct bar for parameters and local variables. BR, Jani. > > Anusha >> Manasi >> >> > >> > > + enum cdclk_sequence action; >> > > + u32 cdclk; >> > > + } steps[MAX_CDCLK_ACTIONS]; >> > > }; >> > > >> > > struct intel_cdclk_state { >> > >> > -- >> > Jani Nikula, Intel Open Source Graphics Center
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 + struct drm_i915_private; struct intel_atomic_state; struct intel_crtc_state; +enum cdclk_sequence { + 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 { + enum cdclk_sequence action; + u32 cdclk; + } steps[MAX_CDCLK_ACTIONS]; }; struct intel_cdclk_state {
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@intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)