> -----Original Message----- > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > Sent: Wednesday, December 14, 2022 9:05 PM > To: Vinod Polimera (QUIC) <quic_vpolimer@xxxxxxxxxxx>; dri- > devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; > freedreno@xxxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxx; > dianders@xxxxxxxxxxxx; swboyd@xxxxxxxxxxxx; Kalyan Thota (QUIC) > <quic_kalyant@xxxxxxxxxxx>; Kuogee Hsieh (QUIC) > <quic_khsieh@xxxxxxxxxxx>; Vishnuvardhan Prodduturi (QUIC) > <quic_vproddut@xxxxxxxxxxx>; Bjorn Andersson (QUIC) > <quic_bjorande@xxxxxxxxxxx>; Aravind Venkateswaran (QUIC) > <quic_aravindh@xxxxxxxxxxx>; Abhinav Kumar (QUIC) > <quic_abhinavk@xxxxxxxxxxx>; Sankeerth Billakanti (QUIC) > <quic_sbillaka@xxxxxxxxxxx> > Subject: Re: [PATCH v9 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and > get crtc from connector state instead of dpu_enc > > WARNING: This email originated from outside of Qualcomm. Please be wary > of any links or attachments, and do not enable macros. > > On 14/12/2022 12:05, Vinod Polimera wrote: > > Update crtc retrieval from dpu_enc to dpu_enc connector state, > > since new links get set as part of the dpu enc virt mode set. > > The dpu_enc->crtc cache is no more needed, hence cleaning it as > > part of this change. > > > > This patch is dependent on the series: > > https://patchwork.freedesktop.org/series/110969/ > > > > Signed-off-by: Vinod Polimera <quic_vpolimer@xxxxxxxxxxx> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 42 +++++++++--------- > ----------- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 ------ > > 3 files changed, 13 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index 3f72d38..289d51e 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -1029,7 +1029,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > > */ > > if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO) > > release_bandwidth = true; > > - dpu_encoder_assign_crtc(encoder, NULL); > > } > > > > /* wait for frame_event_done completion */ > > @@ -1099,9 +1098,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, > > trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); > > dpu_crtc->enabled = true; > > > > - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state- > >encoder_mask) > > - dpu_encoder_assign_crtc(encoder, crtc); > > - > > /* Enable/restore vblank irq handling */ > > drm_crtc_vblank_on(crtc); > > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index a585036..b9b254d 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -132,11 +132,6 @@ enum dpu_enc_rc_states { > > * @intfs_swapped: Whether or not the phys_enc interfaces have been > swapped > > * for partial update right-only cases, such as pingpong > > * split where virtual pingpong does not generate IRQs > > - * @crtc: Pointer to the currently assigned crtc. Normally you > > - * would use crtc->state->encoder_mask to determine the > > - * link between encoder/crtc. However in this case we need > > - * to track crtc in the disable() hook which is called > > - * _after_ encoder_mask is cleared. > > * @connector: If a mode is set, cached pointer to the active > connector > > * @crtc_kickoff_cb: Callback into CRTC that will flush & start > > * all CTL paths > > @@ -181,7 +176,6 @@ struct dpu_encoder_virt { > > > > bool intfs_swapped; > > > > - struct drm_crtc *crtc; > > struct drm_connector *connector; > > > > struct dentry *debugfs_root; > > @@ -1317,7 +1311,7 @@ static void dpu_encoder_vblank_callback(struct > drm_encoder *drm_enc, > > struct dpu_encoder_phys *phy_enc) > > { > > struct dpu_encoder_virt *dpu_enc = NULL; > > - unsigned long lock_flags; > > + struct drm_crtc *crtc; > > > > if (!drm_enc || !phy_enc) > > return; > > @@ -1325,12 +1319,13 @@ static void dpu_encoder_vblank_callback(struct > drm_encoder *drm_enc, > > DPU_ATRACE_BEGIN("encoder_vblank_callback"); > > dpu_enc = to_dpu_encoder_virt(drm_enc); > > > > - atomic_inc(&phy_enc->vsync_cnt); > > + if (!dpu_enc->connector || !dpu_enc->connector->state || > > + !dpu_enc->connector->state->crtc) > > + return; > > > > - spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > - if (dpu_enc->crtc) > > - dpu_crtc_vblank_callback(dpu_enc->crtc); > > - spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > + atomic_inc(&phy_enc->vsync_cnt); > > + crtc = dpu_enc->connector->state->crtc; > > + dpu_crtc_vblank_callback(crtc); > > > > DPU_ATRACE_END("encoder_vblank_callback"); > > } > > @@ -1353,33 +1348,22 @@ static void > dpu_encoder_underrun_callback(struct drm_encoder *drm_enc, > > DPU_ATRACE_END("encoder_underrun_callback"); > > } > > > > -void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct > drm_crtc *crtc) > > -{ > > - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); > > - unsigned long lock_flags; > > - > > - spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > - /* crtc should always be cleared before re-assigning */ > > - WARN_ON(crtc && dpu_enc->crtc); > > - dpu_enc->crtc = crtc; > > - spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > -} > > - > > void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder > *drm_enc, > > struct drm_crtc *crtc, bool enable) > > { > > struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); > > - unsigned long lock_flags; > > + struct drm_crtc *new_crtc; > > int i; > > > > trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); > > > > - spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > - if (dpu_enc->crtc != crtc) { > > - spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > + if (!dpu_enc->connector || !dpu_enc->connector->state) > > + return; > > + > > + new_crtc = dpu_enc->connector->state->crtc; > > + if (!new_crtc || new_crtc != crtc) { > > I don't like going through all the pointers and states without locks. > The connector->state can potentially check. So, I'd propose even simpler > solution: just store the crtc in the dpu_enc at the creation time. Then > you can use the rest of the current dpu_enc->crtc as expected. Can i do as below. After encoder to crtc is pinned, i can update dpu_encoder structure that is attached with drm_enc with the crtc created during drm_obj_init @@ -816,6 +816,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) } priv->crtcs[priv->num_crtcs++] = crtc; encoder->possible_crtcs = 1 << drm_crtc_index(crtc); + dpu_encoder_assign_crtc(encoder, crtc); i++; } Thanks, Vinod P. > > > return; > > } > > - spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > > > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > > struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > index 7f3d823..eb9fc7c 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > @@ -40,14 +40,6 @@ struct msm_display_info { > > }; > > > > /** > > - * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to > > - * @encoder: encoder pointer > > - * @crtc: crtc pointer > > - */ > > -void dpu_encoder_assign_crtc(struct drm_encoder *encoder, > > - struct drm_crtc *crtc); > > - > > -/** > > * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on or > off if > > * the encoder is assigned to the given crtc > > * @encoder: encoder pointer > > -- > With best wishes > Dmitry
