Hi,
Here's a conversion of vc4 to remove the hand-rolled atomic_commit helper from vc4 in favour of the generic one.
This requires some rework of vc4, but also a new hook and some documentation for corner-cases in the DRM core that have been reported and explained by Daniel recently.
Let me know what you think, Maxime
Changes from v1: - Addressed the comments from Dave and Thomas on the documentation - s/last_user/pending_commit/ - Check that the commit is not NULL before waiting on it - Fixed a compilation error on an intermediate patch - Drop the assigned_channels variable redundant with the in_use variable
Maxime Ripard (7): drm: Introduce an atomic_commit_setup function drm: Document use-after-free gotcha with private objects drm/vc4: Simplify a bit the global atomic_check drm/vc4: kms: Wait on previous FIFO users before a commit drm/vc4: kms: Remove unassigned_channels from the HVS state drm/vc4: kms: Remove async modeset semaphore drm/vc4: kms: Convert to atomic helpers
drivers/gpu/drm/drm_atomic_helper.c | 9 + drivers/gpu/drm/vc4/vc4_crtc.c | 13 -- drivers/gpu/drm/vc4/vc4_drv.h | 2 - drivers/gpu/drm/vc4/vc4_kms.c | 248 +++++++++++------------ include/drm/drm_atomic.h | 20 ++ include/drm/drm_modeset_helper_vtables.h | 21 ++ 6 files changed, 172 insertions(+), 141 deletions(-)
Private objects storing a state shared across all CRTCs need to be carefully handled to avoid a use-after-free issue.
The proper way to do this to track all the commits using that shared state and wait for the previous commits to be done before going on with the current one to avoid the reordering of commits that could occur.
However, this commit setup needs to be done after drm_atomic_helper_setup_commit(), because before the CRTC commit structure hasn't been allocated before, and before the workqueue is scheduled, because we would be potentially reordered already otherwise.
That means that drivers currently have to roll their own drm_atomic_helper_commit() function, even though it would be identical if not for the commit setup.
Let's introduce a hook to do so that would be called as part of drm_atomic_helper_commit, allowing us to reuse the atomic helpers.
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ include/drm/drm_modeset_helper_vtables.h | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index f9170b4b22e7..f754e21b96eb 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2034,6 +2034,9 @@ crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc) * should always call this function from their * &drm_mode_config_funcs.atomic_commit hook. * + * Drivers that need to extend the commit setup to private objects can use the + * &drm_mode_config_helper_funcs.atomic_commit_setup hook. + * * To be able to use this support drivers need to use a few more helper * functions. drm_atomic_helper_wait_for_dependencies() must be called before * actually committing the hardware state, and for nonblocking commits this call @@ -2077,8 +2080,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, struct drm_plane *plane; struct drm_plane_state *old_plane_state, *new_plane_state; struct drm_crtc_commit *commit; + const struct drm_mode_config_helper_funcs *funcs; int i, ret;
+ funcs = state->dev->mode_config.helper_private; + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { commit = kzalloc(sizeof(*commit), GFP_KERNEL); if (!commit) @@ -2155,6 +2161,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, new_plane_state->commit = drm_crtc_commit_get(commit); }
+ if (funcs && funcs->atomic_commit_setup) + return funcs->atomic_commit_setup(state); + return 0; } EXPORT_SYMBOL(drm_atomic_helper_setup_commit); diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 4efec30f8bad..0ebb3f191bbc 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -1406,6 +1406,27 @@ struct drm_mode_config_helper_funcs { * drm_atomic_helper_commit_tail(). */ void (*atomic_commit_tail)(struct drm_atomic_state *state); + + /** + * @atomic_commit_setup: + * + * This hook is used by the default atomic_commit() hook implemented in + * drm_atomic_helper_commit() together with the nonblocking helpers (see + * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It + * is not used by the atomic helpers. + * + * This function is called at the end of + * drm_atomic_helper_setup_commit(), so once the commit has been + * properly setup across the generic DRM object states. It allows + * drivers to do some additional commit tracking that isn't related to a + * CRTC, plane or connector, tracked in a &drm_private_obj structure. + * + * Note that the documentation of &drm_private_obj has more details on + * how one should implement this. + * + * This hook is optional. + */ + int (*atomic_commit_setup)(struct drm_atomic_state *state); };
#endif
On Fri, Dec 4, 2020 at 4:11 PM Maxime Ripard maxime@cerno.tech wrote:
Private objects storing a state shared across all CRTCs need to be carefully handled to avoid a use-after-free issue.
The proper way to do this to track all the commits using that shared state and wait for the previous commits to be done before going on with the current one to avoid the reordering of commits that could occur.
However, this commit setup needs to be done after drm_atomic_helper_setup_commit(), because before the CRTC commit structure hasn't been allocated before, and before the workqueue is scheduled, because we would be potentially reordered already otherwise.
That means that drivers currently have to roll their own drm_atomic_helper_commit() function, even though it would be identical if not for the commit setup.
Let's introduce a hook to do so that would be called as part of drm_atomic_helper_commit, allowing us to reuse the atomic helpers.
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Maxime Ripard maxime@cerno.tech
r-b: me too
Cheers, Daniel
drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ include/drm/drm_modeset_helper_vtables.h | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index f9170b4b22e7..f754e21b96eb 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2034,6 +2034,9 @@ crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
- should always call this function from their
- &drm_mode_config_funcs.atomic_commit hook.
- Drivers that need to extend the commit setup to private objects can use the
- &drm_mode_config_helper_funcs.atomic_commit_setup hook.
- To be able to use this support drivers need to use a few more helper
- functions. drm_atomic_helper_wait_for_dependencies() must be called before
- actually committing the hardware state, and for nonblocking commits this call
@@ -2077,8 +2080,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, struct drm_plane *plane; struct drm_plane_state *old_plane_state, *new_plane_state; struct drm_crtc_commit *commit;
const struct drm_mode_config_helper_funcs *funcs; int i, ret;funcs = state->dev->mode_config.helper_private;for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { commit = kzalloc(sizeof(*commit), GFP_KERNEL); if (!commit)@@ -2155,6 +2161,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, new_plane_state->commit = drm_crtc_commit_get(commit); }
if (funcs && funcs->atomic_commit_setup)return funcs->atomic_commit_setup(state);return 0;} EXPORT_SYMBOL(drm_atomic_helper_setup_commit); diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 4efec30f8bad..0ebb3f191bbc 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -1406,6 +1406,27 @@ struct drm_mode_config_helper_funcs { * drm_atomic_helper_commit_tail(). */ void (*atomic_commit_tail)(struct drm_atomic_state *state);
/*** @atomic_commit_setup:** This hook is used by the default atomic_commit() hook implemented in* drm_atomic_helper_commit() together with the nonblocking helpers (see* drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It* is not used by the atomic helpers.** This function is called at the end of* drm_atomic_helper_setup_commit(), so once the commit has been* properly setup across the generic DRM object states. It allows* drivers to do some additional commit tracking that isn't related to a* CRTC, plane or connector, tracked in a &drm_private_obj structure.** Note that the documentation of &drm_private_obj has more details on* how one should implement this.** This hook is optional.*/int (*atomic_commit_setup)(struct drm_atomic_state *state);};
#endif
2.28.0
The private objects have a gotcha that could result in a use-after-free, make sure it's properly documented.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Maxime Ripard maxime@cerno.tech --- include/drm/drm_atomic.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index d07c851d255b..5d34c1df03f3 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -248,6 +248,26 @@ struct drm_private_state_funcs { * drm_dev_register() * 2/ all calls to drm_atomic_private_obj_fini() must be done after calling * drm_dev_unregister() + * + * If that private object is used to store a state shared by multiple + * CRTCs, proper care must be taken to ensure that non-blocking commits are + * properly ordered to avoid a use-after-free issue. + * + * Indeed, assuming a sequence of two non-blocking &drm_atomic_commit on two + * different &drm_crtc using different &drm_plane and &drm_connector, so with no + * resources shared, there's no guarantee on which commit is going to happen + * first. However, the second &drm_atomic_commit will consider the first + * &drm_private_obj its old state, and will be in charge of freeing it whenever + * the second &drm_atomic_commit is done. + * + * If the first &drm_atomic_commit happens after it, it will consider its + * &drm_private_obj the new state and will be likely to access it, resulting in + * an access to a freed memory region. Drivers should store (and get a reference + * to) the &drm_crtc_commit structure in our private state in + * &drm_mode_config_helper_funcs.atomic_commit_setup, and then wait for that + * commit to complete as the first step of + * &drm_mode_config_helper_funcs.atomic_commit_tail, similar to + * drm_atomic_helper_wait_for_dependencies(). */ struct drm_private_obj { /**
When we can't allocate a new channel, we can simply return instead of having to handle both cases, and that simplifies a bit the code.
Reviewed-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index ba310c0ab5f6..8937eb0b751d 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -794,6 +794,7 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, to_vc4_crtc_state(new_crtc_state); struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); unsigned int matching_channels; + unsigned int channel;
/* Nothing to do here, let's skip it */ if (old_crtc_state->enable == new_crtc_state->enable) @@ -834,14 +835,12 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, * but it works so far. */ matching_channels = hvs_new_state->unassigned_channels & vc4_crtc->data->hvs_available_channels; - if (matching_channels) { - unsigned int channel = ffs(matching_channels) - 1; - - new_vc4_crtc_state->assigned_channel = channel; - hvs_new_state->unassigned_channels &= ~BIT(channel); - } else { + if (!matching_channels) return -EINVAL; - } + + channel = ffs(matching_channels) - 1; + new_vc4_crtc_state->assigned_channel = channel; + hvs_new_state->unassigned_channels &= ~BIT(channel); }
return 0;
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"); + } + 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;
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
The HVS state now has both unassigned_channels that reflects the channels that are not used in the associated state, and the in_use boolean for each channel that says whether or not a particular channel is in use.
Both express pretty much the same thing, and we need the in_use variable to properly track the commits, so let's get rid of unassigned_channels.
Suggested-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index fdd698df5fbe..fa40c44eb770 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -39,7 +39,6 @@ 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; @@ -798,7 +797,6 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
- 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; @@ -849,7 +847,6 @@ static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4) if (!state) return -ENOMEM;
- state->unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0); drm_atomic_private_obj_init(&vc4->base, &vc4->hvs_channels, &state->base, &vc4_hvs_state_funcs); @@ -893,12 +890,17 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, struct vc4_hvs_state *hvs_new_state; struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_crtc *crtc; + unsigned int unassigned_channels; unsigned int i;
hvs_new_state = vc4_hvs_get_global_state(state); if (!hvs_new_state) return -EINVAL;
+ for (i = 0; i < HVS_NUM_CHANNELS; i++) + if (!hvs_new_state->fifo_state[i].in_use) + unassigned_channels |= BIT(i); + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { struct vc4_crtc_state *old_vc4_crtc_state = to_vc4_crtc_state(old_crtc_state); @@ -918,8 +920,6 @@ 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) { 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; @@ -949,13 +949,13 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, * the future, we will need to have something smarter, * but it works so far. */ - matching_channels = hvs_new_state->unassigned_channels & vc4_crtc->data->hvs_available_channels; + matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels; if (!matching_channels) return -EINVAL;
channel = ffs(matching_channels) - 1; new_vc4_crtc_state->assigned_channel = channel; - hvs_new_state->unassigned_channels &= ~BIT(channel); + unassigned_channels &= ~BIT(channel); hvs_new_state->fifo_state[channel].in_use = true; }
On Fri, Dec 04, 2020 at 04:11:36PM +0100, Maxime Ripard wrote:
@@ -893,12 +890,17 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, struct vc4_hvs_state *hvs_new_state; struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_crtc *crtc;
- unsigned int unassigned_channels;
This should be initialized to 0, I'll fix it up while applying if there's no other comment.
Maxime
Hi
Am 04.12.20 um 16:11 schrieb Maxime Ripard:
The HVS state now has both unassigned_channels that reflects the channels that are not used in the associated state, and the in_use boolean for each channel that says whether or not a particular channel is in use.
Both express pretty much the same thing, and we need the in_use variable to properly track the commits, so let's get rid of unassigned_channels.
Suggested-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_kms.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index fdd698df5fbe..fa40c44eb770 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -39,7 +39,6 @@ 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;
@@ -798,7 +797,6 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
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;
@@ -849,7 +847,6 @@ static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4) if (!state) return -ENOMEM;
- state->unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0); drm_atomic_private_obj_init(&vc4->base, &vc4->hvs_channels, &state->base, &vc4_hvs_state_funcs);
@@ -893,12 +890,17 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, struct vc4_hvs_state *hvs_new_state; struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_crtc *crtc;
unsigned int unassigned_channels; unsigned int i;
hvs_new_state = vc4_hvs_get_global_state(state); if (!hvs_new_state) return -EINVAL;
for (i = 0; i < HVS_NUM_CHANNELS; i++)
if (!hvs_new_state->fifo_state[i].in_use)unassigned_channels |= BIT(i);
More of a nit: I'd turn this block into a helper of struct vc4_hvs_state. That would also remove the need to initialize unassigned_channels to 0 here.
For the loop's condition, it might be less error prone to use ARRAY_SIZE(hvs_new_state->fifo_state) instead of HVS_NUM_CHANNEL.
In any case
Reviewed-by: Thomas Zimmermann tzimmermann@suse.de
Best regards Thomas
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { struct vc4_crtc_state *old_vc4_crtc_state = to_vc4_crtc_state(old_crtc_state); @@ -918,8 +920,6 @@ 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) { 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;@@ -949,13 +949,13 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, * the future, we will need to have something smarter, * but it works so far. */
matching_channels = hvs_new_state->unassigned_channels & vc4_crtc->data->hvs_available_channels;
matching_channels = unassigned_channels & vc4_crtc->data->hvs_available_channels;if (!matching_channels) return -EINVAL;
channel = ffs(matching_channels) - 1; new_vc4_crtc_state->assigned_channel = channel;
hvs_new_state->unassigned_channels &= ~BIT(channel);
unassigned_channels &= ~BIT(channel);
Now that we have proper ordering guaranteed by the previous patch, the semaphore is redundant and can be removed.
Acked-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_crtc.c | 13 ------------- drivers/gpu/drm/vc4/vc4_drv.h | 2 -- drivers/gpu/drm/vc4/vc4_kms.c | 24 ++---------------------- 3 files changed, 2 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 482219fb4db2..c469594a2d3a 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -690,7 +690,6 @@ vc4_async_page_flip_complete(struct vc4_seqno_cb *cb) container_of(cb, struct vc4_async_flip_state, cb); struct drm_crtc *crtc = flip_state->crtc; struct drm_device *dev = crtc->dev; - struct vc4_dev *vc4 = to_vc4_dev(dev); struct drm_plane *plane = crtc->primary;
vc4_plane_async_set_fb(plane, flip_state->fb); @@ -722,8 +721,6 @@ vc4_async_page_flip_complete(struct vc4_seqno_cb *cb) }
kfree(flip_state); - - up(&vc4->async_modeset); }
/* Implements async (non-vblank-synced) page flips. @@ -738,7 +735,6 @@ static int vc4_async_page_flip(struct drm_crtc *crtc, uint32_t flags) { struct drm_device *dev = crtc->dev; - struct vc4_dev *vc4 = to_vc4_dev(dev); struct drm_plane *plane = crtc->primary; int ret = 0; struct vc4_async_flip_state *flip_state; @@ -767,15 +763,6 @@ static int vc4_async_page_flip(struct drm_crtc *crtc, flip_state->crtc = crtc; flip_state->event = event;
- /* Make sure all other async modesetes have landed. */ - ret = down_interruptible(&vc4->async_modeset); - if (ret) { - drm_framebuffer_put(fb); - vc4_bo_dec_usecnt(bo); - kfree(flip_state); - return ret; - } - /* Save the current FB before it's replaced by the new one in * drm_atomic_set_fb_for_plane(). We'll need the old FB in * vc4_async_page_flip_complete() to decrement the BO usecnt and keep diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index c5f2944d5bc6..4dcef3140dff 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -215,8 +215,6 @@ struct vc4_dev { struct work_struct reset_work; } hangcheck;
- struct semaphore async_modeset; - struct drm_modeset_lock ctm_state_lock; struct drm_private_obj ctm_manager; struct drm_private_obj hvs_channels; diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index fa40c44eb770..ffbfdde55fff 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -418,8 +418,6 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) clk_set_min_rate(hvs->core_clk, 0);
drm_atomic_state_put(state); - - up(&vc4->async_modeset); }
static void commit_work(struct work_struct *work) @@ -477,26 +475,17 @@ static int vc4_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock) { - struct vc4_dev *vc4 = to_vc4_dev(dev); int ret;
if (state->async_update) { - ret = down_interruptible(&vc4->async_modeset); - if (ret) - return ret; - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) { - up(&vc4->async_modeset); + if (ret) return ret; - }
drm_atomic_helper_async_commit(dev, state);
drm_atomic_helper_cleanup_planes(dev, state);
- up(&vc4->async_modeset); - return 0; }
@@ -512,21 +501,14 @@ static int vc4_atomic_commit(struct drm_device *dev,
INIT_WORK(&state->commit_work, commit_work);
- ret = down_interruptible(&vc4->async_modeset); - if (ret) - return ret; - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) { - up(&vc4->async_modeset); + if (ret) return ret; - }
if (!nonblock) { ret = drm_atomic_helper_wait_for_fences(dev, state, true); if (ret) { drm_atomic_helper_cleanup_planes(dev, state); - up(&vc4->async_modeset); return ret; } } @@ -1008,8 +990,6 @@ int vc4_kms_load(struct drm_device *dev) vc4->load_tracker_enabled = true; }
- sema_init(&vc4->async_modeset, 1); - /* Set support for vblank irq fast disable, before drm_vblank_init() */ dev->vblank_disable_immediate = true;
Now that the semaphore is gone, our atomic_commit implementation is basically drm_atomic_helper_commit with a somewhat custom commit_tail, the main difference being that we're using wait_for_flip_done instead of wait_for_vblanks used in the drm_atomic_helper_commit_tail helper.
Let's switch to using drm_atomic_helper_commit.
Acked-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 110 +--------------------------------- 1 file changed, 3 insertions(+), 107 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index ffbfdde55fff..05f451f3e642 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -332,8 +332,7 @@ static void vc5_hvs_pv_muxing_commit(struct vc4_dev *vc4, } }
-static void -vc4_atomic_complete_commit(struct drm_atomic_state *state) +static void vc4_atomic_commit_tail(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); @@ -357,10 +356,6 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state) if (vc4->hvs->hvs5) clk_set_min_rate(hvs->core_clk, 500000000);
- drm_atomic_helper_wait_for_fences(dev, state, false); - - drm_atomic_helper_wait_for_dependencies(state); - old_hvs_state = vc4_hvs_get_old_global_state(state); if (!old_hvs_state) return; @@ -412,20 +407,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
drm_atomic_helper_cleanup_planes(dev, state);
- drm_atomic_helper_commit_cleanup_done(state); - if (vc4->hvs->hvs5) clk_set_min_rate(hvs->core_clk, 0); - - drm_atomic_state_put(state); -} - -static void commit_work(struct work_struct *work) -{ - struct drm_atomic_state *state = container_of(work, - struct drm_atomic_state, - commit_work); - vc4_atomic_complete_commit(state); }
static int vc4_atomic_commit_setup(struct drm_atomic_state *state) @@ -458,94 +441,6 @@ static int vc4_atomic_commit_setup(struct drm_atomic_state *state) return 0; }
-/** - * vc4_atomic_commit - commit validated state object - * @dev: DRM device - * @state: the driver state object - * @nonblock: nonblocking commit - * - * This function commits a with drm_atomic_helper_check() pre-validated state - * object. This can still fail when e.g. the framebuffer reservation fails. For - * now this doesn't implement asynchronous commits. - * - * RETURNS - * Zero for success or -errno. - */ -static int vc4_atomic_commit(struct drm_device *dev, - struct drm_atomic_state *state, - bool nonblock) -{ - int ret; - - if (state->async_update) { - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - - drm_atomic_helper_async_commit(dev, state); - - drm_atomic_helper_cleanup_planes(dev, state); - - return 0; - } - - /* We know for sure we don't want an async update here. Set - * state->legacy_cursor_update to false to prevent - * drm_atomic_helper_setup_commit() from auto-completing - * commit->flip_done. - */ - state->legacy_cursor_update = false; - ret = drm_atomic_helper_setup_commit(state, nonblock); - if (ret) - return ret; - - INIT_WORK(&state->commit_work, commit_work); - - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - - if (!nonblock) { - ret = drm_atomic_helper_wait_for_fences(dev, state, true); - if (ret) { - drm_atomic_helper_cleanup_planes(dev, state); - return ret; - } - } - - /* - * This is the point of no return - everything below never fails except - * when the hw goes bonghits. Which means we can commit the new state on - * the software side now. - */ - - BUG_ON(drm_atomic_helper_swap_state(state, false) < 0); - - /* - * Everything below can be run asynchronously without the need to grab - * any modeset locks at all under one condition: It must be guaranteed - * that the asynchronous work has either been cancelled (if the driver - * supports it, which at least requires that the framebuffers get - * cleaned up with drm_atomic_helper_cleanup_planes()) or completed - * before the new state gets committed on the software side with - * drm_atomic_helper_swap_state(). - * - * This scheme allows new atomic state updates to be prepared and - * checked in parallel to the asynchronous completion of the previous - * update. Which is important since compositors need to figure out the - * composition of the next frame right after having submitted the - * current layout. - */ - - drm_atomic_state_get(state); - if (nonblock) - queue_work(system_unbound_wq, &state->commit_work); - else - vc4_atomic_complete_commit(state); - - return 0; -} - static struct drm_framebuffer *vc4_fb_create(struct drm_device *dev, struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd) @@ -966,11 +861,12 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
static struct drm_mode_config_helper_funcs vc4_mode_config_helpers = { .atomic_commit_setup = vc4_atomic_commit_setup, + .atomic_commit_tail = vc4_atomic_commit_tail, };
static const struct drm_mode_config_funcs vc4_mode_funcs = { .atomic_check = vc4_atomic_check, - .atomic_commit = vc4_atomic_commit, + .atomic_commit = drm_atomic_helper_commit, .fb_create = vc4_fb_create, };
On Fri, Dec 04, 2020 at 04:11:31PM +0100, Maxime Ripard wrote:
Hi,
Here's a conversion of vc4 to remove the hand-rolled atomic_commit helper from vc4 in favour of the generic one.
This requires some rework of vc4, but also a new hook and some documentation for corner-cases in the DRM core that have been reported and explained by Daniel recently.
Let me know what you think, Maxime
Applied, thanks! Maxime
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Maxime Ripard -
Thomas Zimmermann