Re: [PATCH v2 4/7] drm/vc4: kms: Wait on previous FIFO users before a commit

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Dec 04, 2020 at 04:11:35PM +0100, Maxime Ripard wrote:
> If we're having two subsequent, non-blocking, commits on two different
> CRTCs that share no resources, there's no guarantee on the order of
> execution of both commits.
> 
> However, the second one will consider the first one as the old state,
> and will be in charge of freeing it once that second commit is done.
> 
> If the first commit happens after that second commit, it might access
> some resources related to its state that has been freed, resulting in a
> use-after-free bug.
> 
> The standard DRM objects are protected against this, but our HVS private
> state isn't so let's make sure we wait for all the previous FIFO users
> to finish their commit before going with our own.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/vc4/vc4_kms.c | 123 +++++++++++++++++++++++++++++++++-
>  1 file changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 8937eb0b751d..fdd698df5fbe 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -40,6 +40,11 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
>  struct vc4_hvs_state {
>  	struct drm_private_state base;
>  	unsigned int unassigned_channels;
> +
> +	struct {
> +		unsigned in_use: 1;
> +		struct drm_crtc_commit *pending_commit;
> +	} fifo_state[HVS_NUM_CHANNELS];
>  };
>  
>  static struct vc4_hvs_state *
> @@ -182,6 +187,32 @@ vc4_ctm_commit(struct vc4_dev *vc4, struct drm_atomic_state *state)
>  		  VC4_SET_FIELD(ctm_state->fifo, SCALER_OLEDOFFS_DISPFIFO));
>  }
>  
> +static struct vc4_hvs_state *
> +vc4_hvs_get_new_global_state(struct drm_atomic_state *state)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> +	struct drm_private_state *priv_state;
> +
> +	priv_state = drm_atomic_get_new_private_obj_state(state, &vc4->hvs_channels);
> +	if (IS_ERR(priv_state))
> +		return ERR_CAST(priv_state);
> +
> +	return to_vc4_hvs_state(priv_state);
> +}
> +
> +static struct vc4_hvs_state *
> +vc4_hvs_get_old_global_state(struct drm_atomic_state *state)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> +	struct drm_private_state *priv_state;
> +
> +	priv_state = drm_atomic_get_old_private_obj_state(state, &vc4->hvs_channels);
> +	if (IS_ERR(priv_state))
> +		return ERR_CAST(priv_state);
> +
> +	return to_vc4_hvs_state(priv_state);
> +}
> +
>  static struct vc4_hvs_state *
>  vc4_hvs_get_global_state(struct drm_atomic_state *state)
>  {
> @@ -308,8 +339,10 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>  	struct drm_device *dev = state->dev;
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
>  	struct vc4_hvs *hvs = vc4->hvs;
> +	struct drm_crtc_state *old_crtc_state;
>  	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
> +	struct vc4_hvs_state *old_hvs_state;
>  	int i;
>  
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -329,6 +362,36 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>  
>  	drm_atomic_helper_wait_for_dependencies(state);
>  
> +	old_hvs_state = vc4_hvs_get_old_global_state(state);
> +	if (!old_hvs_state)
> +		return;
> +
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> +		struct vc4_crtc_state *vc4_crtc_state =
> +			to_vc4_crtc_state(old_crtc_state);
> +		struct drm_crtc_commit *commit;
> +		unsigned int channel = vc4_crtc_state->assigned_channel;
> +		unsigned long done;
> +
> +		if (channel == VC4_HVS_CHANNEL_DISABLED)
> +			continue;
> +
> +		if (!old_hvs_state->fifo_state[channel].in_use)
> +			continue;
> +
> +		commit = old_hvs_state->fifo_state[i].pending_commit;
> +		if (!commit)
> +			continue;
> +
> +		done = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);
> +		if (!done)
> +			drm_err(dev, "Timed out waiting for hw_done\n");
> +
> +		done = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
> +		if (!done)
> +			drm_err(dev, "Timed out waiting for flip_done\n");

