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@xxxxxxxxxxx>
> > Tested-by: Hoegeun Kwon <hoegeun.kwon@xxxxxxxxxxx>
> > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > ---
> > 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@xxxxxxx>
That works for me, I'll change it.
Thanks!
Maxime
Attachment:
signature.asc
Description: PGP signature
