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@xxxxxxxxxx>
---
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
Attachment:
OpenPGP_0x680DC11D530B7A23.asc
Description: application/pgp-keys
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
