[Intel-gfx] [PATCH 50/89] drm/i915/skl: Read the pipe WM HW state

Ville Syrjälä ville.syrjala at linux.intel.com
Wed Oct 29 20:02:58 CET 2014


On Thu, Sep 04, 2014 at 12:27:16PM +0100, Damien Lespiau wrote:
> From: Pradeep Bhat <pradeep.bhat at intel.com>
> 
> This patch provides the implementation for reading the pipe wm HW
> state.
> 
> v2: Incorporated Damien's review comments and also made modifications
>     to incorporate the plane/cursor split.
> 
> v3: No need to ident a line that was fitting 80 chars
>     Return early instead of indenting the remaining of a function
>     (Damien)
> 
> v4: Rebase on top of nightly (minor conflect in intel_drv.h)
> 
> Signed-off-by: Pradeep Bhat <pradeep.bhat at intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   4 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   1 +
>  drivers/gpu/drm/i915/intel_pm.c      | 104 +++++++++++++++++++++++++++++++++++
>  3 files changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3d9f447..69e023a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13375,7 +13375,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		pll->on = false;
>  	}
>  
> -	if (HAS_PCH_SPLIT(dev))
> +	if (IS_GEN9(dev))
> +		skl_wm_get_hw_state(dev);
> +	else if (HAS_PCH_SPLIT(dev))
>  		ilk_wm_get_hw_state(dev);
>  
>  	if (force_restore) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 268087f..559b747 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1102,6 +1102,7 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
>  void ilk_wm_get_hw_state(struct drm_device *dev);
> +void skl_wm_get_hw_state(struct drm_device *dev);
>  
>  
>  /* intel_sdvo.c */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 756ff16..1e56067 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3591,6 +3591,110 @@ ilk_update_sprite_wm(struct drm_plane *plane,
>  	ilk_update_wm(crtc);
>  }
>  
> +static void skl_pipe_wm_active_state(uint32_t val,
> +				     struct skl_pipe_wm *active,
> +				     bool is_transwm,
> +				     bool is_cursor,
> +				     int i,
> +				     int level)
> +{
> +	bool is_enabled = (val & PLANE_WM_EN) != 0;
> +
> +	if (!is_transwm) {
> +		if (!is_cursor) {
> +			active->wm[level].plane_en[i] = is_enabled;
> +			active->wm[level].plane_res_b[i] =
> +					val & PLANE_WM_BLOCKS_MASK;
> +			active->wm[level].plane_res_l[i] =
> +					(val >> PLANE_WM_LINES_SHIFT) &
> +						PLANE_WM_LINES_MASK;
> +		} else {
> +			active->wm[level].cursor_en = is_enabled;
> +			active->wm[level].cursor_res_b =
> +					val & PLANE_WM_BLOCKS_MASK;
> +			active->wm[level].cursor_res_l =
> +					(val >> PLANE_WM_LINES_SHIFT) &
> +						PLANE_WM_LINES_MASK;
> +		}
> +	} else {
> +		if (!is_cursor) {
> +			active->trans_wm.plane_en[i] = is_enabled;
> +			active->trans_wm.plane_res_b[i] =
> +					val & PLANE_WM_BLOCKS_MASK;
> +			active->trans_wm.plane_res_l[i] =
> +					(val >> PLANE_WM_LINES_SHIFT) &
> +						PLANE_WM_LINES_MASK;
> +		} else {
> +			active->trans_wm.cursor_en = is_enabled;
> +			active->trans_wm.cursor_res_b =
> +					val & PLANE_WM_BLOCKS_MASK;
> +			active->trans_wm.cursor_res_l =
> +					(val >> PLANE_WM_LINES_SHIFT) &
> +						PLANE_WM_LINES_MASK;
> +		}
> +	}
> +}

Am I imagining it or could this function be reduced to four lines if you
would just pass the target struct as a parameter instead of the
(is_transwm,is_cursor,i,level) tuple? Ah no, crap, SoA strikes back. So I
think I mentioned it already during my first round of reviews that I'd
like make a bunch of this stuff AoS instead. But that's a recipe for
massive conflicts so let's get the current stuff in first before we go
tearing into those structures.

Apart from that horror of a function this looks like it does what it's
meant to so:
Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

> +static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct skl_wm_values *hw = &dev_priv->wm.skl_hw;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct skl_pipe_wm *active = &intel_crtc->wm.skl_active;
> +	enum pipe pipe = intel_crtc->pipe;
> +	int level, i, max_level;
> +	uint32_t temp;
> +
> +	max_level = ilk_wm_max_level(dev);
> +
> +	hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
> +
> +	for (level = 0; level <= max_level; level++) {
> +		for (i = 0; i < intel_num_planes(intel_crtc); i++)
> +			hw->plane[pipe][i][level] =
> +					I915_READ(PLANE_WM(pipe, i, level));
> +		hw->cursor[pipe][level] = I915_READ(CUR_WM(pipe, level));
> +	}
> +
> +	for (i = 0; i < intel_num_planes(intel_crtc); i++)
> +		hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i));
> +	hw->cursor_trans[pipe] = I915_READ(CUR_WM_TRANS(pipe));
> +
> +	if (!intel_crtc_active(crtc))
> +		return;
> +
> +	hw->dirty[pipe] = true;
> +
> +	active->linetime = hw->wm_linetime[pipe];
> +
> +	for (level = 0; level <= max_level; level++) {
> +		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> +			temp = hw->plane[pipe][i][level];
> +			skl_pipe_wm_active_state(temp, active, false,
> +						false, i, level);
> +		}
> +		temp = hw->cursor[pipe][level];
> +		skl_pipe_wm_active_state(temp, active, false, true, i, level);
> +	}
> +
> +	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> +		temp = hw->plane_trans[pipe][i];
> +		skl_pipe_wm_active_state(temp, active, true, false, i, 0);
> +	}
> +
> +	temp = hw->cursor_trans[pipe];
> +	skl_pipe_wm_active_state(temp, active, true, true, i, 0);
> +}
> +
> +void skl_wm_get_hw_state(struct drm_device *dev)
> +{
> +	struct drm_crtc *crtc;
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> +		skl_pipe_wm_get_hw_state(crtc);
> +}
> +
>  static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC



More information about the Intel-gfx mailing list