Re: [PATCH 4/9] drm/msm: add kms->wait_flush()

[Sean Paul]

On Tue, Aug 27, 2019 at 02:33:34PM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@xxxxxxxxxxxx>
> 
> First step in re-working the atomic related internal API to prepare for
> async updates pending.. ->wait_flush() is intended to block until there
> is no in-progress flush.
> 
> A crtc_mask is used, rather than an atomic state object, as this will
> later be used for async flush after the atomic state is destroyed.
> 
> This replaces ->wait_for_crtc_commit_done()
> 
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>

Hi Rob,
A few nits below, but lgtm overall


> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 11 ++++++-
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 17 ++++++----
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 17 ++++++----
>  drivers/gpu/drm/msm/msm_atomic.c         | 42 ++++++++++--------------
>  drivers/gpu/drm/msm/msm_kms.h            |  9 +++--
>  5 files changed, 54 insertions(+), 42 deletions(-)
> 

/snip

> index e5aae1645933..639cc88c31a1 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -10,28 +10,6 @@
>  #include "msm_gem.h"
>  #include "msm_kms.h"
>  
> -static void msm_atomic_wait_for_commit_done(struct drm_device *dev,
> -		struct drm_atomic_state *old_state)
> -{
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_crtc_state;
> -	struct msm_drm_private *priv = old_state->dev->dev_private;
> -	struct msm_kms *kms = priv->kms;
> -	int i;
> -
> -	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		if (!new_crtc_state->active)
> -			continue;
> -
> -		if (drm_crtc_vblank_get(crtc))
> -			continue;
> -
> -		kms->funcs->wait_for_crtc_commit_done(kms, crtc);
> -
> -		drm_crtc_vblank_put(crtc);
> -	}
> -}
> -
>  int msm_atomic_prepare_fb(struct drm_plane *plane,
>  			  struct drm_plane_state *new_state)
>  {
> @@ -51,11 +29,28 @@ int msm_atomic_prepare_fb(struct drm_plane *plane,
>  	return msm_framebuffer_prepare(new_state->fb, kms->aspace);
>  }
>  
> +/* Get bitmask of crtcs that will need to be flushed.  The bitmask
> + * can be used with for_each_crtc_mask() iterator, to iterate
> + * effected crtcs without needing to preserve the atomic state.
> + */
> +static unsigned get_crtc_mask(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	unsigned i, mask = 0;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> +		mask |= BIT(crtc->index);

mask |= drm_crtc_mask(crtc);

> +
> +	return mask;
> +}
> +
>  void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct msm_drm_private *priv = dev->dev_private;
>  	struct msm_kms *kms = priv->kms;
> +	unsigned crtc_mask = get_crtc_mask(state);
>  
>  	kms->funcs->prepare_commit(kms, state);
>  
> @@ -70,8 +65,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  		kms->funcs->commit(kms, state);
>  	}
>  
> -	msm_atomic_wait_for_commit_done(dev, state);
> -
> +	kms->funcs->wait_flush(kms, crtc_mask);
>  	kms->funcs->complete_commit(kms, state);
>  
>  	drm_atomic_helper_commit_hw_done(state);
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index c7588a42635e..8562bbfd5dca 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -34,9 +34,8 @@ struct msm_kms_funcs {
>  	void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
>  	void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state);
>  	void (*complete_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
> -	/* functions to wait for atomic commit completed on each CRTC */
> -	void (*wait_for_crtc_commit_done)(struct msm_kms *kms,
> -					struct drm_crtc *crtc);
> +	void (*wait_flush)(struct msm_kms *kms, unsigned crtc_mask);
> +
>  	/* get msm_format w/ optional format modifiers from drm_mode_fb_cmd2 */
>  	const struct msm_format *(*get_format)(struct msm_kms *kms,
>  					const uint32_t format,
> @@ -98,4 +97,8 @@ struct msm_mdss {
>  int mdp5_mdss_init(struct drm_device *dev);
>  int dpu_mdss_init(struct drm_device *dev);
>  
> +#define for_each_crtc_mask(dev, crtc, crtc_mask) \
> +	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head) \

drm_for_each_crtc

> +		for_each_if (BIT((crtc)->index) & (crtc_mask))
> +
>  #endif /* __MSM_KMS_H__ */
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS