diff mbox series

[v2,15/24] drm/msm: dpu: Stop using encoder->crtc pointer

Message ID 20181116184238.170034-16-sean@poorly.run (mailing list archive)
State Not Applicable, archived
Headers show
Series drm/msm: Various dpu locking and legacy cleanups | expand

Commit Message

Sean Paul Nov. 16, 2018, 6:42 p.m. UTC 
From: Sean Paul <seanpaul@chromium.org>

It's for legacy drivers, for atomic drivers crtc->state->encoder_mask
should be used to map encoder to crtc.

Changes in v2:
- None

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46 ++++++++----------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
 2 files changed, 29 insertions(+), 36 deletions(-)

Comments

Jeykumar Sankaran Nov. 16, 2018, 8:05 p.m. UTC | #1 
On 2018-11-16 10:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> It's for legacy drivers, for atomic drivers crtc->state->encoder_mask
> should be used to map encoder to crtc.
> 
> Changes in v2:
> - None
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46 ++++++++----------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
>  2 files changed, 29 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 156f4c77ca44..a008a87a8113 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> drm_crtc *crtc)
>  		return INTF_MODE_NONE;
>  	}
> 
> -	drm_for_each_encoder(encoder, crtc->dev)
> -		if (encoder->crtc == crtc)
> -			return dpu_encoder_get_intf_mode(encoder);
> +	/* TODO: Returns the first INTF_MODE, could there be multiple
> values? */
> +	drm_for_each_encoder_mask(encoder, crtc->dev,
> crtc->state->encoder_mask)
> +		return dpu_encoder_get_intf_mode(encoder);
> 
>  	return INTF_MODE_NONE;
>  }
> @@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> *crtc,
>  		spin_unlock_irqrestore(&dev->event_lock, flags);
>  	}
> 
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> {
> -		if (encoder->crtc != crtc)
> -			continue;
> -
> -		/* encoder will trigger pending mask now */
> +	/* encoder will trigger pending mask now */
> +	drm_for_each_encoder_mask(encoder, crtc->dev,
> crtc->state->encoder_mask)
>  		dpu_encoder_trigger_kickoff_pending(encoder);
> -	}
> 
>  	/*
>  	 * If no mixers have been allocated in dpu_crtc_atomic_check(),
> @@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct
> drm_crtc *crtc)
>  void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  {
>  	struct drm_encoder *encoder;
> -	struct drm_device *dev = crtc->dev;
>  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>  	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
>  	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> @@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc 
> *crtc)
> 
>  	DPU_ATRACE_BEGIN("crtc_commit");
> 
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> {
> +	/*
> +	 * Encoder will flush/start now, unless it has a tx pending. If
> so, it
> +	 * may delay and flush at an irq event (e.g. ppdone)
> +	 */
> +	drm_for_each_encoder_mask(encoder, crtc->dev,
> +				  crtc->state->encoder_mask) {
>  		struct dpu_encoder_kickoff_params params = { 0 };
> -
> -		if (encoder->crtc != crtc)
> -			continue;
> -
> -		/*
> -		 * Encoder will flush/start now, unless it has a tx
> pending.
> -		 * If so, it may delay and flush at an irq event (e.g.
> ppdone)
> -		 */
>  		dpu_encoder_prepare_for_kickoff(encoder, &params);
>  	}
> 
> @@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc 
> *crtc)
> 
>  	dpu_vbif_clear_errors(dpu_kms);
> 
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> {
> -		if (encoder->crtc != crtc)
> -			continue;
> -
> +	drm_for_each_encoder_mask(encoder, crtc->dev,
> crtc->state->encoder_mask)
>  		dpu_encoder_kickoff(encoder);
> -	}
We wont be holding the modeset locks here (and in crtc_atomic_begin) in 
the display thread. Is
it safe to iterate over encoder_mask?
> 
>  end:
>  	reinit_completion(&dpu_crtc->frame_done_comp);
> @@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc 
> *crtc)
> 
>  	dpu_core_perf_crtc_update(crtc, 0, true);
> 
> -	drm_for_each_encoder(encoder, crtc->dev) {
> -		if (encoder->crtc != crtc)
> -			continue;
> +	drm_for_each_encoder_mask(encoder, crtc->dev,
> crtc->state->encoder_mask)
>  		dpu_encoder_register_frame_event_callback(encoder, NULL,
> NULL);
> -	}
> 
>  	memset(cstate->mixers, 0, sizeof(cstate->mixers));
>  	cstate->num_mixers = 0;
> @@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
>  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>  	dpu_crtc = to_dpu_crtc(crtc);
> 
> -	drm_for_each_encoder(encoder, crtc->dev) {
> -		if (encoder->crtc != crtc)
> -			continue;
> +	drm_for_each_encoder_mask(encoder, crtc->dev,
> crtc->state->encoder_mask)
>  		dpu_encoder_register_frame_event_callback(encoder,
>  				dpu_crtc_frame_event_cb, (void *)crtc);
> -	}
> 
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 1bec4540f3e1..64134d619748 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct msm_kms
> *kms,
>  	struct dpu_kms *dpu_kms;
>  	struct msm_drm_private *priv;
>  	struct drm_device *dev;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
>  	struct drm_encoder *encoder;
> +	int i;
> 
>  	if (!kms)
>  		return;
> @@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct msm_kms
> *kms,
>  	priv = dev->dev_private;
>  	pm_runtime_get_sync(&dpu_kms->pdev->dev);
> 
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> -		if (encoder->crtc != NULL)
> +	/* Call prepare_commit for all affected encoders */
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		drm_for_each_encoder_mask(encoder, crtc->dev,
> +					  crtc_state->encoder_mask) {
>  			dpu_encoder_prepare_commit(encoder);
> +		}
> +	}
>  }
> 
>  /*
> @@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct msm_kms
> *kms,
>  void dpu_kms_encoder_enable(struct drm_encoder *encoder)
>  {
>  	const struct drm_encoder_helper_funcs *funcs =
> encoder->helper_private;
> -	struct drm_crtc *crtc = encoder->crtc;
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_crtc *crtc;
> 
>  	/* Forward this enable call to the commit hook */
>  	if (funcs && funcs->commit)
>  		funcs->commit(encoder);
> 
> -	if (crtc && crtc->state->active) {
> +	drm_for_each_crtc(crtc, dev) {
> +		if (!(crtc->state->encoder_mask &
> drm_encoder_mask(encoder)))
> +			continue;
> +
>  		trace_dpu_kms_enc_enable(DRMID(crtc));
>  		dpu_crtc_commit_kickoff(crtc);
>  	}
Sean Paul Nov. 16, 2018, 9:14 p.m. UTC | #2 
On Fri, Nov 16, 2018 at 12:05:09PM -0800, Jeykumar Sankaran wrote:
> On 2018-11-16 10:42, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > It's for legacy drivers, for atomic drivers crtc->state->encoder_mask
> > should be used to map encoder to crtc.
> > 
> > Changes in v2:
> > - None
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46 ++++++++----------------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
> >  2 files changed, 29 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 156f4c77ca44..a008a87a8113 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> > drm_crtc *crtc)
> >  		return INTF_MODE_NONE;
> >  	}
> > 
> > -	drm_for_each_encoder(encoder, crtc->dev)
> > -		if (encoder->crtc == crtc)
> > -			return dpu_encoder_get_intf_mode(encoder);
> > +	/* TODO: Returns the first INTF_MODE, could there be multiple
> > values? */
> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > crtc->state->encoder_mask)
> > +		return dpu_encoder_get_intf_mode(encoder);
> > 
> >  	return INTF_MODE_NONE;
> >  }
> > @@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> > *crtc,
> >  		spin_unlock_irqrestore(&dev->event_lock, flags);
> >  	}
> > 
> > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> > {
> > -		if (encoder->crtc != crtc)
> > -			continue;
> > -
> > -		/* encoder will trigger pending mask now */
> > +	/* encoder will trigger pending mask now */
> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > crtc->state->encoder_mask)
> >  		dpu_encoder_trigger_kickoff_pending(encoder);
> > -	}
> > 
> >  	/*
> >  	 * If no mixers have been allocated in dpu_crtc_atomic_check(),
> > @@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct
> > drm_crtc *crtc)
> >  void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> >  {
> >  	struct drm_encoder *encoder;
> > -	struct drm_device *dev = crtc->dev;
> >  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> >  	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
> >  	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> > @@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > *crtc)
> > 
> >  	DPU_ATRACE_BEGIN("crtc_commit");
> > 
> > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> > {
> > +	/*
> > +	 * Encoder will flush/start now, unless it has a tx pending. If
> > so, it
> > +	 * may delay and flush at an irq event (e.g. ppdone)
> > +	 */
> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > +				  crtc->state->encoder_mask) {
> >  		struct dpu_encoder_kickoff_params params = { 0 };
> > -
> > -		if (encoder->crtc != crtc)
> > -			continue;
> > -
> > -		/*
> > -		 * Encoder will flush/start now, unless it has a tx
> > pending.
> > -		 * If so, it may delay and flush at an irq event (e.g.
> > ppdone)
> > -		 */
> >  		dpu_encoder_prepare_for_kickoff(encoder, &params);
> >  	}
> > 
> > @@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> > 
> >  	dpu_vbif_clear_errors(dpu_kms);
> > 
> > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> > {
> > -		if (encoder->crtc != crtc)
> > -			continue;
> > -
> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > crtc->state->encoder_mask)
> >  		dpu_encoder_kickoff(encoder);
> > -	}
> We wont be holding the modeset locks here (and in crtc_atomic_begin) in the
> display thread. Is
> it safe to iterate over encoder_mask?

