Re: [PATCH v3 6/7] drm/vc4: kms: Store the unassigned channel list in the state

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 8055 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]