On Mon, 2017-07-10 at 22:33 +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Currently the .modeset_calc_cdclk() hooks check the final cdclk value
> against the max allowed. That's not really sufficient since the low
> level calc_cdclk() functions effectively clamp the minimum required
> cdclk to the max supported by the platform. Hence if the minimum
> required exceeds the platforms capabilities we'd keep going anyway
> using the max cdclk frequency.
>
> To fix that let's move the check earlier into
> intel_crtc_compute_min_cdclk() and we'll check the minimum required
> cdclk of the pipe against the maximum supported by the platform.
>
I suppose this should save some power in the case where one of the
CRTC's pixel rate exceeds platform capabilities. And failing the
atomic_check instead of going with max_cdclk will help the userspace try
a lower mode. Is that the idea?
Moving the checks makes sense to me because that seems like the original
intention anyway, but I think it's a good idea to get someone else to
take a look too.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 96 +++++++++++++++++-------------------
> drivers/gpu/drm/i915/intel_display.c | 5 +-
> 2 files changed, 48 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 50f153dbea14..603950f1b87f 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1789,6 +1789,12 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> min_cdclk = max(2 * 96000, min_cdclk);
>
> + if (min_cdclk > dev_priv->max_cdclk_freq) {
> + DRM_DEBUG_KMS("required cdclk (%d kHz) exceeds max (%d kHz)\n",
> + min_cdclk, dev_priv->max_cdclk_freq);
> + return -EINVAL;
> + }
> +
> return min_cdclk;
> }
>
> @@ -1798,16 +1804,21 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_crtc *crtc;
> struct intel_crtc_state *crtc_state;
> - int min_cdclk = 0, i;
> + int min_cdclk, i;
> enum pipe pipe;
>
> memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
> sizeof(intel_state->min_cdclk));
>
> - for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
> - intel_state->min_cdclk[i] =
> - intel_crtc_compute_min_cdclk(crtc_state);
> + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
> + min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> + if (min_cdclk < 0)
> + return min_cdclk;
> +
> + intel_state->min_cdclk[i] = min_cdclk;
> + }
>
> + min_cdclk = 0;
> for_each_pipe(dev_priv, pipe)
> min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
>
> @@ -1817,18 +1828,14 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - struct intel_atomic_state *intel_state =
> - to_intel_atomic_state(state);
> - int cdclk;
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + int min_cdclk, cdclk;
>
> - cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> + cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
>
> intel_state->cdclk.logical.cdclk = cdclk;
>
> @@ -1846,10 +1853,12 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>
> static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> - struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - int cdclk;
> + int min_cdclk, cdclk;
> +
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> /*
> * FIXME should also account for plane ratio
> @@ -1857,12 +1866,6 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> */
> cdclk = bdw_calc_cdclk(min_cdclk);
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> -
> intel_state->cdclk.logical.cdclk = cdclk;
>
> if (!intel_state->active_crtcs) {
> @@ -1879,10 +1882,13 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>
> static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> - struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - int cdclk, vco;
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + int min_cdclk, cdclk, vco;
> +
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> vco = intel_state->cdclk.logical.vco;
> if (!vco)
> @@ -1894,12 +1900,6 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> */
> cdclk = skl_calc_cdclk(min_cdclk, vco);
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> -
> intel_state->cdclk.logical.vco = vco;
> intel_state->cdclk.logical.cdclk = cdclk;
>
> @@ -1919,10 +1919,12 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - struct intel_atomic_state *intel_state =
> - to_intel_atomic_state(state);
> - int cdclk, vco;
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + int min_cdclk, cdclk, vco;
> +
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> if (IS_GEMINILAKE(dev_priv)) {
> cdclk = glk_calc_cdclk(min_cdclk);
> @@ -1932,12 +1934,6 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> vco = bxt_de_pll_vco(dev_priv, cdclk);
> }
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> -
> intel_state->cdclk.logical.vco = vco;
> intel_state->cdclk.logical.cdclk = cdclk;
>
> @@ -1963,20 +1959,16 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - struct intel_atomic_state *intel_state =
> - to_intel_atomic_state(state);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - int cdclk, vco;
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + int min_cdclk, cdclk, vco;
> +
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> cdclk = cnl_calc_cdclk(min_cdclk);
> vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> -
> intel_state->cdclk.logical.vco = vco;
> intel_state->cdclk.logical.cdclk = cdclk;
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b47535f5d95d..1caf0ef82e36 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15590,8 +15590,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>
> intel_crtc_compute_pixel_rate(crtc_state);
>
> - if (dev_priv->display.modeset_calc_cdclk)
> + if (dev_priv->display.modeset_calc_cdclk) {
> min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> + if (WARN_ON(min_cdclk < 0))
> + min_cdclk = 0;
> + }
>
> drm_calc_timestamping_constants(&crtc->base,
> &crtc_state->base.adjusted_mode);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