Hmm, I'm not sure I follow. AFAICT, there are 2 callsites for
dpu_crtc_commit_kickoff():

1- dpu_kms_encoder_enable() which is called via the encoder->funcs->enable hook
2- dpu_kms_commit() which is called in the mode_config->funcs->atomic_commit_tail

Both of these callsites will hold the modeset locks.

Am I missing something?

Thanks for your review,

Sean

> > 
> >  end:
> >  	reinit_completion(&dpu_crtc->frame_done_comp);
> > @@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
> > 
> >  	dpu_core_perf_crtc_update(crtc, 0, true);
> > 
> > -	drm_for_each_encoder(encoder, crtc->dev) {
> > -		if (encoder->crtc != crtc)
> > -			continue;
> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > crtc->state->encoder_mask)
> >  		dpu_encoder_register_frame_event_callback(encoder, NULL,
> > NULL);
> > -	}
> > 
> >  	memset(cstate->mixers, 0, sizeof(cstate->mixers));
> >  	cstate->num_mixers = 0;
> > @@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
> >  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
> >  	dpu_crtc = to_dpu_crtc(crtc);
> > 
> > -	drm_for_each_encoder(encoder, crtc->dev) {
> > -		if (encoder->crtc != crtc)
> > -			continue;
> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > crtc->state->encoder_mask)
> >  		dpu_encoder_register_frame_event_callback(encoder,
> >  				dpu_crtc_frame_event_cb, (void *)crtc);
> > -	}
> > 
> >  	mutex_lock(&dpu_crtc->crtc_lock);
> >  	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 1bec4540f3e1..64134d619748 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct msm_kms
> > *kms,
> >  	struct dpu_kms *dpu_kms;
> >  	struct msm_drm_private *priv;
> >  	struct drm_device *dev;
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *crtc_state;
> >  	struct drm_encoder *encoder;
> > +	int i;
> > 
> >  	if (!kms)
> >  		return;
> > @@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct msm_kms
> > *kms,
> >  	priv = dev->dev_private;
> >  	pm_runtime_get_sync(&dpu_kms->pdev->dev);
> > 
> > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> > -		if (encoder->crtc != NULL)
> > +	/* Call prepare_commit for all affected encoders */
> > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > +		drm_for_each_encoder_mask(encoder, crtc->dev,
> > +					  crtc_state->encoder_mask) {
> >  			dpu_encoder_prepare_commit(encoder);
> > +		}
> > +	}
> >  }
> > 
> >  /*
> > @@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct msm_kms
> > *kms,
> >  void dpu_kms_encoder_enable(struct drm_encoder *encoder)
> >  {
> >  	const struct drm_encoder_helper_funcs *funcs =
> > encoder->helper_private;
> > -	struct drm_crtc *crtc = encoder->crtc;
> > +	struct drm_device *dev = encoder->dev;
> > +	struct drm_crtc *crtc;
> > 
> >  	/* Forward this enable call to the commit hook */
> >  	if (funcs && funcs->commit)
> >  		funcs->commit(encoder);
> > 
> > -	if (crtc && crtc->state->active) {
> > +	drm_for_each_crtc(crtc, dev) {
> > +		if (!(crtc->state->encoder_mask &
> > drm_encoder_mask(encoder)))
> > +			continue;
> > +
> >  		trace_dpu_kms_enc_enable(DRMID(crtc));
> >  		dpu_crtc_commit_kickoff(crtc);
> >  	}
> 
> -- 
> Jeykumar S
Jeykumar Sankaran Nov. 19, 2018, 8:03 p.m. UTC | #3 
On 2018-11-16 13:14, Sean Paul wrote:
> On Fri, Nov 16, 2018 at 12:05:09PM -0800, Jeykumar Sankaran wrote:
>> On 2018-11-16 10:42, Sean Paul wrote:
>> > From: Sean Paul <seanpaul@chromium.org>
>> >
>> > It's for legacy drivers, for atomic drivers crtc->state->encoder_mask
>> > should be used to map encoder to crtc.
>> >
>> > Changes in v2:
>> > - None
>> >
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46
> ++++++++----------------
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
>> >  2 files changed, 29 insertions(+), 36 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > index 156f4c77ca44..a008a87a8113 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > @@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
>> > drm_crtc *crtc)
>> >  		return INTF_MODE_NONE;
>> >  	}
>> >
>> > -	drm_for_each_encoder(encoder, crtc->dev)
>> > -		if (encoder->crtc == crtc)
>> > -			return dpu_encoder_get_intf_mode(encoder);
>> > +	/* TODO: Returns the first INTF_MODE, could there be multiple
>> > values? */
>> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > crtc->state->encoder_mask)
>> > +		return dpu_encoder_get_intf_mode(encoder);
>> >
>> >  	return INTF_MODE_NONE;
>> >  }
>> > @@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
>> > *crtc,
>> >  		spin_unlock_irqrestore(&dev->event_lock, flags);
>> >  	}
>> >
>> > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
>> > {
>> > -		if (encoder->crtc != crtc)
>> > -			continue;
>> > -
>> > -		/* encoder will trigger pending mask now */
>> > +	/* encoder will trigger pending mask now */
>> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > crtc->state->encoder_mask)
>> >  		dpu_encoder_trigger_kickoff_pending(encoder);
>> > -	}
>> >
>> >  	/*
>> >  	 * If no mixers have been allocated in dpu_crtc_atomic_check(),
>> > @@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct
>> > drm_crtc *crtc)
>> >  void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>> >  {
>> >  	struct drm_encoder *encoder;
>> > -	struct drm_device *dev = crtc->dev;
>> >  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>> >  	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
>> >  	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
>> > @@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
>> > *crtc)
>> >
>> >  	DPU_ATRACE_BEGIN("crtc_commit");
>> >
>> > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
>> > {
>> > +	/*
>> > +	 * Encoder will flush/start now, unless it has a tx pending. If
>> > so, it
>> > +	 * may delay and flush at an irq event (e.g. ppdone)
>> > +	 */
>> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > +				  crtc->state->encoder_mask) {
>> >  		struct dpu_encoder_kickoff_params params = { 0 };
>> > -
>> > -		if (encoder->crtc != crtc)
>> > -			continue;
>> > -
>> > -		/*
>> > -		 * Encoder will flush/start now, unless it has a tx
>> > pending.
>> > -		 * If so, it may delay and flush at an irq event (e.g.
>> > ppdone)
>> > -		 */
>> >  		dpu_encoder_prepare_for_kickoff(encoder, &params);
>> >  	}
>> >
>> > @@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> *crtc)
>> >
>> >  	dpu_vbif_clear_errors(dpu_kms);
>> >
>> > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
>> > {
>> > -		if (encoder->crtc != crtc)
>> > -			continue;
>> > -
>> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > crtc->state->encoder_mask)
>> >  		dpu_encoder_kickoff(encoder);
>> > -	}
>> We wont be holding the modeset locks here (and in crtc_atomic_begin) 
>> in
> the
>> display thread. Is
>> it safe to iterate over encoder_mask?
> 
> Hmm, I'm not sure I follow. AFAICT, there are 2 callsites for
> dpu_crtc_commit_kickoff():
> 
> 1- dpu_kms_encoder_enable() which is called via the 
> encoder->funcs->enable
> hook
> 2- dpu_kms_commit() which is called in the
> mode_config->funcs->atomic_commit_tail

