[PATCH 00/10] Improve crc-core driver interface

[PATCH 00/10] Improve crc-core driver interface

[Mahesh Kumar]

This series improves crc-core <-> driver interface.
This series adds following functionality in the crc-core
 - Now control node will print all the available sources if
   implemented by driver along with current source.
 - Setting of sorce will fail if provided source is not supported
 - cleanup of crtc_crc_open function first allocate memory before
   enabling CRC generation
 - Don't block open() call instead wait in crc read call.

Following IGT will fail due to crc-core <-> driver interface change
igt@kms_pipe_crc_basic@bad-source <now setting bad-source itself will fail>
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X 
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X-frame-sequence
In nonblocking crc tests we'll get lesser crc's due to skipping crc

AMD/Rockchip/rcar code path is not validated and need inputs

Mahesh Kumar (10):
  drm: crc: Introduce verify_crc_source callback
  drm: crc: Introduce pre_crc_read function
  drm: crc: Introduce get_crc_sources callback
  drm/rockchip/crc: Implement verify_crc_source callback
  drm/amdgpu_dm/crc: Implement verify_crc_source callback
  drm/rcar-du/crc: Implement verify_crc_source callback
  drm/i915/crc: implement verify_crc_source callback
  drm/i915/crc: implement get_crc_sources callback
  drm/crc: Cleanup crtc_crc_open function
  Revert "drm: crc: Wait for a frame before returning from open()"

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   6 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
 drivers/gpu/drm/drm_debugfs_crc.c                  |  62 ++++++-----
 drivers/gpu/drm/i915/intel_display.c               |   2 +
 drivers/gpu/drm/i915/intel_drv.h                   |   8 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              | 124 ++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  47 +++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  23 +++-
 include/drm/drm_crtc.h                             |  42 ++++++-
 10 files changed, 286 insertions(+), 49 deletions(-)

-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Mahesh Kumar]

This patch adds a new callback function "verify_crc_source" which will
be used during setting the crc source in control node and while opening
data node for crc reading. This will help in avoiding setting of wrong
string for source.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
 include/drm/drm_crtc.h            | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 9f8312137cad..c6a725b79ac6 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	struct drm_crtc *crtc = m->private;
 	struct drm_crtc_crc *crc = &crtc->crc;
 	char *source;
+	size_t values_cnt;
+	int ret = 0;
 
 	if (len == 0)
 		return 0;
@@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 
 	if (crc->opened) {
@@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 			return ret;
 	}
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
+						     &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 	if (!crc->opened)
 		crc->opened = true;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 23eddbccab10..1a6dcbf91744 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -661,6 +661,21 @@ struct drm_crtc_funcs {
 	 */
 	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
 			      size_t *values_cnt);
+	/**
+	 * @verify_crc_source:
+	 *
+	 * verifies the source of CRC checksums of frames before setting the
+	 * source for CRC and during crc open.
+	 *
+	 * This callback is optional if the driver does not support any CRC
+	 * generation functionality.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
+				 size_t *values_cnt);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/10] drm: crc: Introduce pre_crc_read function

[Mahesh Kumar]

This patch implements a callback function "pre_crc_read" which will
be called before crc read. In this function driver can implement and
preparation work required for successfully reading CRC data.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
 include/drm/drm_crtc.h            | 14 ++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index c6a725b79ac6..2b4a737c5aeb 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -278,6 +278,14 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
 		return 0;
 	}
 
+	if (crtc->funcs->pre_crc_read) {
+		ret = crtc->funcs->pre_crc_read(crtc);
+		if (ret) {
+			spin_unlock_irq(&crc->lock);
+			return ret;
+		}
+	}
+
 	/* Nothing to read? */
 	while (crtc_crc_data_count(crc) == 0) {
 		if (filep->f_flags & O_NONBLOCK) {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1a6dcbf91744..bae432469616 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -676,6 +676,20 @@ struct drm_crtc_funcs {
 	 */
 	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
 				 size_t *values_cnt);
+	/**
+	 * @pre_crc_read:
+	 *
+	 * Driver callback for performing any preparation work required by
+	 * driver before reading CRC
+	 *
+	 * This callback is optional if the driver does not support CRC
+	 * generation or no prework is required before reading the crc
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*pre_crc_read)(struct drm_crtc *crtc);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/10] drm: crc: Introduce get_crc_sources callback

[Mahesh Kumar]

This patch implements a callback function "get_crc_sources" which
will be called during read of control node. It is an optional
callback function and if driver implements this callback, driver
should print list of available CRC sources in seq_file privided
as an input to the callback.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c |  3 +++
 include/drm/drm_crtc.h            | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 2b4a737c5aeb..2751d124387d 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -68,6 +68,9 @@ static int crc_control_show(struct seq_file *m, void *data)
 {
 	struct drm_crtc *crtc = m->private;
 
+	if (crtc->funcs->get_crc_sources)
+		crtc->funcs->get_crc_sources(m, crtc);
+
 	seq_printf(m, "%s\n", crtc->crc.source);
 
 	return 0;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index bae432469616..b3824f92c190 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -690,6 +690,16 @@ struct drm_crtc_funcs {
 	 * 0 on success or a negative error code on failure.
 	 */
 	int (*pre_crc_read)(struct drm_crtc *crtc);
+	/**
+	 * @get_crc_sources:
+	 *
+	 * prints a list of available valid sources for CRC generation on
+	 * seq_file
+	 *
+	 * This callback is optional if the driver does not support exporting of
+	 * possible CRC sources list
+	 */
+	void (*get_crc_sources)(struct seq_file *m, struct drm_crtc *crtc);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/10] drm/rockchip/crc: Implement verify_crc_source callback

[Mahesh Kumar]

This patch implements "verify_crc_source" callback function for
rockchip drm driver.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c9222119767d..0629266c8398 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1138,12 +1138,30 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
 
 	return ret;
 }
