On Mon, Jun 01, 2015 at 03:27:11PM +0200, Maarten Lankhorst wrote:
> This may postpone going to HQ mode until the plane is in the
> drm_atomic_state if it's not using scaler 0, but it does allow moving
> intel_atomic_setup_scalers to the crtc check function.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_atomic.c | 41 ++++++++++++++++++------------------
> drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++---------
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> 3 files changed, 39 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 1edd1651c045..a8202fa0daa8 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
> if (ret)
> return ret;
>
> - /*
> - * FIXME: move to crtc atomic check function once this is
> - * more atomic friendly.
> - */
> - ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
> - if (ret)
> - return ret;
> -
> return ret;
> }
>
> @@ -336,21 +328,10 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
> /* find the plane that set the bit as scaler_user */
> plane = drm_state->planes[i];
>
> - /*
> - * to enable/disable hq mode, add planes that are using scaler
> - * into this transaction
> - */
> if (!plane) {
> - struct drm_plane_state *state;
> - plane = drm_plane_from_index(dev, i);
> - state = drm_atomic_get_plane_state(drm_state, plane);
> - if (IS_ERR(state)) {
> - DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state\n",
> - plane->base.id);
> - return PTR_ERR(state);
> - }
> + DRM_DEBUG_KMS("Failed to find [PLANE:%d] in drm_state\n", plane->base.id);
> + continue;
> }
> -
> intel_plane = to_intel_plane(plane);
>
> /* plane on different crtc cannot be a scaler user of this crtc */
> @@ -396,6 +377,24 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
> }
> }
>
> + /* plane not part of mask must leave hq mode? */
> + if (num_scalers_need > 1 && scaler_state->scalers[0].in_use &&
> + scaler_state->scalers[0].mode == PS_SCALER_MODE_HQ) {
> + scaler_state->scalers[0].mode = PS_SCALER_MODE_DYN;
> +
> + intel_crtc->atomic.skl_update_scaler0 =
> + PS_SCALER_EN | PS_SCALER_MODE_DYN;
> + }
> +
> + /* plane not part of mask can enter hq mode? */
> + if (num_scalers_need == 1 && scaler_state->scalers[0].in_use &&
> + intel_crtc->pipe != PIPE_C && scaler_state->scalers[0].mode != PS_SCALER_MODE_HQ) {
> + scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> +
> + intel_crtc->atomic.skl_update_scaler0 =
> + PS_SCALER_EN | PS_SCALER_MODE_HQ;
> + }
> +
I don't have access to the hw spec at the moment; is scaler #0 the only
one that can ever go into HQ mode? If there isn't a hardware
requirement about this, then it seems like we're missing the case where
planes A and B get scalers 0 and 1. Then plane A (and thus scaler 0) is
disabled, which should allow scaler 1 to go into HQ mode.
I guess it's not immediately clear to me why we need to not pull the
other planes into the transaction. Is this just to avoid doing some
extra work for a plane that hasn't changed, or does it cause a problem
somehow?
Chandra should probably take a look at this patch as well since he's the
scaler expert; adding him to the Cc.
Matt
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0060784525dc..94101d1f784c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6530,7 +6530,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> - int ret;
>
> /* FIXME should check pixel clock limits on all platforms */
> if (INTEL_INFO(dev)->gen < 4) {
> @@ -6577,14 +6576,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
> if (pipe_config->has_pch_encoder)
> return ironlake_fdi_compute_config(crtc, pipe_config);
>
> - /* FIXME: remove below call once atomic mode set is place and all crtc
> - * related checks called from atomic_crtc_check function */
> - ret = 0;
> - DRM_DEBUG_KMS("intel_crtc = %p drm_state (pipe_config->base.state) = %p\n",
> - crtc, pipe_config->base.state);
> - ret = intel_atomic_setup_scalers(dev, crtc, pipe_config);
> -
> - return ret;
> + return 0;
> }
>
> static int skylake_get_display_clock_speed(struct drm_device *dev)
> @@ -11457,7 +11449,10 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
> static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> struct drm_crtc_state *crtc_state)
> {
> + struct drm_device *dev = crtc->dev;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_crtc_state *pipe_config =
> + to_intel_crtc_state(crtc_state);
> struct drm_atomic_state *state = crtc_state->state;
> int idx = crtc->base.id;
> bool is_crtc_enabled = crtc_state->active;
> @@ -11476,7 +11471,7 @@ static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
> idx, was_crtc_enabled, is_crtc_enabled);
>
> - return 0;
> + return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
> }
>
> static const struct drm_crtc_helper_funcs intel_helper_funcs = {
> @@ -13423,6 +13418,18 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
> intel_crtc->atomic.evade =
> intel_pipe_update_start(intel_crtc,
> &intel_crtc->atomic.start_vbl_count);
> +
> + if (intel_crtc->atomic.skl_update_scaler0) {
> + uint32_t ps_ctrl, ps_win_sz;
> +
> + ps_ctrl = I915_READ(SKL_PS_CTRL(intel_crtc->pipe, 0));
> + ps_ctrl &= ~PS_SCALER_MODE_MASK;
> + ps_ctrl |= intel_crtc->atomic.skl_update_scaler0 & PS_SCALER_MODE_MASK;
> + I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, 0), ps_ctrl);
> +
> + ps_win_sz = I915_READ(SKL_PS_WIN_SZ(intel_crtc->pipe, 0));
> + I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, 0), ps_win_sz);
> + }
> }
>
> static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1d9feb8727ea..71ad8a1e13ab 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -487,6 +487,8 @@ struct intel_crtc_atomic_commit {
> bool update_wm;
> unsigned disabled_planes;
>
> + unsigned skl_update_scaler0;
> +
> /* Sleepable operations to perform after commit */
> unsigned fb_bits;
> bool wait_vblank;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