atomic_commit_tail will be called from the WQ. Do we hold
the modeset locks in the WQ? I believe userspace thread will drop
the locks at the end of drm_mode_atomic_ioctl.

Thanks
> 
> Both of these callsites will hold the modeset locks.
> 
> Am I missing something?
> 
> Thanks for your review,
> 
> Sean
> 
>> >
>> >  end:
>> >  	reinit_completion(&dpu_crtc->frame_done_comp);
>> > @@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc
> *crtc)
>> >
>> >  	dpu_core_perf_crtc_update(crtc, 0, true);
>> >
>> > -	drm_for_each_encoder(encoder, crtc->dev) {
>> > -		if (encoder->crtc != crtc)
>> > -			continue;
>> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > crtc->state->encoder_mask)
>> >  		dpu_encoder_register_frame_event_callback(encoder, NULL,
>> > NULL);
>> > -	}
>> >
>> >  	memset(cstate->mixers, 0, sizeof(cstate->mixers));
>> >  	cstate->num_mixers = 0;
>> > @@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc
> *crtc,
>> >  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>> >  	dpu_crtc = to_dpu_crtc(crtc);
>> >
>> > -	drm_for_each_encoder(encoder, crtc->dev) {
>> > -		if (encoder->crtc != crtc)
>> > -			continue;
>> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > crtc->state->encoder_mask)
>> >  		dpu_encoder_register_frame_event_callback(encoder,
>> >  				dpu_crtc_frame_event_cb, (void *)crtc);
>> > -	}
>> >
>> >  	mutex_lock(&dpu_crtc->crtc_lock);
>> >  	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > index 1bec4540f3e1..64134d619748 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > @@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct msm_kms
>> > *kms,
>> >  	struct dpu_kms *dpu_kms;
>> >  	struct msm_drm_private *priv;
>> >  	struct drm_device *dev;
>> > +	struct drm_crtc *crtc;
>> > +	struct drm_crtc_state *crtc_state;
>> >  	struct drm_encoder *encoder;
>> > +	int i;
>> >
>> >  	if (!kms)
>> >  		return;
>> > @@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct msm_kms
>> > *kms,
>> >  	priv = dev->dev_private;
>> >  	pm_runtime_get_sync(&dpu_kms->pdev->dev);
>> >
>> > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
>> > -		if (encoder->crtc != NULL)
>> > +	/* Call prepare_commit for all affected encoders */
>> > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> > +		drm_for_each_encoder_mask(encoder, crtc->dev,
>> > +					  crtc_state->encoder_mask) {
>> >  			dpu_encoder_prepare_commit(encoder);
>> > +		}
>> > +	}
>> >  }
>> >
>> >  /*
>> > @@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct
> msm_kms
>> > *kms,
>> >  void dpu_kms_encoder_enable(struct drm_encoder *encoder)
>> >  {
>> >  	const struct drm_encoder_helper_funcs *funcs =
>> > encoder->helper_private;
>> > -	struct drm_crtc *crtc = encoder->crtc;
>> > +	struct drm_device *dev = encoder->dev;
>> > +	struct drm_crtc *crtc;
>> >
>> >  	/* Forward this enable call to the commit hook */
>> >  	if (funcs && funcs->commit)
>> >  		funcs->commit(encoder);
>> >
>> > -	if (crtc && crtc->state->active) {
>> > +	drm_for_each_crtc(crtc, dev) {
>> > +		if (!(crtc->state->encoder_mask &
>> > drm_encoder_mask(encoder)))
>> > +			continue;
>> > +
>> >  		trace_dpu_kms_enc_enable(DRMID(crtc));
>> >  		dpu_crtc_commit_kickoff(crtc);
>> >  	}
>> 
>> --
>> Jeykumar S
Sean Paul Nov. 26, 2018, 9:53 p.m. UTC | #4 
On Mon, Nov 19, 2018 at 12:03:53PM -0800, Jeykumar Sankaran wrote:
> On 2018-11-16 13:14, Sean Paul wrote:
> > On Fri, Nov 16, 2018 at 12:05:09PM -0800, Jeykumar Sankaran wrote:
> > > On 2018-11-16 10:42, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > It's for legacy drivers, for atomic drivers crtc->state->encoder_mask
> > > > should be used to map encoder to crtc.
> > > >
> > > > Changes in v2:
> > > > - None
> > > >
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46
> > ++++++++----------------
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
> > > >  2 files changed, 29 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > index 156f4c77ca44..a008a87a8113 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > @@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> > > > drm_crtc *crtc)
> > > >  		return INTF_MODE_NONE;
> > > >  	}
> > > >
> > > > -	drm_for_each_encoder(encoder, crtc->dev)
> > > > -		if (encoder->crtc == crtc)
> > > > -			return dpu_encoder_get_intf_mode(encoder);
> > > > +	/* TODO: Returns the first INTF_MODE, could there be multiple
> > > > values? */
> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > > > crtc->state->encoder_mask)
> > > > +		return dpu_encoder_get_intf_mode(encoder);
> > > >
> > > >  	return INTF_MODE_NONE;
> > > >  }
> > > > @@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> > > > *crtc,
> > > >  		spin_unlock_irqrestore(&dev->event_lock, flags);
> > > >  	}
> > > >
> > > > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> > > > {
> > > > -		if (encoder->crtc != crtc)
> > > > -			continue;
> > > > -
> > > > -		/* encoder will trigger pending mask now */
> > > > +	/* encoder will trigger pending mask now */
> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > > > crtc->state->encoder_mask)
> > > >  		dpu_encoder_trigger_kickoff_pending(encoder);
> > > > -	}
> > > >
> > > >  	/*
> > > >  	 * If no mixers have been allocated in dpu_crtc_atomic_check(),
> > > > @@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct
> > > > drm_crtc *crtc)
> > > >  void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> > > >  {
> > > >  	struct drm_encoder *encoder;
> > > > -	struct drm_device *dev = crtc->dev;
> > > >  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> > > >  	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
> > > >  	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> > > > @@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > > > *crtc)
> > > >
> > > >  	DPU_ATRACE_BEGIN("crtc_commit");
> > > >
> > > > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> > > > {
> > > > +	/*
> > > > +	 * Encoder will flush/start now, unless it has a tx pending. If
> > > > so, it
> > > > +	 * may delay and flush at an irq event (e.g. ppdone)
> > > > +	 */
> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > > > +				  crtc->state->encoder_mask) {
> > > >  		struct dpu_encoder_kickoff_params params = { 0 };
> > > > -
> > > > -		if (encoder->crtc != crtc)
> > > > -			continue;
> > > > -
> > > > -		/*
> > > > -		 * Encoder will flush/start now, unless it has a tx
> > > > pending.
> > > > -		 * If so, it may delay and flush at an irq event (e.g.
> > > > ppdone)
> > > > -		 */
> > > >  		dpu_encoder_prepare_for_kickoff(encoder, &params);
> > > >  	}
> > > >
> > > > @@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > *crtc)
> > > >
> > > >  	dpu_vbif_clear_errors(dpu_kms);
> > > >
> > > > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> > > > {
> > > > -		if (encoder->crtc != crtc)
> > > > -			continue;
> > > > -
> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > > > crtc->state->encoder_mask)
> > > >  		dpu_encoder_kickoff(encoder);
> > > > -	}
> > > We wont be holding the modeset locks here (and in crtc_atomic_begin)
> > > in
> > the
> > > display thread. Is
> > > it safe to iterate over encoder_mask?
> > 
> > Hmm, I'm not sure I follow. AFAICT, there are 2 callsites for
> > dpu_crtc_commit_kickoff():
> > 
> > 1- dpu_kms_encoder_enable() which is called via the
> > encoder->funcs->enable
> > hook
> > 2- dpu_kms_commit() which is called in the
> > mode_config->funcs->atomic_commit_tail
> 
> atomic_commit_tail will be called from the WQ. Do we hold
> the modeset locks in the WQ? I believe userspace thread will drop
> the locks at the end of drm_mode_atomic_ioctl.

