[PATCH v3 6/7] drm/vc4: kms: Store the unassigned channel list in the state
Thomas Zimmermann
tzimmermann at suse.de
Thu Nov 19 03:59:15 EST 2020
Hi
Am 05.11.20 um 14:56 schrieb Maxime Ripard:
> If a CRTC is enabled but not active, and that we're then doing a page
> flip on another CRTC, drm_atomic_get_crtc_state will bring the first
> CRTC state into the global state, and will make us wait for its vblank
> as well, even though that might never occur.
>
> Instead of creating the list of the free channels each time atomic_check
> is called, and calling drm_atomic_get_crtc_state to retrieve the
> allocated channels, let's create a private state object in the main
> atomic state, and use it to store the available channels.
>
> Since vc4 has a semaphore (with a value of 1, so a lock) in its commit
> implementation to serialize all the commits, even the nonblocking ones, we
> are free from the use-after-free race if two subsequent commits are not ran
> in their submission order.
>
> Fixes: 87ebcd42fb7b ("drm/vc4: crtc: Assign output to channel automatically")
> Reviewed-by: Hoegeun Kwon <hoegeun.kwon at samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon at samsung.com>
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> ---
> drivers/gpu/drm/vc4/vc4_drv.h | 1 +
> drivers/gpu/drm/vc4/vc4_kms.c | 124 +++++++++++++++++++++++++++-------
> 2 files changed, 100 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index bdbb9540d47d..014113823647 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -219,6 +219,7 @@ struct vc4_dev {
>
> struct drm_modeset_lock ctm_state_lock;
> struct drm_private_obj ctm_manager;
> + struct drm_private_obj hvs_channels;
> struct drm_private_obj load_tracker;
>
> /* List of vc4_debugfs_info_entry for adding to debugfs once
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 499c6914fce4..0a231ae500e5 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -37,6 +37,17 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
> return container_of(priv, struct vc4_ctm_state, base);
> }
>
> +struct vc4_hvs_state {
> + struct drm_private_state base;
> + unsigned int unassigned_channels;
> +};
> +
> +static struct vc4_hvs_state *
> +to_vc4_hvs_state(struct drm_private_state *priv)
> +{
> + return container_of(priv, struct vc4_hvs_state, base);
> +}
> +
> struct vc4_load_tracker_state {
> struct drm_private_state base;
> u64 hvs_load;
> @@ -662,6 +673,70 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
> return drmm_add_action_or_reset(&vc4->base, vc4_load_tracker_obj_fini, NULL);
> }
>
> +static struct drm_private_state *
> +vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
> +{
> + struct vc4_hvs_state *state;
> +
> + state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return NULL;
> +
> + __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
> +
> + return &state->base;
> +}
> +
> +static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
> + struct drm_private_state *state)
> +{
> + struct vc4_hvs_state *hvs_state;
> +
> + hvs_state = to_vc4_hvs_state(state);
> + kfree(hvs_state);
> +}
> +
> +static const struct drm_private_state_funcs vc4_hvs_state_funcs = {
> + .atomic_duplicate_state = vc4_hvs_channels_duplicate_state,
> + .atomic_destroy_state = vc4_hvs_channels_destroy_state,
> +};
> +
> +static void vc4_hvs_channels_obj_fini(struct drm_device *dev, void *unused)
> +{
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> +
> + drm_atomic_private_obj_fini(&vc4->hvs_channels);
> +}
> +
> +static int vc4_hvs_channels_obj_init(struct vc4_dev *vc4)
> +{
> + struct vc4_hvs_state *state;
> +
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + 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);
> +
> + return drmm_add_action_or_reset(&vc4->base, vc4_hvs_channels_obj_fini, NULL);
> +}
> +
> +static struct vc4_hvs_state *
> +vc4_hvs_get_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_private_obj_state(state, &vc4->hvs_channels);
> + if (IS_ERR(priv_state))
> + return ERR_CAST(priv_state);
> +
> + return to_vc4_hvs_state(priv_state);
> +}
> +
> /*
> * The BCM2711 HVS has up to 7 output connected to the pixelvalves and
> * the TXP (and therefore all the CRTCs found on that platform).
> @@ -678,6 +753,14 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
> * need to consider all the running CRTCs in the DRM device to assign
> * a FIFO, not just the one in the state.
> *
> + * - To fix the above, we can't use drm_atomic_get_crtc_state on all
> + * enabled CRTCs to pull their CRTC state into the global state, since
> + * a page flip would start considering their vblank to complete. Since
> + * we don't have a guarantee that they are actually active, that
> + * vblank might never happen, and shouldn't even be considered if we
> + * want to do a page flip on a single CRTC. That can be tested by
> + * doing a modetest -v first on HDMI1 and then on HDMI0.
> + *
> * - Since we need the pixelvalve to be disabled and enabled back when
> * the FIFO is changed, we should keep the FIFO assigned for as long
> * as the CRTC is enabled, only considering it free again once that
> @@ -687,46 +770,33 @@ static int vc4_load_tracker_obj_init(struct vc4_dev *vc4)
> static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
> struct drm_atomic_state *state)
> {
> - unsigned long unassigned_channels = GENMASK(HVS_NUM_CHANNELS - 1, 0);
> + struct vc4_hvs_state *hvs_state;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> struct drm_crtc *crtc;
> unsigned int i;
>
> - /*
> - * Since the HVS FIFOs are shared across all the pixelvalves and
> - * the TXP (and thus all the CRTCs), we need to pull the current
> - * state of all the enabled CRTCs so that an update to a single
> - * CRTC still keeps the previous FIFOs enabled and assigned to
> - * the same CRTCs, instead of evaluating only the CRTC being
> - * modified.
> - */
> - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> - struct drm_crtc_state *crtc_state;
> -
> - if (!crtc->state->enable)
> - continue;
> -
> - crtc_state = drm_atomic_get_crtc_state(state, crtc);
> - if (IS_ERR(crtc_state))
> - return PTR_ERR(crtc_state);
> - }
> + hvs_state = vc4_hvs_get_global_state(state);
> + if (!hvs_state)
> + return -EINVAL;
I found this confusing. It's technically correct, but from hvs_state is
not clear that it's the new state. Maybe call it hvs_new_state.
If you want to be pedantic, maybe split the creation of the new state
from the usage. Call vc4_hvs_get_global_state() at the top of
vc4_atomic_check() to make the new state. (Maybe with a short comment.)
And here only call an equivalent of
drm_atomic_get_new_private_obj_state() for hvs_channels.
In any case
Reviewed-by: Thomas Zimmermann <tzimmermann at 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);
> struct vc4_crtc_state *new_vc4_crtc_state =
> to_vc4_crtc_state(new_crtc_state);
> struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> unsigned int matching_channels;
>
> - if (old_crtc_state->enable && !new_crtc_state->enable)
> + if (old_crtc_state->enable && !new_crtc_state->enable) {
> + hvs_state->unassigned_channels |= BIT(old_vc4_crtc_state->assigned_channel);
> new_vc4_crtc_state->assigned_channel = VC4_HVS_CHANNEL_DISABLED;
> + }
>
> if (!new_crtc_state->enable)
> continue;
>
> - if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED) {
> - unassigned_channels &= ~BIT(new_vc4_crtc_state->assigned_channel);
> + if (new_vc4_crtc_state->assigned_channel != VC4_HVS_CHANNEL_DISABLED)
> continue;
> - }
>
> /*
> * The problem we have to solve here is that we have
> @@ -752,12 +822,12 @@ 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 = unassigned_channels & vc4_crtc->data->hvs_available_channels;
> + matching_channels = hvs_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;
> - unassigned_channels &= ~BIT(channel);
> + hvs_state->unassigned_channels &= ~BIT(channel);
> } else {
> return -EINVAL;
> }
> @@ -841,6 +911,10 @@ int vc4_kms_load(struct drm_device *dev)
> if (ret)
> return ret;
>
> + ret = vc4_hvs_channels_obj_init(vc4);
> + if (ret)
> + return ret;
> +
> drm_mode_config_reset(dev);
>
> drm_kms_helper_poll_init(dev);
>
--
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/20201119/a04970d2/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/20201119/a04970d2/attachment-0001.sig>
More information about the linux-arm-kernel
mailing list