| Message ID | 1671012352-1825-2-git-send-email-quic_vpolimer@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add PSR support for eDP | expand |
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@quicinc.com> > --- > 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. > 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
> -----Original Message----- > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Sent: Wednesday, December 14, 2022 9:05 PM > To: Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>; dri- > devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; > freedreno@lists.freedesktop.org; devicetree@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; robdclark@gmail.com; > dianders@chromium.org; swboyd@chromium.org; Kalyan Thota (QUIC) > <quic_kalyant@quicinc.com>; Kuogee Hsieh (QUIC) > <quic_khsieh@quicinc.com>; Vishnuvardhan Prodduturi (QUIC) > <quic_vproddut@quicinc.com>; Bjorn Andersson (QUIC) > <quic_bjorande@quicinc.com>; Aravind Venkateswaran (QUIC) > <quic_aravindh@quicinc.com>; Abhinav Kumar (QUIC) > <quic_abhinavk@quicinc.com>; Sankeerth Billakanti (QUIC) > <quic_sbillaka@quicinc.com> > 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@quicinc.com> > > --- > > 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
On 21/12/2022 16:10, Vinod Polimera wrote: > > >> -----Original Message----- >> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Sent: Wednesday, December 14, 2022 9:05 PM >> To: Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>; dri- >> devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; >> freedreno@lists.freedesktop.org; devicetree@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org; robdclark@gmail.com; >> dianders@chromium.org; swboyd@chromium.org; Kalyan Thota (QUIC) >> <quic_kalyant@quicinc.com>; Kuogee Hsieh (QUIC) >> <quic_khsieh@quicinc.com>; Vishnuvardhan Prodduturi (QUIC) >> <quic_vproddut@quicinc.com>; Bjorn Andersson (QUIC) >> <quic_bjorande@quicinc.com>; Aravind Venkateswaran (QUIC) >> <quic_aravindh@quicinc.com>; Abhinav Kumar (QUIC) >> <quic_abhinavk@quicinc.com>; Sankeerth Billakanti (QUIC) >> <quic_sbillaka@quicinc.com> >> 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@quicinc.com> >>> --- >>> 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++; > } Yes. >> >>> 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 >
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) { 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
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@quicinc.com> --- 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(-)