Accessing state is safe in non-blocking commits due to stalling any subsequent
commits until the previous one has completed. Check out the comment at:

https://cgit.freedesktop.org/drm-tip/tree/drivers/gpu/drm/drm_atomic_helper.c#n1754

Sean

> 
> Thanks
> > 
> > Both of these callsites will hold the modeset locks.
> > 
> > Am I missing something?
> > 
> > Thanks for your review,
> > 
> > Sean
> > 
> > > >
> > > >  end:
> > > >  	reinit_completion(&dpu_crtc->frame_done_comp);
> > > > @@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc
> > *crtc)
> > > >
> > > >  	dpu_core_perf_crtc_update(crtc, 0, true);
> > > >
> > > > -	drm_for_each_encoder(encoder, crtc->dev) {
> > > > -		if (encoder->crtc != crtc)
> > > > -			continue;
> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > > > crtc->state->encoder_mask)
> > > >  		dpu_encoder_register_frame_event_callback(encoder, NULL,
> > > > NULL);
> > > > -	}
> > > >
> > > >  	memset(cstate->mixers, 0, sizeof(cstate->mixers));
> > > >  	cstate->num_mixers = 0;
> > > > @@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc
> > *crtc,
> > > >  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
> > > >  	dpu_crtc = to_dpu_crtc(crtc);
> > > >
> > > > -	drm_for_each_encoder(encoder, crtc->dev) {
> > > > -		if (encoder->crtc != crtc)
> > > > -			continue;
> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > > > crtc->state->encoder_mask)
> > > >  		dpu_encoder_register_frame_event_callback(encoder,
> > > >  				dpu_crtc_frame_event_cb, (void *)crtc);
> > > > -	}
> > > >
> > > >  	mutex_lock(&dpu_crtc->crtc_lock);
> > > >  	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > index 1bec4540f3e1..64134d619748 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > @@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct msm_kms
> > > > *kms,
> > > >  	struct dpu_kms *dpu_kms;
> > > >  	struct msm_drm_private *priv;
> > > >  	struct drm_device *dev;
> > > > +	struct drm_crtc *crtc;
> > > > +	struct drm_crtc_state *crtc_state;
> > > >  	struct drm_encoder *encoder;
> > > > +	int i;
> > > >
> > > >  	if (!kms)
> > > >  		return;
> > > > @@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct msm_kms
> > > > *kms,
> > > >  	priv = dev->dev_private;
> > > >  	pm_runtime_get_sync(&dpu_kms->pdev->dev);
> > > >
> > > > -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> > > > -		if (encoder->crtc != NULL)
> > > > +	/* Call prepare_commit for all affected encoders */
> > > > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > > > +		drm_for_each_encoder_mask(encoder, crtc->dev,
> > > > +					  crtc_state->encoder_mask) {
> > > >  			dpu_encoder_prepare_commit(encoder);
> > > > +		}
> > > > +	}
> > > >  }
> > > >
> > > >  /*
> > > > @@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct
> > msm_kms
> > > > *kms,
> > > >  void dpu_kms_encoder_enable(struct drm_encoder *encoder)
> > > >  {
> > > >  	const struct drm_encoder_helper_funcs *funcs =
> > > > encoder->helper_private;
> > > > -	struct drm_crtc *crtc = encoder->crtc;
> > > > +	struct drm_device *dev = encoder->dev;
> > > > +	struct drm_crtc *crtc;
> > > >
> > > >  	/* Forward this enable call to the commit hook */
> > > >  	if (funcs && funcs->commit)
> > > >  		funcs->commit(encoder);
> > > >
> > > > -	if (crtc && crtc->state->active) {
> > > > +	drm_for_each_crtc(crtc, dev) {
> > > > +		if (!(crtc->state->encoder_mask &
> > > > drm_encoder_mask(encoder)))
> > > > +			continue;
> > > > +
> > > >  		trace_dpu_kms_enc_enable(DRMID(crtc));
> > > >  		dpu_crtc_commit_kickoff(crtc);
> > > >  	}
> > > 
> > > --
> > > Jeykumar S
> 
> -- 
> Jeykumar S
Jeykumar Sankaran Dec. 3, 2018, 7:43 p.m. UTC | #5 
On 2018-11-26 13:53, Sean Paul wrote:
> On Mon, Nov 19, 2018 at 12:03:53PM -0800, Jeykumar Sankaran wrote:
>> On 2018-11-16 13:14, Sean Paul wrote:
>> > On Fri, Nov 16, 2018 at 12:05:09PM -0800, Jeykumar Sankaran wrote:
>> > > On 2018-11-16 10:42, Sean Paul wrote:
>> > > > From: Sean Paul <seanpaul@chromium.org>
>> > > >
>> > > > It's for legacy drivers, for atomic drivers
> crtc->state->encoder_mask
>> > > > should be used to map encoder to crtc.
>> > > >
>> > > > Changes in v2:
>> > > > - None
>> > > >
>> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > > > ---
>> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46
>> > ++++++++----------------
>> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
>> > > >  2 files changed, 29 insertions(+), 36 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > > > index 156f4c77ca44..a008a87a8113 100644
>> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > > > @@ -284,9 +284,9 @@ enum dpu_intf_mode
> dpu_crtc_get_intf_mode(struct
>> > > > drm_crtc *crtc)
>> > > >  		return INTF_MODE_NONE;
>> > > >  	}
>> > > >
>> > > > -	drm_for_each_encoder(encoder, crtc->dev)
>> > > > -		if (encoder->crtc == crtc)
>> > > > -			return dpu_encoder_get_intf_mode(encoder);
>> > > > +	/* TODO: Returns the first INTF_MODE, could there be
> multiple
>> > > > values? */
>> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > > > crtc->state->encoder_mask)
>> > > > +		return dpu_encoder_get_intf_mode(encoder);
>> > > >
>> > > >  	return INTF_MODE_NONE;
>> > > >  }
>> > > > @@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct
> drm_crtc
>> > > > *crtc,
>> > > >  		spin_unlock_irqrestore(&dev->event_lock, flags);
>> > > >  	}
>> > > >
>> > > > -	list_for_each_entry(encoder,
> &dev->mode_config.encoder_list, head)
>> > > > {
>> > > > -		if (encoder->crtc != crtc)
>> > > > -			continue;
>> > > > -
>> > > > -		/* encoder will trigger pending mask now */
>> > > > +	/* encoder will trigger pending mask now */
>> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > > > crtc->state->encoder_mask)
>> > > >  		dpu_encoder_trigger_kickoff_pending(encoder);
>> > > > -	}
>> > > >
>> > > >  	/*
>> > > >  	 * If no mixers have been allocated in
> dpu_crtc_atomic_check(),
>> > > > @@ -704,7 +700,6 @@ static int
> _dpu_crtc_wait_for_frame_done(struct
>> > > > drm_crtc *crtc)
>> > > >  void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>> > > >  {
>> > > >  	struct drm_encoder *encoder;
>> > > > -	struct drm_device *dev = crtc->dev;
>> > > >  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>> > > >  	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
>> > > >  	struct dpu_crtc_state *cstate =
> to_dpu_crtc_state(crtc->state);
>> > > > @@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
>> > > > *crtc)
>> > > >
>> > > >  	DPU_ATRACE_BEGIN("crtc_commit");
>> > > >
>> > > > -	list_for_each_entry(encoder,
> &dev->mode_config.encoder_list, head)
>> > > > {
>> > > > +	/*
>> > > > +	 * Encoder will flush/start now, unless it has a tx
> pending. If
>> > > > so, it
>> > > > +	 * may delay and flush at an irq event (e.g. ppdone)
>> > > > +	 */
>> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > > > +				  crtc->state->encoder_mask) {
>> > > >  		struct dpu_encoder_kickoff_params params = { 0 };
>> > > > -
>> > > > -		if (encoder->crtc != crtc)
>> > > > -			continue;
>> > > > -
>> > > > -		/*
>> > > > -		 * Encoder will flush/start now, unless it has a
> tx
>> > > > pending.
>> > > > -		 * If so, it may delay and flush at an irq event
> (e.g.
>> > > > ppdone)
>> > > > -		 */
>> > > >  		dpu_encoder_prepare_for_kickoff(encoder, &params);
>> > > >  	}
>> > > >
>> > > > @@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
>> > *crtc)
>> > > >
>> > > >  	dpu_vbif_clear_errors(dpu_kms);
>> > > >
>> > > > -	list_for_each_entry(encoder,
> &dev->mode_config.encoder_list, head)
>> > > > {
>> > > > -		if (encoder->crtc != crtc)
>> > > > -			continue;
>> > > > -
>> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > > > crtc->state->encoder_mask)
>> > > >  		dpu_encoder_kickoff(encoder);
>> > > > -	}
>> > > We wont be holding the modeset locks here (and in crtc_atomic_begin)
>> > > in
>> > the
>> > > display thread. Is
>> > > it safe to iterate over encoder_mask?
>> >
>> > Hmm, I'm not sure I follow. AFAICT, there are 2 callsites for
>> > dpu_crtc_commit_kickoff():
>> >
>> > 1- dpu_kms_encoder_enable() which is called via the
>> > encoder->funcs->enable
>> > hook
>> > 2- dpu_kms_commit() which is called in the
>> > mode_config->funcs->atomic_commit_tail
>> 
>> atomic_commit_tail will be called from the WQ. Do we hold
>> the modeset locks in the WQ? I believe userspace thread will drop
>> the locks at the end of drm_mode_atomic_ioctl.
> 
> Accessing state is safe in non-blocking commits due to stalling any
> subsequent
> commits until the previous one has completed. Check out the comment at:
> 
> https://cgit.freedesktop.org/drm-tip/tree/drivers/gpu/drm/drm_atomic_helpe
> r.c#n1754
> 
> Sean
> 
Thanks for the link Sean!

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

