Refactor existing CRC code for layer mixer and add CRC support for interface blocks
Jessica Zhang (3): drm/msm/dpu: Separate LM-specific CRC code from generic CRC code 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 | 115 +++++++++++++++----- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 55 +++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 +- 6 files changed, 233 insertions(+), 31 deletions(-)
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.
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 72 +++++++++++++++--------- 1 file changed, 44 insertions(+), 28 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..ae09466663cf 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 @@ -99,17 +100,32 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc, 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); @@ -146,16 +162,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 +182,24 @@ 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; + struct dpu_crtc_mixer *lm; u32 crcs[CRTC_DUAL_MIXERS];
- int i = 0; int rc = 0; - - crtc_state = to_dpu_crtc_state(crtc->state); + int i;
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; - } - 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, &crcs[i]);
if (rc) { if (rc != -ENODATA) @@ -214,6 +212,24 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc) drm_crtc_accurate_vblank_count(crtc), 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, bool in_vblank_irq, int *vpos, int *hpos,
On 27/05/2022 21:54, Jessica Zhang 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.
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 72 +++++++++++++++--------- 1 file changed, 44 insertions(+), 28 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..ae09466663cf 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
@@ -99,17 +100,32 @@ static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc, 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);
@@ -146,16 +162,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 +182,24 @@ 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;
- struct dpu_crtc_mixer *lm; u32 crcs[CRTC_DUAL_MIXERS];
- int i = 0; int rc = 0;
- crtc_state = to_dpu_crtc_state(crtc->state);
int i;
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;}
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, &crcs[i]);if (rc) { if (rc != -ENODATA)
@@ -214,6 +212,24 @@ static int dpu_crtc_get_crc(struct drm_crtc *crtc) drm_crtc_accurate_vblank_count(crtc), 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, bool in_vblank_irq, int *vpos, int *hpos,
Add support for setting MISR registers within the interface
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 55 ++++++++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 ++- 2 files changed, 61 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..29aaeff9eacd 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,14 @@ #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 +#define INTF_MISR_FRAME_COUNT_MASK 0xFF +#define INTF_MISR_CTRL_ENABLE BIT(8) +#define INTF_MISR_CTRL_STATUS BIT(9) +#define INTF_MISR_CTRL_STATUS_CLEAR BIT(10) +#define INTF_MISR_CTRL_FREE_RUN_MASK BIT(31) + static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf, const struct dpu_mdss_cfg *m, void __iomem *addr, @@ -319,6 +329,47 @@ 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) +{ + struct dpu_hw_blk_reg_map *c = &intf->hw; + u32 config = 0; + + DPU_REG_WRITE(c, INTF_MISR_CTRL, INTF_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 & INTF_MISR_FRAME_COUNT_MASK) | + INTF_MISR_CTRL_ENABLE | INTF_MISR_CTRL_FREE_RUN_MASK; + + DPU_REG_WRITE(c, INTF_MISR_CTRL, config); + } else { + DPU_REG_WRITE(c, INTF_MISR_CTRL, 0); + } +} + +static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) +{ + struct dpu_hw_blk_reg_map *c = &intf->hw; + u32 ctrl = 0; + + if (!misr_value) + return -EINVAL; + + ctrl = DPU_REG_READ(c, INTF_MISR_CTRL); + + if (!(ctrl & INTF_MISR_CTRL_ENABLE)) + return -ENODATA; + + if (!(ctrl & INTF_MISR_CTRL_STATUS)) + return -EINVAL; + + *misr_value = DPU_REG_READ(c, INTF_MISR_SIGNATURE); + + return 0; +} + static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, unsigned long cap) { @@ -329,6 +380,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 {
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for setting MISR registers within the interface
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 55 ++++++++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 ++- 2 files changed, 61 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..29aaeff9eacd 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,14 @@ #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 +#define INTF_MISR_FRAME_COUNT_MASK 0xFF +#define INTF_MISR_CTRL_ENABLE BIT(8) +#define INTF_MISR_CTRL_STATUS BIT(9) +#define INTF_MISR_CTRL_STATUS_CLEAR BIT(10) +#define INTF_MISR_CTRL_FREE_RUN_MASK BIT(31)
I'm tempted to ask to move these bits to some common header. Is there any other hardware block which uses the same bitfields to control MISR?
- static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf, const struct dpu_mdss_cfg *m, void __iomem *addr,
@@ -319,6 +329,47 @@ 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) +{
- struct dpu_hw_blk_reg_map *c = &intf->hw;
- u32 config = 0;
- DPU_REG_WRITE(c, INTF_MISR_CTRL, INTF_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 & INTF_MISR_FRAME_COUNT_MASK) |INTF_MISR_CTRL_ENABLE | INTF_MISR_CTRL_FREE_RUN_MASK;DPU_REG_WRITE(c, INTF_MISR_CTRL, config);- } else {
DPU_REG_WRITE(c, INTF_MISR_CTRL, 0);- }
+}
+static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) +{
- struct dpu_hw_blk_reg_map *c = &intf->hw;
- u32 ctrl = 0;
- if (!misr_value)
return -EINVAL;- ctrl = DPU_REG_READ(c, INTF_MISR_CTRL);
- if (!(ctrl & INTF_MISR_CTRL_ENABLE))
return -ENODATA;- if (!(ctrl & INTF_MISR_CTRL_STATUS))
return -EINVAL;- *misr_value = DPU_REG_READ(c, INTF_MISR_SIGNATURE);
- return 0;
+}
- static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, unsigned long cap) {
@@ -329,6 +380,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
*/ struct dpu_hw_intf_ops { void (*setup_timing_gen)(struct dpu_hw_intf *intf,
- @collect_misr: read MISR signature
@@ -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 {
On 2022-05-27 22:38:24, Dmitry Baryshkov wrote:
[..]
#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 +#define INTF_MISR_FRAME_COUNT_MASK 0xFF +#define INTF_MISR_CTRL_ENABLE BIT(8) +#define INTF_MISR_CTRL_STATUS BIT(9) +#define INTF_MISR_CTRL_STATUS_CLEAR BIT(10) +#define INTF_MISR_CTRL_FREE_RUN_MASK BIT(31)
I'm tempted to ask to move these bits to some common header. Is there any other hardware block which uses the same bitfields to control MISR?
Can we use the "rnndb" register XML and bindings generator for DPU, similar to how it is currently used for Adreno/freedreno and the DSI kernel drivers?
- Marijn
On 5/27/2022 12:38 PM, Dmitry Baryshkov wrote:
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for setting MISR registers within the interface
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 55 ++++++++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 ++- 2 files changed, 61 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..29aaeff9eacd 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,14 @@ #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 +#define INTF_MISR_FRAME_COUNT_MASK 0xFF +#define INTF_MISR_CTRL_ENABLE BIT(8) +#define INTF_MISR_CTRL_STATUS BIT(9) +#define INTF_MISR_CTRL_STATUS_CLEAR BIT(10) +#define INTF_MISR_CTRL_FREE_RUN_MASK BIT(31)
I'm tempted to ask to move these bits to some common header. Is there any other hardware block which uses the same bitfields to control MISR?
dpu_hw_lm.c has similar macros here [1] for _ENABLE, _STATUS, _STATUS_CLEAR, and _FREE_RUN_MASK
[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/d...
static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf, const struct dpu_mdss_cfg *m, void __iomem *addr, @@ -319,6 +329,47 @@ 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) +{ + struct dpu_hw_blk_reg_map *c = &intf->hw; + u32 config = 0;
+ DPU_REG_WRITE(c, INTF_MISR_CTRL, INTF_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 & INTF_MISR_FRAME_COUNT_MASK) | + INTF_MISR_CTRL_ENABLE | INTF_MISR_CTRL_FREE_RUN_MASK;
+ DPU_REG_WRITE(c, INTF_MISR_CTRL, config); + } else { + DPU_REG_WRITE(c, INTF_MISR_CTRL, 0); + } +}
+static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) +{ + struct dpu_hw_blk_reg_map *c = &intf->hw; + u32 ctrl = 0;
+ if (!misr_value) + return -EINVAL;
+ ctrl = DPU_REG_READ(c, INTF_MISR_CTRL);
+ if (!(ctrl & INTF_MISR_CTRL_ENABLE)) + return -ENODATA;
+ if (!(ctrl & INTF_MISR_CTRL_STATUS)) + return -EINVAL;
+ *misr_value = DPU_REG_READ(c, INTF_MISR_SIGNATURE);
+ return 0; +}
static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, unsigned long cap) { @@ -329,6 +380,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 {
-- With best wishes Dmitry
On 27/05/2022 23:11, Jessica Zhang wrote:
On 5/27/2022 12:38 PM, Dmitry Baryshkov wrote:
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for setting MISR registers within the interface
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 55 ++++++++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 ++- 2 files changed, 61 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..29aaeff9eacd 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,14 @@ #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 +#define INTF_MISR_FRAME_COUNT_MASK 0xFF +#define INTF_MISR_CTRL_ENABLE BIT(8) +#define INTF_MISR_CTRL_STATUS BIT(9) +#define INTF_MISR_CTRL_STATUS_CLEAR BIT(10) +#define INTF_MISR_CTRL_FREE_RUN_MASK BIT(31)
I'm tempted to ask to move these bits to some common header. Is there any other hardware block which uses the same bitfields to control MISR?
dpu_hw_lm.c has similar macros here [1] for _ENABLE, _STATUS, _STATUS_CLEAR, and _FREE_RUN_MASK
[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/d...
Please move bit definitions to dpu_hw_util.h. According to what I observe, this might be useful.
static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf, const struct dpu_mdss_cfg *m, void __iomem *addr, @@ -319,6 +329,47 @@ 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) +{ + struct dpu_hw_blk_reg_map *c = &intf->hw; + u32 config = 0;
+ DPU_REG_WRITE(c, INTF_MISR_CTRL, INTF_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 & INTF_MISR_FRAME_COUNT_MASK) | + INTF_MISR_CTRL_ENABLE | INTF_MISR_CTRL_FREE_RUN_MASK;
+ DPU_REG_WRITE(c, INTF_MISR_CTRL, config); + } else { + DPU_REG_WRITE(c, INTF_MISR_CTRL, 0); + }
This should also be abstracted. Please merge these functions with LM's and add corresponding helpers to dpu_hw_util.c
+}
+static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) +{ + struct dpu_hw_blk_reg_map *c = &intf->hw; + u32 ctrl = 0;
+ if (!misr_value) + return -EINVAL;
+ ctrl = DPU_REG_READ(c, INTF_MISR_CTRL);
+ if (!(ctrl & INTF_MISR_CTRL_ENABLE)) + return -ENODATA;
As the users of collect_misr() are going to ignore the -ENODATA, I think it would be worth changing this to set *misr_value to 0 and return 0 here. It would reduce the amount of boilerplate code.
+ if (!(ctrl & INTF_MISR_CTRL_STATUS)) + return -EINVAL;
+ *misr_value = DPU_REG_READ(c, INTF_MISR_SIGNATURE);
+ return 0; +}
static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, unsigned long cap) { @@ -329,6 +380,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 {
-- With best wishes Dmitry
On 6/2/2022 3:31 PM, Dmitry Baryshkov wrote:
On 27/05/2022 23:11, Jessica Zhang wrote:
On 5/27/2022 12:38 PM, Dmitry Baryshkov wrote:
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for setting MISR registers within the interface
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 55 ++++++++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 ++- 2 files changed, 61 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..29aaeff9eacd 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,14 @@ #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 +#define INTF_MISR_FRAME_COUNT_MASK 0xFF +#define INTF_MISR_CTRL_ENABLE BIT(8) +#define INTF_MISR_CTRL_STATUS BIT(9) +#define INTF_MISR_CTRL_STATUS_CLEAR BIT(10) +#define INTF_MISR_CTRL_FREE_RUN_MASK BIT(31)
I'm tempted to ask to move these bits to some common header. Is there any other hardware block which uses the same bitfields to control MISR?
dpu_hw_lm.c has similar macros here [1] for _ENABLE, _STATUS, _STATUS_CLEAR, and _FREE_RUN_MASK
[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/d...
Please move bit definitions to dpu_hw_util.h. According to what I observe, this might be useful.
Noted.
static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf, const struct dpu_mdss_cfg *m, void __iomem *addr, @@ -319,6 +329,47 @@ 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) +{ + struct dpu_hw_blk_reg_map *c = &intf->hw; + u32 config = 0;
+ DPU_REG_WRITE(c, INTF_MISR_CTRL, INTF_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 & INTF_MISR_FRAME_COUNT_MASK) | + INTF_MISR_CTRL_ENABLE | INTF_MISR_CTRL_FREE_RUN_MASK;
+ DPU_REG_WRITE(c, INTF_MISR_CTRL, config); + } else { + DPU_REG_WRITE(c, INTF_MISR_CTRL, 0); + }
This should also be abstracted. Please merge these functions with LM's and add corresponding helpers to dpu_hw_util.c
Good idea.
+}
+static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) +{ + struct dpu_hw_blk_reg_map *c = &intf->hw; + u32 ctrl = 0;
+ if (!misr_value) + return -EINVAL;
+ ctrl = DPU_REG_READ(c, INTF_MISR_CTRL);
+ if (!(ctrl & INTF_MISR_CTRL_ENABLE)) + return -ENODATA;
As the users of collect_misr() are going to ignore the -ENODATA, I think it would be worth changing this to set *misr_value to 0 and return 0 here. It would reduce the amount of boilerplate code.
0 might be a valid misr_value so it might be better to not write it to the debugfs. IMO we should also avoid writing to the debugfs if the misr is disabled -- that way we won't be spamming the debugfs with meaningless entries.
Thanks,
Jessica Zhang
+ if (!(ctrl & INTF_MISR_CTRL_STATUS)) + return -EINVAL;
+ *misr_value = DPU_REG_READ(c, INTF_MISR_SIGNATURE);
+ return 0; +}
static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, unsigned long cap) { @@ -329,6 +380,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 {
-- With best wishes Dmitry
-- With best wishes Dmitry
On Fri, 3 Jun 2022 at 04:00, Jessica Zhang quic_jesszhan@quicinc.com wrote:
On 6/2/2022 3:31 PM, Dmitry Baryshkov wrote:
On 27/05/2022 23:11, Jessica Zhang wrote:
On 5/27/2022 12:38 PM, Dmitry Baryshkov wrote:
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for setting MISR registers within the interface
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 55 ++++++++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 ++- 2 files changed, 61 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..29aaeff9eacd 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.
*/ #include "dpu_hwio.h"
- Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
@@ -67,6 +69,14 @@ #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 +#define INTF_MISR_FRAME_COUNT_MASK 0xFF +#define INTF_MISR_CTRL_ENABLE BIT(8) +#define INTF_MISR_CTRL_STATUS BIT(9) +#define INTF_MISR_CTRL_STATUS_CLEAR BIT(10) +#define INTF_MISR_CTRL_FREE_RUN_MASK BIT(31)
I'm tempted to ask to move these bits to some common header. Is there any other hardware block which uses the same bitfields to control MISR?
dpu_hw_lm.c has similar macros here [1] for _ENABLE, _STATUS, _STATUS_CLEAR, and _FREE_RUN_MASK
[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/d...
Please move bit definitions to dpu_hw_util.h. According to what I observe, this might be useful.
Noted.
- static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf, const struct dpu_mdss_cfg *m, void __iomem *addr,
@@ -319,6 +329,47 @@ 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) +{
- struct dpu_hw_blk_reg_map *c = &intf->hw;
- u32 config = 0;
- DPU_REG_WRITE(c, INTF_MISR_CTRL, INTF_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 & INTF_MISR_FRAME_COUNT_MASK) |INTF_MISR_CTRL_ENABLE | INTF_MISR_CTRL_FREE_RUN_MASK;DPU_REG_WRITE(c, INTF_MISR_CTRL, config);- } else {
DPU_REG_WRITE(c, INTF_MISR_CTRL, 0);- }
This should also be abstracted. Please merge these functions with LM's and add corresponding helpers to dpu_hw_util.c
Good idea.
+}
+static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32 *misr_value) +{
- struct dpu_hw_blk_reg_map *c = &intf->hw;
- u32 ctrl = 0;
- if (!misr_value)
return -EINVAL;- ctrl = DPU_REG_READ(c, INTF_MISR_CTRL);
- if (!(ctrl & INTF_MISR_CTRL_ENABLE))
return -ENODATA;As the users of collect_misr() are going to ignore the -ENODATA, I think it would be worth changing this to set *misr_value to 0 and return 0 here. It would reduce the amount of boilerplate code.
0 might be a valid misr_value so it might be better to not write it to the debugfs. IMO we should also avoid writing to the debugfs if the misr is disabled -- that way we won't be spamming the debugfs with meaningless entries.
Ack, true. Then I'd ask to change encoder code to skip crcs entries corresponding to the phys_encs with no hw_intf bound.
Add support for writing CRC values for the interface block to the debugfs by calling the necessary MISR setup/collect methods.
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ 4 files changed, 128 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index ae09466663cf..e830fb1e910d 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; } @@ -94,8 +96,18 @@ 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 = get_encoder_from_crtc(crtc); + + if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -ENODATA; + } + + *values_cnt = dpu_encoder_get_num_phys(drm_enc); + }
return 0; } @@ -116,6 +128,18 @@ 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 = get_encoder_from_crtc(crtc); + + if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return; + } + + 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); @@ -164,6 +188,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); @@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt drm_crtc_accurate_vblank_count(crtc), crcs); }
+static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) +{ + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc); + + if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -EINVAL; + } + + return dpu_encoder_get_crc(drm_enc); +} + static int dpu_crtc_get_crc(struct drm_crtc *crtc) { struct dpu_crtc_state *crtc_state; @@ -227,6 +265,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 b8785c394fcc..a60af034905d 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..7740515f462d 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,66 @@ 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; + + dpu_enc = to_dpu_encoder_virt(drm_enc); + + return dpu_enc->num_phys_encs; +} + +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) +{ + struct dpu_encoder_virt *dpu_enc; + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; + + int i, rc; + + 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[i]); + + if (rc) { + if (rc != -ENODATA) + DRM_DEBUG_DRIVER("MISR read failed\n"); + return rc; + } + } + + return drm_crtc_add_crc_entry(drm_enc->crtc, true, + drm_crtc_accurate_vblank_count(drm_enc->crtc), crcs); +} + 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..8345599dd01a 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); + /** * dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology. * @drm_enc: Pointer to previously created drm encoder structure
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for writing CRC values for the interface block to the debugfs by calling the necessary MISR setup/collect methods.
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ 4 files changed, 128 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index ae09466663cf..e830fb1e910d 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; }
@@ -94,8 +96,18 @@ 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 = get_encoder_from_crtc(crtc);
Let's do this correctly from the beginning. The CRTC can drive several encoders. Do we want to get CRC from all of them or just from the first one?
if (!drm_enc) {DRM_ERROR("no encoder found for crtc %d\n", crtc->index);return -ENODATA;}*values_cnt = dpu_encoder_get_num_phys(drm_enc);}
return 0; }
@@ -116,6 +128,18 @@ 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 = get_encoder_from_crtc(crtc);
- if (!drm_enc) {
DRM_ERROR("no encoder found for crtc %d\n", crtc->index);return;- }
- 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);
@@ -164,6 +188,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);
@@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt drm_crtc_accurate_vblank_count(crtc), crcs); }
+static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) +{
- struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc);
- if (!drm_enc) {
DRM_ERROR("no encoder found for crtc %d\n", crtc->index);return -EINVAL;- }
- return dpu_encoder_get_crc(drm_enc);
+}
- static int dpu_crtc_get_crc(struct drm_crtc *crtc) { struct dpu_crtc_state *crtc_state;
@@ -227,6 +265,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 b8785c394fcc..a60af034905d 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
*/ enum dpu_crtc_crc_source { DPU_CRTC_CRC_SOURCE_NONE = 0, DPU_CRTC_CRC_SOURCE_LAYER_MIXER,
- @DPU_CRTC_CRC_SOURCE_INTF: CRC in phys interface
- @DPU_CRTC_CRC_SOURCE_INVALID: Invalid source
- 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..7740515f462d 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,66 @@ 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;
- dpu_enc = to_dpu_encoder_virt(drm_enc);
- return dpu_enc->num_phys_encs;
+}
+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;
Does WB support CRC?
phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);- }
+}
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) +{
- struct dpu_encoder_virt *dpu_enc;
- u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
- int i, rc;
- 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[i]);
This doesn't look fully correct. Do we need to skip the indices for the phys without a backing hw_intf?
Extra empty line.
if (rc) {if (rc != -ENODATA)
Do we need to handle ENODATA in any specific way (like zeroing the crcs[i])? If not, I'd suggest to drop this return code. Let's make an error always an error.
DRM_DEBUG_DRIVER("MISR read failed\n");return rc;}- }
- return drm_crtc_add_crc_entry(drm_enc->crtc, true,
drm_crtc_accurate_vblank_count(drm_enc->crtc), crcs);+}
- 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..8345599dd01a 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);
- /**
- dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology.
- @drm_enc: Pointer to previously created drm encoder structure
On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote:
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for writing CRC values for the interface block to the debugfs by calling the necessary MISR setup/collect methods.
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ 4 files changed, 128 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index ae09466663cf..e830fb1e910d 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; } @@ -94,8 +96,18 @@ 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 = get_encoder_from_crtc(crtc);
Let's do this correctly from the beginning. The CRTC can drive several encoders. Do we want to get CRC from all of them or just from the first one?
In the case of multiple encoders, it would be better to collect CRCs from all of them.
+ if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -ENODATA; + }
+ *values_cnt = dpu_encoder_get_num_phys(drm_enc); + } return 0; } @@ -116,6 +128,18 @@ 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 = get_encoder_from_crtc(crtc);
+ if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return; + }
+ 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); @@ -164,6 +188,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); @@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt drm_crtc_accurate_vblank_count(crtc), crcs); } +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) +{ + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc);
+ if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -EINVAL; + }
+ return dpu_encoder_get_crc(drm_enc); +}
static int dpu_crtc_get_crc(struct drm_crtc *crtc) { struct dpu_crtc_state *crtc_state; @@ -227,6 +265,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 b8785c394fcc..a60af034905d 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..7740515f462d 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,66 @@ 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;
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ return dpu_enc->num_phys_encs; +}
+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;
Does WB support CRC?
AFAIK, no.
+ phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); + } +}
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) +{ + struct dpu_encoder_virt *dpu_enc; + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
+ int i, rc;
+ 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[i]);
This doesn't look fully correct. Do we need to skip the indices for the phys without a backing hw_intf?
Sorry if I'm misunderstanding your question, but don't we need to have a backing hw_intf (and skip if there isn't any) since the methods for collecting/setting MISR registers is within the hw_intf?
Extra empty line.
Noted
+ if (rc) { + if (rc != -ENODATA)
Do we need to handle ENODATA in any specific way (like zeroing the crcs[i])? If not, I'd suggest to drop this return code. Let's make an error always an error.
This is a carry-over from this change [1]. We wanted to have the ENODATA check to avoid spamming the driver debug logs when CRC is disabled for this block.
[1] https://gitlab.freedesktop.org/drm/msm/-/commit/3ce8bdca394fc606b55e7c5ed779...
Thanks, Jessica Zhang
+ DRM_DEBUG_DRIVER("MISR read failed\n"); + return rc; + } + }
+ return drm_crtc_add_crc_entry(drm_enc->crtc, true, + drm_crtc_accurate_vblank_count(drm_enc->crtc), crcs); +}
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..8345599dd01a 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);
/** * dpu_encoder_use_dsc_merge - returns true if the encoder uses DSC merge topology. * @drm_enc: Pointer to previously created drm encoder structure
-- With best wishes Dmitry
On 28/05/2022 01:23, Jessica Zhang wrote:
On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote:
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for writing CRC values for the interface block to the debugfs by calling the necessary MISR setup/collect methods.
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ 4 files changed, 128 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index ae09466663cf..e830fb1e910d 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; } @@ -94,8 +96,18 @@ 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 = get_encoder_from_crtc(crtc);
Let's do this correctly from the beginning. The CRTC can drive several encoders. Do we want to get CRC from all of them or just from the first one?
In the case of multiple encoders, it would be better to collect CRCs from all of them.
Then this should become a loop.
+ if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -ENODATA; + }
+ *values_cnt = dpu_encoder_get_num_phys(drm_enc); + } return 0; } @@ -116,6 +128,18 @@ 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 = get_encoder_from_crtc(crtc);
+ if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return; + }
+ 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); @@ -164,6 +188,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); @@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt drm_crtc_accurate_vblank_count(crtc), crcs); } +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) +{ + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc);
+ if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -EINVAL; + }
+ return dpu_encoder_get_crc(drm_enc); +}
static int dpu_crtc_get_crc(struct drm_crtc *crtc) { struct dpu_crtc_state *crtc_state; @@ -227,6 +265,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 b8785c394fcc..a60af034905d 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..7740515f462d 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,66 @@ 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;
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ return dpu_enc->num_phys_encs; +}
+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;
Does WB support CRC?
AFAIK, no.
+ phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); + } +}
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) +{ + struct dpu_encoder_virt *dpu_enc; + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
+ int i, rc;
+ 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[i]);
This doesn't look fully correct. Do we need to skip the indices for the phys without a backing hw_intf?
Sorry if I'm misunderstanding your question, but don't we need to have a backing hw_intf (and skip if there isn't any) since the methods for collecting/setting MISR registers is within the hw_intf?
Yes. So the question if we should skip the phys and leave the crcs[i] untouched, skip the phys and sset crcs[i] to 0 or change dpu_crtc_parse_crc_source() to return the number of intf-backed phys_enc's and do not skip any crcs[i].
Extra empty line.
Noted
+ if (rc) { + if (rc != -ENODATA)
Do we need to handle ENODATA in any specific way (like zeroing the crcs[i])? If not, I'd suggest to drop this return code. Let's make an error always an error.
This is a carry-over from this change [1]. We wanted to have the ENODATA check to avoid spamming the driver debug logs when CRC is disabled for this block.
[1] https://gitlab.freedesktop.org/drm/msm/-/commit/3ce8bdca394fc606b55e7c5ed779...
Thanks for the reminder. I commented this in the previous patch now.
On 6/2/2022 3:51 PM, Dmitry Baryshkov wrote:
On 28/05/2022 01:23, Jessica Zhang wrote:
On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote:
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for writing CRC values for the interface block to the debugfs by calling the necessary MISR setup/collect methods.
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 43 ++++++++++++++- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 61 +++++++++++++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++++++++ 4 files changed, 128 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index ae09466663cf..e830fb1e910d 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; } @@ -94,8 +96,18 @@ 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 = get_encoder_from_crtc(crtc);
Let's do this correctly from the beginning. The CRTC can drive several encoders. Do we want to get CRC from all of them or just from the first one?
In the case of multiple encoders, it would be better to collect CRCs from all of them.
Then this should become a loop.
Ah, I see what you mean. Yea, I can do that.
+ if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -ENODATA; + }
+ *values_cnt = dpu_encoder_get_num_phys(drm_enc); + } return 0; } @@ -116,6 +128,18 @@ 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 = get_encoder_from_crtc(crtc);
+ if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return; + }
+ 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); @@ -164,6 +188,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); @@ -212,6 +238,18 @@ static int dpu_crtc_get_lm_crc(struct drm_crtc *crtc, struct dpu_crtc_state *crt drm_crtc_accurate_vblank_count(crtc), crcs); } +static int dpu_crtc_get_encoder_crc(struct drm_crtc *crtc) +{ + struct drm_encoder *drm_enc = get_encoder_from_crtc(crtc);
+ if (!drm_enc) { + DRM_ERROR("no encoder found for crtc %d\n", crtc->index); + return -EINVAL; + }
+ return dpu_encoder_get_crc(drm_enc); +}
static int dpu_crtc_get_crc(struct drm_crtc *crtc) { struct dpu_crtc_state *crtc_state; @@ -227,6 +265,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 b8785c394fcc..a60af034905d 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..7740515f462d 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,66 @@ 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;
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ return dpu_enc->num_phys_encs; +}
+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;
Does WB support CRC?
AFAIK, no.
+ phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1); + } +}
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) +{ + struct dpu_encoder_virt *dpu_enc; + u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
+ int i, rc;
+ 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[i]);
This doesn't look fully correct. Do we need to skip the indices for the phys without a backing hw_intf?
Sorry if I'm misunderstanding your question, but don't we need to have a backing hw_intf (and skip if there isn't any) since the methods for collecting/setting MISR registers is within the hw_intf?
Yes. So the question if we should skip the phys and leave the crcs[i] untouched, skip the phys and sset crcs[i] to 0 or change dpu_crtc_parse_crc_source() to return the number of intf-backed phys_enc's and do not skip any crcs[i].
Thanks for the clarification.
Is it possible to hit a case where a phys_encoder won't have a corresponding hw_intf?
AFAIK, it seems guaranteed that a phys_encoder will have an hw_intf since dpu_encoder_setup_display will skip incrementing num_phys_encs if dpu_encoder_get_intf fails [1].
[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/d...
Thanks,
Jessica Zhang
Extra empty line.
Noted
+ if (rc) { + if (rc != -ENODATA)
Do we need to handle ENODATA in any specific way (like zeroing the crcs[i])? If not, I'd suggest to drop this return code. Let's make an error always an error.
This is a carry-over from this change [1]. We wanted to have the ENODATA check to avoid spamming the driver debug logs when CRC is disabled for this block.
[1] https://gitlab.freedesktop.org/drm/msm/-/commit/3ce8bdca394fc606b55e7c5ed779...
Thanks for the reminder. I commented this in the previous patch now.
-- With best wishes Dmitry
On Fri, 3 Jun 2022 at 04:02, Jessica Zhang quic_jesszhan@quicinc.com wrote:
On 6/2/2022 3:51 PM, Dmitry Baryshkov wrote:
On 28/05/2022 01:23, Jessica Zhang wrote:
On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote:
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for writing CRC values for the interface block to the debugfs by calling the necessary MISR setup/collect methods.
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
[skipped]
phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);- }
+}
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) +{
- struct dpu_encoder_virt *dpu_enc;
- u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
- int i, rc;
- 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[i]);This doesn't look fully correct. Do we need to skip the indices for the phys without a backing hw_intf?
Sorry if I'm misunderstanding your question, but don't we need to have a backing hw_intf (and skip if there isn't any) since the methods for collecting/setting MISR registers is within the hw_intf?
Yes. So the question if we should skip the phys and leave the crcs[i] untouched, skip the phys and sset crcs[i] to 0 or change dpu_crtc_parse_crc_source() to return the number of intf-backed phys_enc's and do not skip any crcs[i].
Thanks for the clarification.
Is it possible to hit a case where a phys_encoder won't have a corresponding hw_intf?
AFAIK, it seems guaranteed that a phys_encoder will have an hw_intf since dpu_encoder_setup_display will skip incrementing num_phys_encs if dpu_encoder_get_intf fails [1].
WB encoders won't have hw_intf. The code checks that either get_intf or get_wb succeeds.
[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/d...
On 6/3/2022 12:02 AM, Dmitry Baryshkov wrote:
On Fri, 3 Jun 2022 at 04:02, Jessica Zhang quic_jesszhan@quicinc.com wrote:
On 6/2/2022 3:51 PM, Dmitry Baryshkov wrote:
On 28/05/2022 01:23, Jessica Zhang wrote:
On 5/27/2022 12:46 PM, Dmitry Baryshkov wrote:
On 27/05/2022 21:54, Jessica Zhang wrote:
Add support for writing CRC values for the interface block to the debugfs by calling the necessary MISR setup/collect methods.
Signed-off-by: Jessica Zhang quic_jesszhan@quicinc.com
[skipped]
phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);- }
+}
+int dpu_encoder_get_crc(const struct drm_encoder *drm_enc) +{
- struct dpu_encoder_virt *dpu_enc;
- u32 crcs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
- int i, rc;
- 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[i]);This doesn't look fully correct. Do we need to skip the indices for the phys without a backing hw_intf?
Sorry if I'm misunderstanding your question, but don't we need to have a backing hw_intf (and skip if there isn't any) since the methods for collecting/setting MISR registers is within the hw_intf?
Yes. So the question if we should skip the phys and leave the crcs[i] untouched, skip the phys and sset crcs[i] to 0 or change dpu_crtc_parse_crc_source() to return the number of intf-backed phys_enc's and do not skip any crcs[i].
Thanks for the clarification.
Is it possible to hit a case where a phys_encoder won't have a corresponding hw_intf?
AFAIK, it seems guaranteed that a phys_encoder will have an hw_intf since dpu_encoder_setup_display will skip incrementing num_phys_encs if dpu_encoder_get_intf fails [1].
WB encoders won't have hw_intf. The code checks that either get_intf or get_wb succeeds.
Got it, I see your point. I'll change the values_cnt to only include the intf-backed phys_encoders then.
Thanks,
Jessica Zhang
[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/d...
-- With best wishes Dmitry
dri-devel@lists.freedesktop.org
-
Dmitry Baryshkov -
Jessica Zhang -
Marijn Suijten