[Intel-gfx] [PATCH 1/7] drm/i915: Calculate gen3- watermarks semi-atomically.

Mahesh Kumar mahesh1.kumar at intel.com
Tue Aug 8 12:51:36 UTC 2017


Hi,


On Monday 07 August 2017 04:18 PM, Maarten Lankhorst wrote:
> The gen3 watermark calculations are converted to atomic,
> but the wm update calls are still done through the legacy
> functions.
>
> This will make it easier to bisect things if they go wrong.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c |   3 +-
>   drivers/gpu/drm/i915/intel_drv.h     |  14 +++
>   drivers/gpu/drm/i915/intel_pm.c      | 231 +++++++++++++++++++++--------------
>   3 files changed, 152 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 974d4b577189..234b94cafc7d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14709,7 +14709,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>   		skl_wm_get_hw_state(dev);
>   	} else if (HAS_PCH_SPLIT(dev_priv)) {
>   		ilk_wm_get_hw_state(dev);
> -	}
> +	} else if (INTEL_GEN(dev_priv) <= 3 && !IS_PINEVIEW(dev_priv))
> +		i9xx_wm_get_hw_state(dev);
>   
>   	for_each_intel_crtc(dev, crtc) {
>   		u64 put_domains;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f91de9cb9697..d53d34756048 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -547,6 +547,15 @@ struct g4x_wm_state {
>   	bool fbc_en;
>   };
>   
> +struct i9xx_wm_state {
> +	uint16_t plane_wm;
> +	bool cxsr;
> +
> +	struct {
> +		uint16_t plane;
should this also be named as plane_wm, because it's again wm with SR. 
just a nitpick but not a stopper.
> +	} sr;
> +};
> +
>   struct intel_crtc_wm_state {
>   	union {
>   		struct {
> @@ -591,6 +600,9 @@ struct intel_crtc_wm_state {
>   			/* optimal watermarks */
>   			struct g4x_wm_state optimal;
>   		} g4x;
new line please to match the coding style of function
> +		struct {
> +			struct i9xx_wm_state optimal;
> +		} i9xx;
>   	};
>   
>   	/*
> @@ -823,6 +835,7 @@ struct intel_crtc {
>   			struct intel_pipe_wm ilk;
>   			struct vlv_wm_state vlv;
>   			struct g4x_wm_state g4x;
> +			struct i9xx_wm_state i9xx;
>   		} active;
>   	} wm;
>   
> @@ -1845,6 +1858,7 @@ void gen6_rps_boost(struct drm_i915_gem_request *rq,
>   		    struct intel_rps_client *rps);
>   void intel_queue_rps_boost_for_request(struct drm_i915_gem_request *req);
>   void g4x_wm_get_hw_state(struct drm_device *dev);
> +void i9xx_wm_get_hw_state(struct drm_device *dev);
>   void vlv_wm_get_hw_state(struct drm_device *dev);
>   void ilk_wm_get_hw_state(struct drm_device *dev);
>   void skl_wm_get_hw_state(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6e393b217450..807c9a730020 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2224,89 +2224,154 @@ static void i965_update_wm(struct intel_crtc *unused_crtc)
>   
>   #undef FW_WM
>   
> -static void i9xx_update_wm(struct intel_crtc *unused_crtc)
> +static const struct intel_watermark_params *i9xx_get_wm_info(struct drm_i915_private *dev_priv,
> +							     struct intel_crtc *crtc)
>   {
> -	struct drm_i915_private *dev_priv = to_i915(unused_crtc->base.dev);
> -	const struct intel_watermark_params *wm_info;
> -	uint32_t fwater_lo;
> -	uint32_t fwater_hi;
> -	int cwm, srwm = 1;
> -	int fifo_size;
> -	int planea_wm, planeb_wm;
> -	struct intel_crtc *crtc, *enabled = NULL;
> +	struct intel_plane *plane = to_intel_plane(crtc->base.primary);
>   
>   	if (IS_I945GM(dev_priv))
> -		wm_info = &i945_wm_info;
> +		return &i945_wm_info;
>   	else if (!IS_GEN2(dev_priv))
> -		wm_info = &i915_wm_info;
> +		return &i915_wm_info;
> +	else if (plane->plane == PLANE_A)
> +		return &i830_a_wm_info;
>   	else
> -		wm_info = &i830_a_wm_info;
> +		return &i830_bc_wm_info;
> +}
>   
> -	fifo_size = dev_priv->display.get_fifo_size(dev_priv, 0);
> -	crtc = intel_get_crtc_for_plane(dev_priv, 0);
> -	if (intel_crtc_active(crtc)) {
> +static int i9xx_compute_pipe_wm(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_atomic_state *state =
> +		to_intel_atomic_state(crtc_state->base.state);
> +	struct i9xx_wm_state *wm_state = &crtc_state->wm.i9xx.optimal;
> +	struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> +	const struct drm_plane_state *plane_state = NULL;
> +	int fifo_size;
> +	const struct intel_watermark_params *wm_info;
> +
> +	fifo_size = dev_priv->display.get_fifo_size(dev_priv, plane->plane);
> +
> +	wm_info = i9xx_get_wm_info(dev_priv, crtc);
> +
> +	wm_state->cxsr = false;
> +	memset(&wm_state->sr, 0, sizeof(wm_state->sr));
> +
> +	if (crtc_state->base.plane_mask & BIT(drm_plane_index(&plane->base)))
> +		plane_state = __drm_atomic_get_current_plane_state(&state->base, &plane->base);
> +
> +	if (!plane_state || !intel_wm_plane_visible(crtc_state, to_intel_plane_state(plane_state))) {
> +		wm_state->plane_wm = fifo_size - wm_info->guard_size;
> +		if (wm_state->plane_wm > (long)wm_info->max_wm)
> +			wm_state->plane_wm = wm_info->max_wm;
> +	} else if (HAS_FW_BLC(dev_priv)) {
This part will be called only for (GEN > 2,) So we'll never be 
calculating wm for GEN2. but you are changing GEN2 wm also to 2 stages 
compute & update in intel_init_pm.
> +		struct drm_framebuffer *fb = plane_state->fb;
> +		unsigned cpp = IS_GEN2(dev_priv) ? 4 : fb->format->cpp[0];
>   		const struct drm_display_mode *adjusted_mode =
> -			&crtc->config->base.adjusted_mode;
> -		const struct drm_framebuffer *fb =
> -			crtc->base.primary->state->fb;
> -		int cpp;
> +			&crtc_state->base.adjusted_mode;
> +		unsigned active_crtcs;
> +		bool may_cxsr;
>   
> -		if (IS_GEN2(dev_priv))
> -			cpp = 4;
> +		if (state->modeset)
> +			active_crtcs = state->active_crtcs;
>   		else
> -			cpp = fb->format->cpp[0];
> +			active_crtcs = dev_priv->active_crtcs;
>   
> -		planea_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
> -					       wm_info, fifo_size, cpp,
> -					       pessimal_latency_ns);
> -		enabled = crtc;
> -	} else {
> -		planea_wm = fifo_size - wm_info->guard_size;
> -		if (planea_wm > (long)wm_info->max_wm)
> -			planea_wm = wm_info->max_wm;
> +		may_cxsr = active_crtcs == drm_crtc_mask(&crtc->base);
It would be good to have a comment stating if only one CRTC is enabled 
we can go to cxsr.
we can use hweight32() function to check if only one CRTC is enabled 
(not mandatory).
> +
> +		wm_state->plane_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
> +							wm_info, fifo_size, cpp,
> +							pessimal_latency_ns);
> +
> +		DRM_DEBUG_KMS("FIFO watermarks - plane watermarks: %d\n", wm_state->plane_wm);
> +
> +		if (IS_I915GM(dev_priv) && i915_gem_object_is_tiled(intel_fb_obj(fb)))
> +			may_cxsr = false;
> +
> +		if (may_cxsr) {
> +			static const int sr_latency_ns = 6000;
> +			unsigned long entries;
> +
> +			entries = intel_wm_method2(adjusted_mode->crtc_clock,
> +						   adjusted_mode->crtc_htotal,
> +						   crtc_state->pipe_src_w, cpp,
> +						   sr_latency_ns / 100);
> +			entries = DIV_ROUND_UP(entries, wm_info->cacheline_size);
> +
> +			DRM_DEBUG_KMS("self-refresh entries: %ld\n", entries);
> +
> +			if (wm_info->fifo_size >= entries) {
Earlier if (fifo_size < entries) we were assigning srwm = 1 & not 
disabling cxsr, Is there any specific Bspec documentation to not enable 
cxsr if fifo_size < entries ?
> +				wm_state->cxsr = true;
> +				wm_state->sr.plane = wm_info->fifo_size - entries;
> +			} else
> +				may_cxsr = false;
may_cxsr is not used later, so no need to overwrite the value.
> +		}
> +
> +		DRM_DEBUG_KMS("FIFO watermarks - plane watermarks: %d, can cxsr: %s, SR size: %d\n",
> +			      wm_state->plane_wm, yesno(wm_state->cxsr), wm_state->sr.plane);
>   	}
>   
> -	if (IS_GEN2(dev_priv))
> -		wm_info = &i830_bc_wm_info;
> +	return 0;
> +}
>   
> -	fifo_size = dev_priv->display.get_fifo_size(dev_priv, 1);
> -	crtc = intel_get_crtc_for_plane(dev_priv, 1);
> -	if (intel_crtc_active(crtc)) {
> -		const struct drm_display_mode *adjusted_mode =
> -			&crtc->config->base.adjusted_mode;
> -		const struct drm_framebuffer *fb =
> -			crtc->base.primary->state->fb;
> -		int cpp;
> +void i9xx_wm_get_hw_state(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc *crtc;
> +	uint32_t fwater_lo, planea_wm, planeb_wm;
> +
> +	fwater_lo = I915_READ(FW_BLC);
> +
> +	planea_wm = fwater_lo & 0x3f;
> +	planeb_wm = (fwater_lo >> 16) & 0x3f;
> +
> +	for_each_intel_crtc(dev, crtc) {
> +		struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->base.state);
> +		struct i9xx_wm_state *wm_state = &cstate->wm.i9xx.optimal;
> +		struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> +
> +		memset(&wm_state->sr, 0, sizeof(wm_state->sr));
> +		wm_state->cxsr = false;
>   
> -		if (IS_GEN2(dev_priv))
> -			cpp = 4;
> +		if (plane == PLANE_A)
here plane is of type intel_plane but you are comparing with enum plane, 
I think you meant plane->plane
> +			wm_state->plane_wm = planea_wm;
>   		else
> -			cpp = fb->format->cpp[0];
> +			wm_state->plane_wm = planeb_wm;
> +
> +		crtc->wm.active.i9xx = *wm_state;
> +	}
> +}
>   
> -		planeb_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
> -					       wm_info, fifo_size, cpp,
> -					       pessimal_latency_ns);
> +static void i9xx_update_wm(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	uint32_t fwater_lo;
> +	uint32_t fwater_hi;
> +	int cwm, srwm = -1;
> +	int planea_wm, planeb_wm;
> +	struct intel_crtc *enabled = NULL;
enabled is used to check if cxsr can be enabled, IMHO it would be good 
to take a bool variable for same & replace use of enabled with 
respective intel_crtc variable.
if it seems complex then this approach is also good with me.
> +
> +	crtc->wm.active.i9xx = crtc->config->wm.i9xx.optimal;
> +
> +	crtc = intel_get_crtc_for_plane(dev_priv, 0);
> +	planea_wm = crtc->wm.active.i9xx.plane_wm;
> +	if (intel_crtc_active(crtc))
> +		enabled = crtc;
> +
> +	crtc = intel_get_crtc_for_plane(dev_priv, 1);
> +	if (intel_crtc_active(crtc)) {
>   		if (enabled == NULL)
>   			enabled = crtc;
>   		else
>   			enabled = NULL;
> -	} else {
> -		planeb_wm = fifo_size - wm_info->guard_size;
> -		if (planeb_wm > (long)wm_info->max_wm)
> -			planeb_wm = wm_info->max_wm;
>   	}
> +	planeb_wm = crtc->wm.active.i9xx.plane_wm;
>   
>   	DRM_DEBUG_KMS("FIFO watermarks - A: %d, B: %d\n", planea_wm, planeb_wm);
>   
> -	if (IS_I915GM(dev_priv) && enabled) {
> -		struct drm_i915_gem_object *obj;
> -
> -		obj = intel_fb_obj(enabled->base.primary->state->fb);
> -
> -		/* self-refresh seems busted with untiled */
> -		if (!i915_gem_object_is_tiled(obj))
> -			enabled = NULL;
> -	}
> +	if (!enabled->wm.active.i9xx.cxsr)
> +		enabled = NULL;
>   
>   	/*
>   	 * Overlay gets an aggressive default since video jitter is bad.
> @@ -2317,31 +2382,8 @@ static void i9xx_update_wm(struct intel_crtc *unused_crtc)
>   	intel_set_memory_cxsr(dev_priv, false);
>   
>   	/* Calc sr entries for one plane configs */
> -	if (HAS_FW_BLC(dev_priv) && enabled) {
> -		/* self-refresh has much higher latency */
> -		static const int sr_latency_ns = 6000;
> -		const struct drm_display_mode *adjusted_mode =
> -			&enabled->config->base.adjusted_mode;
> -		const struct drm_framebuffer *fb =
> -			enabled->base.primary->state->fb;
> -		int clock = adjusted_mode->crtc_clock;
> -		int htotal = adjusted_mode->crtc_htotal;
> -		int hdisplay = enabled->config->pipe_src_w;
> -		int cpp;
> -		int entries;
> -
> -		if (IS_I915GM(dev_priv) || IS_I945GM(dev_priv))
> -			cpp = 4;
> -		else
> -			cpp = fb->format->cpp[0];
> -
> -		entries = intel_wm_method2(clock, htotal, hdisplay, cpp,
> -					   sr_latency_ns / 100);
> -		entries = DIV_ROUND_UP(entries, wm_info->cacheline_size);
> -		DRM_DEBUG_KMS("self-refresh entries: %d\n", entries);
> -		srwm = wm_info->fifo_size - entries;
> -		if (srwm < 0)
> -			srwm = 1;
> +	if (enabled) {
> +		srwm = enabled->wm.active.i9xx.sr.plane;
>   
>   		if (IS_I945G(dev_priv) || IS_I945GM(dev_priv))
>   			I915_WRITE(FW_BLC_SELF,
> @@ -2367,23 +2409,18 @@ static void i9xx_update_wm(struct intel_crtc *unused_crtc)
>   		intel_set_memory_cxsr(dev_priv, true);
>   }
>   
> -static void i845_update_wm(struct intel_crtc *unused_crtc)
> +static void i845_update_wm(struct intel_crtc *crtc)
>   {
> -	struct drm_i915_private *dev_priv = to_i915(unused_crtc->base.dev);
> -	struct intel_crtc *crtc;
> -	const struct drm_display_mode *adjusted_mode;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   	uint32_t fwater_lo;
>   	int planea_wm;
>   
> -	crtc = single_enabled_crtc(dev_priv);
> -	if (crtc == NULL)
earlier we were checking if enabled crtc's != 1 then return, but here 
logic got changed.
Oh it seems ok, as platform calling i845 will have only one crtc.

-Mahesh
> +	if (!intel_crtc_active(crtc))
>   		return;
>   
> -	adjusted_mode = &crtc->config->base.adjusted_mode;
> -	planea_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
> -				       &i845_wm_info,
> -				       dev_priv->display.get_fifo_size(dev_priv, 0),
> -				       4, pessimal_latency_ns);
> +	crtc->wm.active.i9xx = crtc->config->wm.i9xx.optimal;
> +	planea_wm = crtc->wm.active.i9xx.plane_wm;
> +
>   	fwater_lo = I915_READ(FW_BLC) & ~0xfff;
>   	fwater_lo |= (3<<8) | planea_wm;
>   
> @@ -8813,9 +8850,13 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
>   	} else if (IS_GEN4(dev_priv)) {
>   		dev_priv->display.update_wm = i965_update_wm;
>   	} else if (IS_GEN3(dev_priv)) {
> +		dev_priv->display.compute_pipe_wm = i9xx_compute_pipe_wm;
>   		dev_priv->display.update_wm = i9xx_update_wm;
> +
>   		dev_priv->display.get_fifo_size = i9xx_get_fifo_size;
>   	} else if (IS_GEN2(dev_priv)) {
> +		dev_priv->display.compute_pipe_wm = i9xx_compute_pipe_wm;
> +
>   		if (INTEL_INFO(dev_priv)->num_pipes == 1) {
>   			dev_priv->display.update_wm = i845_update_wm;
>   			dev_priv->display.get_fifo_size = i845_get_fifo_size;



More information about the Intel-gfx mailing list