| Message ID | 20240202131208.453298-1-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/cdclk: Rename intel_cdclk_needs_modeset to intel_cdclk_params_changed | expand |
On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: > Looks like the name and description of intel_cdclk_needs_modeset() > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for > cdclk changes if possible"), when it became possible to update the cdclk > without requiring disabling the pipes when only changing the cd2x > divider was enough. > > Later on we also added the same type of support with squash and crawling > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk > squasher to be reconfigured live") and commit d62686ba3b54 > ("drm/i915/adl_p: CDCLK crawl support for ADL"). > > As such, update that function's name and documentation to something more > appropriate, since the real checks for requiring modeset are done > elsewhere. > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > --- > > One thing worth noting here is that, with this change, we are left with an > awkward situation where two function names related to checking changes in cdclk: > > intel_cdclk_params_changed() and intel_cdclk_changed() > > , > > and I find it weird that we have intel_cdclk_changed(), which checks for the > voltage level as well. Shouldn't the voltage level be a function of cdclk and > ddi clock? Why do we need that? > > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- > drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- > .../drm/i915/display/intel_display_power_well.c | 4 ++-- > 3 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 26200ee3e23f..caadd880865f 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, > } > > /** > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK > - * configurations requires a modeset on all pipes > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed > * @a: first CDCLK configuration > * @b: second CDCLK configuration > * > * Returns: > - * True if changing between the two CDCLK configurations > - * requires all pipes to be off, false if not. > + * True if parameters changed in a way that requires programming the CDCLK > + * and False otherwise. > */ > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, > - const struct intel_cdclk_config *b) > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, > + const struct intel_cdclk_config *b) The new name isn't very descriptive either. Outside the cd2x/crawl/squash cases we stil have to consider two cases: 1. cdclk frequency/pll changes (voltage level can change or not) 2. cdclk frequency/pll doesn't change, but voltage level needs to change And that difference is what intel_cdclk_needs_modeset() is trying convey. And intel_cdclk_changed() tells us whether anything at all is changing.
On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote: > On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: > > Looks like the name and description of intel_cdclk_needs_modeset() > > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for > > cdclk changes if possible"), when it became possible to update the cdclk > > without requiring disabling the pipes when only changing the cd2x > > divider was enough. > > > > Later on we also added the same type of support with squash and crawling > > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash > > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk > > squasher to be reconfigured live") and commit d62686ba3b54 > > ("drm/i915/adl_p: CDCLK crawl support for ADL"). > > > > As such, update that function's name and documentation to something more > > appropriate, since the real checks for requiring modeset are done > > elsewhere. > > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > > --- > > > > One thing worth noting here is that, with this change, we are left with an > > awkward situation where two function names related to checking changes in cdclk: > > > > intel_cdclk_params_changed() and intel_cdclk_changed() > > > > , > > > > and I find it weird that we have intel_cdclk_changed(), which checks for the > > voltage level as well. Shouldn't the voltage level be a function of cdclk and > > ddi clock? Why do we need that? > > > > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- > > drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- > > .../drm/i915/display/intel_display_power_well.c | 4 ++-- > > 3 files changed, 11 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index 26200ee3e23f..caadd880865f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, > > } > > > > /** > > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK > > - * configurations requires a modeset on all pipes > > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed > > * @a: first CDCLK configuration > > * @b: second CDCLK configuration > > * > > * Returns: > > - * True if changing between the two CDCLK configurations > > - * requires all pipes to be off, false if not. > > + * True if parameters changed in a way that requires programming the CDCLK > > + * and False otherwise. > > */ > > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, > > - const struct intel_cdclk_config *b) > > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, > > + const struct intel_cdclk_config *b) > > The new name isn't very descriptive either. > > Outside the cd2x/crawl/squash cases we stil have to consider > two cases: > 1. cdclk frequency/pll changes (voltage level can change or not) > 2. cdclk frequency/pll doesn't change, but voltage level needs to change > > And that difference is what intel_cdclk_needs_modeset() is trying > convey. And intel_cdclk_changed() tells us whether anything at all > is changing. intel_cdclk_clock_changed() is perhaps the best name I can think of, off the top of my head. But I'm notoriously bad at naming things so take that with a pinch of salt.
Quoting Ville Syrjälä (2024-02-02 16:58:37-03:00) >On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: >> Looks like the name and description of intel_cdclk_needs_modeset() >> became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for >> cdclk changes if possible"), when it became possible to update the cdclk >> without requiring disabling the pipes when only changing the cd2x >> divider was enough. >> >> Later on we also added the same type of support with squash and crawling >> with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash >> when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk >> squasher to be reconfigured live") and commit d62686ba3b54 >> ("drm/i915/adl_p: CDCLK crawl support for ADL"). >> >> As such, update that function's name and documentation to something more >> appropriate, since the real checks for requiring modeset are done >> elsewhere. >> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >> --- >> >> One thing worth noting here is that, with this change, we are left with an >> awkward situation where two function names related to checking changes in cdclk: >> >> intel_cdclk_params_changed() and intel_cdclk_changed() >> >> , >> >> and I find it weird that we have intel_cdclk_changed(), which checks for the >> voltage level as well. Shouldn't the voltage level be a function of cdclk and >> ddi clock? Why do we need that? >> >> drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- >> drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- >> .../drm/i915/display/intel_display_power_well.c | 4 ++-- >> 3 files changed, 11 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c >> index 26200ee3e23f..caadd880865f 100644 >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >> @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, >> } >> >> /** >> - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK >> - * configurations requires a modeset on all pipes >> + * intel_cdclk_params_changed - Check whether CDCLK parameters changed >> * @a: first CDCLK configuration >> * @b: second CDCLK configuration >> * >> * Returns: >> - * True if changing between the two CDCLK configurations >> - * requires all pipes to be off, false if not. >> + * True if parameters changed in a way that requires programming the CDCLK >> + * and False otherwise. >> */ >> -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, >> - const struct intel_cdclk_config *b) >> +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, >> + const struct intel_cdclk_config *b) > >The new name isn't very descriptive either. Yeah... I would much rather use intel_cdclk_changed(), but that one is already taken. > >Outside the cd2x/crawl/squash cases we stil have to consider >two cases: >1. cdclk frequency/pll changes (voltage level can change or not) >2. cdclk frequency/pll doesn't change, but voltage level needs to change > >And that difference is what intel_cdclk_needs_modeset() is trying >convey. And intel_cdclk_changed() tells us whether anything at all >is changing. I might be missing something, but, by going through the specs, it looked to me that voltage level was dependent on cdclk (as well as on ddi clock) and not the other way around. That's why I find it odd that we need an intel_cdclk_changed() that, besides looking for changes in cdclk, also checks for the voltage level. In intel_set_cdclk(), we check intel_cdclk_changed() before continuing. If, for example, there is a change in ddi clock that requires a change in voltage level but no changes in cdclk, intel_cdclk_changed() would return true, right? Wouldn't that make us unnecessarily go through intel_set_cdclk()? -- Gustavo Sousa
Quoting Ville Syrjälä (2024-02-02 17:06:02-03:00) >On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote: >> On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: >> > Looks like the name and description of intel_cdclk_needs_modeset() >> > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for >> > cdclk changes if possible"), when it became possible to update the cdclk >> > without requiring disabling the pipes when only changing the cd2x >> > divider was enough. >> > >> > Later on we also added the same type of support with squash and crawling >> > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash >> > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk >> > squasher to be reconfigured live") and commit d62686ba3b54 >> > ("drm/i915/adl_p: CDCLK crawl support for ADL"). >> > >> > As such, update that function's name and documentation to something more >> > appropriate, since the real checks for requiring modeset are done >> > elsewhere. >> > >> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >> > --- >> > >> > One thing worth noting here is that, with this change, we are left with an >> > awkward situation where two function names related to checking changes in cdclk: >> > >> > intel_cdclk_params_changed() and intel_cdclk_changed() >> > >> > , >> > >> > and I find it weird that we have intel_cdclk_changed(), which checks for the >> > voltage level as well. Shouldn't the voltage level be a function of cdclk and >> > ddi clock? Why do we need that? >> > >> > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- >> > drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- >> > .../drm/i915/display/intel_display_power_well.c | 4 ++-- >> > 3 files changed, 11 insertions(+), 12 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c >> > index 26200ee3e23f..caadd880865f 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c >> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >> > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, >> > } >> > >> > /** >> > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK >> > - * configurations requires a modeset on all pipes >> > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed >> > * @a: first CDCLK configuration >> > * @b: second CDCLK configuration >> > * >> > * Returns: >> > - * True if changing between the two CDCLK configurations >> > - * requires all pipes to be off, false if not. >> > + * True if parameters changed in a way that requires programming the CDCLK >> > + * and False otherwise. >> > */ >> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, >> > - const struct intel_cdclk_config *b) >> > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, >> > + const struct intel_cdclk_config *b) >> >> The new name isn't very descriptive either. >> >> Outside the cd2x/crawl/squash cases we stil have to consider >> two cases: >> 1. cdclk frequency/pll changes (voltage level can change or not) >> 2. cdclk frequency/pll doesn't change, but voltage level needs to change >> >> And that difference is what intel_cdclk_needs_modeset() is trying >> convey. And intel_cdclk_changed() tells us whether anything at all >> is changing. > >intel_cdclk_clock_changed() is perhaps the best name I can >think of, off the top of my head. But I'm notoriously bad at >naming things so take that with a pinch of salt. Sounds better indeed. And I think intel_cdclk_changed() would be more concise, but that one is already taken internally. -- Gustavo Sousa
On Sat, Feb 03, 2024 at 10:25:18AM -0300, Gustavo Sousa wrote: > Quoting Ville Syrjälä (2024-02-02 16:58:37-03:00) > >On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: > >> Looks like the name and description of intel_cdclk_needs_modeset() > >> became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for > >> cdclk changes if possible"), when it became possible to update the cdclk > >> without requiring disabling the pipes when only changing the cd2x > >> divider was enough. > >> > >> Later on we also added the same type of support with squash and crawling > >> with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash > >> when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk > >> squasher to be reconfigured live") and commit d62686ba3b54 > >> ("drm/i915/adl_p: CDCLK crawl support for ADL"). > >> > >> As such, update that function's name and documentation to something more > >> appropriate, since the real checks for requiring modeset are done > >> elsewhere. > >> > >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > >> --- > >> > >> One thing worth noting here is that, with this change, we are left with an > >> awkward situation where two function names related to checking changes in cdclk: > >> > >> intel_cdclk_params_changed() and intel_cdclk_changed() > >> > >> , > >> > >> and I find it weird that we have intel_cdclk_changed(), which checks for the > >> voltage level as well. Shouldn't the voltage level be a function of cdclk and > >> ddi clock? Why do we need that? > >> > >> drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- > >> drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- > >> .../drm/i915/display/intel_display_power_well.c | 4 ++-- > >> 3 files changed, 11 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > >> index 26200ee3e23f..caadd880865f 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > >> @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, > >> } > >> > >> /** > >> - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK > >> - * configurations requires a modeset on all pipes > >> + * intel_cdclk_params_changed - Check whether CDCLK parameters changed > >> * @a: first CDCLK configuration > >> * @b: second CDCLK configuration > >> * > >> * Returns: > >> - * True if changing between the two CDCLK configurations > >> - * requires all pipes to be off, false if not. > >> + * True if parameters changed in a way that requires programming the CDCLK > >> + * and False otherwise. > >> */ > >> -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, > >> - const struct intel_cdclk_config *b) > >> +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, > >> + const struct intel_cdclk_config *b) > > > >The new name isn't very descriptive either. > > Yeah... I would much rather use intel_cdclk_changed(), but that one is > already taken. > > > > >Outside the cd2x/crawl/squash cases we stil have to consider > >two cases: > >1. cdclk frequency/pll changes (voltage level can change or not) > >2. cdclk frequency/pll doesn't change, but voltage level needs to change > > > >And that difference is what intel_cdclk_needs_modeset() is trying > >convey. And intel_cdclk_changed() tells us whether anything at all > >is changing. > > I might be missing something, but, by going through the specs, it looked > to me that voltage level was dependent on cdclk (as well as on ddi > clock) and not the other way around. That's why I find it odd that we > need an intel_cdclk_changed() that, besides looking for changes in > cdclk, also checks for the voltage level. > > In intel_set_cdclk(), we check intel_cdclk_changed() before continuing. > If, for example, there is a change in ddi clock that requires a change > in voltage level but no changes in cdclk, intel_cdclk_changed() would > return true, right? Wouldn't that make us unnecessarily go through > intel_set_cdclk()? intel_set_cdclk() is the thing that does the voltage change.
Hi, Ville. Sorry for taking long to get back to this. Quoting Ville Syrjälä (2024-02-05 12:34:57-03:00) >On Sat, Feb 03, 2024 at 10:25:18AM -0300, Gustavo Sousa wrote: >> Quoting Ville Syrjälä (2024-02-02 16:58:37-03:00) >> >On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: >> >> Looks like the name and description of intel_cdclk_needs_modeset() >> >> became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for >> >> cdclk changes if possible"), when it became possible to update the cdclk >> >> without requiring disabling the pipes when only changing the cd2x >> >> divider was enough. >> >> >> >> Later on we also added the same type of support with squash and crawling >> >> with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash >> >> when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk >> >> squasher to be reconfigured live") and commit d62686ba3b54 >> >> ("drm/i915/adl_p: CDCLK crawl support for ADL"). >> >> >> >> As such, update that function's name and documentation to something more >> >> appropriate, since the real checks for requiring modeset are done >> >> elsewhere. >> >> >> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >> >> --- >> >> >> >> One thing worth noting here is that, with this change, we are left with an >> >> awkward situation where two function names related to checking changes in cdclk: >> >> >> >> intel_cdclk_params_changed() and intel_cdclk_changed() >> >> >> >> , >> >> >> >> and I find it weird that we have intel_cdclk_changed(), which checks for the >> >> voltage level as well. Shouldn't the voltage level be a function of cdclk and >> >> ddi clock? Why do we need that? >> >> >> >> drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- >> >> drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- >> >> .../drm/i915/display/intel_display_power_well.c | 4 ++-- >> >> 3 files changed, 11 insertions(+), 12 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c >> >> index 26200ee3e23f..caadd880865f 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c >> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >> >> @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, >> >> } >> >> >> >> /** >> >> - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK >> >> - * configurations requires a modeset on all pipes >> >> + * intel_cdclk_params_changed - Check whether CDCLK parameters changed >> >> * @a: first CDCLK configuration >> >> * @b: second CDCLK configuration >> >> * >> >> * Returns: >> >> - * True if changing between the two CDCLK configurations >> >> - * requires all pipes to be off, false if not. >> >> + * True if parameters changed in a way that requires programming the CDCLK >> >> + * and False otherwise. >> >> */ >> >> -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, >> >> - const struct intel_cdclk_config *b) >> >> +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, >> >> + const struct intel_cdclk_config *b) >> > >> >The new name isn't very descriptive either. >> >> Yeah... I would much rather use intel_cdclk_changed(), but that one is >> already taken. >> >> > >> >Outside the cd2x/crawl/squash cases we stil have to consider >> >two cases: >> >1. cdclk frequency/pll changes (voltage level can change or not) >> >2. cdclk frequency/pll doesn't change, but voltage level needs to change >> > >> >And that difference is what intel_cdclk_needs_modeset() is trying >> >convey. And intel_cdclk_changed() tells us whether anything at all >> >is changing. >> >> I might be missing something, but, by going through the specs, it looked >> to me that voltage level was dependent on cdclk (as well as on ddi >> clock) and not the other way around. That's why I find it odd that we >> need an intel_cdclk_changed() that, besides looking for changes in >> cdclk, also checks for the voltage level. >> >> In intel_set_cdclk(), we check intel_cdclk_changed() before continuing. >> If, for example, there is a change in ddi clock that requires a change >> in voltage level but no changes in cdclk, intel_cdclk_changed() would >> return true, right? Wouldn't that make us unnecessarily go through >> intel_set_cdclk()? > >intel_set_cdclk() is the thing that does the voltage change. Yep and perhaps I provided an incomplete response above. Sorry. I was wondering if handling voltage level should really be intel_set_cdclk()'s responsibility. I might be missing the big picture here, but, at least for the recent platforms, I get the understanding that voltage level handling should be a separate step in the hardware commit process. Would it be possible to have a commit containing (i) update(s) to ddi clk and (ii) no update to cdclk such that (i) require an update to voltage level, right? -- Gustavo Sousa
On Wed, Feb 14, 2024 at 04:56:50PM -0300, Gustavo Sousa wrote: > Hi, Ville. > > Sorry for taking long to get back to this. > > Quoting Ville Syrjälä (2024-02-05 12:34:57-03:00) > >On Sat, Feb 03, 2024 at 10:25:18AM -0300, Gustavo Sousa wrote: > >> Quoting Ville Syrjälä (2024-02-02 16:58:37-03:00) > >> >On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: > >> >> Looks like the name and description of intel_cdclk_needs_modeset() > >> >> became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for > >> >> cdclk changes if possible"), when it became possible to update the cdclk > >> >> without requiring disabling the pipes when only changing the cd2x > >> >> divider was enough. > >> >> > >> >> Later on we also added the same type of support with squash and crawling > >> >> with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash > >> >> when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk > >> >> squasher to be reconfigured live") and commit d62686ba3b54 > >> >> ("drm/i915/adl_p: CDCLK crawl support for ADL"). > >> >> > >> >> As such, update that function's name and documentation to something more > >> >> appropriate, since the real checks for requiring modeset are done > >> >> elsewhere. > >> >> > >> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > >> >> --- > >> >> > >> >> One thing worth noting here is that, with this change, we are left with an > >> >> awkward situation where two function names related to checking changes in cdclk: > >> >> > >> >> intel_cdclk_params_changed() and intel_cdclk_changed() > >> >> > >> >> , > >> >> > >> >> and I find it weird that we have intel_cdclk_changed(), which checks for the > >> >> voltage level as well. Shouldn't the voltage level be a function of cdclk and > >> >> ddi clock? Why do we need that? > >> >> > >> >> drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- > >> >> drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- > >> >> .../drm/i915/display/intel_display_power_well.c | 4 ++-- > >> >> 3 files changed, 11 insertions(+), 12 deletions(-) > >> >> > >> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > >> >> index 26200ee3e23f..caadd880865f 100644 > >> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > >> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > >> >> @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, > >> >> } > >> >> > >> >> /** > >> >> - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK > >> >> - * configurations requires a modeset on all pipes > >> >> + * intel_cdclk_params_changed - Check whether CDCLK parameters changed > >> >> * @a: first CDCLK configuration > >> >> * @b: second CDCLK configuration > >> >> * > >> >> * Returns: > >> >> - * True if changing between the two CDCLK configurations > >> >> - * requires all pipes to be off, false if not. > >> >> + * True if parameters changed in a way that requires programming the CDCLK > >> >> + * and False otherwise. > >> >> */ > >> >> -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, > >> >> - const struct intel_cdclk_config *b) > >> >> +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, > >> >> + const struct intel_cdclk_config *b) > >> > > >> >The new name isn't very descriptive either. > >> > >> Yeah... I would much rather use intel_cdclk_changed(), but that one is > >> already taken. > >> > >> > > >> >Outside the cd2x/crawl/squash cases we stil have to consider > >> >two cases: > >> >1. cdclk frequency/pll changes (voltage level can change or not) > >> >2. cdclk frequency/pll doesn't change, but voltage level needs to change > >> > > >> >And that difference is what intel_cdclk_needs_modeset() is trying > >> >convey. And intel_cdclk_changed() tells us whether anything at all > >> >is changing. > >> > >> I might be missing something, but, by going through the specs, it looked > >> to me that voltage level was dependent on cdclk (as well as on ddi > >> clock) and not the other way around. That's why I find it odd that we > >> need an intel_cdclk_changed() that, besides looking for changes in > >> cdclk, also checks for the voltage level. > >> > >> In intel_set_cdclk(), we check intel_cdclk_changed() before continuing. > >> If, for example, there is a change in ddi clock that requires a change > >> in voltage level but no changes in cdclk, intel_cdclk_changed() would > >> return true, right? Wouldn't that make us unnecessarily go through > >> intel_set_cdclk()? > > > >intel_set_cdclk() is the thing that does the voltage change. > > Yep and perhaps I provided an incomplete response above. Sorry. > > I was wondering if handling voltage level should really be > intel_set_cdclk()'s responsibility. > > I might be missing the big picture here, but, at least for the recent > platforms, I get the understanding that voltage level handling should be > a separate step in the hardware commit process. > > Would it be possible to have a commit containing (i) update(s) to ddi > clk and (ii) no update to cdclk such that (i) require an update to > voltage level, right? That is possible yes. But I don't think there's much point in complicating things by splitting the voltage level stuff into a completely separate thing. What I think we could do is split .set_cdclk() into more fine grained steps so that: - it's easier to reuse individual pieces across platforms without ugly if ladders - perhaps make it a bit easier to skip unnecessary steps although the actual cdclk progrmaming in the nop case really just ends up being a CDLCK_CTL/etc. register write so not a big deal in practice.
Quoting Gustavo Sousa (2024-02-03 10:32:18-03:00) >Quoting Ville Syrjälä (2024-02-02 17:06:02-03:00) >>On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote: >>> On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: >>> > Looks like the name and description of intel_cdclk_needs_modeset() >>> > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for >>> > cdclk changes if possible"), when it became possible to update the cdclk >>> > without requiring disabling the pipes when only changing the cd2x >>> > divider was enough. >>> > >>> > Later on we also added the same type of support with squash and crawling >>> > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash >>> > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk >>> > squasher to be reconfigured live") and commit d62686ba3b54 >>> > ("drm/i915/adl_p: CDCLK crawl support for ADL"). >>> > >>> > As such, update that function's name and documentation to something more >>> > appropriate, since the real checks for requiring modeset are done >>> > elsewhere. >>> > >>> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >>> > --- >>> > >>> > One thing worth noting here is that, with this change, we are left with an >>> > awkward situation where two function names related to checking changes in cdclk: >>> > >>> > intel_cdclk_params_changed() and intel_cdclk_changed() >>> > >>> > , >>> > >>> > and I find it weird that we have intel_cdclk_changed(), which checks for the >>> > voltage level as well. Shouldn't the voltage level be a function of cdclk and >>> > ddi clock? Why do we need that? >>> > >>> > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- >>> > drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- >>> > .../drm/i915/display/intel_display_power_well.c | 4 ++-- >>> > 3 files changed, 11 insertions(+), 12 deletions(-) >>> > >>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c >>> > index 26200ee3e23f..caadd880865f 100644 >>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c >>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >>> > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, >>> > } >>> > >>> > /** >>> > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK >>> > - * configurations requires a modeset on all pipes >>> > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed >>> > * @a: first CDCLK configuration >>> > * @b: second CDCLK configuration >>> > * >>> > * Returns: >>> > - * True if changing between the two CDCLK configurations >>> > - * requires all pipes to be off, false if not. >>> > + * True if parameters changed in a way that requires programming the CDCLK >>> > + * and False otherwise. >>> > */ >>> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, >>> > - const struct intel_cdclk_config *b) >>> > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, >>> > + const struct intel_cdclk_config *b) >>> >>> The new name isn't very descriptive either. >>> >>> Outside the cd2x/crawl/squash cases we stil have to consider >>> two cases: >>> 1. cdclk frequency/pll changes (voltage level can change or not) >>> 2. cdclk frequency/pll doesn't change, but voltage level needs to change >>> >>> And that difference is what intel_cdclk_needs_modeset() is trying >>> convey. And intel_cdclk_changed() tells us whether anything at all >>> is changing. >> >>intel_cdclk_clock_changed() is perhaps the best name I can >>think of, off the top of my head. But I'm notoriously bad at >>naming things so take that with a pinch of salt. > >Sounds better indeed. And I think intel_cdclk_changed() would be more >concise, but that one is already taken internally. Hm... On second thought, I'm not sure intel_cdclk_clock_changed() would be very accurate, at least with the current implementation. We are checking not only on the resulting cdclk frequency, but also on ref and vco values. Although I'm not sure if that could happen in real life, but the current implementation gives me the interpretation that, in theory, a change in ref or vco could happen even if the cdclk value did not change. Or are we safe to say that checking on cdclk should be enough and looking at vco and ref is actually redundant? -- Gustavo Sousa
On Wed, Feb 14, 2024 at 05:08:48PM -0300, Gustavo Sousa wrote: > Quoting Gustavo Sousa (2024-02-03 10:32:18-03:00) > >Quoting Ville Syrjälä (2024-02-02 17:06:02-03:00) > >>On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote: > >>> On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: > >>> > Looks like the name and description of intel_cdclk_needs_modeset() > >>> > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for > >>> > cdclk changes if possible"), when it became possible to update the cdclk > >>> > without requiring disabling the pipes when only changing the cd2x > >>> > divider was enough. > >>> > > >>> > Later on we also added the same type of support with squash and crawling > >>> > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash > >>> > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk > >>> > squasher to be reconfigured live") and commit d62686ba3b54 > >>> > ("drm/i915/adl_p: CDCLK crawl support for ADL"). > >>> > > >>> > As such, update that function's name and documentation to something more > >>> > appropriate, since the real checks for requiring modeset are done > >>> > elsewhere. > >>> > > >>> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > >>> > --- > >>> > > >>> > One thing worth noting here is that, with this change, we are left with an > >>> > awkward situation where two function names related to checking changes in cdclk: > >>> > > >>> > intel_cdclk_params_changed() and intel_cdclk_changed() > >>> > > >>> > , > >>> > > >>> > and I find it weird that we have intel_cdclk_changed(), which checks for the > >>> > voltage level as well. Shouldn't the voltage level be a function of cdclk and > >>> > ddi clock? Why do we need that? > >>> > > >>> > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- > >>> > drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- > >>> > .../drm/i915/display/intel_display_power_well.c | 4 ++-- > >>> > 3 files changed, 11 insertions(+), 12 deletions(-) > >>> > > >>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > >>> > index 26200ee3e23f..caadd880865f 100644 > >>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > >>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > >>> > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, > >>> > } > >>> > > >>> > /** > >>> > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK > >>> > - * configurations requires a modeset on all pipes > >>> > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed > >>> > * @a: first CDCLK configuration > >>> > * @b: second CDCLK configuration > >>> > * > >>> > * Returns: > >>> > - * True if changing between the two CDCLK configurations > >>> > - * requires all pipes to be off, false if not. > >>> > + * True if parameters changed in a way that requires programming the CDCLK > >>> > + * and False otherwise. > >>> > */ > >>> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, > >>> > - const struct intel_cdclk_config *b) > >>> > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, > >>> > + const struct intel_cdclk_config *b) > >>> > >>> The new name isn't very descriptive either. > >>> > >>> Outside the cd2x/crawl/squash cases we stil have to consider > >>> two cases: > >>> 1. cdclk frequency/pll changes (voltage level can change or not) > >>> 2. cdclk frequency/pll doesn't change, but voltage level needs to change > >>> > >>> And that difference is what intel_cdclk_needs_modeset() is trying > >>> convey. And intel_cdclk_changed() tells us whether anything at all > >>> is changing. > >> > >>intel_cdclk_clock_changed() is perhaps the best name I can > >>think of, off the top of my head. But I'm notoriously bad at > >>naming things so take that with a pinch of salt. > > > >Sounds better indeed. And I think intel_cdclk_changed() would be more > >concise, but that one is already taken internally. > > Hm... On second thought, I'm not sure intel_cdclk_clock_changed() would > be very accurate, at least with the current implementation. We are > checking not only on the resulting cdclk frequency, but also on > ref and vco values. That's all about the clock. > > Although I'm not sure if that could happen in real life, but the current > implementation gives me the interpretation that, in theory, a change in > ref or vco could happen even if the cdclk value did not change. Or are > we safe to say that checking on cdclk should be enough and looking at > vco and ref is actually redundant? ref never changes, vco could in theory change such that the resulting cdclk stays the same, but probably no real world examples of that. But I don't see any point in putting those kinds of assumptions into the code given that it wouldn't provide any actual benefits.
Quoting Ville Syrjälä (2024-02-14 17:15:51-03:00) >On Wed, Feb 14, 2024 at 05:08:48PM -0300, Gustavo Sousa wrote: >> Quoting Gustavo Sousa (2024-02-03 10:32:18-03:00) >> >Quoting Ville Syrjälä (2024-02-02 17:06:02-03:00) >> >>On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote: >> >>> On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote: >> >>> > Looks like the name and description of intel_cdclk_needs_modeset() >> >>> > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for >> >>> > cdclk changes if possible"), when it became possible to update the cdclk >> >>> > without requiring disabling the pipes when only changing the cd2x >> >>> > divider was enough. >> >>> > >> >>> > Later on we also added the same type of support with squash and crawling >> >>> > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash >> >>> > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk >> >>> > squasher to be reconfigured live") and commit d62686ba3b54 >> >>> > ("drm/i915/adl_p: CDCLK crawl support for ADL"). >> >>> > >> >>> > As such, update that function's name and documentation to something more >> >>> > appropriate, since the real checks for requiring modeset are done >> >>> > elsewhere. >> >>> > >> >>> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >> >>> > --- >> >>> > >> >>> > One thing worth noting here is that, with this change, we are left with an >> >>> > awkward situation where two function names related to checking changes in cdclk: >> >>> > >> >>> > intel_cdclk_params_changed() and intel_cdclk_changed() >> >>> > >> >>> > , >> >>> > >> >>> > and I find it weird that we have intel_cdclk_changed(), which checks for the >> >>> > voltage level as well. Shouldn't the voltage level be a function of cdclk and >> >>> > ddi clock? Why do we need that? >> >>> > >> >>> > drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- >> >>> > drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- >> >>> > .../drm/i915/display/intel_display_power_well.c | 4 ++-- >> >>> > 3 files changed, 11 insertions(+), 12 deletions(-) >> >>> > >> >>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c >> >>> > index 26200ee3e23f..caadd880865f 100644 >> >>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c >> >>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >> >>> > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, >> >>> > } >> >>> > >> >>> > /** >> >>> > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK >> >>> > - * configurations requires a modeset on all pipes >> >>> > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed >> >>> > * @a: first CDCLK configuration >> >>> > * @b: second CDCLK configuration >> >>> > * >> >>> > * Returns: >> >>> > - * True if changing between the two CDCLK configurations >> >>> > - * requires all pipes to be off, false if not. >> >>> > + * True if parameters changed in a way that requires programming the CDCLK >> >>> > + * and False otherwise. >> >>> > */ >> >>> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, >> >>> > - const struct intel_cdclk_config *b) >> >>> > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, >> >>> > + const struct intel_cdclk_config *b) >> >>> >> >>> The new name isn't very descriptive either. >> >>> >> >>> Outside the cd2x/crawl/squash cases we stil have to consider >> >>> two cases: >> >>> 1. cdclk frequency/pll changes (voltage level can change or not) >> >>> 2. cdclk frequency/pll doesn't change, but voltage level needs to change >> >>> >> >>> And that difference is what intel_cdclk_needs_modeset() is trying >> >>> convey. And intel_cdclk_changed() tells us whether anything at all >> >>> is changing. >> >> >> >>intel_cdclk_clock_changed() is perhaps the best name I can >> >>think of, off the top of my head. But I'm notoriously bad at >> >>naming things so take that with a pinch of salt. >> > >> >Sounds better indeed. And I think intel_cdclk_changed() would be more >> >concise, but that one is already taken internally. >> >> Hm... On second thought, I'm not sure intel_cdclk_clock_changed() would >> be very accurate, at least with the current implementation. We are >> checking not only on the resulting cdclk frequency, but also on >> ref and vco values. > >That's all about the clock. > >> >> Although I'm not sure if that could happen in real life, but the current >> implementation gives me the interpretation that, in theory, a change in >> ref or vco could happen even if the cdclk value did not change. Or are >> we safe to say that checking on cdclk should be enough and looking at >> vco and ref is actually redundant? > >ref never changes, vco could in theory change such that the >resulting cdclk stays the same, but probably no real world >examples of that. > >But I don't see any point in putting those kinds of assumptions >into the code given that it wouldn't provide any actual benefits. Alright. Thanks! I've sent a v2 using the proposed name: https://patchwork.freedesktop.org/series/129908/ -- Gustavo Sousa
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index 26200ee3e23f..caadd880865f 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv, } /** - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK - * configurations requires a modeset on all pipes + * intel_cdclk_params_changed - Check whether CDCLK parameters changed * @a: first CDCLK configuration * @b: second CDCLK configuration * * Returns: - * True if changing between the two CDCLK configurations - * requires all pipes to be off, false if not. + * True if parameters changed in a way that requires programming the CDCLK + * and False otherwise. */ -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, - const struct intel_cdclk_config *b) +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, + const struct intel_cdclk_config *b) { return a->cdclk != b->cdclk || a->vco != b->vco || @@ -2295,7 +2294,7 @@ static bool intel_cdclk_can_cd2x_update(struct drm_i915_private *dev_priv, static bool intel_cdclk_changed(const struct intel_cdclk_config *a, const struct intel_cdclk_config *b) { - return intel_cdclk_needs_modeset(a, b) || + return intel_cdclk_params_changed(a, b) || a->voltage_level != b->voltage_level; } @@ -3202,7 +3201,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state) drm_dbg_kms(&dev_priv->drm, "Can change cdclk cd2x divider with pipe %c active\n", pipe_name(pipe)); - } else if (intel_cdclk_needs_modeset(&old_cdclk_state->actual, + } else if (intel_cdclk_params_changed(&old_cdclk_state->actual, &new_cdclk_state->actual)) { /* All pipes must be switched off while we change the cdclk. */ ret = intel_modeset_all_pipes_late(state, "CDCLK change"); diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h index 48fd7d39e0cd..f8d3b2b3be6a 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.h +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h @@ -60,8 +60,8 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv); void intel_update_max_cdclk(struct drm_i915_private *dev_priv); void intel_update_cdclk(struct drm_i915_private *dev_priv); u32 intel_read_rawclk(struct drm_i915_private *dev_priv); -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a, - const struct intel_cdclk_config *b); +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a, + const struct intel_cdclk_config *b); void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state); void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state); void intel_cdclk_dump_config(struct drm_i915_private *i915, diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c index 47cd6bb04366..7364ca520764 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c @@ -968,8 +968,8 @@ void gen9_disable_dc_states(struct drm_i915_private *dev_priv) intel_cdclk_get_cdclk(dev_priv, &cdclk_config); /* Can't read out voltage_level so can't use intel_cdclk_changed() */ drm_WARN_ON(&dev_priv->drm, - intel_cdclk_needs_modeset(&dev_priv->display.cdclk.hw, - &cdclk_config)); + intel_cdclk_params_changed(&dev_priv->display.cdclk.hw, + &cdclk_config)); gen9_assert_dbuf_enabled(dev_priv);
Looks like the name and description of intel_cdclk_needs_modeset() became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for cdclk changes if possible"), when it became possible to update the cdclk without requiring disabling the pipes when only changing the cd2x divider was enough. Later on we also added the same type of support with squash and crawling with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk squasher to be reconfigured live") and commit d62686ba3b54 ("drm/i915/adl_p: CDCLK crawl support for ADL"). As such, update that function's name and documentation to something more appropriate, since the real checks for requiring modeset are done elsewhere. Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> --- One thing worth noting here is that, with this change, we are left with an awkward situation where two function names related to checking changes in cdclk: intel_cdclk_params_changed() and intel_cdclk_changed() , and I find it weird that we have intel_cdclk_changed(), which checks for the voltage level as well. Shouldn't the voltage level be a function of cdclk and ddi clock? Why do we need that? drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++-------- drivers/gpu/drm/i915/display/intel_cdclk.h | 4 ++-- .../drm/i915/display/intel_display_power_well.c | 4 ++-- 3 files changed, 11 insertions(+), 12 deletions(-)