2021-02-18 14:26:41

by Kalyan Thota

[permalink] [raw]
Subject: [v4] drm/msm/disp/dpu1: turn off vblank irqs aggressively in dpu driver

Set the flag vblank_disable_immediate = true to turn off vblank irqs
immediately as soon as drm_vblank_put is requested so that there are
no irqs triggered during idle state. This will reduce cpu wakeups
and help in power saving.

To enable vblank_disable_immediate flag the underlying KMS driver
needs to support high precision vblank timestamping and also a
reliable way of providing vblank counter which is incrementing
at the leading edge of vblank.

This patch also brings in changes to support vblank_disable_immediate
requirement in dpu driver.

Changes in v1:
- Specify reason to add vblank timestamp support. (Rob).
- Add changes to provide vblank counter from dpu driver.

Changes in v2:
- Fix warn stack reported by Rob Clark with v2 patch.

Changes in v3:
- Move back to HW frame counter (Rob).

Signed-off-by: Kalyan Thota <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 80 ++++++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 30 ++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 11 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 1 +
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 26 +++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 ++
8 files changed, 155 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index d4662e8..9a80981 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -65,6 +65,83 @@ static void dpu_crtc_destroy(struct drm_crtc *crtc)
kfree(dpu_crtc);
}

+static struct drm_encoder *get_encoder_from_crtc(struct drm_crtc *crtc)
+{
+ struct drm_device *dev = crtc->dev;
+ struct drm_encoder *encoder;
+
+ drm_for_each_encoder(encoder, dev)
+ if (encoder->crtc == crtc)
+ return encoder;
+
+ return NULL;
+}
+
+static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
+{
+ struct drm_encoder *encoder;
+
+ encoder = get_encoder_from_crtc(crtc);
+ if (!encoder) {
+ DRM_ERROR("no encoder found for crtc %d\n", crtc->index);
+ return false;
+ }
+
+ return dpu_encoder_get_frame_count(encoder);
+}
+
+static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
+ bool in_vblank_irq,
+ int *vpos, int *hpos,
+ ktime_t *stime, ktime_t *etime,
+ const struct drm_display_mode *mode)
+{
+ unsigned int pipe = crtc->index;
+ struct drm_encoder *encoder;
+ int line, vsw, vbp, vactive_start, vactive_end, vfp_end;
+
+ encoder = get_encoder_from_crtc(crtc);
+ if (!encoder) {
+ DRM_ERROR("no encoder found for crtc %d\n", pipe);
+ return false;
+ }
+
+ vsw = mode->crtc_vsync_end - mode->crtc_vsync_start;
+ vbp = mode->crtc_vtotal - mode->crtc_vsync_end;
+
+ /*
+ * the line counter is 1 at the start of the VSYNC pulse and VTOTAL at
+ * the end of VFP. Translate the porch values relative to the line
+ * counter positions.
+ */
+
+ vactive_start = vsw + vbp + 1;
+ vactive_end = vactive_start + mode->crtc_vdisplay;
+
+ /* last scan line before VSYNC */
+ vfp_end = mode->crtc_vtotal;
+
+ if (stime)
+ *stime = ktime_get();
+
+ line = dpu_encoder_get_linecount(encoder);
+
+ if (line < vactive_start)
+ line -= vactive_start;
+ else if (line > vactive_end)
+ line = line - vfp_end - vactive_start;
+ else
+ line -= vactive_start;
+
+ *vpos = line;
+ *hpos = 0;
+
+ if (etime)
+ *etime = ktime_get();
+
+ return true;
+}
+
static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer,
struct dpu_plane_state *pstate, struct dpu_format *format)
{
@@ -1243,6 +1320,8 @@ static const struct drm_crtc_funcs dpu_crtc_funcs = {
.early_unregister = dpu_crtc_early_unregister,
.enable_vblank = msm_crtc_enable_vblank,
.disable_vblank = msm_crtc_disable_vblank,
+ .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
+ .get_vblank_counter = dpu_crtc_get_vblank_counter,
};

static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = {
@@ -1251,6 +1330,7 @@ static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = {
.atomic_check = dpu_crtc_atomic_check,
.atomic_begin = dpu_crtc_atomic_begin,
.atomic_flush = dpu_crtc_atomic_flush,
+ .get_scanout_position = dpu_crtc_get_scanout_position,
};

/* initialize crtc */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f7f5c25..5cd3f31 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -425,6 +425,36 @@ int dpu_encoder_helper_unregister_irq(struct dpu_encoder_phys *phys_enc,
return 0;
}

+int dpu_encoder_get_frame_count(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+ struct dpu_encoder_phys *phys;
+ int framecount = 0;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ phys = dpu_enc ? dpu_enc->cur_master : NULL;
+
+ if (phys && phys->ops.get_frame_count)
+ framecount = phys->ops.get_frame_count(phys);
+
+ return framecount;
+}
+
+int dpu_encoder_get_linecount(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+ struct dpu_encoder_phys *phys;
+ int linecount = 0;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ phys = dpu_enc ? dpu_enc->cur_master : NULL;
+
+ if (phys && phys->ops.get_line_count)
+ linecount = phys->ops.get_line_count(phys);
+
+ return linecount;
+}
+
void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
struct dpu_encoder_hw_resources *hw_res)
{
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index b491346..99a5d73 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -156,5 +156,16 @@ void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
*/
void dpu_encoder_set_idle_timeout(struct drm_encoder *drm_enc,
u32 idle_timeout);
+/**
+ * dpu_encoder_get_linecount - get interface line count for the encoder.
+ * @drm_enc: Pointer to previously created drm encoder structure
+ */
+int dpu_encoder_get_linecount(struct drm_encoder *drm_enc);
+
+/**
+ * dpu_encoder_get_frame_count - get interface frame count for the encoder.
+ * @drm_enc: Pointer to previously created drm encoder structure
+ */
+int dpu_encoder_get_frame_count(struct drm_encoder *drm_enc);

#endif /* __DPU_ENCODER_H__ */
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 f8f2515..ecbc4be 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -143,6 +143,7 @@ struct dpu_encoder_phys_ops {
void (*prepare_idle_pc)(struct dpu_encoder_phys *phys_enc);
void (*restore)(struct dpu_encoder_phys *phys);
int (*get_line_count)(struct dpu_encoder_phys *phys);
+ int (*get_frame_count)(struct dpu_encoder_phys *phys);
};

/**
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 9a69fad..0e06b7e 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
@@ -658,6 +658,31 @@ static int dpu_encoder_phys_vid_get_line_count(
return phys_enc->hw_intf->ops.get_line_count(phys_enc->hw_intf);
}

+static int dpu_encoder_phys_vid_get_frame_count(
+ struct dpu_encoder_phys *phys_enc)
+{
+ struct intf_status s = {0};
+ u32 fetch_start = 0;
+ struct drm_display_mode mode = phys_enc->cached_mode;
+
+ if (!dpu_encoder_phys_vid_is_master(phys_enc))
+ return -EINVAL;
+
+ if (!phys_enc->hw_intf || !phys_enc->hw_intf->ops.get_status)
+ return -EINVAL;
+
+ phys_enc->hw_intf->ops.get_status(phys_enc->hw_intf, &s);
+
+ if (s.is_prog_fetch_en && s.is_en) {
+ fetch_start = mode.vtotal - (mode.vsync_start - mode.vdisplay);
+ if ((s.line_count > fetch_start) &&
+ (s.line_count <= mode.vtotal))
+ return s.frame_count + 1;
+ }
+
+ return s.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;
@@ -676,6 +701,7 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
ops->handle_post_kickoff = dpu_encoder_phys_vid_handle_post_kickoff;
ops->needs_single_flush = dpu_encoder_phys_vid_needs_single_flush;
ops->get_line_count = dpu_encoder_phys_vid_get_line_count;
+ ops->get_frame_count = dpu_encoder_phys_vid_get_frame_count;
}

struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
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 6f0f545..717178b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -256,6 +256,7 @@ static void dpu_hw_intf_get_status(
struct dpu_hw_blk_reg_map *c = &intf->hw;

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);
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 0ead64d..3568be8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -40,6 +40,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 line_count; /* current line count including blanking */
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 374b0e8..ed636f1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -14,6 +14,7 @@

#include <drm/drm_crtc.h>
#include <drm/drm_file.h>
+#include <drm/drm_vblank.h>

#include "msm_drv.h"
#include "msm_mmu.h"
@@ -1020,6 +1021,10 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
*/
dev->mode_config.allow_fb_modifiers = true;

+ dev->max_vblank_count = 0xffffffff;
+ /* Disable vblank irqs aggressively for power-saving */
+ dev->vblank_disable_immediate = true;
+
/*
* _dpu_kms_drm_obj_init should create the DRM related objects
* i.e. CRTCs, planes, encoders, connectors and so forth
--
2.7.4


2021-02-22 16:10:37

by Rob Clark

[permalink] [raw]
Subject: Re: [v4] drm/msm/disp/dpu1: turn off vblank irqs aggressively in dpu driver

On Thu, Feb 18, 2021 at 4:36 AM Kalyan Thota <[email protected]> wrote:
>
> Set the flag vblank_disable_immediate = true to turn off vblank irqs
> immediately as soon as drm_vblank_put is requested so that there are
> no irqs triggered during idle state. This will reduce cpu wakeups
> and help in power saving.
>
> To enable vblank_disable_immediate flag the underlying KMS driver
> needs to support high precision vblank timestamping and also a
> reliable way of providing vblank counter which is incrementing
> at the leading edge of vblank.
>
> This patch also brings in changes to support vblank_disable_immediate
> requirement in dpu driver.
>
> Changes in v1:
> - Specify reason to add vblank timestamp support. (Rob).
> - Add changes to provide vblank counter from dpu driver.
>
> Changes in v2:
> - Fix warn stack reported by Rob Clark with v2 patch.
>
> Changes in v3:
> - Move back to HW frame counter (Rob).
>

could you let me know what the delta was in v4? (No need to resend
yet, if needed I can amend the commit msg when applying)

BR,
-R

> Signed-off-by: Kalyan Thota <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 80 ++++++++++++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 30 ++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 11 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 1 +
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 26 +++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 1 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 ++
> 8 files changed, 155 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index d4662e8..9a80981 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -65,6 +65,83 @@ static void dpu_crtc_destroy(struct drm_crtc *crtc)
> kfree(dpu_crtc);
> }
>
> +static struct drm_encoder *get_encoder_from_crtc(struct drm_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct drm_encoder *encoder;
> +
> + drm_for_each_encoder(encoder, dev)
> + if (encoder->crtc == crtc)
> + return encoder;
> +
> + return NULL;
> +}
> +
> +static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
> +{
> + struct drm_encoder *encoder;
> +
> + encoder = get_encoder_from_crtc(crtc);
> + if (!encoder) {
> + DRM_ERROR("no encoder found for crtc %d\n", crtc->index);
> + return false;
> + }
> +
> + return dpu_encoder_get_frame_count(encoder);
> +}
> +
> +static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
> + bool in_vblank_irq,
> + int *vpos, int *hpos,
> + ktime_t *stime, ktime_t *etime,
> + const struct drm_display_mode *mode)
> +{
> + unsigned int pipe = crtc->index;
> + struct drm_encoder *encoder;
> + int line, vsw, vbp, vactive_start, vactive_end, vfp_end;
> +
> + encoder = get_encoder_from_crtc(crtc);
> + if (!encoder) {
> + DRM_ERROR("no encoder found for crtc %d\n", pipe);
> + return false;
> + }
> +
> + vsw = mode->crtc_vsync_end - mode->crtc_vsync_start;
> + vbp = mode->crtc_vtotal - mode->crtc_vsync_end;
> +
> + /*
> + * the line counter is 1 at the start of the VSYNC pulse and VTOTAL at
> + * the end of VFP. Translate the porch values relative to the line
> + * counter positions.
> + */
> +
> + vactive_start = vsw + vbp + 1;
> + vactive_end = vactive_start + mode->crtc_vdisplay;
> +
> + /* last scan line before VSYNC */
> + vfp_end = mode->crtc_vtotal;
> +
> + if (stime)
> + *stime = ktime_get();
> +
> + line = dpu_encoder_get_linecount(encoder);
> +
> + if (line < vactive_start)
> + line -= vactive_start;
> + else if (line > vactive_end)
> + line = line - vfp_end - vactive_start;
> + else
> + line -= vactive_start;
> +
> + *vpos = line;
> + *hpos = 0;
> +
> + if (etime)
> + *etime = ktime_get();
> +
> + return true;
> +}
> +
> static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer,
> struct dpu_plane_state *pstate, struct dpu_format *format)
> {
> @@ -1243,6 +1320,8 @@ static const struct drm_crtc_funcs dpu_crtc_funcs = {
> .early_unregister = dpu_crtc_early_unregister,
> .enable_vblank = msm_crtc_enable_vblank,
> .disable_vblank = msm_crtc_disable_vblank,
> + .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
> + .get_vblank_counter = dpu_crtc_get_vblank_counter,
> };
>
> static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = {
> @@ -1251,6 +1330,7 @@ static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = {
> .atomic_check = dpu_crtc_atomic_check,
> .atomic_begin = dpu_crtc_atomic_begin,
> .atomic_flush = dpu_crtc_atomic_flush,
> + .get_scanout_position = dpu_crtc_get_scanout_position,
> };
>
> /* initialize crtc */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index f7f5c25..5cd3f31 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -425,6 +425,36 @@ int dpu_encoder_helper_unregister_irq(struct dpu_encoder_phys *phys_enc,
> return 0;
> }
>
> +int dpu_encoder_get_frame_count(struct drm_encoder *drm_enc)
> +{
> + struct dpu_encoder_virt *dpu_enc;
> + struct dpu_encoder_phys *phys;
> + int framecount = 0;
> +
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> + phys = dpu_enc ? dpu_enc->cur_master : NULL;
> +
> + if (phys && phys->ops.get_frame_count)
> + framecount = phys->ops.get_frame_count(phys);
> +
> + return framecount;
> +}
> +
> +int dpu_encoder_get_linecount(struct drm_encoder *drm_enc)
> +{
> + struct dpu_encoder_virt *dpu_enc;
> + struct dpu_encoder_phys *phys;
> + int linecount = 0;
> +
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> + phys = dpu_enc ? dpu_enc->cur_master : NULL;
> +
> + if (phys && phys->ops.get_line_count)
> + linecount = phys->ops.get_line_count(phys);
> +
> + return linecount;
> +}
> +
> void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
> struct dpu_encoder_hw_resources *hw_res)
> {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index b491346..99a5d73 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -156,5 +156,16 @@ void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> */
> void dpu_encoder_set_idle_timeout(struct drm_encoder *drm_enc,
> u32 idle_timeout);
> +/**
> + * dpu_encoder_get_linecount - get interface line count for the encoder.
> + * @drm_enc: Pointer to previously created drm encoder structure
> + */
> +int dpu_encoder_get_linecount(struct drm_encoder *drm_enc);
> +
> +/**
> + * dpu_encoder_get_frame_count - get interface frame count for the encoder.
> + * @drm_enc: Pointer to previously created drm encoder structure
> + */
> +int dpu_encoder_get_frame_count(struct drm_encoder *drm_enc);
>
> #endif /* __DPU_ENCODER_H__ */
> 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 f8f2515..ecbc4be 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -143,6 +143,7 @@ struct dpu_encoder_phys_ops {
> void (*prepare_idle_pc)(struct dpu_encoder_phys *phys_enc);
> void (*restore)(struct dpu_encoder_phys *phys);
> int (*get_line_count)(struct dpu_encoder_phys *phys);
> + int (*get_frame_count)(struct dpu_encoder_phys *phys);
> };
>
> /**
> 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 9a69fad..0e06b7e 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
> @@ -658,6 +658,31 @@ static int dpu_encoder_phys_vid_get_line_count(
> return phys_enc->hw_intf->ops.get_line_count(phys_enc->hw_intf);
> }
>
> +static int dpu_encoder_phys_vid_get_frame_count(
> + struct dpu_encoder_phys *phys_enc)
> +{
> + struct intf_status s = {0};
> + u32 fetch_start = 0;
> + struct drm_display_mode mode = phys_enc->cached_mode;
> +
> + if (!dpu_encoder_phys_vid_is_master(phys_enc))
> + return -EINVAL;
> +
> + if (!phys_enc->hw_intf || !phys_enc->hw_intf->ops.get_status)
> + return -EINVAL;
> +
> + phys_enc->hw_intf->ops.get_status(phys_enc->hw_intf, &s);
> +
> + if (s.is_prog_fetch_en && s.is_en) {
> + fetch_start = mode.vtotal - (mode.vsync_start - mode.vdisplay);
> + if ((s.line_count > fetch_start) &&
> + (s.line_count <= mode.vtotal))
> + return s.frame_count + 1;
> + }
> +
> + return s.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;
> @@ -676,6 +701,7 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
> ops->handle_post_kickoff = dpu_encoder_phys_vid_handle_post_kickoff;
> ops->needs_single_flush = dpu_encoder_phys_vid_needs_single_flush;
> ops->get_line_count = dpu_encoder_phys_vid_get_line_count;
> + ops->get_frame_count = dpu_encoder_phys_vid_get_frame_count;
> }
>
> struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
> 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 6f0f545..717178b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -256,6 +256,7 @@ static void dpu_hw_intf_get_status(
> struct dpu_hw_blk_reg_map *c = &intf->hw;
>
> 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);
> 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 0ead64d..3568be8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -40,6 +40,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 line_count; /* current line count including blanking */
> };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 374b0e8..ed636f1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -14,6 +14,7 @@
>
> #include <drm/drm_crtc.h>
> #include <drm/drm_file.h>
> +#include <drm/drm_vblank.h>
>
> #include "msm_drv.h"
> #include "msm_mmu.h"
> @@ -1020,6 +1021,10 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
> */
> dev->mode_config.allow_fb_modifiers = true;
>
> + dev->max_vblank_count = 0xffffffff;
> + /* Disable vblank irqs aggressively for power-saving */
> + dev->vblank_disable_immediate = true;
> +
> /*
> * _dpu_kms_drm_obj_init should create the DRM related objects
> * i.e. CRTCs, planes, encoders, connectors and so forth
> --
> 2.7.4
>

2021-02-23 15:15:25

by Kalyan Thota

[permalink] [raw]
Subject: Re: [Freedreno] [v4] drm/msm/disp/dpu1: turn off vblank irqs aggressively in dpu driver

On 2021-02-22 21:38, Rob Clark wrote:
> On Thu, Feb 18, 2021 at 4:36 AM Kalyan Thota <[email protected]>
> wrote:
>>
>> Set the flag vblank_disable_immediate = true to turn off vblank irqs
>> immediately as soon as drm_vblank_put is requested so that there are
>> no irqs triggered during idle state. This will reduce cpu wakeups
>> and help in power saving.
>>
>> To enable vblank_disable_immediate flag the underlying KMS driver
>> needs to support high precision vblank timestamping and also a
>> reliable way of providing vblank counter which is incrementing
>> at the leading edge of vblank.
>>
>> This patch also brings in changes to support vblank_disable_immediate
>> requirement in dpu driver.
>>
>> Changes in v1:
>> - Specify reason to add vblank timestamp support. (Rob).
>> - Add changes to provide vblank counter from dpu driver.
>>
>> Changes in v2:
>> - Fix warn stack reported by Rob Clark with v2 patch.
>>
>> Changes in v3:
>> - Move back to HW frame counter (Rob).
>>
>
> could you let me know what the delta was in v4? (No need to resend
> yet, if needed I can amend the commit msg when applying)
>
- Frame count mismatch was causing a DRM WARN stack spew.
DPU HW will increment the frame count at the end of
the sync, where as vblank will be triggered at the
fetch_start counter which is calculated as v_total - vfp.
This is to start fetching early for panels with low
vbp w.r.t hw latency lines.

Add logic to detect the line count if it falls between
vactive and v_total then return incremented frame count value.
> BR,
> -R
>
>> Signed-off-by: Kalyan Thota <[email protected]>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 80
>> ++++++++++++++++++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 30 ++++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 11 +++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 1 +
>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 26 +++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 1 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 ++
>> 8 files changed, 155 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index d4662e8..9a80981 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -65,6 +65,83 @@ static void dpu_crtc_destroy(struct drm_crtc *crtc)
>> kfree(dpu_crtc);
>> }
>>
>> +static struct drm_encoder *get_encoder_from_crtc(struct drm_crtc
>> *crtc)
>> +{
>> + struct drm_device *dev = crtc->dev;
>> + struct drm_encoder *encoder;
>> +
>> + drm_for_each_encoder(encoder, dev)
>> + if (encoder->crtc == crtc)
>> + return encoder;
>> +
>> + return NULL;
>> +}
>> +
>> +static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
>> +{
>> + struct drm_encoder *encoder;
>> +
>> + encoder = get_encoder_from_crtc(crtc);
>> + if (!encoder) {
>> + DRM_ERROR("no encoder found for crtc %d\n",
>> crtc->index);
>> + return false;
>> + }
>> +
>> + return dpu_encoder_get_frame_count(encoder);
>> +}
>> +
>> +static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
>> + bool in_vblank_irq,
>> + int *vpos, int *hpos,
>> + ktime_t *stime, ktime_t
>> *etime,
>> + const struct
>> drm_display_mode *mode)
>> +{
>> + unsigned int pipe = crtc->index;
>> + struct drm_encoder *encoder;
>> + int line, vsw, vbp, vactive_start, vactive_end, vfp_end;
>> +
>> + encoder = get_encoder_from_crtc(crtc);
>> + if (!encoder) {
>> + DRM_ERROR("no encoder found for crtc %d\n", pipe);
>> + return false;
>> + }
>> +
>> + vsw = mode->crtc_vsync_end - mode->crtc_vsync_start;
>> + vbp = mode->crtc_vtotal - mode->crtc_vsync_end;
>> +
>> + /*
>> + * the line counter is 1 at the start of the VSYNC pulse and
>> VTOTAL at
>> + * the end of VFP. Translate the porch values relative to the
>> line
>> + * counter positions.
>> + */
>> +
>> + vactive_start = vsw + vbp + 1;
>> + vactive_end = vactive_start + mode->crtc_vdisplay;
>> +
>> + /* last scan line before VSYNC */
>> + vfp_end = mode->crtc_vtotal;
>> +
>> + if (stime)
>> + *stime = ktime_get();
>> +
>> + line = dpu_encoder_get_linecount(encoder);
>> +
>> + if (line < vactive_start)
>> + line -= vactive_start;
>> + else if (line > vactive_end)
>> + line = line - vfp_end - vactive_start;
>> + else
>> + line -= vactive_start;
>> +
>> + *vpos = line;
>> + *hpos = 0;
>> +
>> + if (etime)
>> + *etime = ktime_get();
>> +
>> + return true;
>> +}
>> +
>> static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer *mixer,
>> struct dpu_plane_state *pstate, struct dpu_format
>> *format)
>> {
>> @@ -1243,6 +1320,8 @@ static const struct drm_crtc_funcs
>> dpu_crtc_funcs = {
>> .early_unregister = dpu_crtc_early_unregister,
>> .enable_vblank = msm_crtc_enable_vblank,
>> .disable_vblank = msm_crtc_disable_vblank,
>> + .get_vblank_timestamp =
>> drm_crtc_vblank_helper_get_vblank_timestamp,
>> + .get_vblank_counter = dpu_crtc_get_vblank_counter,
>> };
>>
>> static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = {
>> @@ -1251,6 +1330,7 @@ static const struct drm_crtc_helper_funcs
>> dpu_crtc_helper_funcs = {
>> .atomic_check = dpu_crtc_atomic_check,
>> .atomic_begin = dpu_crtc_atomic_begin,
>> .atomic_flush = dpu_crtc_atomic_flush,
>> + .get_scanout_position = dpu_crtc_get_scanout_position,
>> };
>>
>> /* initialize crtc */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index f7f5c25..5cd3f31 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -425,6 +425,36 @@ int dpu_encoder_helper_unregister_irq(struct
>> dpu_encoder_phys *phys_enc,
>> return 0;
>> }
>>
>> +int dpu_encoder_get_frame_count(struct drm_encoder *drm_enc)
>> +{
>> + struct dpu_encoder_virt *dpu_enc;
>> + struct dpu_encoder_phys *phys;
>> + int framecount = 0;
>> +
>> + dpu_enc = to_dpu_encoder_virt(drm_enc);
>> + phys = dpu_enc ? dpu_enc->cur_master : NULL;
>> +
>> + if (phys && phys->ops.get_frame_count)
>> + framecount = phys->ops.get_frame_count(phys);
>> +
>> + return framecount;
>> +}
>> +
>> +int dpu_encoder_get_linecount(struct drm_encoder *drm_enc)
>> +{
>> + struct dpu_encoder_virt *dpu_enc;
>> + struct dpu_encoder_phys *phys;
>> + int linecount = 0;
>> +
>> + dpu_enc = to_dpu_encoder_virt(drm_enc);
>> + phys = dpu_enc ? dpu_enc->cur_master : NULL;
>> +
>> + if (phys && phys->ops.get_line_count)
>> + linecount = phys->ops.get_line_count(phys);
>> +
>> + return linecount;
>> +}
>> +
>> void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
>> struct dpu_encoder_hw_resources
>> *hw_res)
>> {
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index b491346..99a5d73 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -156,5 +156,16 @@ void dpu_encoder_prepare_commit(struct
>> drm_encoder *drm_enc);
>> */
>> void dpu_encoder_set_idle_timeout(struct drm_encoder *drm_enc,
>> u32
>> idle_timeout);
>> +/**
>> + * dpu_encoder_get_linecount - get interface line count for the
>> encoder.
>> + * @drm_enc: Pointer to previously created drm encoder structure
>> + */
>> +int dpu_encoder_get_linecount(struct drm_encoder *drm_enc);
>> +
>> +/**
>> + * dpu_encoder_get_frame_count - get interface frame count for the
>> encoder.
>> + * @drm_enc: Pointer to previously created drm encoder structure
>> + */
>> +int dpu_encoder_get_frame_count(struct drm_encoder *drm_enc);
>>
>> #endif /* __DPU_ENCODER_H__ */
>> 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 f8f2515..ecbc4be 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -143,6 +143,7 @@ struct dpu_encoder_phys_ops {
>> void (*prepare_idle_pc)(struct dpu_encoder_phys *phys_enc);
>> void (*restore)(struct dpu_encoder_phys *phys);
>> int (*get_line_count)(struct dpu_encoder_phys *phys);
>> + int (*get_frame_count)(struct dpu_encoder_phys *phys);
>> };
>>
>> /**
>> 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 9a69fad..0e06b7e 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
>> @@ -658,6 +658,31 @@ static int dpu_encoder_phys_vid_get_line_count(
>> return
>> phys_enc->hw_intf->ops.get_line_count(phys_enc->hw_intf);
>> }
>>
>> +static int dpu_encoder_phys_vid_get_frame_count(
>> + struct dpu_encoder_phys *phys_enc)
>> +{
>> + struct intf_status s = {0};
>> + u32 fetch_start = 0;
>> + struct drm_display_mode mode = phys_enc->cached_mode;
>> +
>> + if (!dpu_encoder_phys_vid_is_master(phys_enc))
>> + return -EINVAL;
>> +
>> + if (!phys_enc->hw_intf || !phys_enc->hw_intf->ops.get_status)
>> + return -EINVAL;
>> +
>> + phys_enc->hw_intf->ops.get_status(phys_enc->hw_intf, &s);
>> +
>> + if (s.is_prog_fetch_en && s.is_en) {
>> + fetch_start = mode.vtotal - (mode.vsync_start -
>> mode.vdisplay);
>> + if ((s.line_count > fetch_start) &&
>> + (s.line_count <= mode.vtotal))
>> + return s.frame_count + 1;
>> + }
>> +
>> + return s.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;
>> @@ -676,6 +701,7 @@ static void dpu_encoder_phys_vid_init_ops(struct
>> dpu_encoder_phys_ops *ops)
>> ops->handle_post_kickoff =
>> dpu_encoder_phys_vid_handle_post_kickoff;
>> ops->needs_single_flush =
>> dpu_encoder_phys_vid_needs_single_flush;
>> ops->get_line_count = dpu_encoder_phys_vid_get_line_count;
>> + ops->get_frame_count = dpu_encoder_phys_vid_get_frame_count;
>> }
>>
>> struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>> 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 6f0f545..717178b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -256,6 +256,7 @@ static void dpu_hw_intf_get_status(
>> struct dpu_hw_blk_reg_map *c = &intf->hw;
>>
>> 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);
>> 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 0ead64d..3568be8 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -40,6 +40,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 line_count; /* current line count including
>> blanking */
>> };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 374b0e8..ed636f1 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -14,6 +14,7 @@
>>
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_file.h>
>> +#include <drm/drm_vblank.h>
>>
>> #include "msm_drv.h"
>> #include "msm_mmu.h"
>> @@ -1020,6 +1021,10 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>> */
>> dev->mode_config.allow_fb_modifiers = true;
>>
>> + dev->max_vblank_count = 0xffffffff;
>> + /* Disable vblank irqs aggressively for power-saving */
>> + dev->vblank_disable_immediate = true;
>> +
>> /*
>> * _dpu_kms_drm_obj_init should create the DRM related objects
>> * i.e. CRTCs, planes, encoders, connectors and so forth
>> --
>> 2.7.4
>>
> _______________________________________________
> Freedreno mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/freedreno