Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm/dpu: wrong pingpong index in dpu_encoder_helper_phys_setup_cwb() --
commit 43be42587de24c595bae6d488f5806f627560896 Author: Xi Zhang <[email protected]> drm/msm/dpu: use cached PINGPONGs from mode set in cwb setup This commit replaces the global state lookup of DPU_HW_BLK_PINGPONG blocks in dpu_encoder_helper_phys_setup_cwb() with the cached dpu_enc->hw_pp array from dpu_encoder_virt_atomic_mode_set(). > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 7d092398fc991..f1d2482b787b0 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -2363,11 +2363,6 @@ void dpu_encoder_helper_phys_setup_cwb(struct > dpu_encoder_phys *phys_enc, > struct dpu_hw_ctl *hw_ctl; > struct dpu_hw_cwb_setup_cfg cwb_cfg; > > - struct dpu_kms *dpu_kms; > - struct dpu_global_state *global_state; > - struct dpu_hw_blk *rt_pp_list[MAX_CHANNELS_PER_ENC]; > - int num_pp; > - > if (!phys_enc->hw_wb) > return; > > @@ -2379,33 +2374,20 @@ void dpu_encoder_helper_phys_setup_cwb(struct > dpu_encoder_phys *phys_enc, > return; > } > > - dpu_kms = phys_enc->dpu_kms; > - global_state = dpu_kms_get_existing_global_state(dpu_kms); > - num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > - phys_enc->parent->crtc, > - DPU_HW_BLK_PINGPONG, rt_pp_list, > - ARRAY_SIZE(rt_pp_list)); > - > - if (num_pp == 0 || num_pp > MAX_CHANNELS_PER_ENC) { > - DPU_DEBUG_ENC(dpu_enc, "invalid num_pp %d\n", num_pp); > - return; > - } > - > /* > * The CWB mux supports using LM or DSPP as tap points. For now, > * always use LM tap point > */ > cwb_cfg.input = INPUT_MODE_LM_OUT; > > - for (int i = 0; i < MAX_CWB_PER_ENC; i++) { > + for (int i = 0; i < ARRAY_SIZE(dpu_enc->hw_cwb); i++) { > hw_cwb = dpu_enc->hw_cwb[i]; > if (!hw_cwb) > continue; > > if (enable) { > - struct dpu_hw_pingpong *hw_pp = > - to_dpu_hw_pingpong(rt_pp_list[i]); > - cwb_cfg.pp_idx = hw_pp->idx; > + cwb_cfg.pp_idx = i < ARRAY_SIZE(dpu_enc->hw_pp) && > dpu_enc->hw_pp[i] ? > + dpu_enc->hw_pp[i]->idx : PINGPONG_NONE; [Severity: High] Does using dpu_enc->hw_pp[i]->idx here pass the dedicated CWB pingpong index (PINGPONG_CWB_*) instead of the real-time display's pingpong index into cwb_cfg.pp_idx? dpu_encoder_helper_phys_setup_cwb() only configures CWB blocks when called on a writeback encoder where dpu_enc->hw_cwb[i] is populated. In dpu_encoder_virt_atomic_mode_set(), whenever dpu_enc->hw_cwb is populated (is_cwb_encoder is true), dpu_enc->hw_pp is populated with DPU_HW_BLK_DCWB_PINGPONG rather than DPU_HW_BLK_PINGPONG: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:dpu_encoder_virt_atomic_mode_set() { ... if (is_cwb_encoder) { num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, drm_enc->crtc, DPU_HW_BLK_DCWB_PINGPONG, hw_pp, ARRAY_SIZE(hw_pp)); num_cwb = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, drm_enc->crtc, DPU_HW_BLK_CWB, hw_cwb, ARRAY_SIZE(hw_cwb)); } else { num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, drm_enc->crtc, DPU_HW_BLK_PINGPONG, hw_pp, ARRAY_SIZE(hw_pp)); } ... } In dpu_rm_get_assigned_resources(), DPU_HW_BLK_PINGPONG and DPU_HW_BLK_DCWB_PINGPONG return mutually exclusive sets of pingpong blocks: drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:dpu_rm_get_assigned_resources() { ... if (type == DPU_HW_BLK_PINGPONG) { struct dpu_hw_pingpong *pp = to_dpu_hw_pingpong(hw_blks[i]); if (pp->idx >= PINGPONG_CWB_0) continue; } if (type == DPU_HW_BLK_DCWB_PINGPONG) { struct dpu_hw_pingpong *pp = to_dpu_hw_pingpong(hw_blks[i]); if (pp->idx < PINGPONG_CWB_0) continue; } ... } However, dpu_hw_cwb_config() expects the real-time display's pingpong index when programming the CWB_MUX register: drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cwb.c:dpu_hw_cwb_config() { ... /* * The CWB_MUX register takes the pingpong index for the real-time * display */ if ((pp != PINGPONG_NONE) && (pp < PINGPONG_MAX)) cwb_mux_cfg = FIELD_PREP(CWB_MUX_MASK, pp - PINGPONG_0); ... } By replacing the DPU_HW_BLK_PINGPONG lookup with dpu_enc->hw_pp[i]->idx on the CWB writeback encoder, won't this program CWB_MUX with the dedicated CWB pingpong block index instead of the real-time display's pingpong index? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
