[Freedreno] [PATCH v2 15/24] drm/msm: dpu: Stop using encoder->crtc pointer
Sean Paul
sean at poorly.run
Fri Nov 16 21:14:17 UTC 2018
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 at 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 at 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, ¶ms);
> > }
> >
> > @@ -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
--
Sean Paul, Software Engineer, Google / Chromium OS
More information about the Freedreno
mailing list