>> 
>> Thanks
>> >
>> > Both of these callsites will hold the modeset locks.
>> >
>> > Am I missing something?
>> >
>> > Thanks for your review,
>> >
>> > Sean
>> >
>> > > >
>> > > >  end:
>> > > >  	reinit_completion(&dpu_crtc->frame_done_comp);
>> > > > @@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc
>> > *crtc)
>> > > >
>> > > >  	dpu_core_perf_crtc_update(crtc, 0, true);
>> > > >
>> > > > -	drm_for_each_encoder(encoder, crtc->dev) {
>> > > > -		if (encoder->crtc != crtc)
>> > > > -			continue;
>> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > > > crtc->state->encoder_mask)
>> > > >  		dpu_encoder_register_frame_event_callback(encoder,
> NULL,
>> > > > NULL);
>> > > > -	}
>> > > >
>> > > >  	memset(cstate->mixers, 0, sizeof(cstate->mixers));
>> > > >  	cstate->num_mixers = 0;
>> > > > @@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc
>> > *crtc,
>> > > >  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>> > > >  	dpu_crtc = to_dpu_crtc(crtc);
>> > > >
>> > > > -	drm_for_each_encoder(encoder, crtc->dev) {
>> > > > -		if (encoder->crtc != crtc)
>> > > > -			continue;
>> > > > +	drm_for_each_encoder_mask(encoder, crtc->dev,
>> > > > crtc->state->encoder_mask)
>> > > >  		dpu_encoder_register_frame_event_callback(encoder,
>> > > >  				dpu_crtc_frame_event_cb, (void
> *)crtc);
>> > > > -	}
>> > > >
>> > > >  	mutex_lock(&dpu_crtc->crtc_lock);
>> > > >  	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
>> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > > > index 1bec4540f3e1..64134d619748 100644
>> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > > > @@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct
> msm_kms
>> > > > *kms,
>> > > >  	struct dpu_kms *dpu_kms;
>> > > >  	struct msm_drm_private *priv;
>> > > >  	struct drm_device *dev;
>> > > > +	struct drm_crtc *crtc;
>> > > > +	struct drm_crtc_state *crtc_state;
>> > > >  	struct drm_encoder *encoder;
>> > > > +	int i;
>> > > >
>> > > >  	if (!kms)
>> > > >  		return;
>> > > > @@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct
> msm_kms
>> > > > *kms,
>> > > >  	priv = dev->dev_private;
>> > > >  	pm_runtime_get_sync(&dpu_kms->pdev->dev);
>> > > >
>> > > > -	list_for_each_entry(encoder,
> &dev->mode_config.encoder_list, head)
>> > > > -		if (encoder->crtc != NULL)
>> > > > +	/* Call prepare_commit for all affected encoders */
>> > > > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> > > > +		drm_for_each_encoder_mask(encoder, crtc->dev,
>> > > > +
> crtc_state->encoder_mask) {
>> > > >  			dpu_encoder_prepare_commit(encoder);
>> > > > +		}
>> > > > +	}
>> > > >  }
>> > > >
>> > > >  /*
>> > > > @@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct
>> > msm_kms
>> > > > *kms,
>> > > >  void dpu_kms_encoder_enable(struct drm_encoder *encoder)
>> > > >  {
>> > > >  	const struct drm_encoder_helper_funcs *funcs =
>> > > > encoder->helper_private;
>> > > > -	struct drm_crtc *crtc = encoder->crtc;
>> > > > +	struct drm_device *dev = encoder->dev;
>> > > > +	struct drm_crtc *crtc;
>> > > >
>> > > >  	/* Forward this enable call to the commit hook */
>> > > >  	if (funcs && funcs->commit)
>> > > >  		funcs->commit(encoder);
>> > > >
>> > > > -	if (crtc && crtc->state->active) {
>> > > > +	drm_for_each_crtc(crtc, dev) {
>> > > > +		if (!(crtc->state->encoder_mask &
>> > > > drm_encoder_mask(encoder)))
>> > > > +			continue;
>> > > > +
>> > > >  		trace_dpu_kms_enc_enable(DRMID(crtc));
>> > > >  		dpu_crtc_commit_kickoff(crtc);
>> > > >  	}
>> > >
>> > > --
>> > > Jeykumar S
>> 
>> --
>> Jeykumar S
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 156f4c77ca44..a008a87a8113 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -284,9 +284,9 @@  enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
 		return INTF_MODE_NONE;
 	}
 