+
+static int vop_crtc_verify_crc_source(struct drm_crtc *crtc,
+				   const char *source_name, size_t *values_cnt)
+{
+	if ((source_name && strcmp(source_name, "auto") == 0) || !source_name) {
+		*values_cnt = 3;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 #else
 static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
 				   const char *source_name, size_t *values_cnt)
 {
 	return -ENODEV;
 }
+
+static int vop_crtc_verify_crc_source(struct drm_crtc *crtc,
+				   const char *source_name, size_t *values_cnt)
+{
+	return -ENODEV;
+}
 #endif
 
 static const struct drm_crtc_funcs vop_crtc_funcs = {
@@ -1156,6 +1174,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.enable_vblank = vop_crtc_enable_vblank,
 	.disable_vblank = vop_crtc_disable_vblank,
 	.set_crc_source = vop_crtc_set_crc_source,
+	.verify_crc_source = vop_crtc_verify_crc_source,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/10] drm/amdgpu_dm/crc: Implement verify_crc_source callback

[Mahesh Kumar]

This patch implements "verify_crc_source" callback function for
AMD drm driver.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c     |  1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h     |  3 +++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 16 ++++++++++++++++
 3 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a1dd49545a5b..95aec2903add 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2565,6 +2565,7 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
 	.atomic_duplicate_state = dm_crtc_duplicate_state,
 	.atomic_destroy_state = dm_crtc_destroy_state,
 	.set_crc_source = amdgpu_dm_crtc_set_crc_source,
+	.verify_crc_source = amdgpu_dm_crtc_verify_crc_source,
 	.enable_vblank = dm_enable_vblank,
 	.disable_vblank = dm_disable_vblank,
 };
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index d5aa89ad5571..e9a6e73ba299 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -263,9 +263,12 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
 #ifdef CONFIG_DEBUG_FS
 int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 				  size_t *values_cnt);
+int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
+				     size_t *values_cnt);
 void amdgpu_dm_crtc_handle_crc_irq(struct drm_crtc *crtc);
 #else
 #define amdgpu_dm_crtc_set_crc_source NULL
+#define amdgpu_dm_crtc_verify_crc_source NULL
 #define amdgpu_dm_crtc_handle_crc_irq(x)
 #endif
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index 52f2c01349e3..dfcca594d52a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -46,6 +46,22 @@ static enum amdgpu_dm_pipe_crc_source dm_parse_crc_source(const char *source)
 	return AMDGPU_DM_PIPE_CRC_SOURCE_INVALID;
 }
 
