| Message ID | 20240522-dpu-revert-ams-v2-1-b37825d708e1@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Revert "drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set" | expand |
On 5/22/2024 3:24 AM, Dmitry Baryshkov wrote: > In the DPU driver blank IRQ handling is called from a vblank worker and > can happen outside of the irq_enable / irq_disable pair. Using the > worker makes that completely asynchronous with the rest of the code. > Revert commit d13f638c9b88 ("drm/msm/dpu: drop > dpu_encoder_phys_ops.atomic_mode_set") to fix vblank IRQ assignment for > CMD DSI panels. > > Call trace: > dpu_encoder_phys_cmd_control_vblank_irq+0x218/0x294 > dpu_encoder_toggle_vblank_for_crtc+0x160/0x194 > dpu_crtc_vblank+0xbc/0x228 > dpu_kms_enable_vblank+0x18/0x24 > vblank_ctrl_worker+0x34/0x6c > process_one_work+0x218/0x620 > worker_thread+0x1ac/0x37c > kthread+0x114/0x118 > ret_from_fork+0x10/0x20 > Thanks for the stack. Agreed that vblank can be controlled asynchronously through the worker. And I am guessing that the worker thread ran and printed this error message because phys_enc->irq[INTR_IDX_VSYNC] was not valid as modeset had not happened by then? 272 end: 273 if (ret) { 274 DRM_ERROR("vblank irq err id:%u pp:%d ret:%d, enable %s/%d\n", 275 DRMID(phys_enc->parent), 276 phys_enc->hw_pp->idx - PINGPONG_0, ret, 277 enable ? "true" : "false", refcount); 278 } But how come this did not happen even with this revert. IOW, I am still missing how moving the irq assignment back to modeset from enable is fixing this? > Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > Changes in v2: > - Expanded commit message to describe the reason for revert and added a > call trace (Abhinav) > - Link to v1: https://lore.kernel.org/r/20240514-dpu-revert-ams-v1-1-b13623d6cd5f@linaro.org > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 ++ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5 ++++ > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 ++++++++++++---------- > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++++++-- > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 11 +++++++- > 5 files changed, 46 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 119f3ea50a7c..a7d8ecf3f5be 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); > > phys->cached_mode = crtc_state->adjusted_mode; > + if (phys->ops.atomic_mode_set) > + phys->ops.atomic_mode_set(phys, crtc_state, conn_state); > } > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > index 002e89cc1705..30470cd15a48 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > @@ -69,6 +69,8 @@ struct dpu_encoder_phys; > * @is_master: Whether this phys_enc is the current master > * encoder. Can be switched at enable time. Based > * on split_role and current mode (CMD/VID). > + * @atomic_mode_set: DRM Call. Set a DRM mode. > + * This likely caches the mode, for use at enable. > * @enable: DRM Call. Enable a DRM mode. > * @disable: DRM Call. Disable mode. > * @control_vblank_irq Register/Deregister for VBLANK IRQ > @@ -93,6 +95,9 @@ struct dpu_encoder_phys; > struct dpu_encoder_phys_ops { > void (*prepare_commit)(struct dpu_encoder_phys *encoder); > bool (*is_master)(struct dpu_encoder_phys *encoder); > + void (*atomic_mode_set)(struct dpu_encoder_phys *encoder, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state); > void (*enable)(struct dpu_encoder_phys *encoder); > void (*disable)(struct dpu_encoder_phys *encoder); > int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > index 489be1c0c704..95cd39b49668 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > @@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg) > dpu_encoder_underrun_callback(phys_enc->parent, phys_enc); > } > > +static void dpu_encoder_phys_cmd_atomic_mode_set( > + struct dpu_encoder_phys *phys_enc, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; > + > + phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; > + > + if (phys_enc->has_intf_te) > + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; > + else > + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; > + > + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > +} > + > static int _dpu_encoder_phys_cmd_handle_ppdone_timeout( > struct dpu_encoder_phys *phys_enc) > { > @@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc) > phys_enc->hw_pp->idx - PINGPONG_0, > phys_enc->vblank_refcount); > > - phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; > - phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; > - > - if (phys_enc->has_intf_te) > - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; > - else > - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; > - > dpu_core_irq_register_callback(phys_enc->dpu_kms, > phys_enc->irq[INTR_IDX_PINGPONG], > dpu_encoder_phys_cmd_pp_tx_done_irq, > @@ -318,10 +327,6 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc) > dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]); > dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false); > dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]); > - > - phys_enc->irq[INTR_IDX_CTL_START] = 0; > - phys_enc->irq[INTR_IDX_PINGPONG] = 0; > - phys_enc->irq[INTR_IDX_RDPTR] = 0; > } > > static void dpu_encoder_phys_cmd_tearcheck_config( > @@ -698,6 +703,7 @@ static void dpu_encoder_phys_cmd_init_ops( > struct dpu_encoder_phys_ops *ops) > { > ops->is_master = dpu_encoder_phys_cmd_is_master; > + ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set; > ops->enable = dpu_encoder_phys_cmd_enable; > ops->disable = dpu_encoder_phys_cmd_disable; > ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq; > @@ -736,8 +742,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(struct drm_device *dev, > > dpu_encoder_phys_cmd_init_ops(&phys_enc->ops); > phys_enc->intf_mode = INTF_MODE_CMD; > - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > - > cmd_enc->stream_sel = 0; > > if (!phys_enc->hw_intf) { > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > index ef69c2f408c3..636a97432d51 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > @@ -356,6 +356,16 @@ static bool dpu_encoder_phys_vid_needs_single_flush( > return phys_enc->split_role != ENC_ROLE_SOLO; > } > > +static void dpu_encoder_phys_vid_atomic_mode_set( > + struct dpu_encoder_phys *phys_enc, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; > + > + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > +} > + > static int dpu_encoder_phys_vid_control_vblank_irq( > struct dpu_encoder_phys *phys_enc, > bool enable) > @@ -699,6 +709,7 @@ static int dpu_encoder_phys_vid_get_frame_count( > static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops) > { > ops->is_master = dpu_encoder_phys_vid_is_master; > + ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set; > ops->enable = dpu_encoder_phys_vid_enable; > ops->disable = dpu_encoder_phys_vid_disable; > ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq; > @@ -737,8 +748,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(struct drm_device *dev, > > dpu_encoder_phys_vid_init_ops(&phys_enc->ops); > phys_enc->intf_mode = INTF_MODE_VIDEO; > - phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; > - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > > DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > index d3ea91c1d7d2..356dca5e5ea9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > @@ -404,6 +404,15 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys) > dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]); > } > > +static void dpu_encoder_phys_wb_atomic_mode_set( > + struct dpu_encoder_phys *phys_enc, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + > + phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; > +} > + > static void _dpu_encoder_phys_wb_handle_wbdone_timeout( > struct dpu_encoder_phys *phys_enc) > { > @@ -640,6 +649,7 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy > static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops) > { > ops->is_master = dpu_encoder_phys_wb_is_master; > + ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set; > ops->enable = dpu_encoder_phys_wb_enable; > ops->disable = dpu_encoder_phys_wb_disable; > ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done; > @@ -685,7 +695,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(struct drm_device *dev, > > dpu_encoder_phys_wb_init_ops(&phys_enc->ops); > phys_enc->intf_mode = INTF_MODE_WB_LINE; > - phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; > > atomic_set(&wb_enc->wbirq_refcount, 0); > > > --- > base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd > change-id: 20240514-dpu-revert-ams-9410abc1ee48 > > Best regards,
On Fri, May 24, 2024 at 12:58:53PM -0700, Abhinav Kumar wrote: > > > On 5/22/2024 3:24 AM, Dmitry Baryshkov wrote: > > In the DPU driver blank IRQ handling is called from a vblank worker and > > can happen outside of the irq_enable / irq_disable pair. Using the > > worker makes that completely asynchronous with the rest of the code. > > Revert commit d13f638c9b88 ("drm/msm/dpu: drop > > dpu_encoder_phys_ops.atomic_mode_set") to fix vblank IRQ assignment for > > CMD DSI panels. > > > > Call trace: > > dpu_encoder_phys_cmd_control_vblank_irq+0x218/0x294 > > dpu_encoder_toggle_vblank_for_crtc+0x160/0x194 > > dpu_crtc_vblank+0xbc/0x228 > > dpu_kms_enable_vblank+0x18/0x24 > > vblank_ctrl_worker+0x34/0x6c > > process_one_work+0x218/0x620 > > worker_thread+0x1ac/0x37c > > kthread+0x114/0x118 > > ret_from_fork+0x10/0x20 > > > > Thanks for the stack. > > Agreed that vblank can be controlled asynchronously through the worker. > > And I am guessing that the worker thread ran and printed this error message > because phys_enc->irq[INTR_IDX_VSYNC] was not valid as modeset had not > happened by then? modeset happened, but the vblanks can be enabled and disabled afterwards. > > 272 end: > 273 if (ret) { > 274 DRM_ERROR("vblank irq err id:%u pp:%d ret:%d, enable %s/%d\n", > 275 DRMID(phys_enc->parent), > 276 phys_enc->hw_pp->idx - PINGPONG_0, ret, > 277 enable ? "true" : "false", refcount); > 278 } > > But how come this did not happen even with this revert. > > IOW, I am still missing how moving the irq assignment back to modeset from > enable is fixing this? It removes clearing of the IRQ fields in the irq_disable path, which removes a requirement that vblank IRQ manipulations are called only within the irq_enable/irq_disable brackets. I didn't have time to debug this further on, so I think it's better to revert it now and return to this cleanup later. > > > Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > Changes in v2: > > - Expanded commit message to describe the reason for revert and added a > > call trace (Abhinav) > > - Link to v1: https://lore.kernel.org/r/20240514-dpu-revert-ams-v1-1-b13623d6cd5f@linaro.org > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 ++ > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5 ++++ > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 ++++++++++++---------- > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++++++-- > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 11 +++++++- > > 5 files changed, 46 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index 119f3ea50a7c..a7d8ecf3f5be 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > > phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); > > phys->cached_mode = crtc_state->adjusted_mode; > > + if (phys->ops.atomic_mode_set) > > + phys->ops.atomic_mode_set(phys, crtc_state, conn_state); > > } > > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > index 002e89cc1705..30470cd15a48 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > @@ -69,6 +69,8 @@ struct dpu_encoder_phys; > > * @is_master: Whether this phys_enc is the current master > > * encoder. Can be switched at enable time. Based > > * on split_role and current mode (CMD/VID). > > + * @atomic_mode_set: DRM Call. Set a DRM mode. > > + * This likely caches the mode, for use at enable. > > * @enable: DRM Call. Enable a DRM mode. > > * @disable: DRM Call. Disable mode. > > * @control_vblank_irq Register/Deregister for VBLANK IRQ > > @@ -93,6 +95,9 @@ struct dpu_encoder_phys; > > struct dpu_encoder_phys_ops { > > void (*prepare_commit)(struct dpu_encoder_phys *encoder); > > bool (*is_master)(struct dpu_encoder_phys *encoder); > > + void (*atomic_mode_set)(struct dpu_encoder_phys *encoder, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state); > > void (*enable)(struct dpu_encoder_phys *encoder); > > void (*disable)(struct dpu_encoder_phys *encoder); > > int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > index 489be1c0c704..95cd39b49668 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > @@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg) > > dpu_encoder_underrun_callback(phys_enc->parent, phys_enc); > > } > > +static void dpu_encoder_phys_cmd_atomic_mode_set( > > + struct dpu_encoder_phys *phys_enc, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state) > > +{ > > + phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; > > + > > + phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; > > + > > + if (phys_enc->has_intf_te) > > + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; > > + else > > + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; > > + > > + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > > +} > > + > > static int _dpu_encoder_phys_cmd_handle_ppdone_timeout( > > struct dpu_encoder_phys *phys_enc) > > { > > @@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc) > > phys_enc->hw_pp->idx - PINGPONG_0, > > phys_enc->vblank_refcount); > > - phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; > > - phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; > > - > > - if (phys_enc->has_intf_te) > > - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; > > - else > > - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; > > - > > dpu_core_irq_register_callback(phys_enc->dpu_kms, > > phys_enc->irq[INTR_IDX_PINGPONG], > > dpu_encoder_phys_cmd_pp_tx_done_irq, > > @@ -318,10 +327,6 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc) > > dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]); > > dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false); > > dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]); > > - > > - phys_enc->irq[INTR_IDX_CTL_START] = 0; > > - phys_enc->irq[INTR_IDX_PINGPONG] = 0; > > - phys_enc->irq[INTR_IDX_RDPTR] = 0; > > } > > static void dpu_encoder_phys_cmd_tearcheck_config( > > @@ -698,6 +703,7 @@ static void dpu_encoder_phys_cmd_init_ops( > > struct dpu_encoder_phys_ops *ops) > > { > > ops->is_master = dpu_encoder_phys_cmd_is_master; > > + ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set; > > ops->enable = dpu_encoder_phys_cmd_enable; > > ops->disable = dpu_encoder_phys_cmd_disable; > > ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq; > > @@ -736,8 +742,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(struct drm_device *dev, > > dpu_encoder_phys_cmd_init_ops(&phys_enc->ops); > > phys_enc->intf_mode = INTF_MODE_CMD; > > - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > > - > > cmd_enc->stream_sel = 0; > > if (!phys_enc->hw_intf) { > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > index ef69c2f408c3..636a97432d51 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > @@ -356,6 +356,16 @@ static bool dpu_encoder_phys_vid_needs_single_flush( > > return phys_enc->split_role != ENC_ROLE_SOLO; > > } > > +static void dpu_encoder_phys_vid_atomic_mode_set( > > + struct dpu_encoder_phys *phys_enc, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state) > > +{ > > + phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; > > + > > + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > > +} > > + > > static int dpu_encoder_phys_vid_control_vblank_irq( > > struct dpu_encoder_phys *phys_enc, > > bool enable) > > @@ -699,6 +709,7 @@ static int dpu_encoder_phys_vid_get_frame_count( > > static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops) > > { > > ops->is_master = dpu_encoder_phys_vid_is_master; > > + ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set; > > ops->enable = dpu_encoder_phys_vid_enable; > > ops->disable = dpu_encoder_phys_vid_disable; > > ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq; > > @@ -737,8 +748,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(struct drm_device *dev, > > dpu_encoder_phys_vid_init_ops(&phys_enc->ops); > > phys_enc->intf_mode = INTF_MODE_VIDEO; > > - phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; > > - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > > DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > index d3ea91c1d7d2..356dca5e5ea9 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > @@ -404,6 +404,15 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys) > > dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]); > > } > > +static void dpu_encoder_phys_wb_atomic_mode_set( > > + struct dpu_encoder_phys *phys_enc, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state) > > +{ > > + > > + phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; > > +} > > + > > static void _dpu_encoder_phys_wb_handle_wbdone_timeout( > > struct dpu_encoder_phys *phys_enc) > > { > > @@ -640,6 +649,7 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy > > static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops) > > { > > ops->is_master = dpu_encoder_phys_wb_is_master; > > + ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set; > > ops->enable = dpu_encoder_phys_wb_enable; > > ops->disable = dpu_encoder_phys_wb_disable; > > ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done; > > @@ -685,7 +695,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(struct drm_device *dev, > > dpu_encoder_phys_wb_init_ops(&phys_enc->ops); > > phys_enc->intf_mode = INTF_MODE_WB_LINE; > > - phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; > > atomic_set(&wb_enc->wbirq_refcount, 0); > > > > --- > > base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd > > change-id: 20240514-dpu-revert-ams-9410abc1ee48 > > > > Best regards,
On 5/24/2024 1:22 PM, Dmitry Baryshkov wrote: > On Fri, May 24, 2024 at 12:58:53PM -0700, Abhinav Kumar wrote: >> >> >> On 5/22/2024 3:24 AM, Dmitry Baryshkov wrote: >>> In the DPU driver blank IRQ handling is called from a vblank worker and >>> can happen outside of the irq_enable / irq_disable pair. Using the >>> worker makes that completely asynchronous with the rest of the code. >>> Revert commit d13f638c9b88 ("drm/msm/dpu: drop >>> dpu_encoder_phys_ops.atomic_mode_set") to fix vblank IRQ assignment for >>> CMD DSI panels. >>> >>> Call trace: >>> dpu_encoder_phys_cmd_control_vblank_irq+0x218/0x294 >>> dpu_encoder_toggle_vblank_for_crtc+0x160/0x194 >>> dpu_crtc_vblank+0xbc/0x228 >>> dpu_kms_enable_vblank+0x18/0x24 >>> vblank_ctrl_worker+0x34/0x6c >>> process_one_work+0x218/0x620 >>> worker_thread+0x1ac/0x37c >>> kthread+0x114/0x118 >>> ret_from_fork+0x10/0x20 >>> >> >> Thanks for the stack. >> >> Agreed that vblank can be controlled asynchronously through the worker. >> >> And I am guessing that the worker thread ran and printed this error message >> because phys_enc->irq[INTR_IDX_VSYNC] was not valid as modeset had not >> happened by then? > > modeset happened, but the vblanks can be enabled and disabled > afterwards. > >> >> 272 end: >> 273 if (ret) { >> 274 DRM_ERROR("vblank irq err id:%u pp:%d ret:%d, enable %s/%d\n", >> 275 DRMID(phys_enc->parent), >> 276 phys_enc->hw_pp->idx - PINGPONG_0, ret, >> 277 enable ? "true" : "false", refcount); >> 278 } >> >> But how come this did not happen even with this revert. >> >> IOW, I am still missing how moving the irq assignment back to modeset from >> enable is fixing this? > > It removes clearing of the IRQ fields in the irq_disable path, which > removes a requirement that vblank IRQ manipulations are called only > within the irq_enable/irq_disable brackets. I didn't have time to debug > this further on, so I think it's better to revert it now and return to > this cleanup later. > I see your point. So before we moved the irq assignment to enable/disable, modeset was setting it up but nothing was clearing it. But after the change, disable was clearing it leading to the error. Probably should have happened quite easily then? Then if we kept only the assignment in enable and drop the parts in disable it should have worked too. If you would like to post that separately after some more investigation, I am fine with this for now, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> >>> Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set") >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> Changes in v2: >>> - Expanded commit message to describe the reason for revert and added a >>> call trace (Abhinav) >>> - Link to v1: https://lore.kernel.org/r/20240514-dpu-revert-ams-v1-1-b13623d6cd5f@linaro.org >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 ++ >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5 ++++ >>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 ++++++++++++---------- >>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++++++-- >>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 11 +++++++- >>> 5 files changed, 46 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 119f3ea50a7c..a7d8ecf3f5be 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, >>> phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); >>> phys->cached_mode = crtc_state->adjusted_mode; >>> + if (phys->ops.atomic_mode_set) >>> + phys->ops.atomic_mode_set(phys, crtc_state, conn_state); >>> } >>> } >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>> index 002e89cc1705..30470cd15a48 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h >>> @@ -69,6 +69,8 @@ struct dpu_encoder_phys; >>> * @is_master: Whether this phys_enc is the current master >>> * encoder. Can be switched at enable time. Based >>> * on split_role and current mode (CMD/VID). >>> + * @atomic_mode_set: DRM Call. Set a DRM mode. >>> + * This likely caches the mode, for use at enable. >>> * @enable: DRM Call. Enable a DRM mode. >>> * @disable: DRM Call. Disable mode. >>> * @control_vblank_irq Register/Deregister for VBLANK IRQ >>> @@ -93,6 +95,9 @@ struct dpu_encoder_phys; >>> struct dpu_encoder_phys_ops { >>> void (*prepare_commit)(struct dpu_encoder_phys *encoder); >>> bool (*is_master)(struct dpu_encoder_phys *encoder); >>> + void (*atomic_mode_set)(struct dpu_encoder_phys *encoder, >>> + struct drm_crtc_state *crtc_state, >>> + struct drm_connector_state *conn_state); >>> void (*enable)(struct dpu_encoder_phys *encoder); >>> void (*disable)(struct dpu_encoder_phys *encoder); >>> int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable); >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>> index 489be1c0c704..95cd39b49668 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c >>> @@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg) >>> dpu_encoder_underrun_callback(phys_enc->parent, phys_enc); >>> } >>> +static void dpu_encoder_phys_cmd_atomic_mode_set( >>> + struct dpu_encoder_phys *phys_enc, >>> + struct drm_crtc_state *crtc_state, >>> + struct drm_connector_state *conn_state) >>> +{ >>> + phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; >>> + >>> + phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; >>> + >>> + if (phys_enc->has_intf_te) >>> + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; >>> + else >>> + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; >>> + >>> + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; >>> +} >>> + >>> static int _dpu_encoder_phys_cmd_handle_ppdone_timeout( >>> struct dpu_encoder_phys *phys_enc) >>> { >>> @@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc) >>> phys_enc->hw_pp->idx - PINGPONG_0, >>> phys_enc->vblank_refcount); >>> - phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; >>> - phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; >>> - >>> - if (phys_enc->has_intf_te) >>> - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; >>> - else >>> - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; >>> - >>> dpu_core_irq_register_callback(phys_enc->dpu_kms, >>> phys_enc->irq[INTR_IDX_PINGPONG], >>> dpu_encoder_phys_cmd_pp_tx_done_irq, >>> @@ -318,10 +327,6 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc) >>> dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]); >>> dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false); >>> dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]); >>> - >>> - phys_enc->irq[INTR_IDX_CTL_START] = 0; >>> - phys_enc->irq[INTR_IDX_PINGPONG] = 0; >>> - phys_enc->irq[INTR_IDX_RDPTR] = 0; >>> } >>> static void dpu_encoder_phys_cmd_tearcheck_config( >>> @@ -698,6 +703,7 @@ static void dpu_encoder_phys_cmd_init_ops( >>> struct dpu_encoder_phys_ops *ops) >>> { >>> ops->is_master = dpu_encoder_phys_cmd_is_master; >>> + ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set; >>> ops->enable = dpu_encoder_phys_cmd_enable; >>> ops->disable = dpu_encoder_phys_cmd_disable; >>> ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq; >>> @@ -736,8 +742,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(struct drm_device *dev, >>> dpu_encoder_phys_cmd_init_ops(&phys_enc->ops); >>> phys_enc->intf_mode = INTF_MODE_CMD; >>> - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; >>> - >>> cmd_enc->stream_sel = 0; >>> if (!phys_enc->hw_intf) { >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>> index ef69c2f408c3..636a97432d51 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>> @@ -356,6 +356,16 @@ static bool dpu_encoder_phys_vid_needs_single_flush( >>> return phys_enc->split_role != ENC_ROLE_SOLO; >>> } >>> +static void dpu_encoder_phys_vid_atomic_mode_set( >>> + struct dpu_encoder_phys *phys_enc, >>> + struct drm_crtc_state *crtc_state, >>> + struct drm_connector_state *conn_state) >>> +{ >>> + phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; >>> + >>> + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; >>> +} >>> + >>> static int dpu_encoder_phys_vid_control_vblank_irq( >>> struct dpu_encoder_phys *phys_enc, >>> bool enable) >>> @@ -699,6 +709,7 @@ static int dpu_encoder_phys_vid_get_frame_count( >>> static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops) >>> { >>> ops->is_master = dpu_encoder_phys_vid_is_master; >>> + ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set; >>> ops->enable = dpu_encoder_phys_vid_enable; >>> ops->disable = dpu_encoder_phys_vid_disable; >>> ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq; >>> @@ -737,8 +748,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(struct drm_device *dev, >>> dpu_encoder_phys_vid_init_ops(&phys_enc->ops); >>> phys_enc->intf_mode = INTF_MODE_VIDEO; >>> - phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; >>> - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; >>> DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx); >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c >>> index d3ea91c1d7d2..356dca5e5ea9 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c >>> @@ -404,6 +404,15 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys) >>> dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]); >>> } >>> +static void dpu_encoder_phys_wb_atomic_mode_set( >>> + struct dpu_encoder_phys *phys_enc, >>> + struct drm_crtc_state *crtc_state, >>> + struct drm_connector_state *conn_state) >>> +{ >>> + >>> + phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; >>> +} >>> + >>> static void _dpu_encoder_phys_wb_handle_wbdone_timeout( >>> struct dpu_encoder_phys *phys_enc) >>> { >>> @@ -640,6 +649,7 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy >>> static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops) >>> { >>> ops->is_master = dpu_encoder_phys_wb_is_master; >>> + ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set; >>> ops->enable = dpu_encoder_phys_wb_enable; >>> ops->disable = dpu_encoder_phys_wb_disable; >>> ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done; >>> @@ -685,7 +695,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(struct drm_device *dev, >>> dpu_encoder_phys_wb_init_ops(&phys_enc->ops); >>> phys_enc->intf_mode = INTF_MODE_WB_LINE; >>> - phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; >>> atomic_set(&wb_enc->wbirq_refcount, 0); >>> >>> --- >>> base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd >>> change-id: 20240514-dpu-revert-ams-9410abc1ee48 >>> >>> Best regards, >
On Wed, May 29, 2024 at 04:44:59PM -0700, Abhinav Kumar wrote: > > > On 5/24/2024 1:22 PM, Dmitry Baryshkov wrote: > > On Fri, May 24, 2024 at 12:58:53PM -0700, Abhinav Kumar wrote: > > > > > > > > > On 5/22/2024 3:24 AM, Dmitry Baryshkov wrote: > > > > In the DPU driver blank IRQ handling is called from a vblank worker and > > > > can happen outside of the irq_enable / irq_disable pair. Using the > > > > worker makes that completely asynchronous with the rest of the code. > > > > Revert commit d13f638c9b88 ("drm/msm/dpu: drop > > > > dpu_encoder_phys_ops.atomic_mode_set") to fix vblank IRQ assignment for > > > > CMD DSI panels. > > > > > > > > Call trace: > > > > dpu_encoder_phys_cmd_control_vblank_irq+0x218/0x294 > > > > dpu_encoder_toggle_vblank_for_crtc+0x160/0x194 > > > > dpu_crtc_vblank+0xbc/0x228 > > > > dpu_kms_enable_vblank+0x18/0x24 > > > > vblank_ctrl_worker+0x34/0x6c > > > > process_one_work+0x218/0x620 > > > > worker_thread+0x1ac/0x37c > > > > kthread+0x114/0x118 > > > > ret_from_fork+0x10/0x20 > > > > > > > > > > Thanks for the stack. > > > > > > Agreed that vblank can be controlled asynchronously through the worker. > > > > > > And I am guessing that the worker thread ran and printed this error message > > > because phys_enc->irq[INTR_IDX_VSYNC] was not valid as modeset had not > > > happened by then? > > > > modeset happened, but the vblanks can be enabled and disabled > > afterwards. > > > > > > > > 272 end: > > > 273 if (ret) { > > > 274 DRM_ERROR("vblank irq err id:%u pp:%d ret:%d, enable %s/%d\n", > > > 275 DRMID(phys_enc->parent), > > > 276 phys_enc->hw_pp->idx - PINGPONG_0, ret, > > > 277 enable ? "true" : "false", refcount); > > > 278 } > > > > > > But how come this did not happen even with this revert. > > > > > > IOW, I am still missing how moving the irq assignment back to modeset from > > > enable is fixing this? > > > > It removes clearing of the IRQ fields in the irq_disable path, which > > removes a requirement that vblank IRQ manipulations are called only > > within the irq_enable/irq_disable brackets. I didn't have time to debug > > this further on, so I think it's better to revert it now and return to > > this cleanup later. > > > > I see your point. > > So before we moved the irq assignment to enable/disable, modeset was setting > it up but nothing was clearing it. > > But after the change, disable was clearing it leading to the error. > > Probably should have happened quite easily then? > > Then if we kept only the assignment in enable and drop the parts in disable > it should have worked too. Well, one of the points of the original patch was to remove stale numbers. I hope to have time to re-review the code at some point. > > If you would like to post that separately after some more investigation, I > am fine with this for now, > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > > > > > > > Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set") > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > Changes in v2: > > > > - Expanded commit message to describe the reason for revert and added a > > > > call trace (Abhinav) > > > > - Link to v1: https://lore.kernel.org/r/20240514-dpu-revert-ams-v1-1-b13623d6cd5f@linaro.org > > > > --- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 ++ > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5 ++++ > > > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 ++++++++++++---------- > > > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++++++-- > > > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 11 +++++++- > > > > 5 files changed, 46 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > index 119f3ea50a7c..a7d8ecf3f5be 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > @@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > > > > phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); > > > > phys->cached_mode = crtc_state->adjusted_mode; > > > > + if (phys->ops.atomic_mode_set) > > > > + phys->ops.atomic_mode_set(phys, crtc_state, conn_state); > > > > } > > > > } > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > > > index 002e89cc1705..30470cd15a48 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > > > > @@ -69,6 +69,8 @@ struct dpu_encoder_phys; > > > > * @is_master: Whether this phys_enc is the current master > > > > * encoder. Can be switched at enable time. Based > > > > * on split_role and current mode (CMD/VID). > > > > + * @atomic_mode_set: DRM Call. Set a DRM mode. > > > > + * This likely caches the mode, for use at enable. > > > > * @enable: DRM Call. Enable a DRM mode. > > > > * @disable: DRM Call. Disable mode. > > > > * @control_vblank_irq Register/Deregister for VBLANK IRQ > > > > @@ -93,6 +95,9 @@ struct dpu_encoder_phys; > > > > struct dpu_encoder_phys_ops { > > > > void (*prepare_commit)(struct dpu_encoder_phys *encoder); > > > > bool (*is_master)(struct dpu_encoder_phys *encoder); > > > > + void (*atomic_mode_set)(struct dpu_encoder_phys *encoder, > > > > + struct drm_crtc_state *crtc_state, > > > > + struct drm_connector_state *conn_state); > > > > void (*enable)(struct dpu_encoder_phys *encoder); > > > > void (*disable)(struct dpu_encoder_phys *encoder); > > > > int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable); > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > > > index 489be1c0c704..95cd39b49668 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > > > @@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg) > > > > dpu_encoder_underrun_callback(phys_enc->parent, phys_enc); > > > > } > > > > +static void dpu_encoder_phys_cmd_atomic_mode_set( > > > > + struct dpu_encoder_phys *phys_enc, > > > > + struct drm_crtc_state *crtc_state, > > > > + struct drm_connector_state *conn_state) > > > > +{ > > > > + phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; > > > > + > > > > + phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; > > > > + > > > > + if (phys_enc->has_intf_te) > > > > + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; > > > > + else > > > > + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; > > > > + > > > > + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > > > > +} > > > > + > > > > static int _dpu_encoder_phys_cmd_handle_ppdone_timeout( > > > > struct dpu_encoder_phys *phys_enc) > > > > { > > > > @@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc) > > > > phys_enc->hw_pp->idx - PINGPONG_0, > > > > phys_enc->vblank_refcount); > > > > - phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; > > > > - phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; > > > > - > > > > - if (phys_enc->has_intf_te) > > > > - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; > > > > - else > > > > - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; > > > > - > > > > dpu_core_irq_register_callback(phys_enc->dpu_kms, > > > > phys_enc->irq[INTR_IDX_PINGPONG], > > > > dpu_encoder_phys_cmd_pp_tx_done_irq, > > > > @@ -318,10 +327,6 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc) > > > > dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]); > > > > dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false); > > > > dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]); > > > > - > > > > - phys_enc->irq[INTR_IDX_CTL_START] = 0; > > > > - phys_enc->irq[INTR_IDX_PINGPONG] = 0; > > > > - phys_enc->irq[INTR_IDX_RDPTR] = 0; > > > > } > > > > static void dpu_encoder_phys_cmd_tearcheck_config( > > > > @@ -698,6 +703,7 @@ static void dpu_encoder_phys_cmd_init_ops( > > > > struct dpu_encoder_phys_ops *ops) > > > > { > > > > ops->is_master = dpu_encoder_phys_cmd_is_master; > > > > + ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set; > > > > ops->enable = dpu_encoder_phys_cmd_enable; > > > > ops->disable = dpu_encoder_phys_cmd_disable; > > > > ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq; > > > > @@ -736,8 +742,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(struct drm_device *dev, > > > > dpu_encoder_phys_cmd_init_ops(&phys_enc->ops); > > > > phys_enc->intf_mode = INTF_MODE_CMD; > > > > - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > > > > - > > > > cmd_enc->stream_sel = 0; > > > > if (!phys_enc->hw_intf) { > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > > > index ef69c2f408c3..636a97432d51 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > > > @@ -356,6 +356,16 @@ static bool dpu_encoder_phys_vid_needs_single_flush( > > > > return phys_enc->split_role != ENC_ROLE_SOLO; > > > > } > > > > +static void dpu_encoder_phys_vid_atomic_mode_set( > > > > + struct dpu_encoder_phys *phys_enc, > > > > + struct drm_crtc_state *crtc_state, > > > > + struct drm_connector_state *conn_state) > > > > +{ > > > > + phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; > > > > + > > > > + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > > > > +} > > > > + > > > > static int dpu_encoder_phys_vid_control_vblank_irq( > > > > struct dpu_encoder_phys *phys_enc, > > > > bool enable) > > > > @@ -699,6 +709,7 @@ static int dpu_encoder_phys_vid_get_frame_count( > > > > static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops) > > > > { > > > > ops->is_master = dpu_encoder_phys_vid_is_master; > > > > + ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set; > > > > ops->enable = dpu_encoder_phys_vid_enable; > > > > ops->disable = dpu_encoder_phys_vid_disable; > > > > ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq; > > > > @@ -737,8 +748,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(struct drm_device *dev, > > > > dpu_encoder_phys_vid_init_ops(&phys_enc->ops); > > > > phys_enc->intf_mode = INTF_MODE_VIDEO; > > > > - phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; > > > > - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; > > > > DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx); > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > > > index d3ea91c1d7d2..356dca5e5ea9 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > > > @@ -404,6 +404,15 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys) > > > > dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]); > > > > } > > > > +static void dpu_encoder_phys_wb_atomic_mode_set( > > > > + struct dpu_encoder_phys *phys_enc, > > > > + struct drm_crtc_state *crtc_state, > > > > + struct drm_connector_state *conn_state) > > > > +{ > > > > + > > > > + phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; > > > > +} > > > > + > > > > static void _dpu_encoder_phys_wb_handle_wbdone_timeout( > > > > struct dpu_encoder_phys *phys_enc) > > > > { > > > > @@ -640,6 +649,7 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy > > > > static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops) > > > > { > > > > ops->is_master = dpu_encoder_phys_wb_is_master; > > > > + ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set; > > > > ops->enable = dpu_encoder_phys_wb_enable; > > > > ops->disable = dpu_encoder_phys_wb_disable; > > > > ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done; > > > > @@ -685,7 +695,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(struct drm_device *dev, > > > > dpu_encoder_phys_wb_init_ops(&phys_enc->ops); > > > > phys_enc->intf_mode = INTF_MODE_WB_LINE; > > > > - phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; > > > > atomic_set(&wb_enc->wbirq_refcount, 0); > > > > > > > > --- > > > > base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd > > > > change-id: 20240514-dpu-revert-ams-9410abc1ee48 > > > > > > > > Best regards, > >
On Wed, 22 May 2024 13:24:28 +0300, Dmitry Baryshkov wrote: > In the DPU driver blank IRQ handling is called from a vblank worker and > can happen outside of the irq_enable / irq_disable pair. Using the > worker makes that completely asynchronous with the rest of the code. > Revert commit d13f638c9b88 ("drm/msm/dpu: drop > dpu_encoder_phys_ops.atomic_mode_set") to fix vblank IRQ assignment for > CMD DSI panels. > > [...] Applied, thanks! [1/1] Revert "drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set" https://gitlab.freedesktop.org/abhinavk/msm-next/-/commit/6e301821c28d Best regards,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 119f3ea50a7c..a7d8ecf3f5be 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); phys->cached_mode = crtc_state->adjusted_mode; + if (phys->ops.atomic_mode_set) + phys->ops.atomic_mode_set(phys, crtc_state, conn_state); } } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h index 002e89cc1705..30470cd15a48 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h @@ -69,6 +69,8 @@ struct dpu_encoder_phys; * @is_master: Whether this phys_enc is the current master * encoder. Can be switched at enable time. Based * on split_role and current mode (CMD/VID). + * @atomic_mode_set: DRM Call. Set a DRM mode. + * This likely caches the mode, for use at enable. * @enable: DRM Call. Enable a DRM mode. * @disable: DRM Call. Disable mode. * @control_vblank_irq Register/Deregister for VBLANK IRQ @@ -93,6 +95,9 @@ struct dpu_encoder_phys; struct dpu_encoder_phys_ops { void (*prepare_commit)(struct dpu_encoder_phys *encoder); bool (*is_master)(struct dpu_encoder_phys *encoder); + void (*atomic_mode_set)(struct dpu_encoder_phys *encoder, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state); void (*enable)(struct dpu_encoder_phys *encoder); void (*disable)(struct dpu_encoder_phys *encoder); int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index 489be1c0c704..95cd39b49668 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg) dpu_encoder_underrun_callback(phys_enc->parent, phys_enc); } +static void dpu_encoder_phys_cmd_atomic_mode_set( + struct dpu_encoder_phys *phys_enc, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; + + phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; + + if (phys_enc->has_intf_te) + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; + else + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; + + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; +} + static int _dpu_encoder_phys_cmd_handle_ppdone_timeout( struct dpu_encoder_phys *phys_enc) { @@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc) phys_enc->hw_pp->idx - PINGPONG_0, phys_enc->vblank_refcount); - phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start; - phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done; - - if (phys_enc->has_intf_te) - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr; - else - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr; - dpu_core_irq_register_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG], dpu_encoder_phys_cmd_pp_tx_done_irq, @@ -318,10 +327,6 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc) dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]); dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false); dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]); - - phys_enc->irq[INTR_IDX_CTL_START] = 0; - phys_enc->irq[INTR_IDX_PINGPONG] = 0; - phys_enc->irq[INTR_IDX_RDPTR] = 0; } static void dpu_encoder_phys_cmd_tearcheck_config( @@ -698,6 +703,7 @@ static void dpu_encoder_phys_cmd_init_ops( struct dpu_encoder_phys_ops *ops) { ops->is_master = dpu_encoder_phys_cmd_is_master; + ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set; ops->enable = dpu_encoder_phys_cmd_enable; ops->disable = dpu_encoder_phys_cmd_disable; ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq; @@ -736,8 +742,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(struct drm_device *dev, dpu_encoder_phys_cmd_init_ops(&phys_enc->ops); phys_enc->intf_mode = INTF_MODE_CMD; - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; - cmd_enc->stream_sel = 0; if (!phys_enc->hw_intf) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index ef69c2f408c3..636a97432d51 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -356,6 +356,16 @@ static bool dpu_encoder_phys_vid_needs_single_flush( return phys_enc->split_role != ENC_ROLE_SOLO; } +static void dpu_encoder_phys_vid_atomic_mode_set( + struct dpu_encoder_phys *phys_enc, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; + + phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; +} + static int dpu_encoder_phys_vid_control_vblank_irq( struct dpu_encoder_phys *phys_enc, bool enable) @@ -699,6 +709,7 @@ static int dpu_encoder_phys_vid_get_frame_count( static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops) { ops->is_master = dpu_encoder_phys_vid_is_master; + ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set; ops->enable = dpu_encoder_phys_vid_enable; ops->disable = dpu_encoder_phys_vid_disable; ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq; @@ -737,8 +748,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(struct drm_device *dev, dpu_encoder_phys_vid_init_ops(&phys_enc->ops); phys_enc->intf_mode = INTF_MODE_VIDEO; - phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync; - phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun; DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c index d3ea91c1d7d2..356dca5e5ea9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c @@ -404,6 +404,15 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys) dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]); } +static void dpu_encoder_phys_wb_atomic_mode_set( + struct dpu_encoder_phys *phys_enc, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + + phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; +} + static void _dpu_encoder_phys_wb_handle_wbdone_timeout( struct dpu_encoder_phys *phys_enc) { @@ -640,6 +649,7 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops) { ops->is_master = dpu_encoder_phys_wb_is_master; + ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set; ops->enable = dpu_encoder_phys_wb_enable; ops->disable = dpu_encoder_phys_wb_disable; ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done; @@ -685,7 +695,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(struct drm_device *dev, dpu_encoder_phys_wb_init_ops(&phys_enc->ops); phys_enc->intf_mode = INTF_MODE_WB_LINE; - phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done; atomic_set(&wb_enc->wbirq_refcount, 0);
In the DPU driver blank IRQ handling is called from a vblank worker and can happen outside of the irq_enable / irq_disable pair. Using the worker makes that completely asynchronous with the rest of the code. Revert commit d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set") to fix vblank IRQ assignment for CMD DSI panels. Call trace: dpu_encoder_phys_cmd_control_vblank_irq+0x218/0x294 dpu_encoder_toggle_vblank_for_crtc+0x160/0x194 dpu_crtc_vblank+0xbc/0x228 dpu_kms_enable_vblank+0x18/0x24 vblank_ctrl_worker+0x34/0x6c process_one_work+0x218/0x620 worker_thread+0x1ac/0x37c kthread+0x114/0x118 ret_from_fork+0x10/0x20 Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Changes in v2: - Expanded commit message to describe the reason for revert and added a call trace (Abhinav) - Link to v1: https://lore.kernel.org/r/20240514-dpu-revert-ams-v1-1-b13623d6cd5f@linaro.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5 ++++ .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 ++++++++++++---------- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++++++-- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 11 +++++++- 5 files changed, 46 insertions(+), 17 deletions(-) --- base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd change-id: 20240514-dpu-revert-ams-9410abc1ee48 Best regards,