| Message ID | 20201105135656.383350-7-maxime@cerno.tech |
|---|---|
| State | Accepted |
| Commit | f2df84e096a8254ddb18c531b185fc2a45879077 |
| Headers | show |
| Series | drm/vc4: Rework the HVS muxing code | expand |
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
Hi Thomas, On Thu, Nov 19, 2020 at 09:59:15AM +0100, Thomas Zimmermann wrote: > 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> That works for me, I'll change it. Thanks! Maxime
Hi! On Thu, Nov 19, 2020 at 09:59:15AM +0100, Thomas Zimmermann wrote: > 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. Yeah, you're right, I'll change it > 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. There's other private states in that driver, and the other states are using the same construct here, I did so to remain consistent > In any case > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Thanks! Maxime
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; 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);
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(-)