+int
+amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
+				 size_t *values_cnt)
+{
+	enum amdgpu_dm_pipe_crc_source source = dm_parse_crc_source(src_name);
+
+	if (source < 0) {
+		DRM_DEBUG_DRIVER("Unknown CRC source %s for CRTC%d\n",
+				 src_name, crtc->index);
+		return -EINVAL;
+	}
+
+	*values_cnt = 3;
+	return 0;
+}
+
 int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 			   size_t *values_cnt)
 {
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/10] drm/rcar-du/crc: Implement verify_crc_source callback

[Mahesh Kumar]

This patch implements "verify_crc_source" callback function for
rcar drm driver.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 42 ++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 15dc9caa128b..d28e7510f8a1 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -756,6 +756,47 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc)
 	rcrtc->vblank_enable = false;
 }
 
+
+static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
+					  const char *source_name,
+					  size_t *values_cnt)
+{
+	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+	unsigned int index = 0;
+	unsigned int i;
+	int ret;
+
+	/*
+	 * Parse the source name. Supported values are "plane%u" to compute the
+	 * CRC on an input plane (%u is the plane ID), and "auto" to compute the
+	 * CRC on the composer (VSP) output.
+	 */
+	if (!source_name || !strcmp(source_name, "auto")) {
+		goto out;
+	} else if (strstarts(source_name, "plane")) {
+
+		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
+			if (index == rcrtc->vsp->planes[i].plane.base.id) {
+				index = i;
+				break;
+			}
+		}
+
+		if (i >= rcrtc->vsp->num_planes)
+			return -EINVAL;
+	} else {
+		return -EINVAL;
+	}
+
+out:
+	*values_cnt = 1;
+	return 0;
+}
+
 static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
 				       const char *source_name,
 				       size_t *values_cnt)
@@ -861,6 +902,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
 	.enable_vblank = rcar_du_crtc_enable_vblank,
 	.disable_vblank = rcar_du_crtc_disable_vblank,
 	.set_crc_source = rcar_du_crtc_set_crc_source,
