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);
>   		hvs_new_state->fifo_state[channel].in_use = 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