[PATCH v2 15/24] drm/msm: dpu: Stop using encoder->crtc pointer
Jeykumar Sankaran
jsanka at codeaurora.org
Mon Dec 3 19:43:43 UTC 2018
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 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
>>
>> 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 at 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
--
Jeykumar S
More information about the dri-devel
mailing list