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 > Tested-by: Hoegeun Kwon > Signed-off-by: Maxime Ripard > --- > 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 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