+	.verify_crc_source = rcar_du_crtc_verify_crc_source,
 };
 
 /* -----------------------------------------------------------------------------
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/10] drm/i915/crc: implement verify_crc_source callback

[Mahesh Kumar]

This patch implements verify_crc_source callback function introduced
earlier in this series.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |   1 +
 drivers/gpu/drm/i915/intel_drv.h      |   3 +
 drivers/gpu/drm/i915/intel_pipe_crc.c | 109 ++++++++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9cdc134b95cb..8951734b1afa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12865,6 +12865,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
 	.set_crc_source = intel_crtc_set_crc_source,
+	.verify_crc_source = intel_crtc_verify_crc_source,
 };
 
 struct wait_rps_boost {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 04d61897b044..083d40eae5c8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2161,10 +2161,13 @@ int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
+int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
+				 const char *source_name, size_t *values_cnt);
 void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
 void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
+#define intel_crtc_verify_crc_source NULL
 static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
 {
 }
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 39a4e4edda07..65148c0476fc 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -913,6 +913,115 @@ int intel_pipe_crc_create(struct drm_minor *minor)
 	return 0;
 }
 
+static int i8xx_crc_source_valid(struct drm_i915_private *dev_priv,
+				 const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int i9xx_crc_source_valid(struct drm_i915_private *dev_priv,
+				 const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_TV:
+	case INTEL_PIPE_CRC_SOURCE_DP_B:
+	case INTEL_PIPE_CRC_SOURCE_DP_C:
+	case INTEL_PIPE_CRC_SOURCE_DP_D:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vlv_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_DP_B:
+	case INTEL_PIPE_CRC_SOURCE_DP_C:
+	case INTEL_PIPE_CRC_SOURCE_DP_D:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ilk_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_PLANE1:
+	case INTEL_PIPE_CRC_SOURCE_PLANE2:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ivb_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_PLANE1:
+	case INTEL_PIPE_CRC_SOURCE_PLANE2:
+	case INTEL_PIPE_CRC_SOURCE_PF:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int
+intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
+			  const enum intel_pipe_crc_source source)
+{
+	if (IS_GEN2(dev_priv))
+		return i8xx_crc_source_valid(dev_priv, source);
+	else if (INTEL_GEN(dev_priv) < 5)
+		return i9xx_crc_source_valid(dev_priv, source);
+	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		return vlv_crc_source_valid(dev_priv, source);
+	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
+		return ilk_crc_source_valid(dev_priv, source);
+	else
+		return ivb_crc_source_valid(dev_priv, source);
+}
+
+
+int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
+				 size_t *values_cnt)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum intel_pipe_crc_source source;
+
+	if (display_crc_ctl_parse_source(source_name, &source) < 0) {
+		DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
+		return -EINVAL;
+	}
+
+	if (source == INTEL_PIPE_CRC_SOURCE_AUTO ||
+			intel_is_valid_crc_source(dev_priv, source) == 0) {
+		*values_cnt = 5;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt)
 {
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/10] drm/i915/crc: implement get_crc_sources callback

[Mahesh Kumar]

This patch implements get_crc_sources callback, which returns list of
all the valid crc sources supported by driver in current platform.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |  1 +
 drivers/gpu/drm/i915/intel_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 11 +++++++++++
 3 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8951734b1afa..fb49b8fdf06a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12866,6 +12866,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.atomic_destroy_state = intel_crtc_destroy_state,
 	.set_crc_source = intel_crtc_set_crc_source,
 	.verify_crc_source = intel_crtc_verify_crc_source,
+	.get_crc_sources = intel_crtc_get_crc_sources,
 };
 
 struct wait_rps_boost {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 083d40eae5c8..9f02fdc6b729 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2163,11 +2163,13 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
 				 const char *source_name, size_t *values_cnt);
+void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc);
 void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
 void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
 #define intel_crtc_verify_crc_source NULL
+#define intel_crtc_get_crc_sources NULL
 static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
 {
 }
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 65148c0476fc..82c4a56928c0 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -1001,6 +1001,17 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
 		return ivb_crc_source_valid(dev_priv, source);
 }
 
+void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum intel_pipe_crc_source source;
+
+	seq_puts(m, "[");
+	for (source = 0; source < INTEL_PIPE_CRC_SOURCE_MAX; source++)
+		if (intel_is_valid_crc_source(dev_priv, source) == 0)
+			seq_printf(m, "%s,", pipe_crc_source_name(source));
+	seq_puts(m, "auto] ");
+}
 
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 				 size_t *values_cnt)
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/10] drm/crc: Cleanup crtc_crc_open function

[Mahesh Kumar]

This patch make changes to allocate crc-entries buffer before
enabling CRC generation.
It moves all the failure check early in the function before setting
the source or memory allocation.
Now set_crc_source takes only two variable input, values_cnt we
already gets as part of verify_crc_source.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |  3 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  4 +-
 drivers/gpu/drm/drm_debugfs_crc.c                  | 52 +++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h                   |  3 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              |  4 +-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  5 +--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  6 +--
 include/drm/drm_crtc.h                             |  3 +-
 8 files changed, 30 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index e9a6e73ba299..0b0f6feea806 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -261,8 +261,7 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
 
 /* amdgpu_dm_crc.c */
 #ifdef CONFIG_DEBUG_FS
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
-				  size_t *values_cnt);
+int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name);
 int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
 				     size_t *values_cnt);
 void amdgpu_dm_crtc_handle_crc_irq(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index dfcca594d52a..e7ad528f5853 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -62,8 +62,7 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
 	return 0;
 }
 
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
-			   size_t *values_cnt)
+int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 {
 	struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
 	struct dc_stream_state *stream_state = crtc_state->stream;
@@ -99,7 +98,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 			return -EINVAL;
 	}
 
-	*values_cnt = 3;
 	/* Reset crc_skipped on dm state */
 	crtc_state->crc_skip_count = 0;
 	return 0;
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 2751d124387d..834bc7ee5550 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -109,11 +109,9 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
-	if (crtc->funcs->verify_crc_source) {
-		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
-		if (ret)
-			return ret;
-	}
+	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+	if (ret)
+		return ret;
 
 	spin_lock_irq(&crc->lock);
 
@@ -178,12 +176,15 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 			return ret;
 	}
 
-	if (crtc->funcs->verify_crc_source) {
-		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
-						     &values_cnt);
-		if (ret)
-			return ret;
-	}
+	ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt);
+	if (ret)
+		return ret;
+
+	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR))
+		return -EINVAL;
+
+	if (WARN_ON(values_cnt == 0))
+		return -EINVAL;
 
 	spin_lock_irq(&crc->lock);
 	if (!crc->opened)
@@ -195,30 +196,22 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	if (ret)
 		return ret;
 
-	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
-	if (ret)
-		goto err;
-
-	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
-		ret = -EINVAL;
-		goto err_disable;
-	}
-
-	if (WARN_ON(values_cnt == 0)) {
-		ret = -EINVAL;
-		goto err_disable;
-	}
-
 	entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
 	if (!entries) {
 		ret = -ENOMEM;
-		goto err_disable;
+		goto err;
 	}
 
 	spin_lock_irq(&crc->lock);
 	crc->entries = entries;
 	crc->values_cnt = values_cnt;
+	spin_unlock_irq(&crc->lock);
 
+	ret = crtc->funcs->set_crc_source(crtc, crc->source);
+	if (ret)
+		goto err;
+
+	spin_lock_irq(&crc->lock);
 	/*
 	 * Only return once we got a first frame, so userspace doesn't have to
 	 * guess when this particular piece of HW will be ready to start
@@ -235,7 +228,7 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	return 0;
 
 err_disable:
-	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+	crtc->funcs->set_crc_source(crtc, NULL);
 err:
 	spin_lock_irq(&crc->lock);
 	crtc_crc_cleanup(crc);
@@ -247,9 +240,8 @@ static int crtc_crc_release(struct inode *inode, struct file *filep)
 {
 	struct drm_crtc *crtc = filep->f_inode->i_private;
 	struct drm_crtc_crc *crc = &crtc->crc;
-	size_t values_cnt;
 
-	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+	crtc->funcs->set_crc_source(crtc, NULL);
 
 	spin_lock_irq(&crc->lock);
 	crtc_crc_cleanup(crc);
@@ -363,7 +355,7 @@ int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
 {
 	struct dentry *crc_ent, *ent;
 
-	if (!crtc->funcs->set_crc_source)
+	if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source)
 		return 0;
 
 	crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9f02fdc6b729..555c6118bb83 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2159,8 +2159,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
 /* intel_pipe_crc.c */
 int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
-int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
-			      size_t *values_cnt);
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name);
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
 				 const char *source_name, size_t *values_cnt);
 void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 82c4a56928c0..d8b9013d2e68 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -1033,8 +1033,7 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 	return -EINVAL;
 }
 
-int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
-			      size_t *values_cnt)
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
@@ -1073,7 +1072,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 	}
 
 	pipe_crc->skipped = 0;
-	*values_cnt = 5;
 
 out:
 	intel_display_power_put(dev_priv, power_domain);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index d28e7510f8a1..ceaded43d16a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -798,8 +798,7 @@ static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
 }
 
 static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
-				       const char *source_name,
-				       size_t *values_cnt)
+				       const char *source_name)
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct drm_modeset_acquire_ctx ctx;
@@ -839,8 +838,6 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	*values_cnt = 1;
-
 	/* Perform an atomic commit to set the CRC source. */
 	drm_modeset_acquire_init(&ctx, 0);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 0629266c8398..1f8899ff5742 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1117,7 +1117,7 @@ static struct drm_connector *vop_get_edp_connector(struct vop *vop)
 }
 
 static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
-				   const char *source_name, size_t *values_cnt)
+				   const char *source_name)
 {
 	struct vop *vop = to_vop(crtc);
 	struct drm_connector *connector;
@@ -1127,8 +1127,6 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
 	if (!connector)
 		return -EINVAL;
 
-	*values_cnt = 3;
-
 	if (source_name && strcmp(source_name, "auto") == 0)
 		ret = analogix_dp_start_crc(connector);
 	else if (!source_name)
@@ -1152,7 +1150,7 @@ static int vop_crtc_verify_crc_source(struct drm_crtc *crtc,
 
 #else
 static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
-				   const char *source_name, size_t *values_cnt)
+				   const char *source_name)
 {
 	return -ENODEV;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index b3824f92c190..257b3d738d94 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -659,8 +659,7 @@ struct drm_crtc_funcs {
 	 *
 	 * 0 on success or a negative error code on failure.
 	 */
-	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
-			      size_t *values_cnt);
+	int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
 	/**
 	 * @verify_crc_source:
 	 *
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/10] Revert "drm: crc: Wait for a frame before returning from open()"

[Mahesh Kumar]

This reverts commit e8fa5671183c80342d520ad81d14fa79a9d4a680.

Don't wait for first CRC during crtc_crc_open. It avoids one frame wait
during open. If application want to wait after read call, it can use
poll/read blocking read() call.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 834bc7ee5550..69ec2728727e 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -211,24 +211,8 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	if (ret)
 		goto err;
 
-	spin_lock_irq(&crc->lock);
-	/*
-	 * Only return once we got a first frame, so userspace doesn't have to
-	 * guess when this particular piece of HW will be ready to start
-	 * generating CRCs.
-	 */
-	ret = wait_event_interruptible_lock_irq(crc->wq,
-						crtc_crc_data_count(crc),
-						crc->lock);
-	spin_unlock_irq(&crc->lock);
-
-	if (ret)
-		goto err_disable;
-
 	return 0;
 
-err_disable:
-	crtc->funcs->set_crc_source(crtc, NULL);
 err:
 	spin_lock_irq(&crc->lock);
 	crtc_crc_cleanup(crc);
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for Improve crc-core driver interface

[Patchwork]

== Series Details ==

Series: Improve crc-core driver interface
URL   : https://patchwork.freedesktop.org/series/45246/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
029f1bb122df drm: crc: Introduce verify_crc_source callback
5ec7aa87b40a drm: crc: Introduce pre_crc_read function
480f7d04332c drm: crc: Introduce get_crc_sources callback
d4423129d7d1 drm/rockchip/crc: Implement verify_crc_source callback
-:21: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#21: FILE: drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1143:
+static int vop_crtc_verify_crc_source(struct drm_crtc *crtc,
+				   const char *source_name, size_t *values_cnt)

-:39: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#39: FILE: drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1161:
+static int vop_crtc_verify_crc_source(struct drm_crtc *crtc,
+				   const char *source_name, size_t *values_cnt)

total: 0 errors, 0 warnings, 2 checks, 37 lines checked
610d591b94d6 drm/amdgpu_dm/crc: Implement verify_crc_source callback
891094e98e2a drm/rcar-du/crc: Implement verify_crc_source callback
-:19: CHECK:LINE_SPACING: Please don't use multiple blank lines
#19: FILE: drivers/gpu/drm/rcar-du/rcar_du_crtc.c:759:
 
+

-:37: CHECK:BRACES: Blank lines aren't necessary after an open brace '{'
#37: FILE: drivers/gpu/drm/rcar-du/rcar_du_crtc.c:777:
+	} else if (strstarts(source_name, "plane")) {
+

total: 0 errors, 0 warnings, 2 checks, 54 lines checked
8bba28049862 drm/i915/crc: implement verify_crc_source callback
-:137: CHECK:LINE_SPACING: Please don't use multiple blank lines
#137: FILE: drivers/gpu/drm/i915/intel_pipe_crc.c:1004:
+
+

-:150: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#150: FILE: drivers/gpu/drm/i915/intel_pipe_crc.c:1017:
+	if (source == INTEL_PIPE_CRC_SOURCE_AUTO ||
+			intel_is_valid_crc_source(dev_priv, source) == 0) {

total: 0 errors, 0 warnings, 2 checks, 135 lines checked
042fb1777a2a drm/i915/crc: implement get_crc_sources callback
4820bcc2bfff drm/crc: Cleanup crtc_crc_open function
3493f69dc14c Revert "drm: crc: Wait for a frame before returning from open()"

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Improve crc-core driver interface

[Patchwork]

== Series Details ==

Series: Improve crc-core driver interface
URL   : https://patchwork.freedesktop.org/series/45246/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4368 -> Patchwork_9398 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9398 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9398, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45246/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9398:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_pipe_crc_basic@bad-source:
      fi-skl-6770hq:      PASS -> FAIL +6
      fi-bwr-2160:        PASS -> FAIL +4
      fi-hsw-4770r:       PASS -> FAIL +6
      fi-glk-j4005:       PASS -> FAIL
      fi-cfl-8700k:       PASS -> FAIL +6
      fi-kbl-guc:         PASS -> FAIL

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      fi-ivb-3520m:       PASS -> FAIL +6
      fi-cfl-s3:          PASS -> FAIL +6
      fi-skl-6700hq:      PASS -> FAIL +6
      fi-skl-guc:         PASS -> FAIL +6
      fi-blb-e6850:       PASS -> FAIL +4
      fi-byt-j1900:       PASS -> FAIL +4

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-ilk-650:         PASS -> FAIL +4
      fi-elk-e7500:       PASS -> FAIL +4
      fi-byt-n2820:       PASS -> FAIL +4

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-bdw-5557u:       PASS -> FAIL +6
      fi-pnv-d510:        PASS -> FAIL +4
      fi-skl-6600u:       PASS -> FAIL +6
      fi-bxt-dsi:         PASS -> FAIL +6
      fi-hsw-4770:        PASS -> FAIL +6
      fi-cfl-guc:         PASS -> FAIL +6
      fi-ivb-3770:        PASS -> FAIL +6

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-hsw-peppy:       PASS -> FAIL +6
      fi-bdw-gvtdvm:      PASS -> FAIL +6
      fi-gdg-551:         PASS -> FAIL +4
      fi-kbl-7500u:       PASS -> FAIL +6
      fi-snb-2600:        PASS -> FAIL +4

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
      fi-kbl-7567u:       PASS -> FAIL +6
      fi-skl-6260u:       PASS -> FAIL +6
      fi-skl-6700k2:      PASS -> FAIL +6
      fi-bxt-j4205:       PASS -> FAIL +6
      fi-kbl-7560u:       PASS -> FAIL +6
      fi-whl-u:           PASS -> FAIL +6
      fi-kbl-r:           PASS -> FAIL +6
      fi-bsw-n3050:       PASS -> FAIL +2

    
== Known issues ==

  Here are the changes found in Patchwork_9398 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    igt@gem_ctx_create@basic-files:
      fi-skl-gvtdvm:      PASS -> INCOMPLETE (fdo#105600, fdo#106988)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
      fi-glk-j4005:       PASS -> FAIL (fdo#106211) +5

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105600 https://bugs.freedesktop.org/show_bug.cgi?id=105600
  fdo#106211 https://bugs.freedesktop.org/show_bug.cgi?id=106211
  fdo#106988 https://bugs.freedesktop.org/show_bug.cgi?id=106988


== Participating hosts (43 -> 37) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-glk-dsi fi-bsw-cyan fi-ctg-p8600 fi-kbl-x1275 


== Build changes ==

    * Linux: CI_DRM_4368 -> Patchwork_9398

  CI_DRM_4368: f9f621dc095a8bfd2157fba018ddb542260d8daa @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4528: 6be300d405de5974b262e8b93a445be4ac618e6a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9398: 3493f69dc14c260fc9274272af2066c2f83aee5b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

3493f69dc14c Revert "drm: crc: Wait for a frame before returning from open()"
4820bcc2bfff drm/crc: Cleanup crtc_crc_open function
042fb1777a2a drm/i915/crc: implement get_crc_sources callback
8bba28049862 drm/i915/crc: implement verify_crc_source callback
891094e98e2a drm/rcar-du/crc: Implement verify_crc_source callback
610d591b94d6 drm/amdgpu_dm/crc: Implement verify_crc_source callback
d4423129d7d1 drm/rockchip/crc: Implement verify_crc_source callback
480f7d04332c drm: crc: Introduce get_crc_sources callback
5ec7aa87b40a drm: crc: Introduce pre_crc_read function
029f1bb122df drm: crc: Introduce verify_crc_source callback

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9398/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/10] Revert "drm: crc: Wait for a frame before returning from open()"

[Maarten Lankhorst]

Op 22-06-18 om 12:41 schreef Mahesh Kumar:
> This reverts commit e8fa5671183c80342d520ad81d14fa79a9d4a680.
>
> Don't wait for first CRC during crtc_crc_open. It avoids one frame wait
> during open. If application want to wait after read call, it can use
> poll/read blocking read() call.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 16 ----------------
>  1 file changed, 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 834bc7ee5550..69ec2728727e 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -211,24 +211,8 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>  	if (ret)
>  		goto err;
>  
> -	spin_lock_irq(&crc->lock);
> -	/*
> -	 * Only return once we got a first frame, so userspace doesn't have to
> -	 * guess when this particular piece of HW will be ready to start
> -	 * generating CRCs.
> -	 */
> -	ret = wait_event_interruptible_lock_irq(crc->wq,
> -						crtc_crc_data_count(crc),
> -						crc->lock);
> -	spin_unlock_irq(&crc->lock);
> -
> -	if (ret)
> -		goto err_disable;
> -
>  	return 0;
>  
> -err_disable:
> -	crtc->funcs->set_crc_source(crtc, NULL);
>  err:
>  	spin_lock_irq(&crc->lock);
>  	crtc_crc_cleanup(crc);

Adding Tomeu Vizoso to the cc.

Can you resend the series, and add dri-devel@lists.freedesktop.org and the driver maintainers to the cc? You'll need to get acks from the maintainers to merge this through drm-misc. :)

~Maarten

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] Revert "drm: crc: Wait for a frame before returning from open()"

[Tomeu Vizoso]

On 06/22/2018 02:30 PM, Maarten Lankhorst wrote:
> Op 22-06-18 om 12:41 schreef Mahesh Kumar:
>> This reverts commit e8fa5671183c80342d520ad81d14fa79a9d4a680.
>>
>> Don't wait for first CRC during crtc_crc_open. It avoids one frame wait
>> during open. If application want to wait after read call, it can use
>> poll/read blocking read() call.

Hi,

unfortunately I don't remember why that was deemed undesirable, and I 
failed to explain so in the commit message.

My only comment is that it would be good to make the motivation for this 
revert explicit, in case someone else in the future wonders.

I would also test it in a machine with eDP, IGT should be able to test there.

Thanks,

Tomeu

>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/drm_debugfs_crc.c | 16 ----------------
>>   1 file changed, 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
>> index 834bc7ee5550..69ec2728727e 100644
>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>> @@ -211,24 +211,8 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>>   	if (ret)
>>   		goto err;
>>   
>> -	spin_lock_irq(&crc->lock);
>> -	/*
>> -	 * Only return once we got a first frame, so userspace doesn't have to
>> -	 * guess when this particular piece of HW will be ready to start
>> -	 * generating CRCs.
>> -	 */
>> -	ret = wait_event_interruptible_lock_irq(crc->wq,
>> -						crtc_crc_data_count(crc),
>> -						crc->lock);
>> -	spin_unlock_irq(&crc->lock);
>> -
>> -	if (ret)
>> -		goto err_disable;
>> -
>>   	return 0;
>>   
>> -err_disable:
>> -	crtc->funcs->set_crc_source(crtc, NULL);
>>   err:
>>   	spin_lock_irq(&crc->lock);
>>   	crtc_crc_cleanup(crc);
> 
> Adding Tomeu Vizoso to the cc.
> 
> Can you resend the series, and add dri-devel@lists.freedesktop.org and the driver maintainers to the cc? You'll need to get acks from the maintainers to merge this through drm-misc. :)
> 
> ~Maarten
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Daniel Vetter]

On Fri, Jun 22, 2018 at 04:11:25PM +0530, Mahesh Kumar wrote:
> This patch adds a new callback function "verify_crc_source" which will
> be used during setting the crc source in control node and while opening
> data node for crc reading. This will help in avoiding setting of wrong
> string for source.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>

drm core patches must be cc'ed to dri-devel. simplest way to do that and
never forget is to add a Cc: line to the commit message.
-Daniel

> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
>  include/drm/drm_crtc.h            | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 9f8312137cad..c6a725b79ac6 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>  	struct drm_crtc *crtc = m->private;
>  	struct drm_crtc_crc *crc = &crtc->crc;
>  	char *source;
> +	size_t values_cnt;
> +	int ret = 0;
>  
>  	if (len == 0)
>  		return 0;
> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>  	if (source[len] == '\n')
>  		source[len] = '\0';
>  
> +	if (crtc->funcs->verify_crc_source) {
> +		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
>  
>  	if (crc->opened) {
> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>  			return ret;
>  	}
>  
> +	if (crtc->funcs->verify_crc_source) {
> +		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
> +						     &values_cnt);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
>  	if (!crc->opened)
>  		crc->opened = true;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 23eddbccab10..1a6dcbf91744 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -661,6 +661,21 @@ struct drm_crtc_funcs {
>  	 */
>  	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
>  			      size_t *values_cnt);
> +	/**
> +	 * @verify_crc_source:
> +	 *
> +	 * verifies the source of CRC checksums of frames before setting the
> +	 * source for CRC and during crc open.
> +	 *
> +	 * This callback is optional if the driver does not support any CRC
> +	 * generation functionality.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or a negative error code on failure.
> +	 */
> +	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
> +				 size_t *values_cnt);
>  
>  	/**
>  	 * @atomic_print_state:
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx