Messages in this thread | | | Date | Thu, 27 Oct 2022 20:40:35 +0300 | Subject | Re: [PATCH v8 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc | From | Dmitry Baryshkov <> |
| |
On 27/10/2022 16:34, Vinod Polimera wrote: >> -----Original Message----- >> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Sent: Monday, October 24, 2022 8:52 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 v8 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 12/10/2022 15:02, 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. >>> >>> 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 13ce321..8ec9a13 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 9c6817b..d05b353 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; >>> @@ -1288,7 +1282,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; >>> @@ -1296,12 +1290,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); >> >> So, what if the user commits the mode setting change on another CPU, >> while we are handling the vblank callback here? Can this happen? >> > If user issues a commit on another CPU, it will wait in the drm_atomic_helper_swap_state > as drm_atomic_helper_commit_hw_done which does the complete_all(&commit->hw_done) > for the current commit didn't finish yet.
Yes. But there is no interlock between commit->hw_done and vblank IRQ processing, isn't it? This call happens when DPU processes the vblank IRQ, so nobody stops other core from swapping the encode state on another core.
>>> >>> DPU_ATRACE_END("encoder_vblank_callback"); >>> } >> -- >> With best wishes >> Dmitry > > Thanks, > Vinod P.
-- With best wishes Dmitry
| |