Re: [PATCH v2 13/16] drm/i915/gen9: Propagate watermark calculation failures up the call chain

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 22-04-16 om 01:20 schreef Matt Roper:
> On Thu, Apr 21, 2016 at 02:19:33PM +0200, Maarten Lankhorst wrote:
>> Op 20-04-16 om 04:26 schreef Matt Roper:
>>> Once we move watermark calculation to the atomic check phase, we'll want
>>> to start rejecting display configurations that exceed out watermark
>>> limits.  At the moment we just assume that there's always a valid set of
>>> watermarks, even though this may not actually be true.  Let's prepare by
>>> passing return codes up through the call stack in preparation.
>>>
>>> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 10 ++--
>>>  drivers/gpu/drm/i915/intel_pm.c      | 90 ++++++++++++++++++++++--------------
>>>  2 files changed, 61 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 80d8f77..7e8fa88 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -13312,7 +13312,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>>   * phase.  The code here should be run after the per-crtc and per-plane 'check'
>>>   * handlers to ensure that all derived state has been updated.
>>>   */
>>> -static void calc_watermark_data(struct drm_atomic_state *state)
>>> +static int calc_watermark_data(struct drm_atomic_state *state)
>>>  {
>>>  	struct drm_device *dev = state->dev;
>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>> @@ -13348,7 +13348,9 @@ static void calc_watermark_data(struct drm_atomic_state *state)
>>>  
>>>  	/* Is there platform-specific watermark information to calculate? */
>>>  	if (dev_priv->display.compute_global_watermarks)
>>> -		dev_priv->display.compute_global_watermarks(state);
>>> +		return dev_priv->display.compute_global_watermarks(state);
>>> +
>>> +	return 0;
>>>  }
>>>  
>>>  /**
>>> @@ -13432,9 +13434,7 @@ static int intel_atomic_check(struct drm_device *dev,
>>>  		return ret;
>>>  
>>>  	intel_fbc_choose_crtc(dev_priv, state);
>>> -	calc_watermark_data(state);
>>> -
>>> -	return 0;
>>> +	return calc_watermark_data(state);
>>>  }
>>>  
>>>  static int intel_atomic_prepare_commit(struct drm_device *dev,
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 21fde05..f07054a 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3241,13 +3241,14 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
>>>  	return false;
>>>  }
>>>  
>>> -static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>>> -				 struct intel_crtc_state *cstate,
>>> -				 struct intel_plane_state *intel_pstate,
>>> -				 uint16_t ddb_allocation,
>>> -				 int level,
>>> -				 uint16_t *out_blocks, /* out */
>>> -				 uint8_t *out_lines /* out */)
>>> +static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>>> +				struct intel_crtc_state *cstate,
>>> +				struct intel_plane_state *intel_pstate,
>>> +				uint16_t ddb_allocation,
>>> +				int level,
>>> +				uint16_t *out_blocks, /* out */
>>> +				uint8_t *out_lines, /* out */
>>> +				bool *enabled /* out */)
>>>  {
>>>  	struct drm_plane_state *pstate = &intel_pstate->base;
>>>  	struct drm_framebuffer *fb = pstate->fb;
>>> @@ -3259,8 +3260,10 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>>>  	uint8_t cpp;
>>>  	uint32_t width = 0, height = 0;
>>>  
>>> -	if (latency == 0 || !cstate->base.active || !intel_pstate->visible)
>>> -		return false;
>>> +	if (latency == 0 || !cstate->base.active || !intel_pstate->visible) {
>>> +		*enabled = false;
>>> +		return 0;
>>> +	}
>>>  
>>>  	width = drm_rect_width(&intel_pstate->src) >> 16;
>>>  	height = drm_rect_height(&intel_pstate->src) >> 16;
>>> @@ -3321,13 +3324,16 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>>>  			res_blocks++;
>>>  	}
>>>  
>>> -	if (res_blocks >= ddb_allocation || res_lines > 31)
>>> -		return false;
>>> +	if (res_blocks >= ddb_allocation || res_lines > 31) {
>>> +		*enabled = false;
>>> +		return 0;
>>> +	}
>>>  
>>>  	*out_blocks = res_blocks;
>>>  	*out_lines = res_lines;
>>> +	*enabled = true;
>>>  
>>> -	return true;
>>> +	return 0;
>>>  }
>>>  
>>>  static int
>>> @@ -3345,6 +3351,7 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>>>  	struct intel_plane_state *intel_pstate;
>>>  	uint16_t ddb_blocks;
>>>  	enum pipe pipe = intel_crtc->pipe;
>>> +	int ret;
>>>  
>>>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>>>  		int i = skl_wm_plane_id(intel_plane);
>>> @@ -3374,13 +3381,16 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>>>  
>>>  		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
>>>  
>>> -		result->plane_en[i] = skl_compute_plane_wm(dev_priv,
>>> -						cstate,
>>> -						intel_pstate,
>>> -						ddb_blocks,
>>> -						level,
>>> -						&result->plane_res_b[i],
>>> -						&result->plane_res_l[i]);
>>> +		ret = skl_compute_plane_wm(dev_priv,
>>> +					   cstate,
>>> +					   intel_pstate,
>>> +					   ddb_blocks,
>>> +					   level,
>>> +					   &result->plane_res_b[i],
>>> +					   &result->plane_res_l[i],
>>> +					   &result->plane_en[i]);
>>> +		if (ret)
>>> +			return ret;
>>>  	}
>>>  
>>>  	return 0;
>>> @@ -3417,21 +3427,26 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
>>>  	}
>>>  }
>>>  
>>> -static void skl_build_pipe_wm(struct intel_crtc_state *cstate,
>>> -			      struct skl_ddb_allocation *ddb,
>>> -			      struct skl_pipe_wm *pipe_wm)
>>> +static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>>> +			     struct skl_ddb_allocation *ddb,
>>> +			     struct skl_pipe_wm *pipe_wm)
>>>  {
>>>  	struct drm_device *dev = cstate->base.crtc->dev;
>>>  	const struct drm_i915_private *dev_priv = dev->dev_private;
>>>  	int level, max_level = ilk_wm_max_level(dev);
>>> +	int ret;
>>>  
>>>  	for (level = 0; level <= max_level; level++) {
>>> -		skl_compute_wm_level(dev_priv, ddb, cstate,
>>> -				     level, &pipe_wm->wm[level]);
>>> +		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
>>> +					   level, &pipe_wm->wm[level]);
>>> +		if (ret)
>>> +			return ret;
>>>  	}
>>>  	pipe_wm->linetime = skl_compute_linetime_wm(cstate);
>>>  
>>>  	skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
>>> +
>>> +	return 0;
>>>  }
>>>  
>>>  static void skl_compute_wm_results(struct drm_device *dev,
>>> @@ -3678,21 +3693,27 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>>>  	}
>>>  }
>>>  
>>> -static bool skl_update_pipe_wm(struct drm_crtc_state *cstate,
>>> -			       struct skl_ddb_allocation *ddb, /* out */
>>> -			       struct skl_pipe_wm *pipe_wm /* out */)
>>> +static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
>>> +			      struct skl_ddb_allocation *ddb, /* out */
>>> +			      struct skl_pipe_wm *pipe_wm, /* out */
>>> +			      bool *changed /* out */)
>>>  {
>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc);
>>>  	struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
>>> +	int ret;
>>>  
>>> -	skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
>>> +	ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
>>> +	if (ret)
>>> +		return ret;
>>>  
>>>  	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
>>> -		return false;
>>> +		*changed = false;
>>> +	else
>>> +		*changed = true;
>>>  
>>>  	intel_crtc->wm.active.skl = *pipe_wm;
>>>  
>>> -	return true;
>>> +	return 0;
>>>  }
>>>  
>>>  static void skl_update_other_pipe_wm(struct drm_device *dev,
>>> @@ -3725,8 +3746,8 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
>>>  		if (!intel_crtc->active)
>>>  			continue;
>>>  
>>> -		wm_changed = skl_update_pipe_wm(intel_crtc->base.state,
>>> -						&r->ddb, &pipe_wm);
>>> +		skl_update_pipe_wm(intel_crtc->base.state,
>>> +				   &r->ddb, &pipe_wm, &wm_changed);
>>>  
>>>  		/*
>>>  		 * If we end up re-computing the other pipe WM values, it's
>>> @@ -3827,14 +3848,15 @@ static void skl_update_wm(struct drm_crtc *crtc)
>>>  	struct skl_wm_values *results = &dev_priv->wm.skl_results;
>>>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>>>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>>> -
>>> +	bool wm_changed;
>>>  
>>>  	/* Clear all dirty flags */
>>>  	results->dirty_pipes = 0;
>>>  
>>>  	skl_clear_wm(results, intel_crtc->pipe);
>>>  
>>> -	if (!skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm))
>>> +	skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm, &wm_changed);
>>> +	if (!wm_changed)
>>>  		return;
>>>  
>>>  	skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
>> With the nitpicks fixed, and with CI happy.
>>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> I'm interpreting this to refer to the entire series (and not just this
> one patch that already had your r-b and no nitpicks); if that's not what
> you meant, please let me know.  :-)
>
Oops indeed. That's the case. :-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux