Messages in this thread | | | Date | Wed, 21 Dec 2022 21:43:47 +0200 | Subject | Re: [PATCH v9 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc | From | Dmitry Baryshkov <> |
| |
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 >
-- With best wishes Dmitry
| |