-	drm_for_each_encoder(encoder, crtc->dev)
-		if (encoder->crtc == crtc)
-			return dpu_encoder_get_intf_mode(encoder);
+	/* TODO: Returns the first INTF_MODE, could there be multiple values? */
+	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
+		return dpu_encoder_get_intf_mode(encoder);
 
 	return INTF_MODE_NONE;
 }
@@ -551,13 +551,9 @@  static void dpu_crtc_atomic_begin(struct drm_crtc *crtc,
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
-		if (encoder->crtc != crtc)
-			continue;
-
-		/* encoder will trigger pending mask now */
+	/* encoder will trigger pending mask now */
+	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
 		dpu_encoder_trigger_kickoff_pending(encoder);
-	}
 
 	/*
 	 * If no mixers have been allocated in dpu_crtc_atomic_check(),
@@ -704,7 +700,6 @@  static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc)
 void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
-	struct drm_device *dev = crtc->dev;
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
@@ -720,16 +715,13 @@  void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 
 	DPU_ATRACE_BEGIN("crtc_commit");
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+	/*
+	 * Encoder will flush/start now, unless it has a tx pending. If so, it
+	 * may delay and flush at an irq event (e.g. ppdone)
+	 */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
+				  crtc->state->encoder_mask) {
 		struct dpu_encoder_kickoff_params params = { 0 };
-
-		if (encoder->crtc != crtc)
-			continue;
-
-		/*
-		 * Encoder will flush/start now, unless it has a tx pending.
-		 * If so, it may delay and flush at an irq event (e.g. ppdone)
-		 */
 		dpu_encoder_prepare_for_kickoff(encoder, &params);
 	}
 
