* [PATCH v2 0/3] Expand CRC to support interface blocks
@ 2022-06-14 21:13 ` Jessica Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-14 21:13 UTC (permalink / raw)
To: freedreno
Cc: Jessica Zhang, linux-arm-msm, dri-devel, robdclark, seanpaul,
swboyd, quic_aravindh, dmitry.baryshkov, quic_abhinavk
Refactor existing CRC code for layer mixer and add CRC support for interface blocks
Changes since V1:
- Create helper methods for collect_misr and setup_misr in dpu_hw_util.c
- Move common bitmasks into dpu_hw_util.h
- Update copyrights
- Create a dynamically allocated crcs array in dpu_crtc_state
- Collect CRCs for all drm_encoders connected to the crtc
Jessica Zhang (3):
drm/msm/dpu: Move LM CRC code into separate method
drm/msm/dpu: Add MISR register support for interface
drm/msm/dpu: Add interface support for CRC debugfs
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 131 +++++++++++++++-----
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 7 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 63 ++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 19 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +------
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 +++
9 files changed, 279 insertions(+), 75 deletions(-)
--
2.35.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 0/3] Expand CRC to support interface blocks
@ 2022-06-14 21:13 ` Jessica Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-14 21:13 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, Jessica Zhang, quic_aravindh
Refactor existing CRC code for layer mixer and add CRC support for interface blocks
Changes since V1:
- Create helper methods for collect_misr and setup_misr in dpu_hw_util.c
- Move common bitmasks into dpu_hw_util.h
- Update copyrights
- Create a dynamically allocated crcs array in dpu_crtc_state
- Collect CRCs for all drm_encoders connected to the crtc
Jessica Zhang (3):
drm/msm/dpu: Move LM CRC code into separate method
drm/msm/dpu: Add MISR register support for interface
drm/msm/dpu: Add interface support for CRC debugfs
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 131 +++++++++++++++-----
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 7 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 63 ++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 19 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +------
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 +++
9 files changed, 279 insertions(+), 75 deletions(-)
--
2.35.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
2022-06-14 21:13 ` Jessica Zhang
@ 2022-06-14 21:13 ` Jessica Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-14 21:13 UTC (permalink / raw)
To: freedreno
Cc: Jessica Zhang, linux-arm-msm, dri-devel, robdclark, seanpaul,
swboyd, quic_aravindh, dmitry.baryshkov, quic_abhinavk
Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
helper method. This way, we can make it easier to get CRCs from other HW
blocks by adding other get_crc helper methods.
Changes since V1:
- Moved common bitmasks to dpu_hw_util.h
- Moved common CRC methods to dpu_hw_util.c
- Updated copyrights
- Changed crcs array to a dynamically allocated array and added it as a
member of crtc_state
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88 +++++++++++++--------
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
5 files changed, 124 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index b56f777dbd0e..16742a66878e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark <robdclark@gmail.com>
@@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
+ if (crtc_state->crcs) {
+ kfree(crtc_state->crcs);
+ crtc_state->crcs = NULL;
+ }
+
if (source < 0) {
DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n", src_name, crtc->index);
return -EINVAL;
@@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
*values_cnt = crtc_state->num_mixers;
+ crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
+
return 0;
}
+static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
+{
+ struct dpu_crtc_mixer *m;
+ int i;
+
+ for (i = 0; i < crtc_state->num_mixers; ++i) {
+ m = &crtc_state->mixers[i];
+
+ if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
+ continue;
+
+ /* Calculate MISR over 1 frame */
+ m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
+ }
+}
+
static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
{
enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
enum dpu_crtc_crc_source current_source;
struct dpu_crtc_state *crtc_state;
struct drm_device *drm_dev = crtc->dev;
- struct dpu_crtc_mixer *m;
bool was_enabled;
bool enable = false;
- int i, ret = 0;
+ int ret = 0;
if (source < 0) {
DRM_DEBUG_DRIVER("Invalid CRC source %s for CRTC%d\n", src_name, crtc->index);
@@ -137,6 +160,10 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
goto cleanup;
} else if (was_enabled && !enable) {
+ if (crtc_state->crcs) {
+ kfree(crtc_state->crcs);
+ crtc_state->crcs = NULL;
+ }
drm_crtc_vblank_put(crtc);
}
@@ -146,16 +173,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
crtc_state->crc_frame_skip_count = 0;
- for (i = 0; i < crtc_state->num_mixers; ++i) {
- m = &crtc_state->mixers[i];
-
- if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
- continue;
-
- /* Calculate MISR over 1 frame */
- m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
- }
-
+ if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
+ dpu_crtc_setup_lm_misr(crtc_state);
cleanup:
drm_modeset_unlock(&crtc->mutex);
@@ -174,34 +193,21 @@ static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
return dpu_encoder_get_vsync_count(encoder);
}
-
-static int dpu_crtc_get_crc(struct drm_crtc *crtc)
+static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crtc_state)
{
- struct dpu_crtc_state *crtc_state;
- struct dpu_crtc_mixer *m;
- u32 crcs[CRTC_DUAL_MIXERS];
+ struct dpu_crtc_mixer *lm;
- int i = 0;
int rc = 0;
-
- crtc_state = to_dpu_crtc_state(crtc->state);
-
- BUILD_BUG_ON(ARRAY_SIZE(crcs) != ARRAY_SIZE(crtc_state->mixers));
-
- /* Skip first 2 frames in case of "uncooked" CRCs */
- if (crtc_state->crc_frame_skip_count < 2) {
- crtc_state->crc_frame_skip_count++;
- return 0;
- }
+ int i;
for (i = 0; i < crtc_state->num_mixers; ++i) {
- m = &crtc_state->mixers[i];
+ lm = &crtc_state->mixers[i];
- if (!m->hw_lm || !m->hw_lm->ops.collect_misr)
+ if (!lm->hw_lm || !lm->hw_lm->ops.collect_misr)
continue;
- rc = m->hw_lm->ops.collect_misr(m->hw_lm, &crcs[i]);
+ rc = lm->hw_lm->ops.collect_misr(lm->hw_lm, &crtc_state->crcs[i]);
if (rc) {
if (rc != -ENODATA)
@@ -211,7 +217,25 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
}
return drm_crtc_add_crc_entry(crtc, true,
- drm_crtc_accurate_vblank_count(crtc), crcs);
+ drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
+}
+
+static int dpu_crtc_get_crc(struct drm_crtc *crtc)
+{
+ struct dpu_crtc_state *crtc_state;
+
+ crtc_state = to_dpu_crtc_state(crtc->state);
+
+ /* Skip first 2 frames in case of "uncooked" CRCs */
+ if (crtc_state->crc_frame_skip_count < 2) {
+ crtc_state->crc_frame_skip_count++;
+ return 0;
+ }
+
+ if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
+ return dpu_crtc_get_lm_crc(crtc, crtc_state);
+
+ return 0;
}
static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index b8785c394fcc..4bf45e3343ef 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -201,6 +201,9 @@ struct dpu_crtc {
* @mixers : List of active mixers
* @num_ctls : Number of ctl paths in use
* @hw_ctls : List of active ctl paths
+ * @crc_source : CRC source
+ * @crc_frame_skip_count: Number of frames skipped before getting CRC
+ * @crcs : Array to store CRC values
*/
struct dpu_crtc_state {
struct drm_crtc_state base;
@@ -222,6 +225,7 @@ struct dpu_crtc_state {
enum dpu_crtc_crc_source crc_source;
int crc_frame_skip_count;
+ u32 *crcs;
};
#define to_dpu_crtc_state(x) \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
index 462f5082099e..06b24d8d1419 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
*/
@@ -27,11 +28,6 @@
#define LM_MISR_CTRL 0x310
#define LM_MISR_SIGNATURE 0x314
-#define LM_MISR_FRAME_COUNT_MASK 0xFF
-#define LM_MISR_CTRL_ENABLE BIT(8)
-#define LM_MISR_CTRL_STATUS BIT(9)
-#define LM_MISR_CTRL_STATUS_CLEAR BIT(10)
-#define LM_MISR_CTRL_FREE_RUN_MASK BIT(31)
static const struct dpu_lm_cfg *_lm_offset(enum dpu_lm mixer,
@@ -108,44 +104,12 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx,
static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count)
{
- struct dpu_hw_blk_reg_map *c = &ctx->hw;
- u32 config = 0;
-
- DPU_REG_WRITE(c, LM_MISR_CTRL, LM_MISR_CTRL_STATUS_CLEAR);
-
- /* Clear old MISR value (in case it's read before a new value is calculated)*/
- wmb();
-
- if (enable) {
- config = (frame_count & LM_MISR_FRAME_COUNT_MASK) |
- LM_MISR_CTRL_ENABLE | LM_MISR_CTRL_FREE_RUN_MASK;
-
- DPU_REG_WRITE(c, LM_MISR_CTRL, config);
- } else {
- DPU_REG_WRITE(c, LM_MISR_CTRL, 0);
- }
-
+ dpu_hw_setup_misr(&ctx->hw, enable, frame_count, LM_MISR_CTRL);
}
static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value)
{
- struct dpu_hw_blk_reg_map *c = &ctx->hw;
- u32 ctrl = 0;
-
- if (!misr_value)
- return -EINVAL;
-
- ctrl = DPU_REG_READ(c, LM_MISR_CTRL);
-
- if (!(ctrl & LM_MISR_CTRL_ENABLE))
- return -ENODATA;
-
- if (!(ctrl & LM_MISR_CTRL_STATUS))
- return -EINVAL;
-
- *misr_value = DPU_REG_READ(c, LM_MISR_SIGNATURE);
-
- return 0;
+ return dpu_hw_collect_misr(&ctx->hw, misr_value, LM_MISR_CTRL, LM_MISR_SIGNATURE);
}
static void dpu_hw_lm_setup_blend_config_sdm845(struct dpu_hw_mixer *ctx,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
index 512316f25a51..244f2f90e99a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
*/
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
@@ -447,3 +449,45 @@ u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
return 0;
}
+
+void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, bool enable,
+ u32 frame_count, u32 misr_ctrl_offset)
+{
+ u32 config = 0;
+
+ DPU_REG_WRITE(c, misr_ctrl_offset, MISR_CTRL_STATUS_CLEAR);
+
+ /* Clear old MISR value (in case it's read before a new value is calculated)*/
+ wmb();
+
+ if (enable) {
+ config = (frame_count & MISR_FRAME_COUNT_MASK) |
+ MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK;
+
+ DPU_REG_WRITE(c, misr_ctrl_offset, config);
+ } else {
+ DPU_REG_WRITE(c, misr_ctrl_offset, 0);
+ }
+
+}
+
+int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, u32 *misr_value,
+ u32 misr_ctrl_offset, u32 misr_signature_offset)
+{
+ u32 ctrl = 0;
+
+ if (!misr_value)
+ return -EINVAL;
+
+ ctrl = DPU_REG_READ(c, misr_ctrl_offset);
+
+ if (!(ctrl & MISR_CTRL_ENABLE))
+ return -ENODATA;
+
+ if (!(ctrl & MISR_CTRL_STATUS))
+ return -EINVAL;
+
+ *misr_value = DPU_REG_READ(c, misr_signature_offset);
+
+ return 0;
+}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
index e4a65eb4f769..88df3a5c5d8e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
*/
@@ -12,6 +13,11 @@
#include "dpu_hw_catalog.h"
#define REG_MASK(n) ((BIT(n)) - 1)
+#define MISR_FRAME_COUNT_MASK 0xFF
+#define MISR_CTRL_ENABLE BIT(8)
+#define MISR_CTRL_STATUS BIT(9)
+#define MISR_CTRL_STATUS_CLEAR BIT(10)
+#define MISR_CTRL_FREE_RUN_MASK BIT(31)
/*
* This is the common struct maintained by each sub block
@@ -343,4 +349,14 @@ void dpu_hw_csc_setup(struct dpu_hw_blk_reg_map *c,
u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
u32 total_fl);
+void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c,
+ bool enable,
+ u32 frame_count,
+ u32 misr_ctrl_offset);
+
+int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c,
+ u32 *misr_value,
+ u32 misr_ctrl_offset,
+ u32 misr_signature_offset);
+
#endif /* _DPU_HW_UTIL_H */
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
@ 2022-06-14 21:13 ` Jessica Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-14 21:13 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, Jessica Zhang, quic_aravindh
Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
helper method. This way, we can make it easier to get CRCs from other HW
blocks by adding other get_crc helper methods.
Changes since V1:
- Moved common bitmasks to dpu_hw_util.h
- Moved common CRC methods to dpu_hw_util.c
- Updated copyrights
- Changed crcs array to a dynamically allocated array and added it as a
member of crtc_state
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88 +++++++++++++--------
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
5 files changed, 124 insertions(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index b56f777dbd0e..16742a66878e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark <robdclark@gmail.com>
@@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
+ if (crtc_state->crcs) {
+ kfree(crtc_state->crcs);
+ crtc_state->crcs = NULL;
+ }
+
if (source < 0) {
DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n", src_name, crtc->index);
return -EINVAL;
@@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
*values_cnt = crtc_state->num_mixers;
+ crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
+
return 0;
}
+static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
+{
+ struct dpu_crtc_mixer *m;
+ int i;
+
+ for (i = 0; i < crtc_state->num_mixers; ++i) {
+ m = &crtc_state->mixers[i];
+
+ if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
+ continue;
+
+ /* Calculate MISR over 1 frame */
+ m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
+ }
+}
+
static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
{
enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
enum dpu_crtc_crc_source current_source;
struct dpu_crtc_state *crtc_state;
struct drm_device *drm_dev = crtc->dev;
- struct dpu_crtc_mixer *m;
bool was_enabled;
bool enable = false;
- int i, ret = 0;
+ int ret = 0;
if (source < 0) {
DRM_DEBUG_DRIVER("Invalid CRC source %s for CRTC%d\n", src_name, crtc->index);
@@ -137,6 +160,10 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
goto cleanup;
} else if (was_enabled && !enable) {
+ if (crtc_state->crcs) {
+ kfree(crtc_state->crcs);
+ crtc_state->crcs = NULL;
+ }
drm_crtc_vblank_put(crtc);
}
@@ -146,16 +173,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
crtc_state->crc_frame_skip_count = 0;
- for (i = 0; i < crtc_state->num_mixers; ++i) {
- m = &crtc_state->mixers[i];
-
- if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
- continue;
-
- /* Calculate MISR over 1 frame */
- m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
- }
-
+ if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
+ dpu_crtc_setup_lm_misr(crtc_state);
cleanup:
drm_modeset_unlock(&crtc->mutex);
@@ -174,34 +193,21 @@ static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
return dpu_encoder_get_vsync_count(encoder);
}
-
-static int dpu_crtc_get_crc(struct drm_crtc *crtc)
+static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crtc_state)
{
- struct dpu_crtc_state *crtc_state;
- struct dpu_crtc_mixer *m;
- u32 crcs[CRTC_DUAL_MIXERS];
+ struct dpu_crtc_mixer *lm;
- int i = 0;
int rc = 0;
-
- crtc_state = to_dpu_crtc_state(crtc->state);
-
- BUILD_BUG_ON(ARRAY_SIZE(crcs) != ARRAY_SIZE(crtc_state->mixers));
-
- /* Skip first 2 frames in case of "uncooked" CRCs */
- if (crtc_state->crc_frame_skip_count < 2) {
- crtc_state->crc_frame_skip_count++;
- return 0;
- }
+ int i;
for (i = 0; i < crtc_state->num_mixers; ++i) {
- m = &crtc_state->mixers[i];
+ lm = &crtc_state->mixers[i];
- if (!m->hw_lm || !m->hw_lm->ops.collect_misr)
+ if (!lm->hw_lm || !lm->hw_lm->ops.collect_misr)
continue;
- rc = m->hw_lm->ops.collect_misr(m->hw_lm, &crcs[i]);
+ rc = lm->hw_lm->ops.collect_misr(lm->hw_lm, &crtc_state->crcs[i]);
if (rc) {
if (rc != -ENODATA)
@@ -211,7 +217,25 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
}
return drm_crtc_add_crc_entry(crtc, true,
- drm_crtc_accurate_vblank_count(crtc), crcs);
+ drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
+}
+
+static int dpu_crtc_get_crc(struct drm_crtc *crtc)
+{
+ struct dpu_crtc_state *crtc_state;
+
+ crtc_state = to_dpu_crtc_state(crtc->state);
+
+ /* Skip first 2 frames in case of "uncooked" CRCs */
+ if (crtc_state->crc_frame_skip_count < 2) {
+ crtc_state->crc_frame_skip_count++;
+ return 0;
+ }
+
+ if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
+ return dpu_crtc_get_lm_crc(crtc, crtc_state);
+
+ return 0;
}
static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index b8785c394fcc..4bf45e3343ef 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -201,6 +201,9 @@ struct dpu_crtc {
* @mixers : List of active mixers
* @num_ctls : Number of ctl paths in use
* @hw_ctls : List of active ctl paths
+ * @crc_source : CRC source
+ * @crc_frame_skip_count: Number of frames skipped before getting CRC
+ * @crcs : Array to store CRC values
*/
struct dpu_crtc_state {
struct drm_crtc_state base;
@@ -222,6 +225,7 @@ struct dpu_crtc_state {
enum dpu_crtc_crc_source crc_source;
int crc_frame_skip_count;
+ u32 *crcs;
};
#define to_dpu_crtc_state(x) \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
index 462f5082099e..06b24d8d1419 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
*/
@@ -27,11 +28,6 @@
#define LM_MISR_CTRL 0x310
#define LM_MISR_SIGNATURE 0x314
-#define LM_MISR_FRAME_COUNT_MASK 0xFF
-#define LM_MISR_CTRL_ENABLE BIT(8)
-#define LM_MISR_CTRL_STATUS BIT(9)
-#define LM_MISR_CTRL_STATUS_CLEAR BIT(10)
-#define LM_MISR_CTRL_FREE_RUN_MASK BIT(31)
static const struct dpu_lm_cfg *_lm_offset(enum dpu_lm mixer,
@@ -108,44 +104,12 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx,
static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count)
{
- struct dpu_hw_blk_reg_map *c = &ctx->hw;
- u32 config = 0;
-
- DPU_REG_WRITE(c, LM_MISR_CTRL, LM_MISR_CTRL_STATUS_CLEAR);
-
- /* Clear old MISR value (in case it's read before a new value is calculated)*/
- wmb();
-
- if (enable) {
- config = (frame_count & LM_MISR_FRAME_COUNT_MASK) |
- LM_MISR_CTRL_ENABLE | LM_MISR_CTRL_FREE_RUN_MASK;
-
- DPU_REG_WRITE(c, LM_MISR_CTRL, config);
- } else {
- DPU_REG_WRITE(c, LM_MISR_CTRL, 0);
- }
-
+ dpu_hw_setup_misr(&ctx->hw, enable, frame_count, LM_MISR_CTRL);
}
static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value)
{
- struct dpu_hw_blk_reg_map *c = &ctx->hw;
- u32 ctrl = 0;
-
- if (!misr_value)
- return -EINVAL;
-
- ctrl = DPU_REG_READ(c, LM_MISR_CTRL);
-
- if (!(ctrl & LM_MISR_CTRL_ENABLE))
- return -ENODATA;
-
- if (!(ctrl & LM_MISR_CTRL_STATUS))
- return -EINVAL;
-
- *misr_value = DPU_REG_READ(c, LM_MISR_SIGNATURE);
-
- return 0;
+ return dpu_hw_collect_misr(&ctx->hw, misr_value, LM_MISR_CTRL, LM_MISR_SIGNATURE);
}
static void dpu_hw_lm_setup_blend_config_sdm845(struct dpu_hw_mixer *ctx,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
index 512316f25a51..244f2f90e99a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
*/
#define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
@@ -447,3 +449,45 @@ u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
return 0;
}
+
+void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, bool enable,
+ u32 frame_count, u32 misr_ctrl_offset)
+{
+ u32 config = 0;
+
+ DPU_REG_WRITE(c, misr_ctrl_offset, MISR_CTRL_STATUS_CLEAR);
+
+ /* Clear old MISR value (in case it's read before a new value is calculated)*/
+ wmb();
+
+ if (enable) {
+ config = (frame_count & MISR_FRAME_COUNT_MASK) |
+ MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK;
+
+ DPU_REG_WRITE(c, misr_ctrl_offset, config);
+ } else {
+ DPU_REG_WRITE(c, misr_ctrl_offset, 0);
+ }
+
+}
+
+int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, u32 *misr_value,
+ u32 misr_ctrl_offset, u32 misr_signature_offset)
+{
+ u32 ctrl = 0;
+
+ if (!misr_value)
+ return -EINVAL;
+
+ ctrl = DPU_REG_READ(c, misr_ctrl_offset);
+
+ if (!(ctrl & MISR_CTRL_ENABLE))
+ return -ENODATA;
+
+ if (!(ctrl & MISR_CTRL_STATUS))
+ return -EINVAL;
+
+ *misr_value = DPU_REG_READ(c, misr_signature_offset);
+
+ return 0;
+}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
index e4a65eb4f769..88df3a5c5d8e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
*/
@@ -12,6 +13,11 @@
#include "dpu_hw_catalog.h"
#define REG_MASK(n) ((BIT(n)) - 1)
+#define MISR_FRAME_COUNT_MASK 0xFF
+#define MISR_CTRL_ENABLE BIT(8)
+#define MISR_CTRL_STATUS BIT(9)
+#define MISR_CTRL_STATUS_CLEAR BIT(10)
+#define MISR_CTRL_FREE_RUN_MASK BIT(31)
/*
* This is the common struct maintained by each sub block
@@ -343,4 +349,14 @@ void dpu_hw_csc_setup(struct dpu_hw_blk_reg_map *c,
u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
u32 total_fl);
+void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c,
+ bool enable,
+ u32 frame_count,
+ u32 misr_ctrl_offset);
+
+int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c,
+ u32 *misr_value,
+ u32 misr_ctrl_offset,
+ u32 misr_signature_offset);
+
#endif /* _DPU_HW_UTIL_H */
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 2/3] drm/msm/dpu: Add MISR register support for interface
2022-06-14 21:13 ` Jessica Zhang
@ 2022-06-14 21:13 ` Jessica Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-14 21:13 UTC (permalink / raw)
To: freedreno
Cc: Jessica Zhang, linux-arm-msm, dri-devel, robdclark, seanpaul,
swboyd, quic_aravindh, dmitry.baryshkov, quic_abhinavk
Add support for setting MISR registers within the interface
Changes since V1:
- Replaced dpu_hw_intf collect_misr and setup_misr implementations with
calls to dpu_hw_utils helper methods
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 19 ++++++++++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 +++++++-
2 files changed, 25 insertions(+), 2 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 3f4d2c6e1b45..0157613224fd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
*/
#include "dpu_hwio.h"
@@ -67,6 +69,9 @@
#define INTF_CFG2_DATABUS_WIDEN BIT(0)
#define INTF_CFG2_DATA_HCTL_EN BIT(4)
+#define INTF_MISR_CTRL 0x180
+#define INTF_MISR_SIGNATURE 0x184
+
static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
const struct dpu_mdss_cfg *m,
void __iomem *addr,
@@ -319,6 +324,16 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf)
return DPU_REG_READ(c, INTF_LINE_COUNT);
}
+static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count)
+{
+ dpu_hw_setup_misr(&intf->hw, enable, frame_count, INTF_MISR_CTRL);
+}
+
+static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value)
+{
+ return dpu_hw_collect_misr(&intf->hw, misr_value, INTF_MISR_CTRL, INTF_MISR_SIGNATURE);
+}
+
static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
unsigned long cap)
{
@@ -329,6 +344,8 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
ops->get_line_count = dpu_hw_intf_get_line_count;
if (cap & BIT(DPU_INTF_INPUT_CTRL))
ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
+ ops->setup_misr = dpu_hw_intf_setup_misr;
+ ops->collect_misr = dpu_hw_intf_collect_misr;
}
struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
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 7b2d96ac61e8..8d0e7b509260 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -1,5 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
*/
#ifndef _DPU_HW_INTF_H
@@ -57,6 +59,8 @@ struct intf_status {
* @ get_line_count: reads current vertical line counter
* @bind_pingpong_blk: enable/disable the connection with pingpong which will
* feed pixels to this interface
+ * @setup_misr: enable/disable MISR
+ * @collect_misr: read MISR signature
*/
struct dpu_hw_intf_ops {
void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -77,6 +81,8 @@ struct dpu_hw_intf_ops {
void (*bind_pingpong_blk)(struct dpu_hw_intf *intf,
bool enable,
const enum dpu_pingpong pp);
+ void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count);
+ int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
};
struct dpu_hw_intf {
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 2/3] drm/msm/dpu: Add MISR register support for interface
@ 2022-06-14 21:13 ` Jessica Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-14 21:13 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, Jessica Zhang, quic_aravindh
Add support for setting MISR registers within the interface
Changes since V1:
- Replaced dpu_hw_intf collect_misr and setup_misr implementations with
calls to dpu_hw_utils helper methods
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 19 ++++++++++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 +++++++-
2 files changed, 25 insertions(+), 2 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 3f4d2c6e1b45..0157613224fd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
*/
#include "dpu_hwio.h"
@@ -67,6 +69,9 @@
#define INTF_CFG2_DATABUS_WIDEN BIT(0)
#define INTF_CFG2_DATA_HCTL_EN BIT(4)
+#define INTF_MISR_CTRL 0x180
+#define INTF_MISR_SIGNATURE 0x184
+
static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
const struct dpu_mdss_cfg *m,
void __iomem *addr,
@@ -319,6 +324,16 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf)
return DPU_REG_READ(c, INTF_LINE_COUNT);
}
+static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count)
+{
+ dpu_hw_setup_misr(&intf->hw, enable, frame_count, INTF_MISR_CTRL);
+}
+
+static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value)
+{
+ return dpu_hw_collect_misr(&intf->hw, misr_value, INTF_MISR_CTRL, INTF_MISR_SIGNATURE);
+}
+
static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
unsigned long cap)
{
@@ -329,6 +344,8 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
ops->get_line_count = dpu_hw_intf_get_line_count;
if (cap & BIT(DPU_INTF_INPUT_CTRL))
ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
+ ops->setup_misr = dpu_hw_intf_setup_misr;
+ ops->collect_misr = dpu_hw_intf_collect_misr;
}
struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
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 7b2d96ac61e8..8d0e7b509260 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -1,5 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0-only */
-/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
*/
#ifndef _DPU_HW_INTF_H
@@ -57,6 +59,8 @@ struct intf_status {
* @ get_line_count: reads current vertical line counter
* @bind_pingpong_blk: enable/disable the connection with pingpong which will
* feed pixels to this interface
+ * @setup_misr: enable/disable MISR
+ * @collect_misr: read MISR signature
*/
struct dpu_hw_intf_ops {
void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -77,6 +81,8 @@ struct dpu_hw_intf_ops {
void (*bind_pingpong_blk)(struct dpu_hw_intf *intf,
bool enable,
const enum dpu_pingpong pp);
+ void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count);
+ int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
};
struct dpu_hw_intf {
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 3/3] drm/msm/dpu: Add interface support for CRC debugfs
2022-06-14 21:13 ` Jessica Zhang
@ 2022-06-14 21:13 ` Jessica Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-14 21:13 UTC (permalink / raw)
To: freedreno
Cc: Jessica Zhang, linux-arm-msm, dri-devel, robdclark, seanpaul,
swboyd, quic_aravindh, dmitry.baryshkov, quic_abhinavk
Add support for writing CRC values for the interface block to
the debugfs by calling the necessary MISR setup/collect methods.
Changes since V1:
- Set values_cnt to only include phys with backing hw_intf
- Loop over all drm_encs connected to crtc
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 49 ++++++++++++++--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 64 +++++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 +++++++
4 files changed, 134 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 16742a66878e..8c9933b2337f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source dpu_crtc_parse_crc_source(const char *src_name)
if (!strcmp(src_name, "auto") ||
!strcmp(src_name, "lm"))
return DPU_CRTC_CRC_SOURCE_LAYER_MIXER;
+ if (!strcmp(src_name, "intf"))
+ return DPU_CRTC_CRC_SOURCE_INTF;
return DPU_CRTC_CRC_SOURCE_INVALID;
}
@@ -99,8 +101,14 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
return -EINVAL;
}
- if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
+ if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) {
*values_cnt = crtc_state->num_mixers;
+ } else if (source == DPU_CRTC_CRC_SOURCE_INTF) {
+ struct drm_encoder *drm_enc;
+
+ drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
+ *values_cnt += dpu_encoder_get_num_phys(drm_enc);
+ }
crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
@@ -123,6 +131,14 @@ static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
}
}
+static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc)
+{
+ struct drm_encoder *drm_enc;
+
+ drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
+ dpu_encoder_setup_misr(drm_enc);
+}
+
static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
{
enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
@@ -175,6 +191,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
dpu_crtc_setup_lm_misr(crtc_state);
+ else if (source == DPU_CRTC_CRC_SOURCE_INTF)
+ dpu_crtc_setup_encoder_misr(crtc);
cleanup:
drm_modeset_unlock(&crtc->mutex);
@@ -220,11 +238,31 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt
drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
}
-static int dpu_crtc_get_crc(struct drm_crtc *crtc)
+static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc)
{
- struct dpu_crtc_state *crtc_state;
+ struct drm_encoder *drm_enc;
+ struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
+ int rc, pos = 0;
- crtc_state = to_dpu_crtc_state(crtc->state);
+ drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) {
+ rc = dpu_encoder_get_crc(drm_enc, crtc_state->crcs, pos);
+ if (rc < 0) {
+ if (rc != -ENODATA)
+ DRM_DEBUG_DRIVER("MISR read failed\n");
+
+ return rc;
+ }
+
+ pos += rc;
+ }
+
+ return drm_crtc_add_crc_entry(crtc, true,
+ drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
+}
+
+static int dpu_crtc_get_crc(struct drm_crtc *crtc)
+{
+ struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
/* Skip first 2 frames in case of "uncooked" CRCs */
if (crtc_state->crc_frame_skip_count < 2) {
@@ -235,6 +273,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
return dpu_crtc_get_lm_crc(crtc, crtc_state);
+ if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF)
+ return dpu_crtc_get_encoder_crc(crtc);
+
return 0;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 4bf45e3343ef..5db84ea796db 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2015-2021 The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark <robdclark@gmail.com>
@@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data {
* enum dpu_crtc_crc_source: CRC source
* @DPU_CRTC_CRC_SOURCE_NONE: no source set
* @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer
+ * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface
* @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source
*/
enum dpu_crtc_crc_source {
DPU_CRTC_CRC_SOURCE_NONE = 0,
DPU_CRTC_CRC_SOURCE_LAYER_MIXER,
+ DPU_CRTC_CRC_SOURCE_INTF,
DPU_CRTC_CRC_SOURCE_MAX,
DPU_CRTC_CRC_SOURCE_INVALID = -1
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 52516eb20cb8..2cbfed5c627e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -14,6 +14,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_file.h>
+#include <drm/drm_vblank.h>
#include <drm/drm_probe_helper.h>
#include "msm_drv.h"
@@ -225,6 +226,69 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
return dpu_enc->wide_bus_en;
}
+int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+ int i, num_intf = 0;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+ if (phys->hw_intf)
+ num_intf++;
+ }
+
+ return num_intf;
+}
+
+void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ int i;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+ if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr)
+ continue;
+
+ phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);
+ }
+}
+
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ int i, rc = 0, entries_added = 0;
+
+ if (!drm_enc->crtc) {
+ DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index);
+ return -EINVAL;
+ }
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+ if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr)
+ continue;
+
+ rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[pos + i]);
+ if (rc)
+ return rc;
+ entries_added++;
+ }
+
+ return entries_added;
+}
+
static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
{
struct dpu_hw_dither_cfg dither_cfg = { 0 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 781d41c91994..375370029cb9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark <robdclark@gmail.com>
@@ -174,6 +175,27 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc);
bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc);
+/**
+ * dpu_encoder_get_num_phys - get number of physical encoders contained in virtual
+ * encoder
+ * @drm_enc: Pointer to previously created drm encoder structure
+ * Returns: Number of physical encoders for given drm encoder
+ */
+int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc);
+
+/**
+ * dpu_encoder_setup_misr - enable misr calculations
+ * @drm_enc: Pointer to previously created drm encoder structure
+ */
+void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder);
+
+/**
+ * dpu_encoder_get_crc - get the crc value from interface blocks
+ * @drm_enc: Pointer to previously created drm encoder structure
+ * Returns: 0 on success, error otherwise
+ */
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos);
+
/**
* dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology.
* @drm_enc: Pointer to previously created drm encoder structure
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v2 3/3] drm/msm/dpu: Add interface support for CRC debugfs
@ 2022-06-14 21:13 ` Jessica Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-14 21:13 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
dmitry.baryshkov, Jessica Zhang, quic_aravindh
Add support for writing CRC values for the interface block to
the debugfs by calling the necessary MISR setup/collect methods.
Changes since V1:
- Set values_cnt to only include phys with backing hw_intf
- Loop over all drm_encs connected to crtc
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 49 ++++++++++++++--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 64 +++++++++++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 +++++++
4 files changed, 134 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 16742a66878e..8c9933b2337f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source dpu_crtc_parse_crc_source(const char *src_name)
if (!strcmp(src_name, "auto") ||
!strcmp(src_name, "lm"))
return DPU_CRTC_CRC_SOURCE_LAYER_MIXER;
+ if (!strcmp(src_name, "intf"))
+ return DPU_CRTC_CRC_SOURCE_INTF;
return DPU_CRTC_CRC_SOURCE_INVALID;
}
@@ -99,8 +101,14 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
return -EINVAL;
}
- if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
+ if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) {
*values_cnt = crtc_state->num_mixers;
+ } else if (source == DPU_CRTC_CRC_SOURCE_INTF) {
+ struct drm_encoder *drm_enc;
+
+ drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
+ *values_cnt += dpu_encoder_get_num_phys(drm_enc);
+ }
crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
@@ -123,6 +131,14 @@ static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
}
}
+static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc)
+{
+ struct drm_encoder *drm_enc;
+
+ drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
+ dpu_encoder_setup_misr(drm_enc);
+}
+
static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
{
enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
@@ -175,6 +191,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
dpu_crtc_setup_lm_misr(crtc_state);
+ else if (source == DPU_CRTC_CRC_SOURCE_INTF)
+ dpu_crtc_setup_encoder_misr(crtc);
cleanup:
drm_modeset_unlock(&crtc->mutex);
@@ -220,11 +238,31 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt
drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
}
-static int dpu_crtc_get_crc(struct drm_crtc *crtc)
+static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc)
{
- struct dpu_crtc_state *crtc_state;
+ struct drm_encoder *drm_enc;
+ struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
+ int rc, pos = 0;
- crtc_state = to_dpu_crtc_state(crtc->state);
+ drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) {
+ rc = dpu_encoder_get_crc(drm_enc, crtc_state->crcs, pos);
+ if (rc < 0) {
+ if (rc != -ENODATA)
+ DRM_DEBUG_DRIVER("MISR read failed\n");
+
+ return rc;
+ }
+
+ pos += rc;
+ }
+
+ return drm_crtc_add_crc_entry(crtc, true,
+ drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
+}
+
+static int dpu_crtc_get_crc(struct drm_crtc *crtc)
+{
+ struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
/* Skip first 2 frames in case of "uncooked" CRCs */
if (crtc_state->crc_frame_skip_count < 2) {
@@ -235,6 +273,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
return dpu_crtc_get_lm_crc(crtc, crtc_state);
+ if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF)
+ return dpu_crtc_get_encoder_crc(crtc);
+
return 0;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 4bf45e3343ef..5db84ea796db 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2015-2021 The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark <robdclark@gmail.com>
@@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data {
* enum dpu_crtc_crc_source: CRC source
* @DPU_CRTC_CRC_SOURCE_NONE: no source set
* @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer
+ * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface
* @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source
*/
enum dpu_crtc_crc_source {
DPU_CRTC_CRC_SOURCE_NONE = 0,
DPU_CRTC_CRC_SOURCE_LAYER_MIXER,
+ DPU_CRTC_CRC_SOURCE_INTF,
DPU_CRTC_CRC_SOURCE_MAX,
DPU_CRTC_CRC_SOURCE_INVALID = -1
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 52516eb20cb8..2cbfed5c627e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -14,6 +14,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_file.h>
+#include <drm/drm_vblank.h>
#include <drm/drm_probe_helper.h>
#include "msm_drv.h"
@@ -225,6 +226,69 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
return dpu_enc->wide_bus_en;
}
+int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+ int i, num_intf = 0;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+ if (phys->hw_intf)
+ num_intf++;
+ }
+
+ return num_intf;
+}
+
+void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ int i;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+ if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr)
+ continue;
+
+ phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);
+ }
+}
+
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ int i, rc = 0, entries_added = 0;
+
+ if (!drm_enc->crtc) {
+ DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index);
+ return -EINVAL;
+ }
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+ struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+
+ if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr)
+ continue;
+
+ rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[pos + i]);
+ if (rc)
+ return rc;
+ entries_added++;
+ }
+
+ return entries_added;
+}
+
static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
{
struct dpu_hw_dither_cfg dither_cfg = { 0 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 781d41c91994..375370029cb9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark <robdclark@gmail.com>
@@ -174,6 +175,27 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc);
bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc);
+/**
+ * dpu_encoder_get_num_phys - get number of physical encoders contained in virtual
+ * encoder
+ * @drm_enc: Pointer to previously created drm encoder structure
+ * Returns: Number of physical encoders for given drm encoder
+ */
+int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc);
+
+/**
+ * dpu_encoder_setup_misr - enable misr calculations
+ * @drm_enc: Pointer to previously created drm encoder structure
+ */
+void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder);
+
+/**
+ * dpu_encoder_get_crc - get the crc value from interface blocks
+ * @drm_enc: Pointer to previously created drm encoder structure
+ * Returns: 0 on success, error otherwise
+ */
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos);
+
/**
* dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology.
* @drm_enc: Pointer to previously created drm encoder structure
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
2022-06-14 21:13 ` Jessica Zhang
@ 2022-06-15 9:35 ` Dmitry Baryshkov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-06-15 9:35 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
quic_aravindh, quic_abhinavk
On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
> helper method. This way, we can make it easier to get CRCs from other HW
> blocks by adding other get_crc helper methods.
>
> Changes since V1:
> - Moved common bitmasks to dpu_hw_util.h
> - Moved common CRC methods to dpu_hw_util.c
> - Updated copyrights
> - Changed crcs array to a dynamically allocated array and added it as a
> member of crtc_state
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Please split this into separate patches. One for hw_util, one for the rest
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88 +++++++++++++--------
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
> 5 files changed, 124 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index b56f777dbd0e..16742a66878e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
> * Copyright (C) 2013 Red Hat
> * Author: Rob Clark <robdclark@gmail.com>
> @@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
> struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
>
> + if (crtc_state->crcs) {
> + kfree(crtc_state->crcs);
> + crtc_state->crcs = NULL;
> + }
> +
> if (source < 0) {
> DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n", src_name, crtc->index);
> return -EINVAL;
> @@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> *values_cnt = crtc_state->num_mixers;
>
> + crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
> +
I do not quite like the idea of constantly allocating and freeing the
crcs array. I'd suggest defining sensible MAX_CRC_VALUES, using a
static array and verifying that no one over commits the
MAX_CRC_VALUES.
If at some point we decide that we really need the dynamic array, we
can implement it later. We already had dynamic arrays and Rob had to
fix it. See 00326bfa4e63 ("drm/msm/dpu: Remove dynamic allocation from
atomic context").
> return 0;
> }
>
> +static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
> +{
> + struct dpu_crtc_mixer *m;
> + int i;
> +
> + for (i = 0; i < crtc_state->num_mixers; ++i) {
> + m = &crtc_state->mixers[i];
> +
> + if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
> + continue;
> +
> + /* Calculate MISR over 1 frame */
> + m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
> + }
> +}
> +
> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
> {
> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
> enum dpu_crtc_crc_source current_source;
> struct dpu_crtc_state *crtc_state;
> struct drm_device *drm_dev = crtc->dev;
> - struct dpu_crtc_mixer *m;
>
> bool was_enabled;
> bool enable = false;
> - int i, ret = 0;
> + int ret = 0;
>
> if (source < 0) {
> DRM_DEBUG_DRIVER("Invalid CRC source %s for CRTC%d\n", src_name, crtc->index);
> @@ -137,6 +160,10 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
> goto cleanup;
>
> } else if (was_enabled && !enable) {
> + if (crtc_state->crcs) {
> + kfree(crtc_state->crcs);
> + crtc_state->crcs = NULL;
> + }
> drm_crtc_vblank_put(crtc);
> }
>
> @@ -146,16 +173,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>
> crtc_state->crc_frame_skip_count = 0;
>
> - for (i = 0; i < crtc_state->num_mixers; ++i) {
> - m = &crtc_state->mixers[i];
> -
> - if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
> - continue;
> -
> - /* Calculate MISR over 1 frame */
> - m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
> - }
> -
> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> + dpu_crtc_setup_lm_misr(crtc_state);
else ?
>
> cleanup:
> drm_modeset_unlock(&crtc->mutex);
> @@ -174,34 +193,21 @@ static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
> return dpu_encoder_get_vsync_count(encoder);
> }
>
> -
> -static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> +static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crtc_state)
> {
> - struct dpu_crtc_state *crtc_state;
> - struct dpu_crtc_mixer *m;
> - u32 crcs[CRTC_DUAL_MIXERS];
> + struct dpu_crtc_mixer *lm;
>
> - int i = 0;
> int rc = 0;
> -
> - crtc_state = to_dpu_crtc_state(crtc->state);
> -
> - BUILD_BUG_ON(ARRAY_SIZE(crcs) != ARRAY_SIZE(crtc_state->mixers));
> -
> - /* Skip first 2 frames in case of "uncooked" CRCs */
> - if (crtc_state->crc_frame_skip_count < 2) {
> - crtc_state->crc_frame_skip_count++;
> - return 0;
> - }
> + int i;
>
> for (i = 0; i < crtc_state->num_mixers; ++i) {
>
> - m = &crtc_state->mixers[i];
> + lm = &crtc_state->mixers[i];
>
> - if (!m->hw_lm || !m->hw_lm->ops.collect_misr)
> + if (!lm->hw_lm || !lm->hw_lm->ops.collect_misr)
> continue;
>
> - rc = m->hw_lm->ops.collect_misr(m->hw_lm, &crcs[i]);
> + rc = lm->hw_lm->ops.collect_misr(lm->hw_lm, &crtc_state->crcs[i]);
>
> if (rc) {
> if (rc != -ENODATA)
> @@ -211,7 +217,25 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> }
>
> return drm_crtc_add_crc_entry(crtc, true,
> - drm_crtc_accurate_vblank_count(crtc), crcs);
> + drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
> +}
> +
> +static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> +{
> + struct dpu_crtc_state *crtc_state;
> +
> + crtc_state = to_dpu_crtc_state(crtc->state);
> +
> + /* Skip first 2 frames in case of "uncooked" CRCs */
> + if (crtc_state->crc_frame_skip_count < 2) {
> + crtc_state->crc_frame_skip_count++;
> + return 0;
> + }
> +
> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> + return dpu_crtc_get_lm_crc(crtc, crtc_state);
> +
> + return 0;
> }
>
> static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index b8785c394fcc..4bf45e3343ef 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -201,6 +201,9 @@ struct dpu_crtc {
> * @mixers : List of active mixers
> * @num_ctls : Number of ctl paths in use
> * @hw_ctls : List of active ctl paths
> + * @crc_source : CRC source
> + * @crc_frame_skip_count: Number of frames skipped before getting CRC
> + * @crcs : Array to store CRC values
> */
> struct dpu_crtc_state {
> struct drm_crtc_state base;
> @@ -222,6 +225,7 @@ struct dpu_crtc_state {
>
> enum dpu_crtc_crc_source crc_source;
> int crc_frame_skip_count;
> + u32 *crcs;
> };
>
> #define to_dpu_crtc_state(x) \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
> index 462f5082099e..06b24d8d1419 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> */
>
> @@ -27,11 +28,6 @@
>
> #define LM_MISR_CTRL 0x310
> #define LM_MISR_SIGNATURE 0x314
> -#define LM_MISR_FRAME_COUNT_MASK 0xFF
> -#define LM_MISR_CTRL_ENABLE BIT(8)
> -#define LM_MISR_CTRL_STATUS BIT(9)
> -#define LM_MISR_CTRL_STATUS_CLEAR BIT(10)
> -#define LM_MISR_CTRL_FREE_RUN_MASK BIT(31)
>
>
> static const struct dpu_lm_cfg *_lm_offset(enum dpu_lm mixer,
> @@ -108,44 +104,12 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx,
>
> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count)
> {
> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
> - u32 config = 0;
> -
> - DPU_REG_WRITE(c, LM_MISR_CTRL, LM_MISR_CTRL_STATUS_CLEAR);
> -
> - /* Clear old MISR value (in case it's read before a new value is calculated)*/
> - wmb();
> -
> - if (enable) {
> - config = (frame_count & LM_MISR_FRAME_COUNT_MASK) |
> - LM_MISR_CTRL_ENABLE | LM_MISR_CTRL_FREE_RUN_MASK;
> -
> - DPU_REG_WRITE(c, LM_MISR_CTRL, config);
> - } else {
> - DPU_REG_WRITE(c, LM_MISR_CTRL, 0);
> - }
> -
> + dpu_hw_setup_misr(&ctx->hw, enable, frame_count, LM_MISR_CTRL);
> }
>
> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value)
> {
> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
> - u32 ctrl = 0;
> -
> - if (!misr_value)
> - return -EINVAL;
> -
> - ctrl = DPU_REG_READ(c, LM_MISR_CTRL);
> -
> - if (!(ctrl & LM_MISR_CTRL_ENABLE))
> - return -ENODATA;
> -
> - if (!(ctrl & LM_MISR_CTRL_STATUS))
> - return -EINVAL;
> -
> - *misr_value = DPU_REG_READ(c, LM_MISR_SIGNATURE);
> -
> - return 0;
> + return dpu_hw_collect_misr(&ctx->hw, misr_value, LM_MISR_CTRL, LM_MISR_SIGNATURE);
> }
>
> static void dpu_hw_lm_setup_blend_config_sdm845(struct dpu_hw_mixer *ctx,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> index 512316f25a51..244f2f90e99a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> */
> #define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
>
> @@ -447,3 +449,45 @@ u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>
> return 0;
> }
> +
> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, bool enable,
> + u32 frame_count, u32 misr_ctrl_offset)
> +{
> + u32 config = 0;
> +
> + DPU_REG_WRITE(c, misr_ctrl_offset, MISR_CTRL_STATUS_CLEAR);
> +
> + /* Clear old MISR value (in case it's read before a new value is calculated)*/
> + wmb();
> +
> + if (enable) {
> + config = (frame_count & MISR_FRAME_COUNT_MASK) |
> + MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK;
> +
> + DPU_REG_WRITE(c, misr_ctrl_offset, config);
> + } else {
> + DPU_REG_WRITE(c, misr_ctrl_offset, 0);
> + }
> +
> +}
> +
> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, u32 *misr_value,
> + u32 misr_ctrl_offset, u32 misr_signature_offset)
> +{
> + u32 ctrl = 0;
> +
> + if (!misr_value)
> + return -EINVAL;
> +
> + ctrl = DPU_REG_READ(c, misr_ctrl_offset);
> +
> + if (!(ctrl & MISR_CTRL_ENABLE))
> + return -ENODATA;
> +
> + if (!(ctrl & MISR_CTRL_STATUS))
> + return -EINVAL;
> +
> + *misr_value = DPU_REG_READ(c, misr_signature_offset);
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> index e4a65eb4f769..88df3a5c5d8e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> @@ -1,5 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> */
>
> @@ -12,6 +13,11 @@
> #include "dpu_hw_catalog.h"
>
> #define REG_MASK(n) ((BIT(n)) - 1)
> +#define MISR_FRAME_COUNT_MASK 0xFF
> +#define MISR_CTRL_ENABLE BIT(8)
> +#define MISR_CTRL_STATUS BIT(9)
> +#define MISR_CTRL_STATUS_CLEAR BIT(10)
> +#define MISR_CTRL_FREE_RUN_MASK BIT(31)
>
> /*
> * This is the common struct maintained by each sub block
> @@ -343,4 +349,14 @@ void dpu_hw_csc_setup(struct dpu_hw_blk_reg_map *c,
> u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
> u32 total_fl);
>
> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c,
> + bool enable,
> + u32 frame_count,
> + u32 misr_ctrl_offset);
> +
> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c,
> + u32 *misr_value,
> + u32 misr_ctrl_offset,
> + u32 misr_signature_offset);
Please move offsets next to the hw_blk_reg_map.
> +
> #endif /* _DPU_HW_UTIL_H */
> --
> 2.35.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
@ 2022-06-15 9:35 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-06-15 9:35 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
quic_aravindh, freedreno
On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
> helper method. This way, we can make it easier to get CRCs from other HW
> blocks by adding other get_crc helper methods.
>
> Changes since V1:
> - Moved common bitmasks to dpu_hw_util.h
> - Moved common CRC methods to dpu_hw_util.c
> - Updated copyrights
> - Changed crcs array to a dynamically allocated array and added it as a
> member of crtc_state
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Please split this into separate patches. One for hw_util, one for the rest
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88 +++++++++++++--------
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
> 5 files changed, 124 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index b56f777dbd0e..16742a66878e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
> * Copyright (C) 2013 Red Hat
> * Author: Rob Clark <robdclark@gmail.com>
> @@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
> struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
>
> + if (crtc_state->crcs) {
> + kfree(crtc_state->crcs);
> + crtc_state->crcs = NULL;
> + }
> +
> if (source < 0) {
> DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n", src_name, crtc->index);
> return -EINVAL;
> @@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> *values_cnt = crtc_state->num_mixers;
>
> + crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
> +
I do not quite like the idea of constantly allocating and freeing the
crcs array. I'd suggest defining sensible MAX_CRC_VALUES, using a
static array and verifying that no one over commits the
MAX_CRC_VALUES.
If at some point we decide that we really need the dynamic array, we
can implement it later. We already had dynamic arrays and Rob had to
fix it. See 00326bfa4e63 ("drm/msm/dpu: Remove dynamic allocation from
atomic context").
> return 0;
> }
>
> +static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
> +{
> + struct dpu_crtc_mixer *m;
> + int i;
> +
> + for (i = 0; i < crtc_state->num_mixers; ++i) {
> + m = &crtc_state->mixers[i];
> +
> + if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
> + continue;
> +
> + /* Calculate MISR over 1 frame */
> + m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
> + }
> +}
> +
> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
> {
> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
> enum dpu_crtc_crc_source current_source;
> struct dpu_crtc_state *crtc_state;
> struct drm_device *drm_dev = crtc->dev;
> - struct dpu_crtc_mixer *m;
>
> bool was_enabled;
> bool enable = false;
> - int i, ret = 0;
> + int ret = 0;
>
> if (source < 0) {
> DRM_DEBUG_DRIVER("Invalid CRC source %s for CRTC%d\n", src_name, crtc->index);
> @@ -137,6 +160,10 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
> goto cleanup;
>
> } else if (was_enabled && !enable) {
> + if (crtc_state->crcs) {
> + kfree(crtc_state->crcs);
> + crtc_state->crcs = NULL;
> + }
> drm_crtc_vblank_put(crtc);
> }
>
> @@ -146,16 +173,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>
> crtc_state->crc_frame_skip_count = 0;
>
> - for (i = 0; i < crtc_state->num_mixers; ++i) {
> - m = &crtc_state->mixers[i];
> -
> - if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
> - continue;
> -
> - /* Calculate MISR over 1 frame */
> - m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
> - }
> -
> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> + dpu_crtc_setup_lm_misr(crtc_state);
else ?
>
> cleanup:
> drm_modeset_unlock(&crtc->mutex);
> @@ -174,34 +193,21 @@ static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
> return dpu_encoder_get_vsync_count(encoder);
> }
>
> -
> -static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> +static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crtc_state)
> {
> - struct dpu_crtc_state *crtc_state;
> - struct dpu_crtc_mixer *m;
> - u32 crcs[CRTC_DUAL_MIXERS];
> + struct dpu_crtc_mixer *lm;
>
> - int i = 0;
> int rc = 0;
> -
> - crtc_state = to_dpu_crtc_state(crtc->state);
> -
> - BUILD_BUG_ON(ARRAY_SIZE(crcs) != ARRAY_SIZE(crtc_state->mixers));
> -
> - /* Skip first 2 frames in case of "uncooked" CRCs */
> - if (crtc_state->crc_frame_skip_count < 2) {
> - crtc_state->crc_frame_skip_count++;
> - return 0;
> - }
> + int i;
>
> for (i = 0; i < crtc_state->num_mixers; ++i) {
>
> - m = &crtc_state->mixers[i];
> + lm = &crtc_state->mixers[i];
>
> - if (!m->hw_lm || !m->hw_lm->ops.collect_misr)
> + if (!lm->hw_lm || !lm->hw_lm->ops.collect_misr)
> continue;
>
> - rc = m->hw_lm->ops.collect_misr(m->hw_lm, &crcs[i]);
> + rc = lm->hw_lm->ops.collect_misr(lm->hw_lm, &crtc_state->crcs[i]);
>
> if (rc) {
> if (rc != -ENODATA)
> @@ -211,7 +217,25 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> }
>
> return drm_crtc_add_crc_entry(crtc, true,
> - drm_crtc_accurate_vblank_count(crtc), crcs);
> + drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
> +}
> +
> +static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> +{
> + struct dpu_crtc_state *crtc_state;
> +
> + crtc_state = to_dpu_crtc_state(crtc->state);
> +
> + /* Skip first 2 frames in case of "uncooked" CRCs */
> + if (crtc_state->crc_frame_skip_count < 2) {
> + crtc_state->crc_frame_skip_count++;
> + return 0;
> + }
> +
> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> + return dpu_crtc_get_lm_crc(crtc, crtc_state);
> +
> + return 0;
> }
>
> static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index b8785c394fcc..4bf45e3343ef 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -201,6 +201,9 @@ struct dpu_crtc {
> * @mixers : List of active mixers
> * @num_ctls : Number of ctl paths in use
> * @hw_ctls : List of active ctl paths
> + * @crc_source : CRC source
> + * @crc_frame_skip_count: Number of frames skipped before getting CRC
> + * @crcs : Array to store CRC values
> */
> struct dpu_crtc_state {
> struct drm_crtc_state base;
> @@ -222,6 +225,7 @@ struct dpu_crtc_state {
>
> enum dpu_crtc_crc_source crc_source;
> int crc_frame_skip_count;
> + u32 *crcs;
> };
>
> #define to_dpu_crtc_state(x) \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
> index 462f5082099e..06b24d8d1419 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> */
>
> @@ -27,11 +28,6 @@
>
> #define LM_MISR_CTRL 0x310
> #define LM_MISR_SIGNATURE 0x314
> -#define LM_MISR_FRAME_COUNT_MASK 0xFF
> -#define LM_MISR_CTRL_ENABLE BIT(8)
> -#define LM_MISR_CTRL_STATUS BIT(9)
> -#define LM_MISR_CTRL_STATUS_CLEAR BIT(10)
> -#define LM_MISR_CTRL_FREE_RUN_MASK BIT(31)
>
>
> static const struct dpu_lm_cfg *_lm_offset(enum dpu_lm mixer,
> @@ -108,44 +104,12 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx,
>
> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count)
> {
> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
> - u32 config = 0;
> -
> - DPU_REG_WRITE(c, LM_MISR_CTRL, LM_MISR_CTRL_STATUS_CLEAR);
> -
> - /* Clear old MISR value (in case it's read before a new value is calculated)*/
> - wmb();
> -
> - if (enable) {
> - config = (frame_count & LM_MISR_FRAME_COUNT_MASK) |
> - LM_MISR_CTRL_ENABLE | LM_MISR_CTRL_FREE_RUN_MASK;
> -
> - DPU_REG_WRITE(c, LM_MISR_CTRL, config);
> - } else {
> - DPU_REG_WRITE(c, LM_MISR_CTRL, 0);
> - }
> -
> + dpu_hw_setup_misr(&ctx->hw, enable, frame_count, LM_MISR_CTRL);
> }
>
> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value)
> {
> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
> - u32 ctrl = 0;
> -
> - if (!misr_value)
> - return -EINVAL;
> -
> - ctrl = DPU_REG_READ(c, LM_MISR_CTRL);
> -
> - if (!(ctrl & LM_MISR_CTRL_ENABLE))
> - return -ENODATA;
> -
> - if (!(ctrl & LM_MISR_CTRL_STATUS))
> - return -EINVAL;
> -
> - *misr_value = DPU_REG_READ(c, LM_MISR_SIGNATURE);
> -
> - return 0;
> + return dpu_hw_collect_misr(&ctx->hw, misr_value, LM_MISR_CTRL, LM_MISR_SIGNATURE);
> }
>
> static void dpu_hw_lm_setup_blend_config_sdm845(struct dpu_hw_mixer *ctx,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> index 512316f25a51..244f2f90e99a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> */
> #define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
>
> @@ -447,3 +449,45 @@ u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>
> return 0;
> }
> +
> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, bool enable,
> + u32 frame_count, u32 misr_ctrl_offset)
> +{
> + u32 config = 0;
> +
> + DPU_REG_WRITE(c, misr_ctrl_offset, MISR_CTRL_STATUS_CLEAR);
> +
> + /* Clear old MISR value (in case it's read before a new value is calculated)*/
> + wmb();
> +
> + if (enable) {
> + config = (frame_count & MISR_FRAME_COUNT_MASK) |
> + MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK;
> +
> + DPU_REG_WRITE(c, misr_ctrl_offset, config);
> + } else {
> + DPU_REG_WRITE(c, misr_ctrl_offset, 0);
> + }
> +
> +}
> +
> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, u32 *misr_value,
> + u32 misr_ctrl_offset, u32 misr_signature_offset)
> +{
> + u32 ctrl = 0;
> +
> + if (!misr_value)
> + return -EINVAL;
> +
> + ctrl = DPU_REG_READ(c, misr_ctrl_offset);
> +
> + if (!(ctrl & MISR_CTRL_ENABLE))
> + return -ENODATA;
> +
> + if (!(ctrl & MISR_CTRL_STATUS))
> + return -EINVAL;
> +
> + *misr_value = DPU_REG_READ(c, misr_signature_offset);
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> index e4a65eb4f769..88df3a5c5d8e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
> @@ -1,5 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> */
>
> @@ -12,6 +13,11 @@
> #include "dpu_hw_catalog.h"
>
> #define REG_MASK(n) ((BIT(n)) - 1)
> +#define MISR_FRAME_COUNT_MASK 0xFF
> +#define MISR_CTRL_ENABLE BIT(8)
> +#define MISR_CTRL_STATUS BIT(9)
> +#define MISR_CTRL_STATUS_CLEAR BIT(10)
> +#define MISR_CTRL_FREE_RUN_MASK BIT(31)
>
> /*
> * This is the common struct maintained by each sub block
> @@ -343,4 +349,14 @@ void dpu_hw_csc_setup(struct dpu_hw_blk_reg_map *c,
> u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
> u32 total_fl);
>
> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c,
> + bool enable,
> + u32 frame_count,
> + u32 misr_ctrl_offset);
> +
> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c,
> + u32 *misr_value,
> + u32 misr_ctrl_offset,
> + u32 misr_signature_offset);
Please move offsets next to the hw_blk_reg_map.
> +
> #endif /* _DPU_HW_UTIL_H */
> --
> 2.35.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] drm/msm/dpu: Add MISR register support for interface
2022-06-14 21:13 ` Jessica Zhang
@ 2022-06-15 9:36 ` Dmitry Baryshkov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-06-15 9:36 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
quic_aravindh, freedreno
On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Add support for setting MISR registers within the interface
>
> Changes since V1:
> - Replaced dpu_hw_intf collect_misr and setup_misr implementations with
> calls to dpu_hw_utils helper methods
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 19 ++++++++++++++++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 +++++++-
> 2 files changed, 25 insertions(+), 2 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 3f4d2c6e1b45..0157613224fd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> */
>
> #include "dpu_hwio.h"
> @@ -67,6 +69,9 @@
> #define INTF_CFG2_DATABUS_WIDEN BIT(0)
> #define INTF_CFG2_DATA_HCTL_EN BIT(4)
>
> +#define INTF_MISR_CTRL 0x180
> +#define INTF_MISR_SIGNATURE 0x184
> +
> static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
> const struct dpu_mdss_cfg *m,
> void __iomem *addr,
> @@ -319,6 +324,16 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf)
> return DPU_REG_READ(c, INTF_LINE_COUNT);
> }
>
> +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count)
> +{
> + dpu_hw_setup_misr(&intf->hw, enable, frame_count, INTF_MISR_CTRL);
> +}
> +
> +static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value)
> +{
> + return dpu_hw_collect_misr(&intf->hw, misr_value, INTF_MISR_CTRL, INTF_MISR_SIGNATURE);
> +}
> +
> static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> unsigned long cap)
> {
> @@ -329,6 +344,8 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> ops->get_line_count = dpu_hw_intf_get_line_count;
> if (cap & BIT(DPU_INTF_INPUT_CTRL))
> ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> + ops->setup_misr = dpu_hw_intf_setup_misr;
> + ops->collect_misr = dpu_hw_intf_collect_misr;
> }
>
> struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
> 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 7b2d96ac61e8..8d0e7b509260 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -1,5 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> */
>
> #ifndef _DPU_HW_INTF_H
> @@ -57,6 +59,8 @@ struct intf_status {
> * @ get_line_count: reads current vertical line counter
> * @bind_pingpong_blk: enable/disable the connection with pingpong which will
> * feed pixels to this interface
> + * @setup_misr: enable/disable MISR
> + * @collect_misr: read MISR signature
> */
> struct dpu_hw_intf_ops {
> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
> @@ -77,6 +81,8 @@ struct dpu_hw_intf_ops {
> void (*bind_pingpong_blk)(struct dpu_hw_intf *intf,
> bool enable,
> const enum dpu_pingpong pp);
> + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count);
> + int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
> };
>
> struct dpu_hw_intf {
> --
> 2.35.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/3] drm/msm/dpu: Add MISR register support for interface
@ 2022-06-15 9:36 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-06-15 9:36 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
quic_aravindh, quic_abhinavk
On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Add support for setting MISR registers within the interface
>
> Changes since V1:
> - Replaced dpu_hw_intf collect_misr and setup_misr implementations with
> calls to dpu_hw_utils helper methods
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 19 ++++++++++++++++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 +++++++-
> 2 files changed, 25 insertions(+), 2 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 3f4d2c6e1b45..0157613224fd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> */
>
> #include "dpu_hwio.h"
> @@ -67,6 +69,9 @@
> #define INTF_CFG2_DATABUS_WIDEN BIT(0)
> #define INTF_CFG2_DATA_HCTL_EN BIT(4)
>
> +#define INTF_MISR_CTRL 0x180
> +#define INTF_MISR_SIGNATURE 0x184
> +
> static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
> const struct dpu_mdss_cfg *m,
> void __iomem *addr,
> @@ -319,6 +324,16 @@ static u32 dpu_hw_intf_get_line_count(struct dpu_hw_intf *intf)
> return DPU_REG_READ(c, INTF_LINE_COUNT);
> }
>
> +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool enable, u32 frame_count)
> +{
> + dpu_hw_setup_misr(&intf->hw, enable, frame_count, INTF_MISR_CTRL);
> +}
> +
> +static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value)
> +{
> + return dpu_hw_collect_misr(&intf->hw, misr_value, INTF_MISR_CTRL, INTF_MISR_SIGNATURE);
> +}
> +
> static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> unsigned long cap)
> {
> @@ -329,6 +344,8 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> ops->get_line_count = dpu_hw_intf_get_line_count;
> if (cap & BIT(DPU_INTF_INPUT_CTRL))
> ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> + ops->setup_misr = dpu_hw_intf_setup_misr;
> + ops->collect_misr = dpu_hw_intf_collect_misr;
> }
>
> struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
> 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 7b2d96ac61e8..8d0e7b509260 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -1,5 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> */
>
> #ifndef _DPU_HW_INTF_H
> @@ -57,6 +59,8 @@ struct intf_status {
> * @ get_line_count: reads current vertical line counter
> * @bind_pingpong_blk: enable/disable the connection with pingpong which will
> * feed pixels to this interface
> + * @setup_misr: enable/disable MISR
> + * @collect_misr: read MISR signature
> */
> struct dpu_hw_intf_ops {
> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
> @@ -77,6 +81,8 @@ struct dpu_hw_intf_ops {
> void (*bind_pingpong_blk)(struct dpu_hw_intf *intf,
> bool enable,
> const enum dpu_pingpong pp);
> + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count);
> + int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
> };
>
> struct dpu_hw_intf {
> --
> 2.35.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/3] drm/msm/dpu: Add interface support for CRC debugfs
2022-06-14 21:13 ` Jessica Zhang
@ 2022-06-15 9:44 ` Dmitry Baryshkov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-06-15 9:44 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
quic_aravindh, quic_abhinavk
On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Add support for writing CRC values for the interface block to
> the debugfs by calling the necessary MISR setup/collect methods.
>
> Changes since V1:
> - Set values_cnt to only include phys with backing hw_intf
> - Loop over all drm_encs connected to crtc
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 49 ++++++++++++++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 64 +++++++++++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 +++++++
> 4 files changed, 134 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 16742a66878e..8c9933b2337f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source dpu_crtc_parse_crc_source(const char *src_name)
> if (!strcmp(src_name, "auto") ||
> !strcmp(src_name, "lm"))
> return DPU_CRTC_CRC_SOURCE_LAYER_MIXER;
> + if (!strcmp(src_name, "intf"))
> + return DPU_CRTC_CRC_SOURCE_INTF;
What about "encoder" / DPU_CRTC_CRC_SOURCE_ENCODER? You basically
offload CRC generation/collection to the dpu_encoder, so I'd ignore
the fact that only INTF's support MISR generation and use a more
generic word here.
>
> return DPU_CRTC_CRC_SOURCE_INVALID;
> }
> @@ -99,8 +101,14 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
> return -EINVAL;
> }
>
> - if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) {
> *values_cnt = crtc_state->num_mixers;
> + } else if (source == DPU_CRTC_CRC_SOURCE_INTF) {
> + struct drm_encoder *drm_enc;
Zero values_cnt here.
> +
> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
> + *values_cnt += dpu_encoder_get_num_phys(drm_enc);
> + }
>
> crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
>
> @@ -123,6 +131,14 @@ static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
> }
> }
>
> +static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc)
> +{
> + struct drm_encoder *drm_enc;
> +
> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
> + dpu_encoder_setup_misr(drm_enc);
> +}
> +
> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
> {
> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
> @@ -175,6 +191,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>
> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> dpu_crtc_setup_lm_misr(crtc_state);
> + else if (source == DPU_CRTC_CRC_SOURCE_INTF)
> + dpu_crtc_setup_encoder_misr(crtc);
else?
>
> cleanup:
> drm_modeset_unlock(&crtc->mutex);
> @@ -220,11 +238,31 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt
> drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
> }
>
> -static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc)
> {
> - struct dpu_crtc_state *crtc_state;
> + struct drm_encoder *drm_enc;
> + struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
> + int rc, pos = 0;
>
> - crtc_state = to_dpu_crtc_state(crtc->state);
> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) {
> + rc = dpu_encoder_get_crc(drm_enc, crtc_state->crcs, pos);
> + if (rc < 0) {
> + if (rc != -ENODATA)
> + DRM_DEBUG_DRIVER("MISR read failed\n");
> +
> + return rc;
> + }
> +
> + pos += rc;
> + }
> +
> + return drm_crtc_add_crc_entry(crtc, true,
> + drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
> +}
> +
> +static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> +{
> + struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
Unnecessary change here. Please move it to the patch 1, which
refactors this function.
>
> /* Skip first 2 frames in case of "uncooked" CRCs */
> if (crtc_state->crc_frame_skip_count < 2) {
> @@ -235,6 +273,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> return dpu_crtc_get_lm_crc(crtc, crtc_state);
>
> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF)
> + return dpu_crtc_get_encoder_crc(crtc);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 4bf45e3343ef..5db84ea796db 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -1,5 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2015-2021 The Linux Foundation. All rights reserved.
> * Copyright (C) 2013 Red Hat
> * Author: Rob Clark <robdclark@gmail.com>
> @@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data {
> * enum dpu_crtc_crc_source: CRC source
> * @DPU_CRTC_CRC_SOURCE_NONE: no source set
> * @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer
> + * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface
> * @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source
> */
> enum dpu_crtc_crc_source {
> DPU_CRTC_CRC_SOURCE_NONE = 0,
> DPU_CRTC_CRC_SOURCE_LAYER_MIXER,
> + DPU_CRTC_CRC_SOURCE_INTF,
> DPU_CRTC_CRC_SOURCE_MAX,
> DPU_CRTC_CRC_SOURCE_INVALID = -1
> };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 52516eb20cb8..2cbfed5c627e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -14,6 +14,7 @@
>
> #include <drm/drm_crtc.h>
> #include <drm/drm_file.h>
> +#include <drm/drm_vblank.h>
Why?
> #include <drm/drm_probe_helper.h>
>
> #include "msm_drv.h"
> @@ -225,6 +226,69 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
> return dpu_enc->wide_bus_en;
> }
>
> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc)
The function name is misleading. It doesn't return the number of phys.
It returns a number of hw_intfs. And in reality you'd like to get the
number of crc entries supported. If at some point WB (or any other
possible dpu_encoder backend) gains support for CRC, we won't have to
change the name. Please consider adjusting it.
> +{
> + struct dpu_encoder_virt *dpu_enc;
> + int i, num_intf = 0;
> +
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> + if (phys->hw_intf)
> + num_intf++;
You have to check for hw_intf->ops.setup_misr too.
> + }
> +
> + return num_intf;
> +}
> +
> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc)
> +{
> + struct dpu_encoder_virt *dpu_enc;
> +
> + int i;
> +
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> + if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr)
> + continue;
> +
> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);
> + }
> +}
> +
> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos)
> +{
> + struct dpu_encoder_virt *dpu_enc;
> +
> + int i, rc = 0, entries_added = 0;
> +
> + if (!drm_enc->crtc) {
> + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index);
> + return -EINVAL;
> + }
> +
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr)
> + continue;
> +
> + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[pos + i]);
No, you should be indexing the crcs with entries_added rather than i.
> + if (rc)
> + return rc;
> + entries_added++;
> + }
> +
> + return entries_added;
> +}
> +
> static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
> {
> struct dpu_hw_dither_cfg dither_cfg = { 0 };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 781d41c91994..375370029cb9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -1,5 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> * Copyright (C) 2013 Red Hat
> * Author: Rob Clark <robdclark@gmail.com>
> @@ -174,6 +175,27 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc);
>
> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc);
>
> +/**
> + * dpu_encoder_get_num_phys - get number of physical encoders contained in virtual
> + * encoder
> + * @drm_enc: Pointer to previously created drm encoder structure
> + * Returns: Number of physical encoders for given drm encoder
> + */
> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc);
> +
> +/**
> + * dpu_encoder_setup_misr - enable misr calculations
> + * @drm_enc: Pointer to previously created drm encoder structure
> + */
> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder);
> +
> +/**
> + * dpu_encoder_get_crc - get the crc value from interface blocks
> + * @drm_enc: Pointer to previously created drm encoder structure
> + * Returns: 0 on success, error otherwise
> + */
> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos);
> +
> /**
> * dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology.
> * @drm_enc: Pointer to previously created drm encoder structure
> --
> 2.35.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/3] drm/msm/dpu: Add interface support for CRC debugfs
@ 2022-06-15 9:44 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-06-15 9:44 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
quic_aravindh, freedreno
On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
> Add support for writing CRC values for the interface block to
> the debugfs by calling the necessary MISR setup/collect methods.
>
> Changes since V1:
> - Set values_cnt to only include phys with backing hw_intf
> - Loop over all drm_encs connected to crtc
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 49 ++++++++++++++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 64 +++++++++++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 +++++++
> 4 files changed, 134 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 16742a66878e..8c9933b2337f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source dpu_crtc_parse_crc_source(const char *src_name)
> if (!strcmp(src_name, "auto") ||
> !strcmp(src_name, "lm"))
> return DPU_CRTC_CRC_SOURCE_LAYER_MIXER;
> + if (!strcmp(src_name, "intf"))
> + return DPU_CRTC_CRC_SOURCE_INTF;
What about "encoder" / DPU_CRTC_CRC_SOURCE_ENCODER? You basically
offload CRC generation/collection to the dpu_encoder, so I'd ignore
the fact that only INTF's support MISR generation and use a more
generic word here.
>
> return DPU_CRTC_CRC_SOURCE_INVALID;
> }
> @@ -99,8 +101,14 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
> return -EINVAL;
> }
>
> - if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) {
> *values_cnt = crtc_state->num_mixers;
> + } else if (source == DPU_CRTC_CRC_SOURCE_INTF) {
> + struct drm_encoder *drm_enc;
Zero values_cnt here.
> +
> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
> + *values_cnt += dpu_encoder_get_num_phys(drm_enc);
> + }
>
> crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
>
> @@ -123,6 +131,14 @@ static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
> }
> }
>
> +static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc)
> +{
> + struct drm_encoder *drm_enc;
> +
> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
> + dpu_encoder_setup_misr(drm_enc);
> +}
> +
> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
> {
> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
> @@ -175,6 +191,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>
> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> dpu_crtc_setup_lm_misr(crtc_state);
> + else if (source == DPU_CRTC_CRC_SOURCE_INTF)
> + dpu_crtc_setup_encoder_misr(crtc);
else?
>
> cleanup:
> drm_modeset_unlock(&crtc->mutex);
> @@ -220,11 +238,31 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt
> drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
> }
>
> -static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc)
> {
> - struct dpu_crtc_state *crtc_state;
> + struct drm_encoder *drm_enc;
> + struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
> + int rc, pos = 0;
>
> - crtc_state = to_dpu_crtc_state(crtc->state);
> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) {
> + rc = dpu_encoder_get_crc(drm_enc, crtc_state->crcs, pos);
> + if (rc < 0) {
> + if (rc != -ENODATA)
> + DRM_DEBUG_DRIVER("MISR read failed\n");
> +
> + return rc;
> + }
> +
> + pos += rc;
> + }
> +
> + return drm_crtc_add_crc_entry(crtc, true,
> + drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
> +}
> +
> +static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> +{
> + struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
Unnecessary change here. Please move it to the patch 1, which
refactors this function.
>
> /* Skip first 2 frames in case of "uncooked" CRCs */
> if (crtc_state->crc_frame_skip_count < 2) {
> @@ -235,6 +273,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
> if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
> return dpu_crtc_get_lm_crc(crtc, crtc_state);
>
> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF)
> + return dpu_crtc_get_encoder_crc(crtc);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 4bf45e3343ef..5db84ea796db 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -1,5 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2015-2021 The Linux Foundation. All rights reserved.
> * Copyright (C) 2013 Red Hat
> * Author: Rob Clark <robdclark@gmail.com>
> @@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data {
> * enum dpu_crtc_crc_source: CRC source
> * @DPU_CRTC_CRC_SOURCE_NONE: no source set
> * @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer
> + * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface
> * @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source
> */
> enum dpu_crtc_crc_source {
> DPU_CRTC_CRC_SOURCE_NONE = 0,
> DPU_CRTC_CRC_SOURCE_LAYER_MIXER,
> + DPU_CRTC_CRC_SOURCE_INTF,
> DPU_CRTC_CRC_SOURCE_MAX,
> DPU_CRTC_CRC_SOURCE_INVALID = -1
> };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 52516eb20cb8..2cbfed5c627e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -14,6 +14,7 @@
>
> #include <drm/drm_crtc.h>
> #include <drm/drm_file.h>
> +#include <drm/drm_vblank.h>
Why?
> #include <drm/drm_probe_helper.h>
>
> #include "msm_drv.h"
> @@ -225,6 +226,69 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
> return dpu_enc->wide_bus_en;
> }
>
> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc)
The function name is misleading. It doesn't return the number of phys.
It returns a number of hw_intfs. And in reality you'd like to get the
number of crc entries supported. If at some point WB (or any other
possible dpu_encoder backend) gains support for CRC, we won't have to
change the name. Please consider adjusting it.
> +{
> + struct dpu_encoder_virt *dpu_enc;
> + int i, num_intf = 0;
> +
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> + if (phys->hw_intf)
> + num_intf++;
You have to check for hw_intf->ops.setup_misr too.
> + }
> +
> + return num_intf;
> +}
> +
> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc)
> +{
> + struct dpu_encoder_virt *dpu_enc;
> +
> + int i;
> +
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> + if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr)
> + continue;
> +
> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);
> + }
> +}
> +
> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos)
> +{
> + struct dpu_encoder_virt *dpu_enc;
> +
> + int i, rc = 0, entries_added = 0;
> +
> + if (!drm_enc->crtc) {
> + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index);
> + return -EINVAL;
> + }
> +
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +
> + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr)
> + continue;
> +
> + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[pos + i]);
No, you should be indexing the crcs with entries_added rather than i.
> + if (rc)
> + return rc;
> + entries_added++;
> + }
> +
> + return entries_added;
> +}
> +
> static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
> {
> struct dpu_hw_dither_cfg dither_cfg = { 0 };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 781d41c91994..375370029cb9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -1,5 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> /*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> * Copyright (C) 2013 Red Hat
> * Author: Rob Clark <robdclark@gmail.com>
> @@ -174,6 +175,27 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc);
>
> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc);
>
> +/**
> + * dpu_encoder_get_num_phys - get number of physical encoders contained in virtual
> + * encoder
> + * @drm_enc: Pointer to previously created drm encoder structure
> + * Returns: Number of physical encoders for given drm encoder
> + */
> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc);
> +
> +/**
> + * dpu_encoder_setup_misr - enable misr calculations
> + * @drm_enc: Pointer to previously created drm encoder structure
> + */
> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder);
> +
> +/**
> + * dpu_encoder_get_crc - get the crc value from interface blocks
> + * @drm_enc: Pointer to previously created drm encoder structure
> + * Returns: 0 on success, error otherwise
> + */
> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos);
> +
> /**
> * dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology.
> * @drm_enc: Pointer to previously created drm encoder structure
> --
> 2.35.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
2022-06-15 9:35 ` Dmitry Baryshkov
@ 2022-06-15 16:11 ` Jessica Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-15 16:11 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
quic_aravindh, quic_abhinavk
On 6/15/2022 2:35 AM, Dmitry Baryshkov wrote:
> On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>> Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
>> helper method. This way, we can make it easier to get CRCs from other HW
>> blocks by adding other get_crc helper methods.
>>
>> Changes since V1:
>> - Moved common bitmasks to dpu_hw_util.h
>> - Moved common CRC methods to dpu_hw_util.c
>> - Updated copyrights
>> - Changed crcs array to a dynamically allocated array and added it as a
>> member of crtc_state
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>
> Please split this into separate patches. One for hw_util, one for the rest
Sure
>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88 +++++++++++++--------
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
>> 5 files changed, 124 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index b56f777dbd0e..16742a66878e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
>> * Copyright (C) 2013 Red Hat
>> * Author: Rob Clark <robdclark@gmail.com>
>> @@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
>> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
>> struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
>>
>> + if (crtc_state->crcs) {
>> + kfree(crtc_state->crcs);
>> + crtc_state->crcs = NULL;
>> + }
>> +
>> if (source < 0) {
>> DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n", src_name, crtc->index);
>> return -EINVAL;
>> @@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> *values_cnt = crtc_state->num_mixers;
>>
>> + crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
>> +
>
> I do not quite like the idea of constantly allocating and freeing the
> crcs array. I'd suggest defining sensible MAX_CRC_VALUES, using a
> static array and verifying that no one over commits the
> MAX_CRC_VALUES.
> If at some point we decide that we really need the dynamic array, we
> can implement it later. We already had dynamic arrays and Rob had to
> fix it. See 00326bfa4e63 ("drm/msm/dpu: Remove dynamic allocation from
> atomic context").
Ah, got it... the reason I used a dynamic array here was because AFAIK
we don't have a defined upper limit for how many drm_encoders can be
connected to a CRTC simultaneously. Do you have a suggestion on what
value we can set for MAX_CRC_VALUES?
>
>> return 0;
>> }
>>
>> +static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
>> +{
>> + struct dpu_crtc_mixer *m;
>> + int i;
>> +
>> + for (i = 0; i < crtc_state->num_mixers; ++i) {
>> + m = &crtc_state->mixers[i];
>> +
>> + if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
>> + continue;
>> +
>> + /* Calculate MISR over 1 frame */
>> + m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
>> + }
>> +}
>> +
>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>> {
>> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
>> enum dpu_crtc_crc_source current_source;
>> struct dpu_crtc_state *crtc_state;
>> struct drm_device *drm_dev = crtc->dev;
>> - struct dpu_crtc_mixer *m;
>>
>> bool was_enabled;
>> bool enable = false;
>> - int i, ret = 0;
>> + int ret = 0;
>>
>> if (source < 0) {
>> DRM_DEBUG_DRIVER("Invalid CRC source %s for CRTC%d\n", src_name, crtc->index);
>> @@ -137,6 +160,10 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>> goto cleanup;
>>
>> } else if (was_enabled && !enable) {
>> + if (crtc_state->crcs) {
>> + kfree(crtc_state->crcs);
>> + crtc_state->crcs = NULL;
>> + }
>> drm_crtc_vblank_put(crtc);
>> }
>>
>> @@ -146,16 +173,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>>
>> crtc_state->crc_frame_skip_count = 0;
>>
>> - for (i = 0; i < crtc_state->num_mixers; ++i) {
>> - m = &crtc_state->mixers[i];
>> -
>> - if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
>> - continue;
>> -
>> - /* Calculate MISR over 1 frame */
>> - m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
>> - }
>> -
>> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> + dpu_crtc_setup_lm_misr(crtc_state);
>
> else ?
Noted.
>
>>
>> cleanup:
>> drm_modeset_unlock(&crtc->mutex);
>> @@ -174,34 +193,21 @@ static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
>> return dpu_encoder_get_vsync_count(encoder);
>> }
>>
>> -
>> -static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crtc_state)
>> {
>> - struct dpu_crtc_state *crtc_state;
>> - struct dpu_crtc_mixer *m;
>> - u32 crcs[CRTC_DUAL_MIXERS];
>> + struct dpu_crtc_mixer *lm;
>>
>> - int i = 0;
>> int rc = 0;
>> -
>> - crtc_state = to_dpu_crtc_state(crtc->state);
>> -
>> - BUILD_BUG_ON(ARRAY_SIZE(crcs) != ARRAY_SIZE(crtc_state->mixers));
>> -
>> - /* Skip first 2 frames in case of "uncooked" CRCs */
>> - if (crtc_state->crc_frame_skip_count < 2) {
>> - crtc_state->crc_frame_skip_count++;
>> - return 0;
>> - }
>> + int i;
>>
>> for (i = 0; i < crtc_state->num_mixers; ++i) {
>>
>> - m = &crtc_state->mixers[i];
>> + lm = &crtc_state->mixers[i];
>>
>> - if (!m->hw_lm || !m->hw_lm->ops.collect_misr)
>> + if (!lm->hw_lm || !lm->hw_lm->ops.collect_misr)
>> continue;
>>
>> - rc = m->hw_lm->ops.collect_misr(m->hw_lm, &crcs[i]);
>> + rc = lm->hw_lm->ops.collect_misr(lm->hw_lm, &crtc_state->crcs[i]);
>>
>> if (rc) {
>> if (rc != -ENODATA)
>> @@ -211,7 +217,25 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> }
>>
>> return drm_crtc_add_crc_entry(crtc, true,
>> - drm_crtc_accurate_vblank_count(crtc), crcs);
>> + drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
>> +}
>> +
>> +static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +{
>> + struct dpu_crtc_state *crtc_state;
>> +
>> + crtc_state = to_dpu_crtc_state(crtc->state);
>> +
>> + /* Skip first 2 frames in case of "uncooked" CRCs */
>> + if (crtc_state->crc_frame_skip_count < 2) {
>> + crtc_state->crc_frame_skip_count++;
>> + return 0;
>> + }
>> +
>> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> + return dpu_crtc_get_lm_crc(crtc, crtc_state);
>> +
>> + return 0;
>> }
>>
>> static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index b8785c394fcc..4bf45e3343ef 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -201,6 +201,9 @@ struct dpu_crtc {
>> * @mixers : List of active mixers
>> * @num_ctls : Number of ctl paths in use
>> * @hw_ctls : List of active ctl paths
>> + * @crc_source : CRC source
>> + * @crc_frame_skip_count: Number of frames skipped before getting CRC
>> + * @crcs : Array to store CRC values
>> */
>> struct dpu_crtc_state {
>> struct drm_crtc_state base;
>> @@ -222,6 +225,7 @@ struct dpu_crtc_state {
>>
>> enum dpu_crtc_crc_source crc_source;
>> int crc_frame_skip_count;
>> + u32 *crcs;
>> };
>>
>> #define to_dpu_crtc_state(x) \
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>> index 462f5082099e..06b24d8d1419 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>> */
>>
>> @@ -27,11 +28,6 @@
>>
>> #define LM_MISR_CTRL 0x310
>> #define LM_MISR_SIGNATURE 0x314
>> -#define LM_MISR_FRAME_COUNT_MASK 0xFF
>> -#define LM_MISR_CTRL_ENABLE BIT(8)
>> -#define LM_MISR_CTRL_STATUS BIT(9)
>> -#define LM_MISR_CTRL_STATUS_CLEAR BIT(10)
>> -#define LM_MISR_CTRL_FREE_RUN_MASK BIT(31)
>>
>>
>> static const struct dpu_lm_cfg *_lm_offset(enum dpu_lm mixer,
>> @@ -108,44 +104,12 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx,
>>
>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count)
>> {
>> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
>> - u32 config = 0;
>> -
>> - DPU_REG_WRITE(c, LM_MISR_CTRL, LM_MISR_CTRL_STATUS_CLEAR);
>> -
>> - /* Clear old MISR value (in case it's read before a new value is calculated)*/
>> - wmb();
>> -
>> - if (enable) {
>> - config = (frame_count & LM_MISR_FRAME_COUNT_MASK) |
>> - LM_MISR_CTRL_ENABLE | LM_MISR_CTRL_FREE_RUN_MASK;
>> -
>> - DPU_REG_WRITE(c, LM_MISR_CTRL, config);
>> - } else {
>> - DPU_REG_WRITE(c, LM_MISR_CTRL, 0);
>> - }
>> -
>> + dpu_hw_setup_misr(&ctx->hw, enable, frame_count, LM_MISR_CTRL);
>> }
>>
>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value)
>> {
>> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
>> - u32 ctrl = 0;
>> -
>> - if (!misr_value)
>> - return -EINVAL;
>> -
>> - ctrl = DPU_REG_READ(c, LM_MISR_CTRL);
>> -
>> - if (!(ctrl & LM_MISR_CTRL_ENABLE))
>> - return -ENODATA;
>> -
>> - if (!(ctrl & LM_MISR_CTRL_STATUS))
>> - return -EINVAL;
>> -
>> - *misr_value = DPU_REG_READ(c, LM_MISR_SIGNATURE);
>> -
>> - return 0;
>> + return dpu_hw_collect_misr(&ctx->hw, misr_value, LM_MISR_CTRL, LM_MISR_SIGNATURE);
>> }
>>
>> static void dpu_hw_lm_setup_blend_config_sdm845(struct dpu_hw_mixer *ctx,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> index 512316f25a51..244f2f90e99a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> @@ -1,5 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> */
>> #define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
>>
>> @@ -447,3 +449,45 @@ u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>>
>> return 0;
>> }
>> +
>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, bool enable,
>> + u32 frame_count, u32 misr_ctrl_offset)
>> +{
>> + u32 config = 0;
>> +
>> + DPU_REG_WRITE(c, misr_ctrl_offset, MISR_CTRL_STATUS_CLEAR);
>> +
>> + /* Clear old MISR value (in case it's read before a new value is calculated)*/
>> + wmb();
>> +
>> + if (enable) {
>> + config = (frame_count & MISR_FRAME_COUNT_MASK) |
>> + MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK;
>> +
>> + DPU_REG_WRITE(c, misr_ctrl_offset, config);
>> + } else {
>> + DPU_REG_WRITE(c, misr_ctrl_offset, 0);
>> + }
>> +
>> +}
>> +
>> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, u32 *misr_value,
>> + u32 misr_ctrl_offset, u32 misr_signature_offset)
>> +{
>> + u32 ctrl = 0;
>> +
>> + if (!misr_value)
>> + return -EINVAL;
>> +
>> + ctrl = DPU_REG_READ(c, misr_ctrl_offset);
>> +
>> + if (!(ctrl & MISR_CTRL_ENABLE))
>> + return -ENODATA;
>> +
>> + if (!(ctrl & MISR_CTRL_STATUS))
>> + return -EINVAL;
>> +
>> + *misr_value = DPU_REG_READ(c, misr_signature_offset);
>> +
>> + return 0;
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> index e4a65eb4f769..88df3a5c5d8e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> @@ -1,5 +1,6 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>> */
>>
>> @@ -12,6 +13,11 @@
>> #include "dpu_hw_catalog.h"
>>
>> #define REG_MASK(n) ((BIT(n)) - 1)
>> +#define MISR_FRAME_COUNT_MASK 0xFF
>> +#define MISR_CTRL_ENABLE BIT(8)
>> +#define MISR_CTRL_STATUS BIT(9)
>> +#define MISR_CTRL_STATUS_CLEAR BIT(10)
>> +#define MISR_CTRL_FREE_RUN_MASK BIT(31)
>>
>> /*
>> * This is the common struct maintained by each sub block
>> @@ -343,4 +349,14 @@ void dpu_hw_csc_setup(struct dpu_hw_blk_reg_map *c,
>> u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>> u32 total_fl);
>>
>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c,
>> + bool enable,
>> + u32 frame_count,
>> + u32 misr_ctrl_offset);
>> +
>> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c,
>> + u32 *misr_value,
>> + u32 misr_ctrl_offset,
>> + u32 misr_signature_offset);
>
> Please move offsets next to the hw_blk_reg_map.
Noted.
Thanks,
Jessica Zhang
>
>> +
>> #endif /* _DPU_HW_UTIL_H */
>> --
>> 2.35.1
>>
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
@ 2022-06-15 16:11 ` Jessica Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-15 16:11 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
quic_aravindh, freedreno
On 6/15/2022 2:35 AM, Dmitry Baryshkov wrote:
> On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>> Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
>> helper method. This way, we can make it easier to get CRCs from other HW
>> blocks by adding other get_crc helper methods.
>>
>> Changes since V1:
>> - Moved common bitmasks to dpu_hw_util.h
>> - Moved common CRC methods to dpu_hw_util.c
>> - Updated copyrights
>> - Changed crcs array to a dynamically allocated array and added it as a
>> member of crtc_state
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>
> Please split this into separate patches. One for hw_util, one for the rest
Sure
>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88 +++++++++++++--------
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
>> 5 files changed, 124 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index b56f777dbd0e..16742a66878e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
>> * Copyright (C) 2013 Red Hat
>> * Author: Rob Clark <robdclark@gmail.com>
>> @@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
>> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
>> struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
>>
>> + if (crtc_state->crcs) {
>> + kfree(crtc_state->crcs);
>> + crtc_state->crcs = NULL;
>> + }
>> +
>> if (source < 0) {
>> DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n", src_name, crtc->index);
>> return -EINVAL;
>> @@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> *values_cnt = crtc_state->num_mixers;
>>
>> + crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
>> +
>
> I do not quite like the idea of constantly allocating and freeing the
> crcs array. I'd suggest defining sensible MAX_CRC_VALUES, using a
> static array and verifying that no one over commits the
> MAX_CRC_VALUES.
> If at some point we decide that we really need the dynamic array, we
> can implement it later. We already had dynamic arrays and Rob had to
> fix it. See 00326bfa4e63 ("drm/msm/dpu: Remove dynamic allocation from
> atomic context").
Ah, got it... the reason I used a dynamic array here was because AFAIK
we don't have a defined upper limit for how many drm_encoders can be
connected to a CRTC simultaneously. Do you have a suggestion on what
value we can set for MAX_CRC_VALUES?
>
>> return 0;
>> }
>>
>> +static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
>> +{
>> + struct dpu_crtc_mixer *m;
>> + int i;
>> +
>> + for (i = 0; i < crtc_state->num_mixers; ++i) {
>> + m = &crtc_state->mixers[i];
>> +
>> + if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
>> + continue;
>> +
>> + /* Calculate MISR over 1 frame */
>> + m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
>> + }
>> +}
>> +
>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>> {
>> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
>> enum dpu_crtc_crc_source current_source;
>> struct dpu_crtc_state *crtc_state;
>> struct drm_device *drm_dev = crtc->dev;
>> - struct dpu_crtc_mixer *m;
>>
>> bool was_enabled;
>> bool enable = false;
>> - int i, ret = 0;
>> + int ret = 0;
>>
>> if (source < 0) {
>> DRM_DEBUG_DRIVER("Invalid CRC source %s for CRTC%d\n", src_name, crtc->index);
>> @@ -137,6 +160,10 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>> goto cleanup;
>>
>> } else if (was_enabled && !enable) {
>> + if (crtc_state->crcs) {
>> + kfree(crtc_state->crcs);
>> + crtc_state->crcs = NULL;
>> + }
>> drm_crtc_vblank_put(crtc);
>> }
>>
>> @@ -146,16 +173,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>>
>> crtc_state->crc_frame_skip_count = 0;
>>
>> - for (i = 0; i < crtc_state->num_mixers; ++i) {
>> - m = &crtc_state->mixers[i];
>> -
>> - if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
>> - continue;
>> -
>> - /* Calculate MISR over 1 frame */
>> - m->hw_lm->ops.setup_misr(m->hw_lm, true, 1);
>> - }
>> -
>> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> + dpu_crtc_setup_lm_misr(crtc_state);
>
> else ?
Noted.
>
>>
>> cleanup:
>> drm_modeset_unlock(&crtc->mutex);
>> @@ -174,34 +193,21 @@ static u32 dpu_crtc_get_vblank_counter(struct drm_crtc *crtc)
>> return dpu_encoder_get_vsync_count(encoder);
>> }
>>
>> -
>> -static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crtc_state)
>> {
>> - struct dpu_crtc_state *crtc_state;
>> - struct dpu_crtc_mixer *m;
>> - u32 crcs[CRTC_DUAL_MIXERS];
>> + struct dpu_crtc_mixer *lm;
>>
>> - int i = 0;
>> int rc = 0;
>> -
>> - crtc_state = to_dpu_crtc_state(crtc->state);
>> -
>> - BUILD_BUG_ON(ARRAY_SIZE(crcs) != ARRAY_SIZE(crtc_state->mixers));
>> -
>> - /* Skip first 2 frames in case of "uncooked" CRCs */
>> - if (crtc_state->crc_frame_skip_count < 2) {
>> - crtc_state->crc_frame_skip_count++;
>> - return 0;
>> - }
>> + int i;
>>
>> for (i = 0; i < crtc_state->num_mixers; ++i) {
>>
>> - m = &crtc_state->mixers[i];
>> + lm = &crtc_state->mixers[i];
>>
>> - if (!m->hw_lm || !m->hw_lm->ops.collect_misr)
>> + if (!lm->hw_lm || !lm->hw_lm->ops.collect_misr)
>> continue;
>>
>> - rc = m->hw_lm->ops.collect_misr(m->hw_lm, &crcs[i]);
>> + rc = lm->hw_lm->ops.collect_misr(lm->hw_lm, &crtc_state->crcs[i]);
>>
>> if (rc) {
>> if (rc != -ENODATA)
>> @@ -211,7 +217,25 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> }
>>
>> return drm_crtc_add_crc_entry(crtc, true,
>> - drm_crtc_accurate_vblank_count(crtc), crcs);
>> + drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
>> +}
>> +
>> +static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +{
>> + struct dpu_crtc_state *crtc_state;
>> +
>> + crtc_state = to_dpu_crtc_state(crtc->state);
>> +
>> + /* Skip first 2 frames in case of "uncooked" CRCs */
>> + if (crtc_state->crc_frame_skip_count < 2) {
>> + crtc_state->crc_frame_skip_count++;
>> + return 0;
>> + }
>> +
>> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> + return dpu_crtc_get_lm_crc(crtc, crtc_state);
>> +
>> + return 0;
>> }
>>
>> static bool dpu_crtc_get_scanout_position(struct drm_crtc *crtc,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index b8785c394fcc..4bf45e3343ef 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -201,6 +201,9 @@ struct dpu_crtc {
>> * @mixers : List of active mixers
>> * @num_ctls : Number of ctl paths in use
>> * @hw_ctls : List of active ctl paths
>> + * @crc_source : CRC source
>> + * @crc_frame_skip_count: Number of frames skipped before getting CRC
>> + * @crcs : Array to store CRC values
>> */
>> struct dpu_crtc_state {
>> struct drm_crtc_state base;
>> @@ -222,6 +225,7 @@ struct dpu_crtc_state {
>>
>> enum dpu_crtc_crc_source crc_source;
>> int crc_frame_skip_count;
>> + u32 *crcs;
>> };
>>
>> #define to_dpu_crtc_state(x) \
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>> index 462f5082099e..06b24d8d1419 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>> */
>>
>> @@ -27,11 +28,6 @@
>>
>> #define LM_MISR_CTRL 0x310
>> #define LM_MISR_SIGNATURE 0x314
>> -#define LM_MISR_FRAME_COUNT_MASK 0xFF
>> -#define LM_MISR_CTRL_ENABLE BIT(8)
>> -#define LM_MISR_CTRL_STATUS BIT(9)
>> -#define LM_MISR_CTRL_STATUS_CLEAR BIT(10)
>> -#define LM_MISR_CTRL_FREE_RUN_MASK BIT(31)
>>
>>
>> static const struct dpu_lm_cfg *_lm_offset(enum dpu_lm mixer,
>> @@ -108,44 +104,12 @@ static void dpu_hw_lm_setup_border_color(struct dpu_hw_mixer *ctx,
>>
>> static void dpu_hw_lm_setup_misr(struct dpu_hw_mixer *ctx, bool enable, u32 frame_count)
>> {
>> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
>> - u32 config = 0;
>> -
>> - DPU_REG_WRITE(c, LM_MISR_CTRL, LM_MISR_CTRL_STATUS_CLEAR);
>> -
>> - /* Clear old MISR value (in case it's read before a new value is calculated)*/
>> - wmb();
>> -
>> - if (enable) {
>> - config = (frame_count & LM_MISR_FRAME_COUNT_MASK) |
>> - LM_MISR_CTRL_ENABLE | LM_MISR_CTRL_FREE_RUN_MASK;
>> -
>> - DPU_REG_WRITE(c, LM_MISR_CTRL, config);
>> - } else {
>> - DPU_REG_WRITE(c, LM_MISR_CTRL, 0);
>> - }
>> -
>> + dpu_hw_setup_misr(&ctx->hw, enable, frame_count, LM_MISR_CTRL);
>> }
>>
>> static int dpu_hw_lm_collect_misr(struct dpu_hw_mixer *ctx, u32 *misr_value)
>> {
>> - struct dpu_hw_blk_reg_map *c = &ctx->hw;
>> - u32 ctrl = 0;
>> -
>> - if (!misr_value)
>> - return -EINVAL;
>> -
>> - ctrl = DPU_REG_READ(c, LM_MISR_CTRL);
>> -
>> - if (!(ctrl & LM_MISR_CTRL_ENABLE))
>> - return -ENODATA;
>> -
>> - if (!(ctrl & LM_MISR_CTRL_STATUS))
>> - return -EINVAL;
>> -
>> - *misr_value = DPU_REG_READ(c, LM_MISR_SIGNATURE);
>> -
>> - return 0;
>> + return dpu_hw_collect_misr(&ctx->hw, misr_value, LM_MISR_CTRL, LM_MISR_SIGNATURE);
>> }
>>
>> static void dpu_hw_lm_setup_blend_config_sdm845(struct dpu_hw_mixer *ctx,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> index 512316f25a51..244f2f90e99a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c
>> @@ -1,5 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> */
>> #define pr_fmt(fmt) "[drm:%s:%d] " fmt, __func__, __LINE__
>>
>> @@ -447,3 +449,45 @@ u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>>
>> return 0;
>> }
>> +
>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c, bool enable,
>> + u32 frame_count, u32 misr_ctrl_offset)
>> +{
>> + u32 config = 0;
>> +
>> + DPU_REG_WRITE(c, misr_ctrl_offset, MISR_CTRL_STATUS_CLEAR);
>> +
>> + /* Clear old MISR value (in case it's read before a new value is calculated)*/
>> + wmb();
>> +
>> + if (enable) {
>> + config = (frame_count & MISR_FRAME_COUNT_MASK) |
>> + MISR_CTRL_ENABLE | MISR_CTRL_FREE_RUN_MASK;
>> +
>> + DPU_REG_WRITE(c, misr_ctrl_offset, config);
>> + } else {
>> + DPU_REG_WRITE(c, misr_ctrl_offset, 0);
>> + }
>> +
>> +}
>> +
>> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c, u32 *misr_value,
>> + u32 misr_ctrl_offset, u32 misr_signature_offset)
>> +{
>> + u32 ctrl = 0;
>> +
>> + if (!misr_value)
>> + return -EINVAL;
>> +
>> + ctrl = DPU_REG_READ(c, misr_ctrl_offset);
>> +
>> + if (!(ctrl & MISR_CTRL_ENABLE))
>> + return -ENODATA;
>> +
>> + if (!(ctrl & MISR_CTRL_STATUS))
>> + return -EINVAL;
>> +
>> + *misr_value = DPU_REG_READ(c, misr_signature_offset);
>> +
>> + return 0;
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> index e4a65eb4f769..88df3a5c5d8e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h
>> @@ -1,5 +1,6 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>> */
>>
>> @@ -12,6 +13,11 @@
>> #include "dpu_hw_catalog.h"
>>
>> #define REG_MASK(n) ((BIT(n)) - 1)
>> +#define MISR_FRAME_COUNT_MASK 0xFF
>> +#define MISR_CTRL_ENABLE BIT(8)
>> +#define MISR_CTRL_STATUS BIT(9)
>> +#define MISR_CTRL_STATUS_CLEAR BIT(10)
>> +#define MISR_CTRL_FREE_RUN_MASK BIT(31)
>>
>> /*
>> * This is the common struct maintained by each sub block
>> @@ -343,4 +349,14 @@ void dpu_hw_csc_setup(struct dpu_hw_blk_reg_map *c,
>> u64 _dpu_hw_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>> u32 total_fl);
>>
>> +void dpu_hw_setup_misr(struct dpu_hw_blk_reg_map *c,
>> + bool enable,
>> + u32 frame_count,
>> + u32 misr_ctrl_offset);
>> +
>> +int dpu_hw_collect_misr(struct dpu_hw_blk_reg_map *c,
>> + u32 *misr_value,
>> + u32 misr_ctrl_offset,
>> + u32 misr_signature_offset);
>
> Please move offsets next to the hw_blk_reg_map.
Noted.
Thanks,
Jessica Zhang
>
>> +
>> #endif /* _DPU_HW_UTIL_H */
>> --
>> 2.35.1
>>
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/3] drm/msm/dpu: Add interface support for CRC debugfs
2022-06-15 9:44 ` Dmitry Baryshkov
@ 2022-06-15 16:11 ` Jessica Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-15 16:11 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
quic_aravindh, quic_abhinavk
On 6/15/2022 2:44 AM, Dmitry Baryshkov wrote:
> On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>> Add support for writing CRC values for the interface block to
>> the debugfs by calling the necessary MISR setup/collect methods.
>>
>> Changes since V1:
>> - Set values_cnt to only include phys with backing hw_intf
>> - Loop over all drm_encs connected to crtc
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 49 ++++++++++++++--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 64 +++++++++++++++++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 +++++++
>> 4 files changed, 134 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 16742a66878e..8c9933b2337f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source dpu_crtc_parse_crc_source(const char *src_name)
>> if (!strcmp(src_name, "auto") ||
>> !strcmp(src_name, "lm"))
>> return DPU_CRTC_CRC_SOURCE_LAYER_MIXER;
>> + if (!strcmp(src_name, "intf"))
>> + return DPU_CRTC_CRC_SOURCE_INTF;
>
> What about "encoder" / DPU_CRTC_CRC_SOURCE_ENCODER? You basically
> offload CRC generation/collection to the dpu_encoder, so I'd ignore
> the fact that only INTF's support MISR generation and use a more
> generic word here.
>
>>
>> return DPU_CRTC_CRC_SOURCE_INVALID;
>> }
>> @@ -99,8 +101,14 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
>> return -EINVAL;
>> }
>>
>> - if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) {
>> *values_cnt = crtc_state->num_mixers;
>> + } else if (source == DPU_CRTC_CRC_SOURCE_INTF) {
>> + struct drm_encoder *drm_enc;
>
> Zero values_cnt here.
Noted.
>
>> +
>> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
>> + *values_cnt += dpu_encoder_get_num_phys(drm_enc);
>> + }
>>
>> crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
>>
>> @@ -123,6 +131,14 @@ static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
>> }
>> }
>>
>> +static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc)
>> +{
>> + struct drm_encoder *drm_enc;
>> +
>> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
>> + dpu_encoder_setup_misr(drm_enc);
>> +}
>> +
>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>> {
>> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
>> @@ -175,6 +191,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>>
>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> dpu_crtc_setup_lm_misr(crtc_state);
>> + else if (source == DPU_CRTC_CRC_SOURCE_INTF)
>> + dpu_crtc_setup_encoder_misr(crtc);
>
> else? >
>>
>> cleanup:
>> drm_modeset_unlock(&crtc->mutex);
>> @@ -220,11 +238,31 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt
>> drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
>> }
>>
>> -static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc)
>> {
>> - struct dpu_crtc_state *crtc_state;
>> + struct drm_encoder *drm_enc;
>> + struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
>> + int rc, pos = 0;
>>
>> - crtc_state = to_dpu_crtc_state(crtc->state);
>> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) {
>> + rc = dpu_encoder_get_crc(drm_enc, crtc_state->crcs, pos);
>> + if (rc < 0) {
>> + if (rc != -ENODATA)
>> + DRM_DEBUG_DRIVER("MISR read failed\n");
>> +
>> + return rc;
>> + }
>> +
>> + pos += rc;
>> + }
>> +
>> + return drm_crtc_add_crc_entry(crtc, true,
>> + drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
>> +}
>> +
>> +static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +{
>> + struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
>
> Unnecessary change here. Please move it to the patch 1, which
> refactors this function.
Noted.
>
>>
>> /* Skip first 2 frames in case of "uncooked" CRCs */
>> if (crtc_state->crc_frame_skip_count < 2) {
>> @@ -235,6 +273,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> return dpu_crtc_get_lm_crc(crtc, crtc_state);
>>
>> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF)
>> + return dpu_crtc_get_encoder_crc(crtc);
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 4bf45e3343ef..5db84ea796db 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -1,5 +1,6 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2021 The Linux Foundation. All rights reserved.
>> * Copyright (C) 2013 Red Hat
>> * Author: Rob Clark <robdclark@gmail.com>
>> @@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data {
>> * enum dpu_crtc_crc_source: CRC source
>> * @DPU_CRTC_CRC_SOURCE_NONE: no source set
>> * @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer
>> + * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface
>> * @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source
>> */
>> enum dpu_crtc_crc_source {
>> DPU_CRTC_CRC_SOURCE_NONE = 0,
>> DPU_CRTC_CRC_SOURCE_LAYER_MIXER,
>> + DPU_CRTC_CRC_SOURCE_INTF,
>> DPU_CRTC_CRC_SOURCE_MAX,
>> DPU_CRTC_CRC_SOURCE_INVALID = -1
>> };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 52516eb20cb8..2cbfed5c627e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -14,6 +14,7 @@
>>
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_file.h>
>> +#include <drm/drm_vblank.h>
>
> Why?
Leftover from the v1 patch -- not needed anymore since the
`drm_crtc_add_crc_entry` call was moved to dpu_crtc.c. Will remove it.
>
>> #include <drm/drm_probe_helper.h>
>>
>> #include "msm_drv.h"
>> @@ -225,6 +226,69 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
>> return dpu_enc->wide_bus_en;
>> }
>>
>> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc)
>
> The function name is misleading. It doesn't return the number of phys.
> It returns a number of hw_intfs. And in reality you'd like to get the
> number of crc entries supported. If at some point WB (or any other
> possible dpu_encoder backend) gains support for CRC, we won't have to
> change the name. Please consider adjusting it.
Noted.
>
>> +{
>> + struct dpu_encoder_virt *dpu_enc;
>> + int i, num_intf = 0;
>> +
>> + dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +
>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> +
>> + if (phys->hw_intf)
>> + num_intf++;
>
> You have to check for hw_intf->ops.setup_misr too.
Noted.
>
>> + }
>> +
>> + return num_intf;
>> +}
>> +
>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc)
>> +{
>> + struct dpu_encoder_virt *dpu_enc;
>> +
>> + int i;
>> +
>> + dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +
>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> +
>> + if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr)
>> + continue;
>> +
>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);
>> + }
>> +}
>> +
>> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos)
>> +{
>> + struct dpu_encoder_virt *dpu_enc;
>> +
>> + int i, rc = 0, entries_added = 0;
>> +
>> + if (!drm_enc->crtc) {
>> + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index);
>> + return -EINVAL;
>> + }
>> +
>> + dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +
>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> +
>> + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr)
>> + continue;
>> +
>> + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[pos + i]);
>
> No, you should be indexing the crcs with entries_added rather than i.
Noted.
Thanks,
Jessica Zhang
>
>> + if (rc)
>> + return rc;
>> + entries_added++;
>> + }
>> +
>> + return entries_added;
>> +}
>> +
>> static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
>> {
>> struct dpu_hw_dither_cfg dither_cfg = { 0 };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index 781d41c91994..375370029cb9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -1,5 +1,6 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> * Copyright (C) 2013 Red Hat
>> * Author: Rob Clark <robdclark@gmail.com>
>> @@ -174,6 +175,27 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc);
>>
>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc);
>>
>> +/**
>> + * dpu_encoder_get_num_phys - get number of physical encoders contained in virtual
>> + * encoder
>> + * @drm_enc: Pointer to previously created drm encoder structure
>> + * Returns: Number of physical encoders for given drm encoder
>> + */
>> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc);
>> +
>> +/**
>> + * dpu_encoder_setup_misr - enable misr calculations
>> + * @drm_enc: Pointer to previously created drm encoder structure
>> + */
>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder);
>> +
>> +/**
>> + * dpu_encoder_get_crc - get the crc value from interface blocks
>> + * @drm_enc: Pointer to previously created drm encoder structure
>> + * Returns: 0 on success, error otherwise
>> + */
>> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos);
>> +
>> /**
>> * dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology.
>> * @drm_enc: Pointer to previously created drm encoder structure
>> --
>> 2.35.1
>>
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/3] drm/msm/dpu: Add interface support for CRC debugfs
@ 2022-06-15 16:11 ` Jessica Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-15 16:11 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
quic_aravindh, freedreno
On 6/15/2022 2:44 AM, Dmitry Baryshkov wrote:
> On Wed, 15 Jun 2022 at 00:13, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>> Add support for writing CRC values for the interface block to
>> the debugfs by calling the necessary MISR setup/collect methods.
>>
>> Changes since V1:
>> - Set values_cnt to only include phys with backing hw_intf
>> - Loop over all drm_encs connected to crtc
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 49 ++++++++++++++--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 64 +++++++++++++++++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 +++++++
>> 4 files changed, 134 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 16742a66878e..8c9933b2337f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -79,6 +79,8 @@ static enum dpu_crtc_crc_source dpu_crtc_parse_crc_source(const char *src_name)
>> if (!strcmp(src_name, "auto") ||
>> !strcmp(src_name, "lm"))
>> return DPU_CRTC_CRC_SOURCE_LAYER_MIXER;
>> + if (!strcmp(src_name, "intf"))
>> + return DPU_CRTC_CRC_SOURCE_INTF;
>
> What about "encoder" / DPU_CRTC_CRC_SOURCE_ENCODER? You basically
> offload CRC generation/collection to the dpu_encoder, so I'd ignore
> the fact that only INTF's support MISR generation and use a more
> generic word here.
>
>>
>> return DPU_CRTC_CRC_SOURCE_INVALID;
>> }
>> @@ -99,8 +101,14 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
>> return -EINVAL;
>> }
>>
>> - if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> + if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER) {
>> *values_cnt = crtc_state->num_mixers;
>> + } else if (source == DPU_CRTC_CRC_SOURCE_INTF) {
>> + struct drm_encoder *drm_enc;
>
> Zero values_cnt here.
Noted.
>
>> +
>> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
>> + *values_cnt += dpu_encoder_get_num_phys(drm_enc);
>> + }
>>
>> crtc_state->crcs = kcalloc(*values_cnt, sizeof(crtc_state->crcs), GFP_KERNEL);
>>
>> @@ -123,6 +131,14 @@ static void dpu_crtc_setup_lm_misr(struct dpu_crtc_state *crtc_state)
>> }
>> }
>>
>> +static void dpu_crtc_setup_encoder_misr(struct drm_crtc *crtc)
>> +{
>> + struct drm_encoder *drm_enc;
>> +
>> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask)
>> + dpu_encoder_setup_misr(drm_enc);
>> +}
>> +
>> static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>> {
>> enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
>> @@ -175,6 +191,8 @@ static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>>
>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> dpu_crtc_setup_lm_misr(crtc_state);
>> + else if (source == DPU_CRTC_CRC_SOURCE_INTF)
>> + dpu_crtc_setup_encoder_misr(crtc);
>
> else? >
>>
>> cleanup:
>> drm_modeset_unlock(&crtc->mutex);
>> @@ -220,11 +238,31 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt
>> drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
>> }
>>
>> -static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc)
>> {
>> - struct dpu_crtc_state *crtc_state;
>> + struct drm_encoder *drm_enc;
>> + struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
>> + int rc, pos = 0;
>>
>> - crtc_state = to_dpu_crtc_state(crtc->state);
>> + drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc->state->encoder_mask) {
>> + rc = dpu_encoder_get_crc(drm_enc, crtc_state->crcs, pos);
>> + if (rc < 0) {
>> + if (rc != -ENODATA)
>> + DRM_DEBUG_DRIVER("MISR read failed\n");
>> +
>> + return rc;
>> + }
>> +
>> + pos += rc;
>> + }
>> +
>> + return drm_crtc_add_crc_entry(crtc, true,
>> + drm_crtc_accurate_vblank_count(crtc), crtc_state->crcs);
>> +}
>> +
>> +static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> +{
>> + struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
>
> Unnecessary change here. Please move it to the patch 1, which
> refactors this function.
Noted.
>
>>
>> /* Skip first 2 frames in case of "uncooked" CRCs */
>> if (crtc_state->crc_frame_skip_count < 2) {
>> @@ -235,6 +273,9 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc)
>> if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>> return dpu_crtc_get_lm_crc(crtc, crtc_state);
>>
>> + if (crtc_state->crc_source == DPU_CRTC_CRC_SOURCE_INTF)
>> + return dpu_crtc_get_encoder_crc(crtc);
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 4bf45e3343ef..5db84ea796db 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -1,5 +1,6 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2021 The Linux Foundation. All rights reserved.
>> * Copyright (C) 2013 Red Hat
>> * Author: Rob Clark <robdclark@gmail.com>
>> @@ -73,11 +74,13 @@ struct dpu_crtc_smmu_state_data {
>> * enum dpu_crtc_crc_source: CRC source
>> * @DPU_CRTC_CRC_SOURCE_NONE: no source set
>> * @DPU_CRTC_CRC_SOURCE_LAYER_MIXER: CRC in layer mixer
>> + * @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface
>> * @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source
>> */
>> enum dpu_crtc_crc_source {
>> DPU_CRTC_CRC_SOURCE_NONE = 0,
>> DPU_CRTC_CRC_SOURCE_LAYER_MIXER,
>> + DPU_CRTC_CRC_SOURCE_INTF,
>> DPU_CRTC_CRC_SOURCE_MAX,
>> DPU_CRTC_CRC_SOURCE_INVALID = -1
>> };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 52516eb20cb8..2cbfed5c627e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -14,6 +14,7 @@
>>
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_file.h>
>> +#include <drm/drm_vblank.h>
>
> Why?
Leftover from the v1 patch -- not needed anymore since the
`drm_crtc_add_crc_entry` call was moved to dpu_crtc.c. Will remove it.
>
>> #include <drm/drm_probe_helper.h>
>>
>> #include "msm_drv.h"
>> @@ -225,6 +226,69 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
>> return dpu_enc->wide_bus_en;
>> }
>>
>> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc)
>
> The function name is misleading. It doesn't return the number of phys.
> It returns a number of hw_intfs. And in reality you'd like to get the
> number of crc entries supported. If at some point WB (or any other
> possible dpu_encoder backend) gains support for CRC, we won't have to
> change the name. Please consider adjusting it.
Noted.
>
>> +{
>> + struct dpu_encoder_virt *dpu_enc;
>> + int i, num_intf = 0;
>> +
>> + dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +
>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> +
>> + if (phys->hw_intf)
>> + num_intf++;
>
> You have to check for hw_intf->ops.setup_misr too.
Noted.
>
>> + }
>> +
>> + return num_intf;
>> +}
>> +
>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc)
>> +{
>> + struct dpu_encoder_virt *dpu_enc;
>> +
>> + int i;
>> +
>> + dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +
>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> +
>> + if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr)
>> + continue;
>> +
>> + phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);
>> + }
>> +}
>> +
>> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos)
>> +{
>> + struct dpu_encoder_virt *dpu_enc;
>> +
>> + int i, rc = 0, entries_added = 0;
>> +
>> + if (!drm_enc->crtc) {
>> + DRM_ERROR("no crtc found for encoder %d\n", drm_enc->index);
>> + return -EINVAL;
>> + }
>> +
>> + dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +
>> + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> + struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> +
>> + if (!phys->hw_intf || !phys->hw_intf->ops.collect_misr)
>> + continue;
>> +
>> + rc = phys->hw_intf->ops.collect_misr(phys->hw_intf, &crcs[pos + i]);
>
> No, you should be indexing the crcs with entries_added rather than i.
Noted.
Thanks,
Jessica Zhang
>
>> + if (rc)
>> + return rc;
>> + entries_added++;
>> + }
>> +
>> + return entries_added;
>> +}
>> +
>> static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
>> {
>> struct dpu_hw_dither_cfg dither_cfg = { 0 };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index 781d41c91994..375370029cb9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -1,5 +1,6 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> /*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>> * Copyright (C) 2013 Red Hat
>> * Author: Rob Clark <robdclark@gmail.com>
>> @@ -174,6 +175,27 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc);
>>
>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc);
>>
>> +/**
>> + * dpu_encoder_get_num_phys - get number of physical encoders contained in virtual
>> + * encoder
>> + * @drm_enc: Pointer to previously created drm encoder structure
>> + * Returns: Number of physical encoders for given drm encoder
>> + */
>> +int dpu_encoder_get_num_phys(const struct drm_encoder *drm_enc);
>> +
>> +/**
>> + * dpu_encoder_setup_misr - enable misr calculations
>> + * @drm_enc: Pointer to previously created drm encoder structure
>> + */
>> +void dpu_encoder_setup_misr(const struct drm_encoder *drm_encoder);
>> +
>> +/**
>> + * dpu_encoder_get_crc - get the crc value from interface blocks
>> + * @drm_enc: Pointer to previously created drm encoder structure
>> + * Returns: 0 on success, error otherwise
>> + */
>> +int dpu_encoder_get_crc(const struct drm_encoder *drm_enc, u32 *crcs, int pos);
>> +
>> /**
>> * dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology.
>> * @drm_enc: Pointer to previously created drm encoder structure
>> --
>> 2.35.1
>>
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
2022-06-15 16:11 ` Jessica Zhang
@ 2022-06-15 16:17 ` Dmitry Baryshkov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-06-15 16:17 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
quic_aravindh, freedreno
On 15/06/2022 19:11, Jessica Zhang wrote:
> On 6/15/2022 2:35 AM, Dmitry Baryshkov wrote:
>> On Wed, 15 Jun 2022 at 00:13, Jessica Zhang
>> <quic_jesszhan@quicinc.com> wrote:
>>>
>>> Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
>>> helper method. This way, we can make it easier to get CRCs from other HW
>>> blocks by adding other get_crc helper methods.
>>>
>>> Changes since V1:
>>> - Moved common bitmasks to dpu_hw_util.h
>>> - Moved common CRC methods to dpu_hw_util.c
>>> - Updated copyrights
>>> - Changed crcs array to a dynamically allocated array and added it as a
>>> member of crtc_state
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>
>> Please split this into separate patches. One for hw_util, one for the
>> rest
>
> Sure
>
>>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88 +++++++++++++--------
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
>>> 5 files changed, 124 insertions(+), 72 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index b56f777dbd0e..16742a66878e 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -1,5 +1,6 @@
>>> // SPDX-License-Identifier: GPL-2.0-only
>>> /*
>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
>>> reserved.
>>> * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
>>> * Copyright (C) 2013 Red Hat
>>> * Author: Rob Clark <robdclark@gmail.com>
>>> @@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct
>>> drm_crtc *crtc,
>>> enum dpu_crtc_crc_source source =
>>> dpu_crtc_parse_crc_source(src_name);
>>> struct dpu_crtc_state *crtc_state =
>>> to_dpu_crtc_state(crtc->state);
>>>
>>> + if (crtc_state->crcs) {
>>> + kfree(crtc_state->crcs);
>>> + crtc_state->crcs = NULL;
>>> + }
>>> +
>>> if (source < 0) {
>>> DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n",
>>> src_name, crtc->index);
>>> return -EINVAL;
>>> @@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct
>>> drm_crtc *crtc,
>>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>>> *values_cnt = crtc_state->num_mixers;
>>>
>>> + crtc_state->crcs = kcalloc(*values_cnt,
>>> sizeof(crtc_state->crcs), GFP_KERNEL);
>>> +
>>
>> I do not quite like the idea of constantly allocating and freeing the
>> crcs array. I'd suggest defining sensible MAX_CRC_VALUES, using a
>> static array and verifying that no one over commits the
>> MAX_CRC_VALUES.
>> If at some point we decide that we really need the dynamic array, we
>> can implement it later. We already had dynamic arrays and Rob had to
>> fix it. See 00326bfa4e63 ("drm/msm/dpu: Remove dynamic allocation from
>> atomic context").
>
> Ah, got it... the reason I used a dynamic array here was because AFAIK
> we don't have a defined upper limit for how many drm_encoders can be
> connected to a CRTC simultaneously. Do you have a suggestion on what
> value we can set for MAX_CRC_VALUES?
Three? Two for two hw_intfs?
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
@ 2022-06-15 16:17 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-06-15 16:17 UTC (permalink / raw)
To: Jessica Zhang
Cc: freedreno, linux-arm-msm, dri-devel, robdclark, seanpaul, swboyd,
quic_aravindh, quic_abhinavk
On 15/06/2022 19:11, Jessica Zhang wrote:
> On 6/15/2022 2:35 AM, Dmitry Baryshkov wrote:
>> On Wed, 15 Jun 2022 at 00:13, Jessica Zhang
>> <quic_jesszhan@quicinc.com> wrote:
>>>
>>> Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
>>> helper method. This way, we can make it easier to get CRCs from other HW
>>> blocks by adding other get_crc helper methods.
>>>
>>> Changes since V1:
>>> - Moved common bitmasks to dpu_hw_util.h
>>> - Moved common CRC methods to dpu_hw_util.c
>>> - Updated copyrights
>>> - Changed crcs array to a dynamically allocated array and added it as a
>>> member of crtc_state
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>
>> Please split this into separate patches. One for hw_util, one for the
>> rest
>
> Sure
>
>>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88 +++++++++++++--------
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
>>> 5 files changed, 124 insertions(+), 72 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index b56f777dbd0e..16742a66878e 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -1,5 +1,6 @@
>>> // SPDX-License-Identifier: GPL-2.0-only
>>> /*
>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
>>> reserved.
>>> * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
>>> * Copyright (C) 2013 Red Hat
>>> * Author: Rob Clark <robdclark@gmail.com>
>>> @@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct
>>> drm_crtc *crtc,
>>> enum dpu_crtc_crc_source source =
>>> dpu_crtc_parse_crc_source(src_name);
>>> struct dpu_crtc_state *crtc_state =
>>> to_dpu_crtc_state(crtc->state);
>>>
>>> + if (crtc_state->crcs) {
>>> + kfree(crtc_state->crcs);
>>> + crtc_state->crcs = NULL;
>>> + }
>>> +
>>> if (source < 0) {
>>> DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n",
>>> src_name, crtc->index);
>>> return -EINVAL;
>>> @@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct
>>> drm_crtc *crtc,
>>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>>> *values_cnt = crtc_state->num_mixers;
>>>
>>> + crtc_state->crcs = kcalloc(*values_cnt,
>>> sizeof(crtc_state->crcs), GFP_KERNEL);
>>> +
>>
>> I do not quite like the idea of constantly allocating and freeing the
>> crcs array. I'd suggest defining sensible MAX_CRC_VALUES, using a
>> static array and verifying that no one over commits the
>> MAX_CRC_VALUES.
>> If at some point we decide that we really need the dynamic array, we
>> can implement it later. We already had dynamic arrays and Rob had to
>> fix it. See 00326bfa4e63 ("drm/msm/dpu: Remove dynamic allocation from
>> atomic context").
>
> Ah, got it... the reason I used a dynamic array here was because AFAIK
> we don't have a defined upper limit for how many drm_encoders can be
> connected to a CRTC simultaneously. Do you have a suggestion on what
> value we can set for MAX_CRC_VALUES?
Three? Two for two hw_intfs?
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Freedreno] [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
2022-06-15 16:17 ` Dmitry Baryshkov
@ 2022-06-15 17:56 ` Jessica Zhang
-1 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-15 17:56 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
quic_aravindh, freedreno
On 6/15/2022 9:17 AM, Dmitry Baryshkov wrote:
> On 15/06/2022 19:11, Jessica Zhang wrote:
>> On 6/15/2022 2:35 AM, Dmitry Baryshkov wrote:
>>> On Wed, 15 Jun 2022 at 00:13, Jessica Zhang
>>> <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>> Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
>>>> helper method. This way, we can make it easier to get CRCs from
>>>> other HW
>>>> blocks by adding other get_crc helper methods.
>>>>
>>>> Changes since V1:
>>>> - Moved common bitmasks to dpu_hw_util.h
>>>> - Moved common CRC methods to dpu_hw_util.c
>>>> - Updated copyrights
>>>> - Changed crcs array to a dynamically allocated array and added it as a
>>>> member of crtc_state
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>
>>> Please split this into separate patches. One for hw_util, one for the
>>> rest
>>
>> Sure
>>
>>>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88
>>>> +++++++++++++--------
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
>>>> 5 files changed, 124 insertions(+), 72 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> index b56f777dbd0e..16742a66878e 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> @@ -1,5 +1,6 @@
>>>> // SPDX-License-Identifier: GPL-2.0-only
>>>> /*
>>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
>>>> reserved.
>>>> * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
>>>> * Copyright (C) 2013 Red Hat
>>>> * Author: Rob Clark <robdclark@gmail.com>
>>>> @@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct
>>>> drm_crtc *crtc,
>>>> enum dpu_crtc_crc_source source =
>>>> dpu_crtc_parse_crc_source(src_name);
>>>> struct dpu_crtc_state *crtc_state =
>>>> to_dpu_crtc_state(crtc->state);
>>>>
>>>> + if (crtc_state->crcs) {
>>>> + kfree(crtc_state->crcs);
>>>> + crtc_state->crcs = NULL;
>>>> + }
>>>> +
>>>> if (source < 0) {
>>>> DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n",
>>>> src_name, crtc->index);
>>>> return -EINVAL;
>>>> @@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct
>>>> drm_crtc *crtc,
>>>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>>>> *values_cnt = crtc_state->num_mixers;
>>>>
>>>> + crtc_state->crcs = kcalloc(*values_cnt,
>>>> sizeof(crtc_state->crcs), GFP_KERNEL);
>>>> +
>>>
>>> I do not quite like the idea of constantly allocating and freeing the
>>> crcs array. I'd suggest defining sensible MAX_CRC_VALUES, using a
>>> static array and verifying that no one over commits the
>>> MAX_CRC_VALUES.
>>> If at some point we decide that we really need the dynamic array, we
>>> can implement it later. We already had dynamic arrays and Rob had to
>>> fix it. See 00326bfa4e63 ("drm/msm/dpu: Remove dynamic allocation from
>>> atomic context").
>>
>> Ah, got it... the reason I used a dynamic array here was because AFAIK
>> we don't have a defined upper limit for how many drm_encoders can be
>> connected to a CRTC simultaneously. Do you have a suggestion on what
>> value we can set for MAX_CRC_VALUES?
>
> Three? Two for two hw_intfs?
Do you mean 2 hw_intfs per drm_encoder or 2 hw_intfs overall? IIRC we
also wanted to take into account the possibility of there being multiple
drm_encoders attached to a single CRTC.
Also, looking through Rob's fix, the warning was occuring because we
were trying to call kcalloc in an IRQ context. However, the way I'm
currently doing dynamic allocation will avoid the warning (since I'm
doing kcalloc in verify_crc_source, which is called during the debugfs
read/write/open and not during vblank). So I don't believe that we'll
encounter the warning related to Rob's fix with my current
implementation of the crcs dynamic array.
Thanks,
Jessica Zhang
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Freedreno] [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method
@ 2022-06-15 17:56 ` Jessica Zhang
0 siblings, 0 replies; 22+ messages in thread
From: Jessica Zhang @ 2022-06-15 17:56 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, robdclark,
seanpaul, quic_aravindh, freedreno
On 6/15/2022 9:17 AM, Dmitry Baryshkov wrote:
> On 15/06/2022 19:11, Jessica Zhang wrote:
>> On 6/15/2022 2:35 AM, Dmitry Baryshkov wrote:
>>> On Wed, 15 Jun 2022 at 00:13, Jessica Zhang
>>> <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>> Move layer mixer-specific section of dpu_crtc_get_crc() into a separate
>>>> helper method. This way, we can make it easier to get CRCs from
>>>> other HW
>>>> blocks by adding other get_crc helper methods.
>>>>
>>>> Changes since V1:
>>>> - Moved common bitmasks to dpu_hw_util.h
>>>> - Moved common CRC methods to dpu_hw_util.c
>>>> - Updated copyrights
>>>> - Changed crcs array to a dynamically allocated array and added it as a
>>>> member of crtc_state
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>
>>> Please split this into separate patches. One for hw_util, one for the
>>> rest
>>
>> Sure
>>
>>>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 88
>>>> +++++++++++++--------
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 42 +---------
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 46 ++++++++++-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 16 ++++
>>>> 5 files changed, 124 insertions(+), 72 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> index b56f777dbd0e..16742a66878e 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> @@ -1,5 +1,6 @@
>>>> // SPDX-License-Identifier: GPL-2.0-only
>>>> /*
>>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
>>>> reserved.
>>>> * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
>>>> * Copyright (C) 2013 Red Hat
>>>> * Author: Rob Clark <robdclark@gmail.com>
>>>> @@ -88,6 +89,11 @@ static int dpu_crtc_verify_crc_source(struct
>>>> drm_crtc *crtc,
>>>> enum dpu_crtc_crc_source source =
>>>> dpu_crtc_parse_crc_source(src_name);
>>>> struct dpu_crtc_state *crtc_state =
>>>> to_dpu_crtc_state(crtc->state);
>>>>
>>>> + if (crtc_state->crcs) {
>>>> + kfree(crtc_state->crcs);
>>>> + crtc_state->crcs = NULL;
>>>> + }
>>>> +
>>>> if (source < 0) {
>>>> DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n",
>>>> src_name, crtc->index);
>>>> return -EINVAL;
>>>> @@ -96,20 +102,37 @@ static int dpu_crtc_verify_crc_source(struct
>>>> drm_crtc *crtc,
>>>> if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
>>>> *values_cnt = crtc_state->num_mixers;
>>>>
>>>> + crtc_state->crcs = kcalloc(*values_cnt,
>>>> sizeof(crtc_state->crcs), GFP_KERNEL);
>>>> +
>>>
>>> I do not quite like the idea of constantly allocating and freeing the
>>> crcs array. I'd suggest defining sensible MAX_CRC_VALUES, using a
>>> static array and verifying that no one over commits the
>>> MAX_CRC_VALUES.
>>> If at some point we decide that we really need the dynamic array, we
>>> can implement it later. We already had dynamic arrays and Rob had to
>>> fix it. See 00326bfa4e63 ("drm/msm/dpu: Remove dynamic allocation from
>>> atomic context").
>>
>> Ah, got it... the reason I used a dynamic array here was because AFAIK
>> we don't have a defined upper limit for how many drm_encoders can be
>> connected to a CRTC simultaneously. Do you have a suggestion on what
>> value we can set for MAX_CRC_VALUES?
>
> Three? Two for two hw_intfs?
Do you mean 2 hw_intfs per drm_encoder or 2 hw_intfs overall? IIRC we
also wanted to take into account the possibility of there being multiple
drm_encoders attached to a single CRTC.
Also, looking through Rob's fix, the warning was occuring because we
were trying to call kcalloc in an IRQ context. However, the way I'm
currently doing dynamic allocation will avoid the warning (since I'm
doing kcalloc in verify_crc_source, which is called during the debugfs
read/write/open and not during vblank). So I don't believe that we'll
encounter the warning related to Rob's fix with my current
implementation of the crcs dynamic array.
Thanks,
Jessica Zhang
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-06-15 17:56 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-14 21:13 [PATCH v2 0/3] Expand CRC to support interface blocks Jessica Zhang
2022-06-14 21:13 ` Jessica Zhang
2022-06-14 21:13 ` [PATCH v2 1/3] drm/msm/dpu: Move LM CRC code into separate method Jessica Zhang
2022-06-14 21:13 ` Jessica Zhang
2022-06-15 9:35 ` Dmitry Baryshkov
2022-06-15 9:35 ` Dmitry Baryshkov
2022-06-15 16:11 ` Jessica Zhang
2022-06-15 16:11 ` Jessica Zhang
2022-06-15 16:17 ` Dmitry Baryshkov
2022-06-15 16:17 ` Dmitry Baryshkov
2022-06-15 17:56 ` [Freedreno] " Jessica Zhang
2022-06-15 17:56 ` Jessica Zhang
2022-06-14 21:13 ` [PATCH v2 2/3] drm/msm/dpu: Add MISR register support for interface Jessica Zhang
2022-06-14 21:13 ` Jessica Zhang
2022-06-15 9:36 ` Dmitry Baryshkov
2022-06-15 9:36 ` Dmitry Baryshkov
2022-06-14 21:13 ` [PATCH v2 3/3] drm/msm/dpu: Add interface support for CRC debugfs Jessica Zhang
2022-06-14 21:13 ` Jessica Zhang
2022-06-15 9:44 ` Dmitry Baryshkov
2022-06-15 9:44 ` Dmitry Baryshkov
2022-06-15 16:11 ` Jessica Zhang
2022-06-15 16:11 ` Jessica Zhang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.