Re: [PATCH v2 11/16] drm/i915/gen9: Allow watermark calculation on in-flight atomic state (v2)

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 20-04-16 om 04:26 schreef Matt Roper:
> In an upcoming patch we'll move this calculation to the atomic 'check'
> phase so that the display update can be rejected early if no valid
> watermark programming is possible.
>
> v2:
>  - Drop intel_pstate_for_cstate_plane() helper and add note about how
>    the code needs to evolve in the future if we start allowing more than
>    one pending commit against a CRTC.  (Maarten)
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0c0a312..d114375 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3330,14 +3330,17 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  	return true;
>  }
>  
> -static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
> -				 struct skl_ddb_allocation *ddb,
> -				 struct intel_crtc_state *cstate,
> -				 int level,
> -				 struct skl_wm_level *result)
> +static int
> +skl_compute_wm_level(const struct drm_i915_private *dev_priv,
> +		     struct skl_ddb_allocation *ddb,
> +		     struct intel_crtc_state *cstate,
> +		     int level,
> +		     struct skl_wm_level *result)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> +	struct drm_atomic_state *state = cstate->base.state;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> +	struct drm_plane *plane;
>  	struct intel_plane *intel_plane;
>  	struct intel_plane_state *intel_pstate;
>  	uint16_t ddb_blocks;
> @@ -3346,7 +3349,29 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>  		int i = skl_wm_plane_id(intel_plane);
>  
> -		intel_pstate = to_intel_plane_state(intel_plane->base.state);
> +		plane = &intel_plane->base;
> +		intel_pstate = NULL;
> +		if (state)
> +			intel_pstate =
> +				intel_atomic_get_existing_plane_state(state,
> +								      intel_plane);
> +
> +		/*
> +		 * Note: If we start supporting multiple pending atomic commits
> +		 * against the same planes/CRTC's in the future, plane->state
> +		 * will no longer be the correct pre-state to use for the
> +		 * calculations here and we'll need to change where we get the
> +		 * 'unchanged' plane data from.
> +		 *
> +		 * For now this is fine because we only allow one queued commit
> +		 * against a CRTC.  Even if the plane isn't modified by this
> +		 * transaction and we don't have a plane lock, we still have
> +		 * the CRTC's lock, so we know that no other transactions are
> +		 * racing with us to update it.
> +		 */
> +		if (!intel_pstate)
> +			intel_pstate = to_intel_plane_state(plane->state);
>
You can make this race-free by using using for_each_intel_plane_mask
with crtc_state->plane_mask ¦ old_crtc_state->plane_mask.

Other planes can be assumed to have a data rate of 0, and can unfortunately be updated
in parallel. Since you can only update atomic properties that are not ->crtc or ->fb it wouldn't
change anything visibly on the screen, but it would cause your plane->state to be freed behind
your back if you're not careful.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx