[PATCH 0/3] Expand CRC to support interface blocks

[PATCH 0/3] Expand CRC to support interface blocks

[Jessica Zhang]

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(-)

-- 
2.35.1

[PATCH 0/3] Expand CRC to support interface blocks

[Jessica Zhang]

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(-)

-- 
2.35.1

[PATCH 1/3] drm/msm/dpu: Move LM CRC code into separate method

[Jessica Zhang]

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,
-- 
2.35.1

[PATCH 1/3] drm/msm/dpu: Move LM CRC code into separate method

[Jessica Zhang]

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,
-- 
2.35.1

Re: [PATCH 1/3] drm/msm/dpu: Move LM CRC code into separate method

[Dmitry Baryshkov]

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,


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/msm/dpu: Move LM CRC code into separate method

[Dmitry Baryshkov]

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,


-- 
With best wishes
Dmitry

[PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Jessica Zhang]

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 {
-- 
2.35.1

[PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Jessica Zhang]

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 {
-- 
2.35.1

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Dmitry Baryshkov]

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
> + * @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

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Dmitry Baryshkov]

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
> + * @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

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Marijn Suijten]

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

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Marijn Suijten]

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

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Jessica Zhang]

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/disp/dpu1/dpu_hw_lm.c#L31

> 
>> +
>>   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

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Jessica Zhang]

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/disp/dpu1/dpu_hw_lm.c#L31

> 
>> +
>>   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

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Dmitry Baryshkov]

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/disp/dpu1/dpu_hw_lm.c#L31 

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


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Dmitry Baryshkov]

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/disp/dpu1/dpu_hw_lm.c#L31 

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


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Jessica Zhang]

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/disp/dpu1/dpu_hw_lm.c#L31 
> 
> 
> 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

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Jessica Zhang]

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/disp/dpu1/dpu_hw_lm.c#L31 
> 
> 
> 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

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Dmitry Baryshkov]

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.
> >>>> + * 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/disp/dpu1/dpu_hw_lm.c#L31
> >
> >
> > 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.


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Dmitry Baryshkov]

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.
> >>>> + * 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/disp/dpu1/dpu_hw_lm.c#L31
> >
> >
> > 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.


-- 
With best wishes
Dmitry

[PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Jessica Zhang]

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
-- 
2.35.1

[PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Jessica Zhang]

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
-- 
2.35.1

Re: [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Dmitry Baryshkov]

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
> + * @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?

> +
> +		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


-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Dmitry Baryshkov]

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
> + * @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?

> +
> +		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


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Jessica Zhang]

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/3ce8bdca394fc606b55e7c5ed779d171aaae5d09

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

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Jessica Zhang]

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/3ce8bdca394fc606b55e7c5ed779d171aaae5d09

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

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Dmitry Baryshkov]

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/3ce8bdca394fc606b55e7c5ed779d171aaae5d09 

Thanks for the reminder. I commented this in the previous patch now.



-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Dmitry Baryshkov]

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/3ce8bdca394fc606b55e7c5ed779d171aaae5d09 

Thanks for the reminder. I commented this in the previous patch now.



-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Jessica Zhang]

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/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/3ce8bdca394fc606b55e7c5ed779d171aaae5d09 
> 
> 
> Thanks for the reminder. I commented this in the previous patch now.
> 
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Jessica Zhang]

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/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/3ce8bdca394fc606b55e7c5ed779d171aaae5d09 
> 
> 
> Thanks for the reminder. I commented this in the previous patch now.
> 
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Dmitry Baryshkov]

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/disp/dpu1/dpu_encoder.c#L2263


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Dmitry Baryshkov]

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/disp/dpu1/dpu_encoder.c#L2263


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Jessica Zhang]

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/disp/dpu1/dpu_encoder.c#L2263
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [Freedreno] [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs

[Jessica Zhang]

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/disp/dpu1/dpu_encoder.c#L2263
> 
> 
> -- 
> With best wishes
> Dmitry