On Wed, Aug 11, 2021 at 11:12 AM Mark Yacoub <markyacoub@xxxxxxxxxxxx> wrote:
>
> From: Mark Yacoub <markyacoub@xxxxxxxxxx>
>
> [why]
> Reading frame count register used to get the vblank counter, which calls
> dpu_encoder_phys to get the frame count. Even when it's disabled, the
> vblank counter (through frame count) should return a valid value for the
> count. An invalid value of 0, when compared to vblank->last (in
> drm_vblank.c::drm_update_vblank_count()) returns an invalid number that
> throws off the vblank counter for the lifetime of the process.
>
> Rationale:
> In drm_vblank.c::drm_update_vblank_count(), the new diff is calculated
> through:
> diff = (cur_vblank - vblank->last) & max_vblank_count;
> cur_vblank comes from: cur_vblank = __get_vblank_counter(dev, pipe);
> When the value is 0, diff results in a negative number (a very large
> number as it's unsigned), which inflates the vblank count when the diff
> is added to the current vblank->count.
>
> [How]
> Read frame_count register whether interface timing engine is enabled or
> not.
>
> Fixes: IGT:kms_flip::modeset-vs-vblank-race-interruptible
> Tested on ChromeOS Trogdor(msm)
>
> Signed-off-by: Mark Yacoub <markyacoub@xxxxxxxxxxxx>
Reviewed-by: Rob Clark <robdclark@xxxxxxxxxxxx>
But I suspect we may have a bit more work for the display-off case..
or at least I'm not seeing anything obviously doing a pm_runtime_get()
in this call path.
I'm also not sure if the line/frame-count registers loose state across
a suspend->resume cycle, it might be that we need to save/restore
these registers in the suspend/resume path? Abhinav?
BR,
-R
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 ++-------
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +-
> 2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 116e2b5b1a90f..c436d901629f3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -266,13 +266,8 @@ static void dpu_hw_intf_get_status(
>
> s->is_en = DPU_REG_READ(c, INTF_TIMING_ENGINE_EN);
> s->is_prog_fetch_en = !!(DPU_REG_READ(c, INTF_CONFIG) & BIT(31));
> - if (s->is_en) {
> - s->frame_count = DPU_REG_READ(c, INTF_FRAME_COUNT);
> - s->line_count = DPU_REG_READ(c, INTF_LINE_COUNT);
> - } else {
> - s->line_count = 0;
> - s->frame_count = 0;
> - }
> + s->frame_count = DPU_REG_READ(c, INTF_FRAME_COUNT);
> + s->line_count = DPU_REG_READ(c, INTF_LINE_COUNT);
> }
>
> static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index 3568be80dab51..877ff48bfef04 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -41,7 +41,7 @@ struct intf_prog_fetch {
> struct intf_status {
> u8 is_en; /* interface timing engine is enabled or not */
> u8 is_prog_fetch_en; /* interface prog fetch counter is enabled or not */
> - u32 frame_count; /* frame count since timing engine enabled */
> + u32 frame_count; /* frame count since timing engine first enabled */
> u32 line_count; /* current line count including blanking */
> };
>
> --
> 2.33.0.rc1.237.g0d66db33f3-goog
>