Idea for a follow-up patch: Add something like drm_crtc_commit_wait which
skips on a NULL commit and does the two waits here. And use it here and in
drm_atomic_helper_wait_for_dependencies, we have four copies of the same
code by now :-)

> +	}
> +
>  	drm_atomic_helper_commit_modeset_disables(dev, state);
>  
>  	vc4_ctm_commit(vc4, state);
> @@ -368,6 +431,36 @@ static void commit_work(struct work_struct *work)
>  	vc4_atomic_complete_commit(state);
>  }
>  
> +static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct vc4_hvs_state *hvs_state;
> +	struct drm_crtc *crtc;
> +	unsigned int i;
> +
> +	hvs_state = vc4_hvs_get_new_global_state(state);
> +	if (!hvs_state)
> +		return -EINVAL;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct vc4_crtc_state *vc4_crtc_state =
> +			to_vc4_crtc_state(crtc_state);
> +		unsigned int channel =
> +			vc4_crtc_state->assigned_channel;
> +
> +		if (channel == VC4_HVS_CHANNEL_DISABLED)
> +			continue;
> +
> +		if (!hvs_state->fifo_state[channel].in_use)
> +			continue;
> +
> +		hvs_state->fifo_state[channel].pending_commit =
> +			drm_crtc_commit_get(crtc_state->commit);
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * vc4_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -697,6 +790,7 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>  {
>  	struct vc4_hvs_state *old_state = to_vc4_hvs_state(obj->state);
>  	struct vc4_hvs_state *state;
> +	unsigned int i;
>  
>  	state = kzalloc(sizeof(*state), GFP_KERNEL);
>  	if (!state)
> @@ -706,6 +800,16 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
>  
>  	state->unassigned_channels = old_state->unassigned_channels;
>  
> +	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
> +		state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
> +
> +		if (!old_state->fifo_state[i].pending_commit)
> +			continue;
> +
> +		state->fifo_state[i].pending_commit =
> +			drm_crtc_commit_get(old_state->fifo_state[i].pending_commit);
> +	}
> +
>  	return &state->base;
>  }
>  
> @@ -713,6 +817,14 @@ static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
>  					   struct drm_private_state *state)
>  {
>  	struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
> +	unsigned int i;
> +
> +	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
> +		if (!hvs_state->fifo_state[i].pending_commit)
> +			continue;
> +
> +		drm_crtc_commit_put(hvs_state->fifo_state[i].pending_commit);
> +	}
>  
>  	kfree(hvs_state);
>  }
> @@ -805,7 +917,10 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>  
>  		/* If we're disabling our CRTC, we put back our channel */
>  		if (!new_crtc_state->enable) {
> -			hvs_new_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			channel = old_vc4_crtc_state->assigned_channel;
> +
> +			hvs_new_state->unassigned_channels |= BIT(channel);
> +			hvs_new_state->fifo_state[channel].in_use = false;
>  			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>  			continue;
>  		}
> @@ -841,6 +956,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>  		channel = ffs(matching_channels) - 1;
>  		new_vc4_crtc_state->assigned_channel = channel;
>  		hvs_new_state->unassigned_channels &= ~BIT(channel);
> +		hvs_new_state->fifo_state[channel].in_use = true;
>  	}
>  
>  	return 0;
> @@ -866,6 +982,10 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  	return vc4_load_tracker_atomic_check(state);
>  }
>  
> +static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = {
> +	.atomic_commit_setup	= vc4_atomic_commit_setup,
> +};
> +
>  static const struct drm_mode_config_funcs vc4_mode_funcs = {
>  	.atomic_check = vc4_atomic_check,
>  	.atomic_commit = vc4_atomic_commit,
> @@ -909,6 +1029,7 @@ int vc4_kms_load(struct drm_device *dev)
>  	}
>  
>  	dev->mode_config.funcs = &vc4_mode_funcs;
> +	dev->mode_config.helper_private = &vc4_mode_config_helpers;
>  	dev->mode_config.preferred_depth = 24;
>  	dev->mode_config.async_page_flip = true;
>  	dev->mode_config.allow_fb_modifiers = true;

Since I suggested this entire thing kinda:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -- 
> 2.28.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch