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@xxxxxxxxx/
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
