Re: [PATCH v3 6/7] drm/vc4: kms: Store the unassigned channel list in the state

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 9701 bytes --]

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@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Signed-off-by: Maxime Ripard <maxime@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@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

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel