| Message ID | 20230221184256.1436-2-quic_jesszhan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Move TE setup to prepare_for_kickoff() | expand |
On 2023-02-21 10:42:53, Jessica Zhang wrote: > Currently, DPU will enable TE during prepare_commit(). However, this > will cause a crash and reboot to sahara when trying to read/write to > register in get_autorefresh_config(), because the core clock rates > aren't set at that time. Haven't seeen a crash like this on any of my devices (after implementing INTF TE). get_autorefresh_config() always reads zero (or 1 for frame_count) except the first time it is called (autorefresh is left enabled by our bootloader on SM6125) and triggers the disable codepath. It does look like prepare_for_kickoff() is much more susceptible to delays in code than prepare_commit(). The debug logs used to figure this out together with this series result in FPS drops on SM6125 and SM8150. Not an issue with this series, just something to keep in mind. > This used to work because phys_enc->hw_pp is only initialized in mode > set [1], so the first prepare_commit() will return before any register > read/write as hw_pp would be NULL. > > However, when we try to implement support for INTF TE, we will run into > the clock issue described above as hw_intf will *not* be NULL on the > first prepare_commit(). This is because the initialization of > dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2]. > > To avoid this issue, let's enable TE during prepare_for_kickoff() > instead as the core clock rates are guaranteed to be set then. For the change itself: Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> And tested on SM6125, SM8150 (INTF TE) and SDM845 (PP TE). Then, for some patch hygiene, starting here: > Depends on: "Implement tearcheck support on INTF block" [3] > > Changes in V3: > - Added function prototypes > - Reordered function definitions to make change more legible > - Removed prepare_commit() function from dpu_encoder_phys_cmd > > Changes in V4: > - Reworded commit message to be more specific > - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype ... until here: all this info belongs /below the cut/ outside of the messge that becomes part of the commit when this patch is applied to the tree. > [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109 > [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339 Please replace these with "permalinks" (to a commit hash): a branch with line number annotation will fall out of date soon as more patches are applied that touch these files. > [3] https://patchwork.freedesktop.org/series/112332/ Is this a hard dependency? It seems this series applies cleanly on -next and - from a cursory view - should be applicable and testable without my INTF TE series. However, Dmitry asked me to move some code around in review resulting in separate callbacks in the encoder, rather than having various if(has_intf_te) within those callbacks. That'll cause conflicts when I eventually get to respin a v2. - Marijn > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > 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 cb05036f2916..98958c75864a 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 > @@ -40,6 +40,8 @@ > > #define DPU_ENC_MAX_POLL_TIMEOUT_US 2000 > > +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc); > + > static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc) > { > return (phys_enc->split_role != ENC_ROLE_SLAVE); > @@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff( > phys_enc->hw_pp->idx - PINGPONG_0); > } > > + dpu_encoder_phys_cmd_enable_te(phys_enc); > + > DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n", > phys_enc->hw_pp->idx - PINGPONG_0, > atomic_read(&phys_enc->pending_kickoff_cnt)); > @@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx( > return false; > } > > -static void dpu_encoder_phys_cmd_prepare_commit( > - struct dpu_encoder_phys *phys_enc) > +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc) > { > struct dpu_encoder_phys_cmd *cmd_enc = > to_dpu_encoder_phys_cmd(phys_enc); > @@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start( > static void dpu_encoder_phys_cmd_init_ops( > struct dpu_encoder_phys_ops *ops) > { > - ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit; > 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; > -- > 2.39.2 >
On 3/1/2023 2:03 AM, Marijn Suijten wrote: > On 2023-02-21 10:42:53, Jessica Zhang wrote: >> Currently, DPU will enable TE during prepare_commit(). However, this >> will cause a crash and reboot to sahara when trying to read/write to >> register in get_autorefresh_config(), because the core clock rates >> aren't set at that time. > > Haven't seeen a crash like this on any of my devices (after implementing > INTF TE). get_autorefresh_config() always reads zero (or 1 for > frame_count) except the first time it is called (autorefresh is left > enabled by our bootloader on SM6125) and triggers the disable codepath. > I feel that the fact that bootloader keeps things on for you is the reason you dont see the issue. With continuoush splash, clocks are kept enabled. We dont have it enabled (confirmed that). > It does look like prepare_for_kickoff() is much more susceptible to > delays in code than prepare_commit(). The debug logs used to figure > this out together with this series result in FPS drops on SM6125 and > SM8150. Not an issue with this series, just something to keep in mind. > >> This used to work because phys_enc->hw_pp is only initialized in mode >> set [1], so the first prepare_commit() will return before any register >> read/write as hw_pp would be NULL. >> >> However, when we try to implement support for INTF TE, we will run into >> the clock issue described above as hw_intf will *not* be NULL on the >> first prepare_commit(). This is because the initialization of >> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2]. >> >> To avoid this issue, let's enable TE during prepare_for_kickoff() >> instead as the core clock rates are guaranteed to be set then. > > For the change itself: > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > > And tested on SM6125, SM8150 (INTF TE) and SDM845 (PP TE). > Thanks. > Then, for some patch hygiene, starting here: > >> Depends on: "Implement tearcheck support on INTF block" [3] >> >> Changes in V3: >> - Added function prototypes >> - Reordered function definitions to make change more legible >> - Removed prepare_commit() function from dpu_encoder_phys_cmd >> >> Changes in V4: >> - Reworded commit message to be more specific >> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype > > ... until here: all this info belongs /below the cut/ outside of the > messge that becomes part of the commit when this patch is applied to the > tree. For DRM, I thought we are keeping the change log above the ---- ? Which means its allowed in the commit message. > >> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109 >> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339 > > Please replace these with "permalinks" (to a commit hash): a branch with > line number annotation will fall out of date soon as more patches are > applied that touch these files. > >> [3] https://patchwork.freedesktop.org/series/112332/ > > Is this a hard dependency? It seems this series applies cleanly on > -next and - from a cursory view - should be applicable and testable > without my INTF TE series. However, Dmitry asked me to move some code > around in review resulting in separate callbacks in the encoder, rather > than having various if(has_intf_te) within those callbacks. That'll > cause conflicts when I eventually get to respin a v2. > I guess Jessica listed this because without intf_te series there is no crash because hw_pp would be NULL and autorefresh() would return early. So dependency is from the standpoint of when this series is needed and not from compilation point of view. > - Marijn > >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> 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 cb05036f2916..98958c75864a 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 >> @@ -40,6 +40,8 @@ >> >> #define DPU_ENC_MAX_POLL_TIMEOUT_US 2000 >> >> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc); >> + >> static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc) >> { >> return (phys_enc->split_role != ENC_ROLE_SLAVE); >> @@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff( >> phys_enc->hw_pp->idx - PINGPONG_0); >> } >> >> + dpu_encoder_phys_cmd_enable_te(phys_enc); >> + >> DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n", >> phys_enc->hw_pp->idx - PINGPONG_0, >> atomic_read(&phys_enc->pending_kickoff_cnt)); >> @@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx( >> return false; >> } >> >> -static void dpu_encoder_phys_cmd_prepare_commit( >> - struct dpu_encoder_phys *phys_enc) >> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc) >> { >> struct dpu_encoder_phys_cmd *cmd_enc = >> to_dpu_encoder_phys_cmd(phys_enc); >> @@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start( >> static void dpu_encoder_phys_cmd_init_ops( >> struct dpu_encoder_phys_ops *ops) >> { >> - ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit; >> 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; >> -- >> 2.39.2 >>
On 3/1/2023 9:08 AM, Marijn Suijten wrote: > On 2023-03-01 08:23:28, Abhinav Kumar wrote: >> >> On 3/1/2023 2:03 AM, Marijn Suijten wrote: >>> On 2023-02-21 10:42:53, Jessica Zhang wrote: >>>> Currently, DPU will enable TE during prepare_commit(). However, this >>>> will cause a crash and reboot to sahara when trying to read/write to >>>> register in get_autorefresh_config(), because the core clock rates >>>> aren't set at that time. >>> >>> Haven't seeen a crash like this on any of my devices (after implementing >>> INTF TE). get_autorefresh_config() always reads zero (or 1 for >>> frame_count) except the first time it is called (autorefresh is left >>> enabled by our bootloader on SM6125) and triggers the disable codepath. >>> >> >> I feel that the fact that bootloader keeps things on for you is the >> reason you dont see the issue. With continuoush splash, clocks are kept >> enabled. We dont have it enabled (confirmed that). > > That is quite likely, we may even have them enabled because of > simple-framebuffer in DTs; turning those off likely won't have any > effect for testing this. > > For what it's worth, my SM8150 reads 0 for autorefresh. > the value shouldnt really matter. The fact that you are able to read that register without crashing like we are means your clocks are on and ours arent. Thats what this change is fixing. > <snip> > >>> Then, for some patch hygiene, starting here: >>> >>>> Depends on: "Implement tearcheck support on INTF block" [3] >>>> >>>> Changes in V3: >>>> - Added function prototypes >>>> - Reordered function definitions to make change more legible >>>> - Removed prepare_commit() function from dpu_encoder_phys_cmd >>>> >>>> Changes in V4: >>>> - Reworded commit message to be more specific >>>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype >>> >>> ... until here: all this info belongs /below the cut/ outside of the >>> messge that becomes part of the commit when this patch is applied to the >>> tree. >> >> For DRM, I thought we are keeping the change log above the ---- ? >> Which means its allowed in the commit message. > > I hope not, seems unlikely to have different rules across kernel > subsystems. The main point is that this changelog and dependency chain > isn't of any value when the final patch is applied, regardless of > whether it is "allowed". > I looked at a recently posted change by Rob and change log is above the --- https://patchwork.kernel.org/project/dri-devel/patch/20230301185432.3010939-1-robdclark@gmail.com/ So we will follow that. >>>> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109 >>>> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339 >>> >>> Please replace these with "permalinks" (to a commit hash): a branch with >>> line number annotation will fall out of date soon as more patches are >>> applied that touch these files. >>> >>>> [3] https://patchwork.freedesktop.org/series/112332/ >>> >>> Is this a hard dependency? It seems this series applies cleanly on >>> -next and - from a cursory view - should be applicable and testable >>> without my INTF TE series. However, Dmitry asked me to move some code >>> around in review resulting in separate callbacks in the encoder, rather >>> than having various if(has_intf_te) within those callbacks. That'll >>> cause conflicts when I eventually get to respin a v2. >>> >> >> I guess Jessica listed this because without intf_te series there is no >> crash because hw_pp would be NULL and autorefresh() would return early. >> So dependency is from the standpoint of when this series is needed and >> not from compilation point of view. > > That is indeed the question. I'll leave it to the maintainers to decide > what order to apply these in, which we should be made aware of before > submitting v2 so that one of us can resolve the conflicts. > It should be first the intf TE series and then this one. You can go ahead and post your v2, we will rebase on top of yours. > - Marijn
On 2023-03-01 13:42:55, Abhinav Kumar wrote: <snip> > >>> Then, for some patch hygiene, starting here: > >>> > >>>> Depends on: "Implement tearcheck support on INTF block" [3] > >>>> > >>>> Changes in V3: > >>>> - Added function prototypes > >>>> - Reordered function definitions to make change more legible > >>>> - Removed prepare_commit() function from dpu_encoder_phys_cmd > >>>> > >>>> Changes in V4: > >>>> - Reworded commit message to be more specific > >>>> - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype > >>> > >>> ... until here: all this info belongs /below the cut/ outside of the > >>> messge that becomes part of the commit when this patch is applied to the > >>> tree. > >> > >> For DRM, I thought we are keeping the change log above the ---- ? > >> Which means its allowed in the commit message. > > > > I hope not, seems unlikely to have different rules across kernel > > subsystems. The main point is that this changelog and dependency chain > > isn't of any value when the final patch is applied, regardless of > > whether it is "allowed". > > > > I looked at a recently posted change by Rob and change log is above the --- > > https://patchwork.kernel.org/project/dri-devel/patch/20230301185432.3010939-1-robdclark@gmail.com/ > > So we will follow that. I hope that was in error, or no-one pointed it out to Rob. As said before there is no use to having this information in the applied patch, even the kernel guidelines state so: https://docs.kernel.org/process/submitting-patches.html Other comments relevant only to the moment or the maintainer, not suitable for the permanent changelog, should also go here. **A good example of such comments might be patch changelogs which describe what has changed between the v1 and v2 version of the patch.** **Please put this information after the --- line** which separates the changelog from the rest of the patch. The version information is not part of the changelog which gets committed to the git tree. It is additional information for the reviewers. If it’s placed above the commit tags, it needs manual interaction to remove it. If it is below the separator line, it gets automatically stripped off when applying the patch: <snip> > It should be first the intf TE series and then this one. You can go > ahead and post your v2, we will rebase on top of yours. Sounds good; though as said before I'm extremely short on time making it hard to actively commit to this, especially as the catalog changes are really hard to juggle between various "local" branches to test on the many (Sony) devices we are working on. As usual, a preview of v2 is still available at: https://github.com/SoMainline/linux/commits/marijn/dpu And I will do my best to get the last comments worked out. - Marijn
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 cb05036f2916..98958c75864a 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 @@ -40,6 +40,8 @@ #define DPU_ENC_MAX_POLL_TIMEOUT_US 2000 +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc); + static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc) { return (phys_enc->split_role != ENC_ROLE_SLAVE); @@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff( phys_enc->hw_pp->idx - PINGPONG_0); } + dpu_encoder_phys_cmd_enable_te(phys_enc); + DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n", phys_enc->hw_pp->idx - PINGPONG_0, atomic_read(&phys_enc->pending_kickoff_cnt)); @@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx( return false; } -static void dpu_encoder_phys_cmd_prepare_commit( - struct dpu_encoder_phys *phys_enc) +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc) { struct dpu_encoder_phys_cmd *cmd_enc = to_dpu_encoder_phys_cmd(phys_enc); @@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start( static void dpu_encoder_phys_cmd_init_ops( struct dpu_encoder_phys_ops *ops) { - ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit; 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;
Currently, DPU will enable TE during prepare_commit(). However, this will cause a crash and reboot to sahara when trying to read/write to register in get_autorefresh_config(), because the core clock rates aren't set at that time. This used to work because phys_enc->hw_pp is only initialized in mode set [1], so the first prepare_commit() will return before any register read/write as hw_pp would be NULL. However, when we try to implement support for INTF TE, we will run into the clock issue described above as hw_intf will *not* be NULL on the first prepare_commit(). This is because the initialization of dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2]. To avoid this issue, let's enable TE during prepare_for_kickoff() instead as the core clock rates are guaranteed to be set then. Depends on: "Implement tearcheck support on INTF block" [3] Changes in V3: - Added function prototypes - Reordered function definitions to make change more legible - Removed prepare_commit() function from dpu_encoder_phys_cmd Changes in V4: - Reworded commit message to be more specific - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109 [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339 [3] https://patchwork.freedesktop.org/series/112332/ Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)