@@ -754,12 +746,8 @@  void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 
 	dpu_vbif_clear_errors(dpu_kms);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
-		if (encoder->crtc != crtc)
-			continue;
-
+	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
 		dpu_encoder_kickoff(encoder);
-	}
 
 end:
 	reinit_completion(&dpu_crtc->frame_done_comp);
@@ -883,11 +871,8 @@  static void dpu_crtc_disable(struct drm_crtc *crtc)
 
 	dpu_core_perf_crtc_update(crtc, 0, true);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
-	}
 
 	memset(cstate->mixers, 0, sizeof(cstate->mixers));
 	cstate->num_mixers = 0;
@@ -922,12 +907,9 @@  static void dpu_crtc_enable(struct drm_crtc *crtc,
 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 	dpu_crtc = to_dpu_crtc(crtc);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder,
 				dpu_crtc_frame_event_cb, (void *)crtc);
-	}
 
 	mutex_lock(&dpu_crtc->crtc_lock);
 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 1bec4540f3e1..64134d619748 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -320,7 +320,10 @@  static void dpu_kms_prepare_commit(struct msm_kms *kms,
 	struct dpu_kms *dpu_kms;
 	struct msm_drm_private *priv;
 	struct drm_device *dev;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
 	struct drm_encoder *encoder;
+	int i;
 
 	if (!kms)
 		return;
@@ -332,9 +335,13 @@  static void dpu_kms_prepare_commit(struct msm_kms *kms,
 	priv = dev->dev_private;
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc != NULL)
+	/* Call prepare_commit for all affected encoders */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		drm_for_each_encoder_mask(encoder, crtc->dev,
+					  crtc_state->encoder_mask) {
 			dpu_encoder_prepare_commit(encoder);
+		}
+	}
 }
 
 /*
@@ -344,13 +351,17 @@  static void dpu_kms_prepare_commit(struct msm_kms *kms,
 void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 {
 	const struct drm_encoder_helper_funcs *funcs = encoder->helper_private;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_device *dev = encoder->dev;
+	struct drm_crtc *crtc;
 
 	/* Forward this enable call to the commit hook */
 	if (funcs && funcs->commit)
 		funcs->commit(encoder);
 
-	if (crtc && crtc->state->active) {
+	drm_for_each_crtc(crtc, dev) {
+		if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
+			continue;
+
 		trace_dpu_kms_enc_enable(DRMID(crtc));
 		dpu_crtc_commit_kickoff(crtc);
 	}