| Message ID | 20190820110631.15793-1-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/tgl: Lower cdclk for sub 4k resolutions | expand |
On Tue, Aug 20, 2019 at 02:06:31PM +0300, Mika Kahola wrote: > In order to achieve improved power savings we can tune down CD clock frequency > for sub 4k resolutions. The maximum CD clock frequency for sub 4k > resolutions is set to 172.8 MHz. > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++++++++++++++++++++- > drivers/gpu/drm/i915/display/intel_cdclk.h | 3 +++ > 2 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index d0bc42e5039c..1d6c7bc79470 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -2610,6 +2610,24 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv) > return max_cdclk_freq*90/100; > } > > +bool mode_is_4k(struct drm_i915_private *dev_priv) > +{ > + struct intel_crtc *crtc; > + struct intel_crtc_state *pipe_config; > + > + for_each_intel_crtc(&dev_priv->drm, crtc) { > + pipe_config = to_intel_crtc_state(crtc->base.state); > + > + if (pipe_config->base.active) { > + if (pipe_config->pipe_src_w >= WIDTH_4K && > + pipe_config->pipe_src_h >= HEIGHT_4K) > + return true; > + } > + } > + > + return false; > +} > + > /** > * intel_update_max_cdclk - Determine the maximum support CDCLK frequency > * @dev_priv: i915 device > @@ -2620,7 +2638,13 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv) > */ > void intel_update_max_cdclk(struct drm_i915_private *dev_priv) > { > - if (IS_ELKHARTLAKE(dev_priv)) { > + /* > + * Use lower CDCLK frequency on Tigerlake when selected > + * mode is less than 4k. > + */ > + if (INTEL_GEN(dev_priv) >= 12 && !mode_is_4k(dev_priv)) { > + dev_priv->max_cdclk_freq = 172800; The maximum is just that, the maximum. It doesn't affect the actual cdclk chosen (outside of rejecting everything exceeding the max). And the maximum won't ever change, so trying to calculate it based on some ephemeral crtc states doesn't make sense. Given that our policy is to always go for the minimum acceptable cdclk frequency I don't think there is any work to be done to get proper power savings for <4k. What is the actual problem you're seeing? > + } else if (IS_ELKHARTLAKE(dev_priv)) { > if (dev_priv->cdclk.hw.ref == 24000) > dev_priv->max_cdclk_freq = 552000; > else > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h > index 4d6f7f5f8930..cefb5146ddca 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > @@ -15,6 +15,9 @@ struct intel_atomic_state; > struct intel_cdclk_state; > struct intel_crtc_state; > > +#define WIDTH_4K 3860 > +#define HEIGHT_4K 2160 > + > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state); > void intel_cdclk_init(struct drm_i915_private *i915); > void intel_cdclk_uninit(struct drm_i915_private *i915); > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2019-08-20 at 16:03 +0300, Ville Syrjälä wrote: > On Tue, Aug 20, 2019 at 02:06:31PM +0300, Mika Kahola wrote: > > In order to achieve improved power savings we can tune down CD > > clock frequency > > for sub 4k resolutions. The maximum CD clock frequency for sub 4k > > resolutions is set to 172.8 MHz. > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 26 > > +++++++++++++++++++++- > > drivers/gpu/drm/i915/display/intel_cdclk.h | 3 +++ > > 2 files changed, 28 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > > b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index d0bc42e5039c..1d6c7bc79470 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -2610,6 +2610,24 @@ static int intel_compute_max_dotclk(struct > > drm_i915_private *dev_priv) > > return max_cdclk_freq*90/100; > > } > > > > +bool mode_is_4k(struct drm_i915_private *dev_priv) > > +{ > > + struct intel_crtc *crtc; > > + struct intel_crtc_state *pipe_config; > > + > > + for_each_intel_crtc(&dev_priv->drm, crtc) { > > + pipe_config = to_intel_crtc_state(crtc->base.state); > > + > > + if (pipe_config->base.active) { > > + if (pipe_config->pipe_src_w >= WIDTH_4K && > > + pipe_config->pipe_src_h >= HEIGHT_4K) > > + return true; > > + } > > + } > > + > > + return false; > > +} > > + > > /** > > * intel_update_max_cdclk - Determine the maximum support CDCLK > > frequency > > * @dev_priv: i915 device > > @@ -2620,7 +2638,13 @@ static int intel_compute_max_dotclk(struct > > drm_i915_private *dev_priv) > > */ > > void intel_update_max_cdclk(struct drm_i915_private *dev_priv) > > { > > - if (IS_ELKHARTLAKE(dev_priv)) { > > + /* > > + * Use lower CDCLK frequency on Tigerlake when selected > > + * mode is less than 4k. > > + */ > > + if (INTEL_GEN(dev_priv) >= 12 && !mode_is_4k(dev_priv)) { > > + dev_priv->max_cdclk_freq = 172800; > > The maximum is just that, the maximum. It doesn't affect the actual > cdclk chosen (outside of rejecting everything exceeding the max). > And the maximum won't ever change, so trying to calculate it based > on some ephemeral crtc states doesn't make sense. > > Given that our policy is to always go for the minimum acceptable > cdclk > frequency I don't think there is any work to be done to get proper > power savings for <4k. What is the actual problem you're seeing? The actual problem is that this is a requested feature for TGL. I admit, with these suggested optimizations the gains will be marginal. My interpretation of this feature was that we should not exceed 172.8MHz with the sub 4k modes, hence I'm suggesting in this patch to limit the max cdclock to this number. So, how do we get forward? Should I propose that we drop this feature or should we implement this differently? > > > + } else if (IS_ELKHARTLAKE(dev_priv)) { > > if (dev_priv->cdclk.hw.ref == 24000) > > dev_priv->max_cdclk_freq = 552000; > > else > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h > > b/drivers/gpu/drm/i915/display/intel_cdclk.h > > index 4d6f7f5f8930..cefb5146ddca 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > > @@ -15,6 +15,9 @@ struct intel_atomic_state; > > struct intel_cdclk_state; > > struct intel_crtc_state; > > > > +#define WIDTH_4K 3860 > > +#define HEIGHT_4K 2160 > > + > > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state > > *crtc_state); > > void intel_cdclk_init(struct drm_i915_private *i915); > > void intel_cdclk_uninit(struct drm_i915_private *i915); > > -- > > 2.17.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
>-----Original Message----- >From: Kahola, Mika >Sent: Tuesday, August 20, 2019 4:37 PM >To: intel-gfx@lists.freedesktop.org >Cc: Shankar, Uma <uma.shankar@intel.com>; Kahola, Mika ><mika.kahola@intel.com> >Subject: [PATCH] drm/i915/tgl: Lower cdclk for sub 4k resolutions > >In order to achieve improved power savings we can tune down CD clock frequency for >sub 4k resolutions. The maximum CD clock frequency for sub 4k resolutions is set to >172.8 MHz. > >Signed-off-by: Mika Kahola <mika.kahola@intel.com> >--- > drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++++++++++++++++++++- >drivers/gpu/drm/i915/display/intel_cdclk.h | 3 +++ > 2 files changed, 28 insertions(+), 1 deletion(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c >b/drivers/gpu/drm/i915/display/intel_cdclk.c >index d0bc42e5039c..1d6c7bc79470 100644 >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >@@ -2610,6 +2610,24 @@ static int intel_compute_max_dotclk(struct >drm_i915_private *dev_priv) > return max_cdclk_freq*90/100; > } > >+bool mode_is_4k(struct drm_i915_private *dev_priv) { >+ struct intel_crtc *crtc; >+ struct intel_crtc_state *pipe_config; >+ >+ for_each_intel_crtc(&dev_priv->drm, crtc) { >+ pipe_config = to_intel_crtc_state(crtc->base.state); >+ >+ if (pipe_config->base.active) { >+ if (pipe_config->pipe_src_w >= WIDTH_4K && >+ pipe_config->pipe_src_h >= HEIGHT_4K) >+ return true; >+ } >+ } >+ >+ return false; >+} >+ > /** > * intel_update_max_cdclk - Determine the maximum support CDCLK frequency > * @dev_priv: i915 device >@@ -2620,7 +2638,13 @@ static int intel_compute_max_dotclk(struct >drm_i915_private *dev_priv) > */ > void intel_update_max_cdclk(struct drm_i915_private *dev_priv) { >- if (IS_ELKHARTLAKE(dev_priv)) { >+ /* >+ * Use lower CDCLK frequency on Tigerlake when selected >+ * mode is less than 4k. >+ */ >+ if (INTEL_GEN(dev_priv) >= 12 && !mode_is_4k(dev_priv)) { >+ dev_priv->max_cdclk_freq = 172800; >+ } else if (IS_ELKHARTLAKE(dev_priv)) { Setting the max cd clock supported itself to a lower value is not a good idea. This should return what is the max frequency of cd clock the hardware supports. Driver is not going to program this to max based on this data. Actual cd clock which will be programmed in hardware should be based on the maximum pixel clock we are driving. So if we are not driving 4K and say at 1920x1080, pixel clock will be less and we should be selecting a lower value of cd clock in that case which gets programmed in CDCLK_CTL. But max cd clock still remains what maximum the platform can support. You can check intel_compute_min_cdclk which calculates the minimum cd clock required. So when we just have 1920x1080@60Hz pixel clock of 148500, min cd clock required should be 148500 and nearest higher value of CD Clock will be 172.8Mhz. Similarly for 4096x2160@60, pixel clock 556188, min cd clock calculated will be 556.188 MHz, thus getting nearest supported cd clock value of 556 or 648Mhz. This should be taken care by this logic in driver. Also with the current patch, modes like 2560x1600@60, pixel clock 268.5MHz will not work. This will need cd clock of 324Mhz (or 307, 312 whatever are supported on the platform. > if (dev_priv->cdclk.hw.ref == 24000) > dev_priv->max_cdclk_freq = 552000; > else >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h >b/drivers/gpu/drm/i915/display/intel_cdclk.h >index 4d6f7f5f8930..cefb5146ddca 100644 >--- a/drivers/gpu/drm/i915/display/intel_cdclk.h >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h >@@ -15,6 +15,9 @@ struct intel_atomic_state; struct intel_cdclk_state; struct >intel_crtc_state; > >+#define WIDTH_4K 3860 >+#define HEIGHT_4K 2160 >+ > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state); void >intel_cdclk_init(struct drm_i915_private *i915); void intel_cdclk_uninit(struct >drm_i915_private *i915); >-- >2.17.1
On Tue, Aug 20, 2019 at 01:35:37PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Kahola, Mika > >Sent: Tuesday, August 20, 2019 4:37 PM > >To: intel-gfx@lists.freedesktop.org > >Cc: Shankar, Uma <uma.shankar@intel.com>; Kahola, Mika > ><mika.kahola@intel.com> > >Subject: [PATCH] drm/i915/tgl: Lower cdclk for sub 4k resolutions > > > >In order to achieve improved power savings we can tune down CD clock frequency for > >sub 4k resolutions. The maximum CD clock frequency for sub 4k resolutions is set to > >172.8 MHz. > > > >Signed-off-by: Mika Kahola <mika.kahola@intel.com> > >--- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++++++++++++++++++++- > >drivers/gpu/drm/i915/display/intel_cdclk.h | 3 +++ > > 2 files changed, 28 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > >b/drivers/gpu/drm/i915/display/intel_cdclk.c > >index d0bc42e5039c..1d6c7bc79470 100644 > >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c > >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > >@@ -2610,6 +2610,24 @@ static int intel_compute_max_dotclk(struct > >drm_i915_private *dev_priv) > > return max_cdclk_freq*90/100; > > } > > > >+bool mode_is_4k(struct drm_i915_private *dev_priv) { > >+ struct intel_crtc *crtc; > >+ struct intel_crtc_state *pipe_config; > >+ > >+ for_each_intel_crtc(&dev_priv->drm, crtc) { > >+ pipe_config = to_intel_crtc_state(crtc->base.state); > >+ > >+ if (pipe_config->base.active) { > >+ if (pipe_config->pipe_src_w >= WIDTH_4K && > >+ pipe_config->pipe_src_h >= HEIGHT_4K) > >+ return true; > >+ } > >+ } > >+ > >+ return false; > >+} > >+ > > /** > > * intel_update_max_cdclk - Determine the maximum support CDCLK frequency > > * @dev_priv: i915 device > >@@ -2620,7 +2638,13 @@ static int intel_compute_max_dotclk(struct > >drm_i915_private *dev_priv) > > */ > > void intel_update_max_cdclk(struct drm_i915_private *dev_priv) { > >- if (IS_ELKHARTLAKE(dev_priv)) { > >+ /* > >+ * Use lower CDCLK frequency on Tigerlake when selected > >+ * mode is less than 4k. > >+ */ > >+ if (INTEL_GEN(dev_priv) >= 12 && !mode_is_4k(dev_priv)) { > >+ dev_priv->max_cdclk_freq = 172800; > >+ } else if (IS_ELKHARTLAKE(dev_priv)) { > > Setting the max cd clock supported itself to a lower value is not a good idea. > This should return what is the max frequency of cd clock the hardware supports. > Driver is not going to program this to max based on this data. > > Actual cd clock which will be programmed in hardware should be based on the > maximum pixel clock we are driving. So if we are not driving 4K and say at 1920x1080, pixel > clock will be less and we should be selecting a lower value of cd clock in that case which > gets programmed in CDCLK_CTL. But max cd clock still remains what maximum the platform can > support. You can check intel_compute_min_cdclk which calculates the minimum cd clock required. > > So when we just have 1920x1080@60Hz pixel clock of 148500, > min cd clock required should be 148500 and nearest higher value of CD Clock will be 172.8Mhz. > Similarly for 4096x2160@60, pixel clock 556188, min cd clock calculated will be 556.188 MHz, thus > getting nearest supported cd clock value of 556 or 648Mhz. > > This should be taken care by this logic in driver. > > Also with the current patch, modes like 2560x1600@60, pixel clock 268.5MHz will not work. This will need > cd clock of 324Mhz (or 307, 312 whatever are supported on the platform. glk+ pump out two pixels per clock, so we need half of what you said. > > > if (dev_priv->cdclk.hw.ref == 24000) > > dev_priv->max_cdclk_freq = 552000; > > else > >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h > >b/drivers/gpu/drm/i915/display/intel_cdclk.h > >index 4d6f7f5f8930..cefb5146ddca 100644 > >--- a/drivers/gpu/drm/i915/display/intel_cdclk.h > >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > >@@ -15,6 +15,9 @@ struct intel_atomic_state; struct intel_cdclk_state; struct > >intel_crtc_state; > > > >+#define WIDTH_4K 3860 > >+#define HEIGHT_4K 2160 > >+ > > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state); void > >intel_cdclk_init(struct drm_i915_private *i915); void intel_cdclk_uninit(struct > >drm_i915_private *i915); > >-- > >2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Aug 20, 2019 at 01:22:00PM +0000, Kahola, Mika wrote: > On Tue, 2019-08-20 at 16:03 +0300, Ville Syrjälä wrote: > > On Tue, Aug 20, 2019 at 02:06:31PM +0300, Mika Kahola wrote: > > > In order to achieve improved power savings we can tune down CD > > > clock frequency > > > for sub 4k resolutions. The maximum CD clock frequency for sub 4k > > > resolutions is set to 172.8 MHz. > > > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_cdclk.c | 26 > > > +++++++++++++++++++++- > > > drivers/gpu/drm/i915/display/intel_cdclk.h | 3 +++ > > > 2 files changed, 28 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c > > > index d0bc42e5039c..1d6c7bc79470 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > > @@ -2610,6 +2610,24 @@ static int intel_compute_max_dotclk(struct > > > drm_i915_private *dev_priv) > > > return max_cdclk_freq*90/100; > > > } > > > > > > +bool mode_is_4k(struct drm_i915_private *dev_priv) > > > +{ > > > + struct intel_crtc *crtc; > > > + struct intel_crtc_state *pipe_config; > > > + > > > + for_each_intel_crtc(&dev_priv->drm, crtc) { > > > + pipe_config = to_intel_crtc_state(crtc->base.state); > > > + > > > + if (pipe_config->base.active) { > > > + if (pipe_config->pipe_src_w >= WIDTH_4K && > > > + pipe_config->pipe_src_h >= HEIGHT_4K) > > > + return true; > > > + } > > > + } > > > + > > > + return false; > > > +} > > > + > > > /** > > > * intel_update_max_cdclk - Determine the maximum support CDCLK > > > frequency > > > * @dev_priv: i915 device > > > @@ -2620,7 +2638,13 @@ static int intel_compute_max_dotclk(struct > > > drm_i915_private *dev_priv) > > > */ > > > void intel_update_max_cdclk(struct drm_i915_private *dev_priv) > > > { > > > - if (IS_ELKHARTLAKE(dev_priv)) { > > > + /* > > > + * Use lower CDCLK frequency on Tigerlake when selected > > > + * mode is less than 4k. > > > + */ > > > + if (INTEL_GEN(dev_priv) >= 12 && !mode_is_4k(dev_priv)) { > > > + dev_priv->max_cdclk_freq = 172800; > > > > The maximum is just that, the maximum. It doesn't affect the actual > > cdclk chosen (outside of rejecting everything exceeding the max). > > And the maximum won't ever change, so trying to calculate it based > > on some ephemeral crtc states doesn't make sense. > > > > Given that our policy is to always go for the minimum acceptable > > cdclk > > frequency I don't think there is any work to be done to get proper > > power savings for <4k. What is the actual problem you're seeing? > The actual problem is that this is a requested feature for TGL. I > admit, with these suggested optimizations the gains will be marginal. > > My interpretation of this feature was that we should not exceed > 172.8MHz with the sub 4k modes, hence I'm suggesting in this patch to limit the max cdclock to this number. > > So, how do we get forward? Should I propose that we drop this feature > or should we implement this differently? There is nothing to implement. The current policy already picks the minimum cdclk that will work.
>> >-----Original Message----- >> >From: Kahola, Mika >> >Sent: Tuesday, August 20, 2019 4:37 PM >> >To: intel-gfx@lists.freedesktop.org >> >Cc: Shankar, Uma <uma.shankar@intel.com>; Kahola, Mika >> ><mika.kahola@intel.com> >> >Subject: [PATCH] drm/i915/tgl: Lower cdclk for sub 4k resolutions >> > >> >In order to achieve improved power savings we can tune down CD clock >> >frequency for sub 4k resolutions. The maximum CD clock frequency for >> >sub 4k resolutions is set to >> >172.8 MHz. >> > >> >Signed-off-by: Mika Kahola <mika.kahola@intel.com> >> >--- >> > drivers/gpu/drm/i915/display/intel_cdclk.c | 26 >> >+++++++++++++++++++++- drivers/gpu/drm/i915/display/intel_cdclk.h | >> >3 +++ >> > 2 files changed, 28 insertions(+), 1 deletion(-) >> > >> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c >> >b/drivers/gpu/drm/i915/display/intel_cdclk.c >> >index d0bc42e5039c..1d6c7bc79470 100644 >> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c >> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >> >@@ -2610,6 +2610,24 @@ static int intel_compute_max_dotclk(struct >> >drm_i915_private *dev_priv) >> > return max_cdclk_freq*90/100; >> > } >> > >> >+bool mode_is_4k(struct drm_i915_private *dev_priv) { >> >+ struct intel_crtc *crtc; >> >+ struct intel_crtc_state *pipe_config; >> >+ >> >+ for_each_intel_crtc(&dev_priv->drm, crtc) { >> >+ pipe_config = to_intel_crtc_state(crtc->base.state); >> >+ >> >+ if (pipe_config->base.active) { >> >+ if (pipe_config->pipe_src_w >= WIDTH_4K && >> >+ pipe_config->pipe_src_h >= HEIGHT_4K) >> >+ return true; >> >+ } >> >+ } >> >+ >> >+ return false; >> >+} >> >+ >> > /** >> > * intel_update_max_cdclk - Determine the maximum support CDCLK >> >frequency >> > * @dev_priv: i915 device >> >@@ -2620,7 +2638,13 @@ static int intel_compute_max_dotclk(struct >> >drm_i915_private *dev_priv) >> > */ >> > void intel_update_max_cdclk(struct drm_i915_private *dev_priv) { >> >- if (IS_ELKHARTLAKE(dev_priv)) { >> >+ /* >> >+ * Use lower CDCLK frequency on Tigerlake when selected >> >+ * mode is less than 4k. >> >+ */ >> >+ if (INTEL_GEN(dev_priv) >= 12 && !mode_is_4k(dev_priv)) { >> >+ dev_priv->max_cdclk_freq = 172800; >> >+ } else if (IS_ELKHARTLAKE(dev_priv)) { >> >> Setting the max cd clock supported itself to a lower value is not a good idea. >> This should return what is the max frequency of cd clock the hardware supports. >> Driver is not going to program this to max based on this data. >> >> Actual cd clock which will be programmed in hardware should be based >> on the maximum pixel clock we are driving. So if we are not driving 4K >> and say at 1920x1080, pixel clock will be less and we should be >> selecting a lower value of cd clock in that case which gets programmed >> in CDCLK_CTL. But max cd clock still remains what maximum the platform can >support. You can check intel_compute_min_cdclk which calculates the minimum cd >clock required. >> >> So when we just have 1920x1080@60Hz pixel clock of 148500, min cd >> clock required should be 148500 and nearest higher value of CD Clock will be >172.8Mhz. >> Similarly for 4096x2160@60, pixel clock 556188, min cd clock >> calculated will be 556.188 MHz, thus getting nearest supported cd clock value of >556 or 648Mhz. >> >> This should be taken care by this logic in driver. >> >> Also with the current patch, modes like 2560x1600@60, pixel clock >> 268.5MHz will not work. This will need cd clock of 324Mhz (or 307, 312 whatever >are supported on the platform. > >glk+ pump out two pixels per clock, so we need half of what you said. Yeah correct Ville. From GLK+ its 2 pixels per clock. >> >> > if (dev_priv->cdclk.hw.ref == 24000) >> > dev_priv->max_cdclk_freq = 552000; >> > else >> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h >> >b/drivers/gpu/drm/i915/display/intel_cdclk.h >> >index 4d6f7f5f8930..cefb5146ddca 100644 >> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.h >> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h >> >@@ -15,6 +15,9 @@ struct intel_atomic_state; struct >> >intel_cdclk_state; struct intel_crtc_state; >> > >> >+#define WIDTH_4K 3860 >> >+#define HEIGHT_4K 2160 >> >+ >> > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state >> >*crtc_state); void intel_cdclk_init(struct drm_i915_private *i915); >> >void intel_cdclk_uninit(struct drm_i915_private *i915); >> >-- >> >2.17.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Ville Syrjälä >Intel
On Tue, 2019-08-20 at 19:05 +0530, Shankar, Uma wrote: > > -----Original Message----- > > From: Kahola, Mika > > Sent: Tuesday, August 20, 2019 4:37 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Shankar, Uma <uma.shankar@intel.com>; Kahola, Mika > > <mika.kahola@intel.com> > > Subject: [PATCH] drm/i915/tgl: Lower cdclk for sub 4k resolutions > > > > In order to achieve improved power savings we can tune down CD > > clock frequency for > > sub 4k resolutions. The maximum CD clock frequency for sub 4k > > resolutions is set to > > 172.8 MHz. > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 26 > > +++++++++++++++++++++- > > drivers/gpu/drm/i915/display/intel_cdclk.h | 3 +++ > > 2 files changed, 28 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > > b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index d0bc42e5039c..1d6c7bc79470 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -2610,6 +2610,24 @@ static int intel_compute_max_dotclk(struct > > drm_i915_private *dev_priv) > > return max_cdclk_freq*90/100; > > } > > > > +bool mode_is_4k(struct drm_i915_private *dev_priv) { > > + struct intel_crtc *crtc; > > + struct intel_crtc_state *pipe_config; > > + > > + for_each_intel_crtc(&dev_priv->drm, crtc) { > > + pipe_config = to_intel_crtc_state(crtc->base.state); > > + > > + if (pipe_config->base.active) { > > + if (pipe_config->pipe_src_w >= WIDTH_4K && > > + pipe_config->pipe_src_h >= HEIGHT_4K) > > + return true; > > + } > > + } > > + > > + return false; > > +} > > + > > /** > > * intel_update_max_cdclk - Determine the maximum support CDCLK > > frequency > > * @dev_priv: i915 device > > @@ -2620,7 +2638,13 @@ static int intel_compute_max_dotclk(struct > > drm_i915_private *dev_priv) > > */ > > void intel_update_max_cdclk(struct drm_i915_private *dev_priv) { > > - if (IS_ELKHARTLAKE(dev_priv)) { > > + /* > > + * Use lower CDCLK frequency on Tigerlake when selected > > + * mode is less than 4k. > > + */ > > + if (INTEL_GEN(dev_priv) >= 12 && !mode_is_4k(dev_priv)) { > > + dev_priv->max_cdclk_freq = 172800; > > + } else if (IS_ELKHARTLAKE(dev_priv)) { > > Setting the max cd clock supported itself to a lower value is not a > good idea. > This should return what is the max frequency of cd clock the hardware > supports. > Driver is not going to program this to max based on this data. > > Actual cd clock which will be programmed in hardware should be based > on the > maximum pixel clock we are driving. So if we are not driving 4K and > say at 1920x1080, pixel > clock will be less and we should be selecting a lower value of cd > clock in that case which > gets programmed in CDCLK_CTL. But max cd clock still remains what > maximum the platform can > support. You can check intel_compute_min_cdclk which calculates the > minimum cd clock required. > > So when we just have 1920x1080@60Hz pixel clock of 148500, > min cd clock required should be 148500 and nearest higher value of CD > Clock will be 172.8Mhz. > Similarly for 4096x2160@60, pixel clock 556188, min cd clock > calculated will be 556.188 MHz, thus > getting nearest supported cd clock value of 556 or 648Mhz. > > This should be taken care by this logic in driver. > > Also with the current patch, modes like 2560x1600@60, pixel clock > 268.5MHz will not work. This will need > cd clock of 324Mhz (or 307, 312 whatever are supported on the > platform. Right. The dev_priv->max_cdclk_freq was HW max cdclock. We don't want to mess with that. Since we already pick up the minimum cdclock, the feature is already built-in cd clock selection process. Thanks for clarification! I'll propose that we drop this task since its obsolete. > > > if (dev_priv->cdclk.hw.ref == 24000) > > dev_priv->max_cdclk_freq = 552000; > > else > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h > > b/drivers/gpu/drm/i915/display/intel_cdclk.h > > index 4d6f7f5f8930..cefb5146ddca 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > > @@ -15,6 +15,9 @@ struct intel_atomic_state; struct > > intel_cdclk_state; struct > > intel_crtc_state; > > > > +#define WIDTH_4K 3860 > > +#define HEIGHT_4K 2160 > > + > > int intel_crtc_compute_min_cdclk(const struct intel_crtc_state > > *crtc_state); void > > intel_cdclk_init(struct drm_i915_private *i915); void > > intel_cdclk_uninit(struct > > drm_i915_private *i915); > > -- > > 2.17.1 > >
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index d0bc42e5039c..1d6c7bc79470 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -2610,6 +2610,24 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv) return max_cdclk_freq*90/100; } +bool mode_is_4k(struct drm_i915_private *dev_priv) +{ + struct intel_crtc *crtc; + struct intel_crtc_state *pipe_config; + + for_each_intel_crtc(&dev_priv->drm, crtc) { + pipe_config = to_intel_crtc_state(crtc->base.state); + + if (pipe_config->base.active) { + if (pipe_config->pipe_src_w >= WIDTH_4K && + pipe_config->pipe_src_h >= HEIGHT_4K) + return true; + } + } + + return false; +} + /** * intel_update_max_cdclk - Determine the maximum support CDCLK frequency * @dev_priv: i915 device @@ -2620,7 +2638,13 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv) */ void intel_update_max_cdclk(struct drm_i915_private *dev_priv) { - if (IS_ELKHARTLAKE(dev_priv)) { + /* + * Use lower CDCLK frequency on Tigerlake when selected + * mode is less than 4k. + */ + if (INTEL_GEN(dev_priv) >= 12 && !mode_is_4k(dev_priv)) { + dev_priv->max_cdclk_freq = 172800; + } else if (IS_ELKHARTLAKE(dev_priv)) { if (dev_priv->cdclk.hw.ref == 24000) dev_priv->max_cdclk_freq = 552000; else diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h index 4d6f7f5f8930..cefb5146ddca 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.h +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h @@ -15,6 +15,9 @@ struct intel_atomic_state; struct intel_cdclk_state; struct intel_crtc_state; +#define WIDTH_4K 3860 +#define HEIGHT_4K 2160 + int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state); void intel_cdclk_init(struct drm_i915_private *i915); void intel_cdclk_uninit(struct drm_i915_private *i915);
In order to achieve improved power savings we can tune down CD clock frequency for sub 4k resolutions. The maximum CD clock frequency for sub 4k resolutions is set to 172.8 MHz. Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++++++++++++++++++++- drivers/gpu/drm/i915/display/intel_cdclk.h | 3 +++ 2 files changed, 28 insertions(+), 1 deletion(-)