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 <seanp...@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 <seanp...@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 _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno