Re: [PATCH v9 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc
On 21/12/2022 16:10, Vinod Polimera wrote: -Original Message- From: Dmitry Baryshkov Sent: Wednesday, December 14, 2022 9:05 PM To: Vinod Polimera (QUIC) ; dri- de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org; freedr...@lists.freedesktop.org; devicet...@vger.kernel.org Cc: linux-ker...@vger.kernel.org; robdcl...@gmail.com; diand...@chromium.org; swb...@chromium.org; Kalyan Thota (QUIC) ; Kuogee Hsieh (QUIC) ; Vishnuvardhan Prodduturi (QUIC) ; Bjorn Andersson (QUIC) ; Aravind Venkateswaran (QUIC) ; Abhinav Kumar (QUIC) ; Sankeerth Billakanti (QUIC) 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 --- 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(_enc->vsync_cnt); + if (!dpu_enc->connector || !dpu_enc->connector->state || + !dpu_enc->connector->state->crtc) + return; - spin_lock_irqsave(_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc) - dpu_crtc_vblank_callback(dpu_enc->crtc); - spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags); + atomic_inc(_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_a
RE: [PATCH v9 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc
> -Original Message- > From: Dmitry Baryshkov > Sent: Wednesday, December 14, 2022 9:05 PM > To: Vinod Polimera (QUIC) ; dri- > de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org; > freedr...@lists.freedesktop.org; devicet...@vger.kernel.org > Cc: linux-ker...@vger.kernel.org; robdcl...@gmail.com; > diand...@chromium.org; swb...@chromium.org; Kalyan Thota (QUIC) > ; Kuogee Hsieh (QUIC) > ; Vishnuvardhan Prodduturi (QUIC) > ; Bjorn Andersson (QUIC) > ; Aravind Venkateswaran (QUIC) > ; Abhinav Kumar (QUIC) > ; Sankeerth Billakanti (QUIC) > > 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 > > --- > > 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"); > >
Re: [PATCH v9 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc
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 --- 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(_enc->vsync_cnt); + if (!dpu_enc->connector || !dpu_enc->connector->state || + !dpu_enc->connector->state->crtc) + return; - spin_lock_irqsave(_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc) - dpu_crtc_vblank_callback(dpu_enc->crtc); - spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags); + atomic_inc(_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(_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(_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(_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc != crtc) { -
[PATCH v9 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc
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 --- 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(_enc->vsync_cnt); + if (!dpu_enc->connector || !dpu_enc->connector->state || + !dpu_enc->connector->state->crtc) + return; - spin_lock_irqsave(_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc) - dpu_crtc_vblank_callback(dpu_enc->crtc); - spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags); + atomic_inc(_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(_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(_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(_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc != crtc) { -