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@xxxxxxxxxxx> --- 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.cindex 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 *crtdrm_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.hindex 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@xxxxxxxxx> @@ -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.cindex 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/disp/dpu1/dpu_encoder.c#L2263
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/3ce8bdca394fc606b55e7c5ed779d171aaae5d09Thanks for the reminder. I commented this in the previous patch now. -- With best wishes Dmitry
