Re: [Freedreno] [PATCH v2 15/24] drm/msm: dpu: Stop using encoder->crtc pointer
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 > > > > > > 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 > > > --- > > > 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 atomic_commit_tail will be called from the WQ. Do we hold the modeset locks in the WQ? I believe userspace thread will dr
Re: [Freedreno] [PATCH v2 15/24] drm/msm: dpu: Stop using encoder->crtc pointer
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 > > > > > > > > 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 > > > > --- > > > > 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 > > mod
Re: [Freedreno] [PATCH v2 15/24] drm/msm: dpu: Stop using encoder->crtc pointer
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 > > 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 > --- > 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 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_c
Re: [Freedreno] [PATCH v2 15/24] drm/msm: dpu: Stop using encoder->crtc pointer
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 > > > > 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 > > --- > > 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(
Re: [Freedreno] [PATCH v2 15/24] drm/msm: dpu: Stop using encoder->crtc pointer
On 2018-11-16 10:42, Sean Paul wrote: From: Sean Paul 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 --- 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? 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 1bec454
[Freedreno] [PATCH v2 15/24] drm/msm: dpu: Stop using encoder->crtc pointer
From: Sean Paul 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 --- 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); - } 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_pre