| Message ID | 20220820005822.102716-2-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CDCLK churn: move checks to atomic check | expand |
On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote: > This is a prep patch for what the rest of the series does. > > Add existing actions that change cdclk - squash, crawl, modeset to > intel_cdclk_state so we have access to the cdclk values > that are in transition. > > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h > index b535cf6a7d9e..43835688ee02 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > @@ -15,6 +15,14 @@ struct drm_i915_private; > struct intel_atomic_state; > struct intel_crtc_state; > > +enum cdclk_actions { > + INTEL_CDCLK_MODESET = 0, > + INTEL_CDCLK_SQUASH, > + INTEL_CDCLK_CRAWL, > + INTEL_CDCLK_NOOP, > + MAX_CDCLK_ACTIONS > +}; > + > struct intel_cdclk_config { > unsigned int cdclk, vco, ref, bypass; > u8 voltage_level; > @@ -51,6 +59,11 @@ struct intel_cdclk_state { > > /* bitmask of active pipes */ > u8 active_pipes; > + > + struct cdclk_step { > + enum cdclk_actions action; > + u32 cdclk; > + } steps[MAX_CDCLK_ACTIONS]; If this is what you need to access later in bxt_set_cdclk , you needto add this to intel_cdclk_config which is then part of cdclk_state and that is what will get programmed in atomic_check and it gets sent to bxt_set_cdclk in atomic_commit_tail. This is the way ypu can access it in bxt_set_cdclk, you cannot access the new_cdclk_state there, you need to use cdclk_config that is already getting passed to it Manasi > }; > > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state); > -- > 2.25.1 >
On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote: > This is a prep patch for what the rest of the series does. > > Add existing actions that change cdclk - squash, crawl, modeset to > intel_cdclk_state so we have access to the cdclk values > that are in transition. > > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h > index b535cf6a7d9e..43835688ee02 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > @@ -15,6 +15,14 @@ struct drm_i915_private; > struct intel_atomic_state; > struct intel_crtc_state; > > +enum cdclk_actions { > + INTEL_CDCLK_MODESET = 0, > + INTEL_CDCLK_SQUASH, > + INTEL_CDCLK_CRAWL, > + INTEL_CDCLK_NOOP, > + MAX_CDCLK_ACTIONS > +}; This whole actions thing feels overly complicated to me. I think we should only need something like this: if (new.squash > old.squash) { mid.vco = old.vco; mid.squash = new.squash; } else { mid.vco = new.vco; mid.squash = old.squash; } /* * bunch of asserts here to make sure * the mid state looks sane. */ set_cdclk(mid); set_cdclk(new); And perhaps the current set_cdclk needs to get chunked up into smaller pieces so we don't do all the pre/post stuff more than once needlessly. > + > struct intel_cdclk_config { > unsigned int cdclk, vco, ref, bypass; > u8 voltage_level; > @@ -51,6 +59,11 @@ struct intel_cdclk_state { > > /* bitmask of active pipes */ > u8 active_pipes; > + > + struct cdclk_step { > + enum cdclk_actions action; > + u32 cdclk; > + } steps[MAX_CDCLK_ACTIONS]; > }; > > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state); > -- > 2.25.1
On Thu, Sep 15, 2022 at 12:22:53AM +0300, Ville Syrjälä wrote: > On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote: > > This is a prep patch for what the rest of the series does. > > > > Add existing actions that change cdclk - squash, crawl, modeset to > > intel_cdclk_state so we have access to the cdclk values > > that are in transition. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h > > index b535cf6a7d9e..43835688ee02 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > > @@ -15,6 +15,14 @@ struct drm_i915_private; > > struct intel_atomic_state; > > struct intel_crtc_state; > > > > +enum cdclk_actions { > > + INTEL_CDCLK_MODESET = 0, > > + INTEL_CDCLK_SQUASH, > > + INTEL_CDCLK_CRAWL, > > + INTEL_CDCLK_NOOP, > > + MAX_CDCLK_ACTIONS > > +}; > > This whole actions thing feels overly complicated to me. > I think we should only need something like this: > > if (new.squash > old.squash) { > mid.vco = old.vco; > mid.squash = new.squash; > } else { > mid.vco = new.vco; > mid.squash = old.squash; > } > /* > * bunch of asserts here to make sure > * the mid state looks sane. > */ > set_cdclk(mid); > set_cdclk(new); > > And perhaps the current set_cdclk needs to get chunked up > into smaller pieces so we don't do all the pre/post stuff > more than once needlessly. One idea might be to pass just a pair of flags to set_cdclk() whether to skip the pre/post steps.
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Wednesday, September 14, 2022 2:43 PM > To: Srivatsa, Anusha <anusha.srivatsa@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to > intel_cdclk_state > > On Thu, Sep 15, 2022 at 12:22:53AM +0300, Ville Syrjälä wrote: > > On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote: > > > This is a prep patch for what the rest of the series does. > > > > > > Add existing actions that change cdclk - squash, crawl, modeset to > > > intel_cdclk_state so we have access to the cdclk values that are in > > > transition. > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h > > > b/drivers/gpu/drm/i915/display/intel_cdclk.h > > > index b535cf6a7d9e..43835688ee02 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > > > @@ -15,6 +15,14 @@ struct drm_i915_private; struct > > > intel_atomic_state; struct intel_crtc_state; > > > > > > +enum cdclk_actions { > > > + INTEL_CDCLK_MODESET = 0, > > > + INTEL_CDCLK_SQUASH, > > > + INTEL_CDCLK_CRAWL, > > > + INTEL_CDCLK_NOOP, > > > + MAX_CDCLK_ACTIONS > > > +}; > > > > This whole actions thing feels overly complicated to me. > > I think we should only need something like this: > > > > if (new.squash > old.squash) { > > mid.vco = old.vco; > > mid.squash = new.squash; > > } else { > > mid.vco = new.vco; > > mid.squash = old.squash; > > } > > /* > > * bunch of asserts here to make sure > > * the mid state looks sane. > > */ > > set_cdclk(mid); > > set_cdclk(new); > > > > And perhaps the current set_cdclk needs to get chunked up into smaller > > pieces so we don't do all the pre/post stuff more than once > > needlessly. > > One idea might be to pass just a pair of flags to set_cdclk() whether to skip > the pre/post steps. This is all considering that the new struct cdclk_step is embedded in cdclk_config and not cdclk_state. I am not understanding why cdclk-state is not accessible from bxt_set_cdclk. What if I add cdclk_state to the dev_priv? bxt_set_cdclk() anyway has dev_priv. Anusha > -- > Ville Syrjälä > Intel
On Wed, Sep 14, 2022 at 09:58:59PM +0000, Srivatsa, Anusha wrote: > > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Wednesday, September 14, 2022 2:43 PM > > To: Srivatsa, Anusha <anusha.srivatsa@intel.com> > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to > > intel_cdclk_state > > > > On Thu, Sep 15, 2022 at 12:22:53AM +0300, Ville Syrjälä wrote: > > > On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote: > > > > This is a prep patch for what the rest of the series does. > > > > > > > > Add existing actions that change cdclk - squash, crawl, modeset to > > > > intel_cdclk_state so we have access to the cdclk values that are in > > > > transition. > > > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h > > > > b/drivers/gpu/drm/i915/display/intel_cdclk.h > > > > index b535cf6a7d9e..43835688ee02 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > > > > @@ -15,6 +15,14 @@ struct drm_i915_private; struct > > > > intel_atomic_state; struct intel_crtc_state; > > > > > > > > +enum cdclk_actions { > > > > + INTEL_CDCLK_MODESET = 0, > > > > + INTEL_CDCLK_SQUASH, > > > > + INTEL_CDCLK_CRAWL, > > > > + INTEL_CDCLK_NOOP, > > > > + MAX_CDCLK_ACTIONS > > > > +}; > > > > > > This whole actions thing feels overly complicated to me. > > > I think we should only need something like this: > > > > > > if (new.squash > old.squash) { > > > mid.vco = old.vco; > > > mid.squash = new.squash; > > > } else { > > > mid.vco = new.vco; > > > mid.squash = old.squash; > > > } > > > /* > > > * bunch of asserts here to make sure > > > * the mid state looks sane. > > > */ > > > set_cdclk(mid); > > > set_cdclk(new); > > > > > > And perhaps the current set_cdclk needs to get chunked up into smaller > > > pieces so we don't do all the pre/post stuff more than once > > > needlessly. > > > > One idea might be to pass just a pair of flags to set_cdclk() whether to skip > > the pre/post steps. > > This is all considering that the new struct cdclk_step is embedded in cdclk_config and not cdclk_state. I am not understanding why cdclk-state is not accessible from bxt_set_cdclk. .set_cdclk() is lower level than that. It must be able to operate outside atomic commits (eg. during display core init/uninit), and thus must be able to do its work purely based on the passed in cdclk_config (which is the lower level state structure, a pair of which are embedded inside the atomic intel_cdclk_state). > What if I add cdclk_state to the dev_priv? bxt_set_cdclk() anyway has dev_priv. We definitely don't want to hack around the core ideas of how the atomic machinery works. That way leads madness because no one will be able to understand how anything works.
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h index b535cf6a7d9e..43835688ee02 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.h +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h @@ -15,6 +15,14 @@ struct drm_i915_private; struct intel_atomic_state; struct intel_crtc_state; +enum cdclk_actions { + INTEL_CDCLK_MODESET = 0, + INTEL_CDCLK_SQUASH, + INTEL_CDCLK_CRAWL, + INTEL_CDCLK_NOOP, + MAX_CDCLK_ACTIONS +}; + struct intel_cdclk_config { unsigned int cdclk, vco, ref, bypass; u8 voltage_level; @@ -51,6 +59,11 @@ struct intel_cdclk_state { /* bitmask of active pipes */ u8 active_pipes; + + struct cdclk_step { + enum cdclk_actions action; + u32 cdclk; + } steps[MAX_CDCLK_ACTIONS]; }; int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
This is a prep patch for what the rest of the series does. Add existing actions that change cdclk - squash, crawl, modeset to intel_cdclk_state so we have access to the cdclk values that are in transition. Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)