Re: [PATCH v2 5/7] drm/vc4: kms: Remove unassigned_channels from the HVS state

Thomas Zimmermann
2020-12-11T10:13:04Z

[-- Attachment #1.1.1: Type: text/plain, Size: 4286 bytes --]

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

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

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