| Message ID | 20231128011122.14711-1-quic_parellan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: Capture dpu snapshot when frame_done_timer timeouts | expand |
On Tue, 28 Nov 2023 at 03:12, Paloma Arellano <quic_parellan@quicinc.com> wrote: > > Trigger a devcoredump to dump dpu registers and capture the drm atomic > state when the frame_done_timer timeouts. > > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 1cf7ff6caff4..5cf7594feb5a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -191,6 +191,7 @@ struct dpu_encoder_virt { > void *crtc_frame_event_cb_data; > > atomic_t frame_done_timeout_ms; > + atomic_t frame_done_timeout_cnt; > struct timer_list frame_done_timer; > > struct msm_display_info disp_info; > @@ -1204,6 +1205,8 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, > > dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc); > > + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); > + > if (disp_info->intf_type == INTF_DP) > dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]); > else if (disp_info->intf_type == INTF_DSI) > @@ -2115,11 +2118,12 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data) > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > > - seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d ", > + seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d frame_done_cnt:%d", > phys->hw_intf ? phys->hw_intf->idx - INTF_0 : -1, > phys->hw_wb ? phys->hw_wb->idx - WB_0 : -1, > atomic_read(&phys->vsync_cnt), > - atomic_read(&phys->underrun_cnt)); > + atomic_read(&phys->underrun_cnt), > + atomic_read(&dpu_enc->frame_done_timeout_cnt)); > > seq_printf(s, "mode: %s\n", dpu_encoder_helper_get_intf_type(phys->intf_mode)); > } > @@ -2341,6 +2345,10 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t) > > DPU_ERROR_ENC(dpu_enc, "frame done timeout\n"); > > + atomic_inc(&dpu_enc->frame_done_timeout_cnt); > + if (atomic_read(&dpu_enc->frame_done_timeout_cnt) == 1) > + msm_disp_snapshot_state(drm_enc->dev); atomic_inc_and_test(), please > + > event = DPU_ENCODER_FRAME_EVENT_ERROR; > trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event); > dpu_enc->crtc_frame_event_cb(dpu_enc->crtc_frame_event_cb_data, event); > @@ -2392,6 +2400,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, > goto fail; > > atomic_set(&dpu_enc->frame_done_timeout_ms, 0); > + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); > timer_setup(&dpu_enc->frame_done_timer, > dpu_encoder_frame_done_timeout, 0); > > -- > 2.41.0 >
On 11/27/2023 5:48 PM, Dmitry Baryshkov wrote: > On Tue, 28 Nov 2023 at 03:12, Paloma Arellano <quic_parellan@quicinc.com> wrote: >> Trigger a devcoredump to dump dpu registers and capture the drm atomic >> state when the frame_done_timer timeouts. >> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 1cf7ff6caff4..5cf7594feb5a 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -191,6 +191,7 @@ struct dpu_encoder_virt { >> void *crtc_frame_event_cb_data; >> >> atomic_t frame_done_timeout_ms; >> + atomic_t frame_done_timeout_cnt; >> struct timer_list frame_done_timer; >> >> struct msm_display_info disp_info; >> @@ -1204,6 +1205,8 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, >> >> dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc); >> >> + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); >> + >> if (disp_info->intf_type == INTF_DP) >> dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]); >> else if (disp_info->intf_type == INTF_DSI) >> @@ -2115,11 +2118,12 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data) >> for (i = 0; i < dpu_enc->num_phys_encs; i++) { >> struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; >> >> - seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d ", >> + seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d frame_done_cnt:%d", >> phys->hw_intf ? phys->hw_intf->idx - INTF_0 : -1, >> phys->hw_wb ? phys->hw_wb->idx - WB_0 : -1, >> atomic_read(&phys->vsync_cnt), >> - atomic_read(&phys->underrun_cnt)); >> + atomic_read(&phys->underrun_cnt), >> + atomic_read(&dpu_enc->frame_done_timeout_cnt)); >> >> seq_printf(s, "mode: %s\n", dpu_encoder_helper_get_intf_type(phys->intf_mode)); >> } >> @@ -2341,6 +2345,10 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t) >> >> DPU_ERROR_ENC(dpu_enc, "frame done timeout\n"); >> >> + atomic_inc(&dpu_enc->frame_done_timeout_cnt); >> + if (atomic_read(&dpu_enc->frame_done_timeout_cnt) == 1) >> + msm_disp_snapshot_state(drm_enc->dev); > atomic_inc_and_test(), please Hi Dmitry, We only want to create a snapshot for the first instance in which the timer timeouts. atomic_int_and_test() increments the value and then returns whether it has a value of zero or not. FWIW I think I should change it to 'atomic_add_return(1, &dpu_enc->frame_done_timeout_cnt)' so that we can check only when this value equals one. Thank you, Paloma > >> + >> event = DPU_ENCODER_FRAME_EVENT_ERROR; >> trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event); >> dpu_enc->crtc_frame_event_cb(dpu_enc->crtc_frame_event_cb_data, event); >> @@ -2392,6 +2400,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, >> goto fail; >> >> atomic_set(&dpu_enc->frame_done_timeout_ms, 0); >> + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); >> timer_setup(&dpu_enc->frame_done_timer, >> dpu_encoder_frame_done_timeout, 0); >> >> -- >> 2.41.0 >> >
On Tue, 28 Nov 2023 at 19:43, Paloma Arellano <quic_parellan@quicinc.com> wrote: > > > On 11/27/2023 5:48 PM, Dmitry Baryshkov wrote: > > On Tue, 28 Nov 2023 at 03:12, Paloma Arellano <quic_parellan@quicinc.com> wrote: > >> Trigger a devcoredump to dump dpu registers and capture the drm atomic > >> state when the frame_done_timer timeouts. > >> > >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 +++++++++++-- > >> 1 file changed, 11 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> index 1cf7ff6caff4..5cf7594feb5a 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> @@ -191,6 +191,7 @@ struct dpu_encoder_virt { > >> void *crtc_frame_event_cb_data; > >> > >> atomic_t frame_done_timeout_ms; > >> + atomic_t frame_done_timeout_cnt; > >> struct timer_list frame_done_timer; > >> > >> struct msm_display_info disp_info; > >> @@ -1204,6 +1205,8 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, > >> > >> dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc); > >> > >> + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); > >> + > >> if (disp_info->intf_type == INTF_DP) > >> dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]); > >> else if (disp_info->intf_type == INTF_DSI) > >> @@ -2115,11 +2118,12 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data) > >> for (i = 0; i < dpu_enc->num_phys_encs; i++) { > >> struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > >> > >> - seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d ", > >> + seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d frame_done_cnt:%d", > >> phys->hw_intf ? phys->hw_intf->idx - INTF_0 : -1, > >> phys->hw_wb ? phys->hw_wb->idx - WB_0 : -1, > >> atomic_read(&phys->vsync_cnt), > >> - atomic_read(&phys->underrun_cnt)); > >> + atomic_read(&phys->underrun_cnt), > >> + atomic_read(&dpu_enc->frame_done_timeout_cnt)); > >> > >> seq_printf(s, "mode: %s\n", dpu_encoder_helper_get_intf_type(phys->intf_mode)); > >> } > >> @@ -2341,6 +2345,10 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t) > >> > >> DPU_ERROR_ENC(dpu_enc, "frame done timeout\n"); > >> > >> + atomic_inc(&dpu_enc->frame_done_timeout_cnt); > >> + if (atomic_read(&dpu_enc->frame_done_timeout_cnt) == 1) > >> + msm_disp_snapshot_state(drm_enc->dev); > > atomic_inc_and_test(), please > > Hi Dmitry, > > We only want to create a snapshot for the first instance in which the > timer timeouts. atomic_int_and_test() increments the value and then > returns whether it has a value of zero or not. FWIW I think I should > change it to 'atomic_add_return(1, &dpu_enc->frame_done_timeout_cnt)' so > that we can check only when this value equals one. Works for me too. I suggested atomic_inc_test() because then we can let devcoredump take care of duplicate events. > > Thank you, > > Paloma > > > > >> + > >> event = DPU_ENCODER_FRAME_EVENT_ERROR; > >> trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event); > >> dpu_enc->crtc_frame_event_cb(dpu_enc->crtc_frame_event_cb_data, event); > >> @@ -2392,6 +2400,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, > >> goto fail; > >> > >> atomic_set(&dpu_enc->frame_done_timeout_ms, 0); > >> + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); > >> timer_setup(&dpu_enc->frame_done_timer, > >> dpu_encoder_frame_done_timeout, 0); > >> > >> -- > >> 2.41.0 > >> > >
On 11/28/2023 12:24 PM, Dmitry Baryshkov wrote: > On Tue, 28 Nov 2023 at 19:43, Paloma Arellano <quic_parellan@quicinc.com> wrote: >> >> On 11/27/2023 5:48 PM, Dmitry Baryshkov wrote: >>> On Tue, 28 Nov 2023 at 03:12, Paloma Arellano <quic_parellan@quicinc.com> wrote: >>>> Trigger a devcoredump to dump dpu registers and capture the drm atomic >>>> state when the frame_done_timer timeouts. >>>> >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> index 1cf7ff6caff4..5cf7594feb5a 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> @@ -191,6 +191,7 @@ struct dpu_encoder_virt { >>>> void *crtc_frame_event_cb_data; >>>> >>>> atomic_t frame_done_timeout_ms; >>>> + atomic_t frame_done_timeout_cnt; >>>> struct timer_list frame_done_timer; >>>> >>>> struct msm_display_info disp_info; >>>> @@ -1204,6 +1205,8 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, >>>> >>>> dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc); >>>> >>>> + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); >>>> + >>>> if (disp_info->intf_type == INTF_DP) >>>> dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]); >>>> else if (disp_info->intf_type == INTF_DSI) >>>> @@ -2115,11 +2118,12 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data) >>>> for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>>> struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; >>>> >>>> - seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d ", >>>> + seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d frame_done_cnt:%d", >>>> phys->hw_intf ? phys->hw_intf->idx - INTF_0 : -1, >>>> phys->hw_wb ? phys->hw_wb->idx - WB_0 : -1, >>>> atomic_read(&phys->vsync_cnt), >>>> - atomic_read(&phys->underrun_cnt)); >>>> + atomic_read(&phys->underrun_cnt), >>>> + atomic_read(&dpu_enc->frame_done_timeout_cnt)); >>>> >>>> seq_printf(s, "mode: %s\n", dpu_encoder_helper_get_intf_type(phys->intf_mode)); >>>> } >>>> @@ -2341,6 +2345,10 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t) >>>> >>>> DPU_ERROR_ENC(dpu_enc, "frame done timeout\n"); >>>> >>>> + atomic_inc(&dpu_enc->frame_done_timeout_cnt); >>>> + if (atomic_read(&dpu_enc->frame_done_timeout_cnt) == 1) >>>> + msm_disp_snapshot_state(drm_enc->dev); >>> atomic_inc_and_test(), please >> Hi Dmitry, >> >> We only want to create a snapshot for the first instance in which the >> timer timeouts. atomic_int_and_test() increments the value and then >> returns whether it has a value of zero or not. FWIW I think I should >> change it to 'atomic_add_return(1, &dpu_enc->frame_done_timeout_cnt)' so >> that we can check only when this value equals one. > Works for me too. > > I suggested atomic_inc_test() because then we can let devcoredump take > care of duplicate events. Ack -Paloma > >> Thank you, >> >> Paloma >> >>>> + >>>> event = DPU_ENCODER_FRAME_EVENT_ERROR; >>>> trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event); >>>> dpu_enc->crtc_frame_event_cb(dpu_enc->crtc_frame_event_cb_data, event); >>>> @@ -2392,6 +2400,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, >>>> goto fail; >>>> >>>> atomic_set(&dpu_enc->frame_done_timeout_ms, 0); >>>> + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); >>>> timer_setup(&dpu_enc->frame_done_timer, >>>> dpu_encoder_frame_done_timeout, 0); >>>> >>>> -- >>>> 2.41.0 >>>> > >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 1cf7ff6caff4..5cf7594feb5a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -191,6 +191,7 @@ struct dpu_encoder_virt { void *crtc_frame_event_cb_data; atomic_t frame_done_timeout_ms; + atomic_t frame_done_timeout_cnt; struct timer_list frame_done_timer; struct msm_display_info disp_info; @@ -1204,6 +1205,8 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc); + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); + if (disp_info->intf_type == INTF_DP) dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]); else if (disp_info->intf_type == INTF_DSI) @@ -2115,11 +2118,12 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data) for (i = 0; i < dpu_enc->num_phys_encs; i++) { struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; - seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d ", + seq_printf(s, "intf:%d wb:%d vsync:%8d underrun:%8d frame_done_cnt:%d", phys->hw_intf ? phys->hw_intf->idx - INTF_0 : -1, phys->hw_wb ? phys->hw_wb->idx - WB_0 : -1, atomic_read(&phys->vsync_cnt), - atomic_read(&phys->underrun_cnt)); + atomic_read(&phys->underrun_cnt), + atomic_read(&dpu_enc->frame_done_timeout_cnt)); seq_printf(s, "mode: %s\n", dpu_encoder_helper_get_intf_type(phys->intf_mode)); } @@ -2341,6 +2345,10 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t) DPU_ERROR_ENC(dpu_enc, "frame done timeout\n"); + atomic_inc(&dpu_enc->frame_done_timeout_cnt); + if (atomic_read(&dpu_enc->frame_done_timeout_cnt) == 1) + msm_disp_snapshot_state(drm_enc->dev); + event = DPU_ENCODER_FRAME_EVENT_ERROR; trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event); dpu_enc->crtc_frame_event_cb(dpu_enc->crtc_frame_event_cb_data, event); @@ -2392,6 +2400,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, goto fail; atomic_set(&dpu_enc->frame_done_timeout_ms, 0); + atomic_set(&dpu_enc->frame_done_timeout_cnt, 0); timer_setup(&dpu_enc->frame_done_timer, dpu_encoder_frame_done_timeout, 0);
Trigger a devcoredump to dump dpu registers and capture the drm atomic state when the frame_done_timer timeouts. Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)