[PATCH 6/8] drm/vc4: kms: Wait on previous FIFO users before a commit
Thomas Zimmermann
tzimmermann at suse.de
Fri Nov 20 13:19:45 UTC 2020
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: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201120/2de425a0/attachment-0001.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201120/2de425a0/attachment-0001.sig>
More information about the dri-devel
mailing list