[PATCH 6/8] drm/vc4: kms: Wait on previous FIFO users before a commit

[Thomas Zimmermann]

Hi

Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> 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.

Can there only ever be two commits that flip order?

> 
> 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.

I'd appreciate a comment in the code that explains a bit how this works. 
It's sort of clear to me, but not enough to fully get it.

> 
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_kms.c | 118 +++++++++++++++++++++++++++++++++-
>   1 file changed, 117 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 3034a5a6637e..849bc6b4cea4 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 *last_user;

Can these updates run concurrently? If so, the concurrency control via 
in_use is dubious.

I find last_user to be confusing. Maybe pending_commit makes sense?


> +	} 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)
>   {
> @@ -310,6 +341,7 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>   	struct vc4_hvs *hvs = vc4->hvs;
>   	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 +361,32 @@ 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, 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 (!old_hvs_state->fifo_state[channel].in_use)
> +			continue;
> +
> +		commit = old_hvs_state->fifo_state[i].last_user;
> +		ret = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);

For these returned values I'd use a separate variable, say 'complete' or 
'done'. It's an unsigned long and not the negative errno code that one 
would expect in ret.

> +		if (!ret)
> +			DRM_DEV_ERROR(dev, "Timed out waiting for hw_done\n");

 From the comments in drm_print.h, I think drm_err() is preferred over 
DRM_DEV_ERROR().

> +
> +		ret = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
> +		if (!ret)
> +			DRM_DEV_ERROR(dev, "Timed out waiting for flip_done\n");
> +	}
> +
>   	drm_atomic_helper_commit_modeset_disables(dev, state);
>   
>   	vc4_ctm_commit(vc4, state);
> @@ -368,6 +426,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].last_user =
> +			drm_crtc_commit_get(crtc_state->commit);
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * vc4_atomic_commit - commit validated state object
>    * @dev: DRM device
> @@ -697,6 +785,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 +795,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].last_user)
> +			continue;
> +
> +		state->fifo_state[i].last_user =
> +			drm_crtc_commit_get(old_state->fifo_state[i].last_user);

A pure style issue: I'd avoid continue and instead write this as

if (old_state->fifo_state[i].last_user)
     state->fifo_state[i].last_user = drm_crtc_commit_get(...)

Here and in destroy_state.

> +	}
> +
>   	return &state->base;
>   }
>   
> @@ -713,6 +812,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].last_user)
> +			continue;
> +
> +		drm_crtc_commit_put(hvs_state->fifo_state[i].last_user);
> +	}
>   
>   	kfree(hvs_state);
>   }
> @@ -808,7 +915,10 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
>   
>   		/* If we're disabling our CRTC, we put back our channel */
>   		if (old_crtc_state->enable && !new_crtc_state->enable) {
> -			hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> +			channel = old_vc4_crtc_state->assigned_channel;
> +
> +			hvs_state->unassigned_channels |= BIT(channel);
> +			hvs_state->fifo_state[channel].in_use = false;

It looks like in_use correlates with the bit in unassigned_channels. 
Could you drop in_use entirely?

>   			new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
>   			continue;
>   		}
> @@ -844,6 +954,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_state->unassigned_channels &= ~BIT(channel);
> +		hvs_state->fifo_state[channel].in_use = true;
>   	}
>   
>   	return 0;
> @@ -869,6 +980,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,

OK, I assume we'll go with this callback.

Best regards
Thomas

> +};
> +
>   static const struct drm_mode_config_funcs vc4_mode_funcs = {
>   	.atomic_check = vc4_atomic_check,
>   	.atomic_commit = vc4_atomic_commit,
> @@ -912,6 +1027,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;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0x680DC11D530B7A23.asc
Type: application/pgp-keys
Size: 7435 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20201120/2de425a0/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20201120/2de425a0/attachment-0001.sig>