[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

Cc: dri-devel@lists.freedesktop.org

Mahesh Kumar (10):
  drm: crc: Introduce verify_crc_source callback
  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()"
  drm/rcar-du/crc: Implement get_crc_sources callback

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
 drivers/gpu/drm/drm_debugfs_crc.c                  |  92 +++++++++-------
 drivers/gpu/drm/i915/intel_display.c               |   2 +
 drivers/gpu/drm/i915/intel_drv.h                   |   9 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              | 119 ++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  82 +++++++++++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c              |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c              |  67 ++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.h              |   1 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  26 ++++-
 include/drm/drm_crtc.h                             |  40 ++++++-
 14 files changed, 396 insertions(+), 74 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. This will help
in avoiding setting of wrong string for source.

Changes since V1:
 - do not yet verify_crc_source during open.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
 include/drm/drm_crtc.h            | 15 +++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 99961192bf03..1956acb61cc8 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) {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 17f4f93340b8..3ce5d2fe133d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -684,6 +684,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

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

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

[Mahesh Kumar]

This patch introduce 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 return a constant pointer to an array of crc sources list
and update count according to the number of source in the list.

Changes Since V1: (Daniel)
 - return const pointer to an array of crc sources list
 - do validation of sources in CRC-core
Changes Since V2:
 - update commit message
 - update callback documentation
 - print one source name per line

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 23 ++++++++++++++++++++++-
 include/drm/drm_crtc.h            | 22 ++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 1956acb61cc8..940b76a1ab71 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -68,8 +68,29 @@ static int crc_control_show(struct seq_file *m, void *data)
 {
 	struct drm_crtc *crtc = m->private;
 
-	seq_printf(m, "%s\n", crtc->crc.source);
+	if (crtc->funcs->get_crc_sources) {
+		size_t count;
+		const char *const *sources = crtc->funcs->get_crc_sources(crtc,
+									&count);
+		size_t values_cnt;
+		int i;
+
+		if (count == 0 || !sources)
+			goto out;
+
+		for (i = 0; i < count; i++)
+			if (!crtc->funcs->verify_crc_source(crtc, sources[i],
+							    &values_cnt)) {
+				if (strcmp(sources[i], crtc->crc.source))
+					seq_printf(m, "%s\n", sources[i]);
+				else
+					seq_printf(m, "%s*\n", sources[i]);
+			}
+	}
+	return 0;
 
+out:
+	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 3ce5d2fe133d..69886025e628 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -699,6 +699,28 @@ struct drm_crtc_funcs {
 	 */
 	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
 				 size_t *values_cnt);
+	/**
+	 * @get_crc_sources:
+	 *
+	 * Driver callback for getting a list of all the available sources for
+	 * CRC generation. This callback depends upon verify_crc_source, So
+	 * verify_crc_source callback should be implemented before implementing
+	 * this. Driver can pass full list of available crc sources, this
+	 * callback does the verification on each crc-source before passing it
+	 * to userspace.
+	 *
+	 * This callback is optional if the driver does not support exporting of
+	 * possible CRC sources list.
+	 *
+	 * RETURNS:
+	 *
+	 * a constant character pointer to the list of all the available CRC
+	 * sources. On failure driver should return NULL. count should be
+	 * updated with number of sources in list. if zero we don't process any
+	 * source from the list.
+	 */
+	const char *const *(*get_crc_sources)(struct drm_crtc *crtc,
+					      size_t *count);
 
 	/**
 	 * @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/rockchip/crc: Implement verify_crc_source callback

[Mahesh Kumar]

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

Changes since V1:
 - simplify the verification (Jani N)

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index effecbed2d11..77e91b15ddb4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1138,12 +1138,31 @@ 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)
+		return -EINVAL;
+
+	*values_cnt = 3;
+	return 0;
+}
+
 #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 +1175,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 04/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>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c     |  1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h     |  4 ++++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 16 ++++++++++++++++
 3 files changed, 21 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 f0fc3067ab5d..be127ba7e604 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2608,6 +2608,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 a29dc35954c9..e43ed064dc46 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -260,9 +260,13 @@ 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 05/10] drm/rcar-du/crc: Implement verify_crc_source callback

[Mahesh Kumar]

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

Changes Since V1:
 - avoid duplication of code

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 +++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 15dc9caa128b..7124918372f8 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -756,17 +756,66 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc)
 	rcrtc->vblank_enable = false;
 }
 
+static int rcar_du_get_plane_index(struct drm_crtc *crtc,
+				   const char *source_name,
+				   unsigned int *index)
+{
+	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+	unsigned int i;
+	int ret = 0;
+
+	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;
+
+	return ret;
+}
+
+static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
+					  const char *source_name,
+					  size_t *values_cnt)
+{
+	int ret;
+	unsigned int index;
+	/*
+	 * 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 = rcar_du_get_plane_index(crtc, source_name, &index);
+		if (ret < 0)
+			return ret;
+	} 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)
 {
-	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_crtc_state *crtc_state;
 	struct drm_atomic_state *state;
 	enum vsp1_du_crc_source source;
 	unsigned int index = 0;
-	unsigned int i;
 	int ret;
 
 	/*
@@ -781,19 +830,9 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
 	} else if (strstarts(source_name, "plane")) {
 		source = VSP1_DU_CRC_PLANE;
 
-		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
+		ret = rcar_du_get_plane_index(crtc, source_name, &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;
 	}
@@ -861,6 +900,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

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

[PATCH 06/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>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.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 | 108 ++++++++++++++++++++++++++++++++++
 3 files changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8f3199b06d1f..07c1eff77e7d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12895,6 +12895,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 61e715ddd0d5..eaea10a7a600 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2157,10 +2157,13 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
 #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 849e1b69ba73..03025c50cb38 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -468,6 +468,114 @@ void intel_display_crc_init(struct drm_i915_private *dev_priv)
 	}
 }
 
+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

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

[PATCH 07/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.

Changes since V1:
 - Return array of crc sources

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.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 | 7 +++++++
 3 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 07c1eff77e7d..4f2a75f9c15d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12896,6 +12896,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 eaea10a7a600..29b054de9e13 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2159,11 +2159,14 @@ 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);
+const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
+					      size_t *count);
 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 03025c50cb38..83f9ade0cd81 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -556,6 +556,13 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
 		return ivb_crc_source_valid(dev_priv, source);
 }
 
+const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
+					      size_t *count)
+{
+	*count = ARRAY_SIZE(pipe_crc_sources);
+	return pipe_crc_sources;
+}
+
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 				 size_t *values_cnt)
 {
-- 
2.16.2

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

[PATCH 08/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 inputs, values_cnt we
already gets as part of verify_crc_source.

Changes since V1:
 - refactor code to use single spin lock

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Acked-by: Leo Li <sunpeng.li@amd.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                  | 61 ++++++++++------------
 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, 37 insertions(+), 52 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 e43ed064dc46..54056d180003 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -258,8 +258,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);
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 940b76a1ab71..7386391636e3 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -127,11 +127,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);
 
@@ -197,40 +195,40 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 			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;
+
+	entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;
+
 	spin_lock_irq(&crc->lock);
-	if (!crc->opened)
+	if (!crc->opened) {
 		crc->opened = true;
-	else
+		crc->entries = entries;
+		crc->values_cnt = values_cnt;
+	} else {
 		ret = -EBUSY;
+	}
 	spin_unlock_irq(&crc->lock);
 
-	if (ret)
+	if (ret) {
+		kfree(entries);
 		return ret;
+	}
 
-	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
+	ret = crtc->funcs->set_crc_source(crtc, crc->source);
 	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;
-	}
-
 	spin_lock_irq(&crc->lock);
-	crc->entries = entries;
-	crc->values_cnt = values_cnt;
-
 	/*
 	 * 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
@@ -247,7 +245,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);
@@ -259,9 +257,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);
@@ -367,7 +364,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 29b054de9e13..814f6f3e1595 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2155,8 +2155,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
 
 /* intel_pipe_crc.c */
 #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);
 const char *const *intel_crtc_get_crc_sources(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 83f9ade0cd81..f3c9010e332a 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -583,8 +583,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];
@@ -623,7 +622,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 7124918372f8..72db1834dd50 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -808,8 +808,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 drm_modeset_acquire_ctx ctx;
 	struct drm_crtc_state *crtc_state;
@@ -837,8 +836,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 77e91b15ddb4..657372306623 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 @@ vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 #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 69886025e628..1013d6596408 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -682,8 +682,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

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

[PATCH 09/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>
Cc: dri-devel@lists.freedesktop.org
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 7386391636e3..973b3968ad17 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -228,24 +228,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

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

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

[Mahesh Kumar]

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

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 ++++++
 drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c  | 67 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.h  |  1 +
 5 files changed, 83 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 72db1834dd50..8da064daf9c9 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -807,6 +807,16 @@ static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
 	return 0;
 }
 
+const char *const *rcar_du_crtc_get_crc_sources(struct drm_crtc *crtc,
+						size_t *count)
+{
+	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+	struct rcar_du_device *rcdu = rcrtc->group->dev;
+
+	*count = rcdu->sources_count;
+	return (const char * const*)rcdu->sources;
+}
+
 static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
 				       const char *source_name)
 {
@@ -898,6 +908,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
 	.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,
+	.get_crc_sources = rcar_du_crtc_get_crc_sources,
 };
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 02aee6cb0e53..b2c9b7e862d5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -401,6 +401,8 @@ static int rcar_du_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(ddev);
 
+	rcar_du_crc_sources_list_uninit(rcdu);
+
 	if (rcdu->fbdev)
 		drm_fbdev_cma_fini(rcdu->fbdev);
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index b3a25e8e07d0..fe0e2ec724b5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -92,6 +92,8 @@ struct rcar_du_device {
 
 	unsigned int dpad0_source;
 	unsigned int vspd1_sink;
+	char **sources;
+	size_t sources_count;
 };
 
 static inline bool rcar_du_has(struct rcar_du_device *rcdu,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index f0bc7cc0e913..c74ced480c74 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -421,6 +421,70 @@ static int rcar_du_properties_init(struct rcar_du_device *rcdu)
 	return 0;
 }
 
+static void rcar_du_crc_sources_list_init(struct rcar_du_device *rcdu)
+{
+	struct drm_device *dev = rcdu->ddev;
+	struct drm_plane *plane;
+	char **sources;
+	size_t count;
+	int i = 0;
+
+	/* reserve 1 for "auto" source */
+	count = 1;
+
+	drm_for_each_plane(plane, dev)
+		count++;
+
+	sources = kmalloc_array(count, sizeof(char *), GFP_KERNEL);
+	if (!sources)
+		goto fail;
+
+	sources[i] = kstrdup("auto", GFP_KERNEL);
+	if (!sources[i])
+		goto fail_no_mem;
+
+	i++;
+	drm_for_each_plane(plane, dev) {
+		char name[16];
+
+		sprintf(name, "plane%d", plane->base.id);
+		sources[i] = kstrdup(name, GFP_KERNEL);
+		if (!sources[i])
+			goto fail_no_mem;
+		i++;
+	}
+
+	rcdu->sources = sources;
+	rcdu->sources_count = count;
+	return;
+
+fail_no_mem:
+	while (i > 0) {
+		i--;
+		kfree(sources[i]);
+	}
+	kfree(sources);
+fail:
+	rcdu->sources = NULL;
+	rcdu->sources_count = 0;
+}
+
+void rcar_du_crc_sources_list_uninit(struct rcar_du_device *rcdu)
+{
+	int i = rcdu->sources_count;
+
+	if (!rcdu->sources)
+		return;
+
+	while (i > 0) {
+		i--;
+		kfree(rcdu->sources[i]);
+	}
+	kfree(rcdu->sources);
+	rcdu->sources = NULL;
+	rcdu->sources_count = 0;
+}
+
 static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
 {
 	const struct device_node *np = rcdu->dev->of_node;
@@ -591,6 +655,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 			return ret;
 	}
 
+	/* Initialize CRC-sources list */
+	rcar_du_crc_sources_list_init(rcdu);
+
 	/* Initialize the encoders. */
 	ret = rcar_du_encoders_init(rcdu);
 	if (ret < 0)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.h b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
index 07951d5fe38b..29120e6b5666 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
@@ -32,6 +32,7 @@ struct rcar_du_format_info {
 const struct rcar_du_format_info *rcar_du_format_info(u32 fourcc);
 
 int rcar_du_modeset_init(struct rcar_du_device *rcdu);
+void rcar_du_crc_sources_list_uninit(struct rcar_du_device *rcdu);
 
 int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
 			struct drm_mode_create_dumb *args);
-- 
2.16.2

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

✗ Fi.CI.SPARSE: warning for Improve crc-core driver interface (rev6)

[Patchwork]

== Series Details ==

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

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm: crc: Introduce verify_crc_source callback
Okay!

Commit: drm: crc: Introduce get_crc_sources callback
Okay!

Commit: drm/rockchip/crc: Implement verify_crc_source callback
Okay!

Commit: drm/amdgpu_dm/crc: Implement verify_crc_source callback
-drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3043:6: warning: symbol 'dm_drm_plane_destroy_state' was not declared. Should it be static?
-drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3821:27: warning: expression using sizeof(void)
-drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3821:27: warning: expression using sizeof(void)
-drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3825:27: warning: expression using sizeof(void)
-drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3825:27: warning: expression using sizeof(void)
-drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3932:58: warning: Using plain integer as NULL pointer
+drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3044:6: warning: symbol 'dm_drm_plane_destroy_state' was not declared. Should it be static?
+drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3822:27: warning: expression using sizeof(void)
+drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3822:27: warning: expression using sizeof(void)
+drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3826:27: warning: expression using sizeof(void)
+drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3826:27: warning: expression using sizeof(void)
+drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:3933:58: warning: Using plain integer as NULL pointer

Commit: drm/rcar-du/crc: Implement verify_crc_source callback
Okay!

Commit: drm/i915/crc: implement verify_crc_source callback
Okay!

Commit: drm/i915/crc: implement get_crc_sources callback
Okay!

Commit: drm/crc: Cleanup crtc_crc_open function
Okay!

Commit: Revert "drm: crc: Wait for a frame before returning from open()"
Okay!

Commit: drm/rcar-du/crc: Implement get_crc_sources callback
Okay!

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

[Patchwork]

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from CI_DRM_4474 -> Patchwork_9625 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9625 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9625, 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/6/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_pipe_crc_basic@bad-source:
      {fi-cfl-8109u}:     PASS -> FAIL +6
      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-x1275:       PASS -> FAIL
      fi-kbl-guc:         PASS -> FAIL
      fi-glk-dsi:         PASS -> FAIL
      fi-bsw-n3050:       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-byt-n2820:       PASS -> FAIL +4
      fi-snb-2520m:       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-skl-gvtdvm:      PASS -> FAIL +6
      {fi-skl-iommu}:     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

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-FAIL (fdo#102614)

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

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

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#106211 https://bugs.freedesktop.org/show_bug.cgi?id=106211


== Participating hosts (45 -> 41) ==

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4474 -> Patchwork_9625

  CI_DRM_4474: c298e0700edde3d016ae9b16d0ce2a098bee1022 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4549: e19fd5549e9cf603251704117fc64f4068be5016 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9625: a1bc1a978b7563fd9e1b66d6926229a4cd501f47 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a1bc1a978b75 drm/rcar-du/crc: Implement get_crc_sources callback
db679cae185b Revert "drm: crc: Wait for a frame before returning from open()"
95e9e595af65 drm/crc: Cleanup crtc_crc_open function
c6d9fef8d371 drm/i915/crc: implement get_crc_sources callback
b99344d50a7c drm/i915/crc: implement verify_crc_source callback
2a47e1ef9ce0 drm/rcar-du/crc: Implement verify_crc_source callback
ae604ad08730 drm/amdgpu_dm/crc: Implement verify_crc_source callback
ed1d349d1af1 drm/rockchip/crc: Implement verify_crc_source callback
bad8a20d0674 drm: crc: Introduce get_crc_sources callback
35784eaaf4f1 drm: crc: Introduce verify_crc_source callback

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9625/issues.html
_______________________________________________
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

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Thursday, 12 July 2018 11:36:26 EEST 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. This will help
> in avoiding setting of wrong string for source.
> 
> Changes since V1:
>  - do not yet verify_crc_source during open.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
>  include/drm/drm_crtc.h            | 15 +++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index 99961192bf03..1956acb61cc8 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;

No need to initialize ret to 0 here.

> 
>  	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) {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 17f4f93340b8..3ce5d2fe133d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -684,6 +684,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.

Please also document whether the source parameter can be NULL.

It would also be nice to document values_cnt, but as that's not documented for 
the .set_crc_source() operation at the moment, I won't block the patch for 
this.

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

-- 
Regards,

Laurent Pinchart



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

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

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patches.

When resubmitting patch series, could you please add a version number to the 
[PATCH] prefix ? Otherwise it gets difficult to figure out which version is 
the latest. This can be done automatically with the -v argument to git-format-
patch.

On Thursday, 12 July 2018 11:36:25 EEST Mahesh Kumar wrote:
> 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
> 
> Cc: dri-devel@lists.freedesktop.org
> 
> Mahesh Kumar (10):
>   drm: crc: Introduce verify_crc_source callback
>   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()"
>   drm/rcar-du/crc: Implement get_crc_sources callback
> 
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
>  drivers/gpu/drm/drm_debugfs_crc.c                  |  92 +++++++++-------
>  drivers/gpu/drm/i915/intel_display.c               |   2 +
>  drivers/gpu/drm/i915/intel_drv.h                   |   9 +-
>  drivers/gpu/drm/i915/intel_pipe_crc.c              | 119 +++++++++++++++++-
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  82 +++++++++++---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c              |   2 +
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   2 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c              |  67 ++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.h              |   1 +
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  26 ++++-
>  include/drm/drm_crtc.h                             |  40 ++++++-
>  14 files changed, 396 insertions(+), 74 deletions(-)

-- 
Regards,

Laurent Pinchart



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

Re: [PATCH 02/10] drm: crc: Introduce get_crc_sources callback

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Thursday, 12 July 2018 11:36:27 EEST Mahesh Kumar wrote:
> This patch introduce 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 return a constant pointer to an array of crc sources list
> and update count according to the number of source in the list.
> 
> Changes Since V1: (Daniel)
>  - return const pointer to an array of crc sources list
>  - do validation of sources in CRC-core
> Changes Since V2:
>  - update commit message
>  - update callback documentation
>  - print one source name per line
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 23 ++++++++++++++++++++++-
>  include/drm/drm_crtc.h            | 22 ++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index 1956acb61cc8..940b76a1ab71 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -68,8 +68,29 @@ static int crc_control_show(struct seq_file *m, void
> *data) {
>  	struct drm_crtc *crtc = m->private;
> 
> -	seq_printf(m, "%s\n", crtc->crc.source);
> +	if (crtc->funcs->get_crc_sources) {
> +		size_t count;
> +		const char *const *sources = crtc->funcs->get_crc_sources(crtc,
> +									&count);
> +		size_t values_cnt;
> +		int i;

i only takes positive values, you can make it an unsigned int.

> +		if (count == 0 || !sources)
> +			goto out;
> +
> +		for (i = 0; i < count; i++)
> +			if (!crtc->funcs->verify_crc_source(crtc, sources[i],
> +							    &values_cnt)) {
> +				if (strcmp(sources[i], crtc->crc.source))
> +					seq_printf(m, "%s\n", sources[i]);
> +				else
> +					seq_printf(m, "%s*\n", sources[i]);
> +			}
> +	}
> +	return 0;
> 
> +out:
> +	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 3ce5d2fe133d..69886025e628 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -699,6 +699,28 @@ struct drm_crtc_funcs {
>  	 */
>  	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
>  				 size_t *values_cnt);
> +	/**
> +	 * @get_crc_sources:
> +	 *
> +	 * Driver callback for getting a list of all the available sources for
> +	 * CRC generation. This callback depends upon verify_crc_source, So

s/So/so/

> +	 * verify_crc_source callback should be implemented before implementing
> +	 * this. Driver can pass full list of available crc sources, this
> +	 * callback does the verification on each crc-source before passing it
> +	 * to userspace.
> +	 *
> +	 * This callback is optional if the driver does not support exporting of
> +	 * possible CRC sources list.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * a constant character pointer to the list of all the available CRC
> +	 * sources. On failure driver should return NULL. count should be
> +	 * updated with number of sources in list. if zero we don't process any

s/if zero/If zero/

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 * source from the list.
> +	 */
> +	const char *const *(*get_crc_sources)(struct drm_crtc *crtc,
> +					      size_t *count);
> 
>  	/**
>  	 * @atomic_print_state:

-- 
Regards,

Laurent Pinchart



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

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

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Thursday, 12 July 2018 11:36:30 EEST Mahesh Kumar wrote:
> This patch implements "verify_crc_source" callback function for
> rcar drm driver.
> 
> Changes Since V1:
>  - avoid duplication of code
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 ++++++++++++++++++++++++-------
>  1 file changed, 53 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 15dc9caa128b..7124918372f8
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -756,17 +756,66 @@ static void rcar_du_crtc_disable_vblank(struct
> drm_crtc *crtc) rcrtc->vblank_enable = false;
>  }
> 
> +static int rcar_du_get_plane_index(struct drm_crtc *crtc,

Please pass a struct rcar_du_crtc pointer to this function, as callers should 
already have it (or can easily compute it).

> +				   const char *source_name,
> +				   unsigned int *index)

I think you can simplify this by returning the index, which is always 
positive, or a negative error code in case of error.

I think you could share even more code if you named this function 
rcar_du_crtc_parse_crc_source() and returned both the index and the 
vsp1_du_crc_source (one of them through a pointer).

> +{
> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +	unsigned int i;
> +	int ret = 0;

No need to initialize ret to 0.

> +
> +	ret = kstrtouint(source_name + strlen("plane"), 10, index);

This assumes that the caller will always ensure that source_nam starts with 
"plane", otherwise you could access the string beyond its end. This patch is 
fine, but it would be easy to use the function erroneously in the future. How 
about moving the strstarts(source_name, "plane") check at the beginning of 
this function ?

> +	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;
> +
> +	return ret;
> +}
> +
> +static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
> +					  const char *source_name,
> +					  size_t *values_cnt)
> +{
> +	int ret;
> +	unsigned int index;

Please sort variable declarations roughly by line length to match the style of 
the driver.

You need a blank line here.

> +	/*
> +	 * 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")) {

Can source_name be NULL here ?

> +		goto out;
> +	} else if (strstarts(source_name, "plane")) {
> +		ret = rcar_du_get_plane_index(crtc, source_name, &index);
> +		if (ret < 0)
> +			return ret;
> +	} 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)
>  {
> -	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>  	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_atomic_state *state;
>  	enum vsp1_du_crc_source source;
>  	unsigned int index = 0;
> -	unsigned int i;
>  	int ret;
> 
>  	/*
> @@ -781,19 +830,9 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc
> *crtc, } else if (strstarts(source_name, "plane")) {
>  		source = VSP1_DU_CRC_PLANE;
> 
> -		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
> +		ret = rcar_du_get_plane_index(crtc, source_name, &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;
>  	}
> @@ -861,6 +900,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,
>  };
> 
>  /* ------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart



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

Re: [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Thursday, 12 July 2018 11:36:33 EEST Mahesh Kumar wrote:
> 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 inputs, values_cnt we
> already gets as part of verify_crc_source.
> 
> Changes since V1:
>  - refactor code to use single spin lock
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Acked-by: Leo Li <sunpeng.li@amd.com>

For core code and the R-Car DU driver,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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                  | 61 +++++++++----------
>  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, 37 insertions(+), 52 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
> e43ed064dc46..54056d180003 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -258,8 +258,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);
> 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 940b76a1ab71..7386391636e3 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -127,11 +127,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);
> 
> @@ -197,40 +195,40 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) 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;
> +
> +	entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
> +	if (!entries)
> +		return -ENOMEM;
> +
>  	spin_lock_irq(&crc->lock);
> -	if (!crc->opened)
> +	if (!crc->opened) {
>  		crc->opened = true;
> -	else
> +		crc->entries = entries;
> +		crc->values_cnt = values_cnt;
> +	} else {
>  		ret = -EBUSY;
> +	}
>  	spin_unlock_irq(&crc->lock);
> 
> -	if (ret)
> +	if (ret) {
> +		kfree(entries);
>  		return ret;
> +	}
> 
> -	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> +	ret = crtc->funcs->set_crc_source(crtc, crc->source);
>  	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;
> -	}
> -
>  	spin_lock_irq(&crc->lock);
> -	crc->entries = entries;
> -	crc->values_cnt = values_cnt;
> -
>  	/*
>  	 * 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
> @@ -247,7 +245,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);
> @@ -259,9 +257,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);
> @@ -367,7 +364,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 29b054de9e13..814f6f3e1595 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2155,8 +2155,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon
> *lspcon);
> 
>  /* intel_pipe_crc.c */
>  #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);
>  const char *const *intel_crtc_get_crc_sources(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 83f9ade0cd81..f3c9010e332a
> 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -583,8 +583,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];
> @@ -623,7 +622,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 7124918372f8..72db1834dd50
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -808,8 +808,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 drm_modeset_acquire_ctx ctx;
>  	struct drm_crtc_state *crtc_state;
> @@ -837,8 +836,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
> 77e91b15ddb4..657372306623 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 @@ vop_crtc_verify_crc_source(struct drm_crtc *crtc,
> const char *source_name,
> 
>  #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 69886025e628..1013d6596408 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -682,8 +682,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:
>  	 *


-- 
Regards,

Laurent Pinchart



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

Re: [PATCH 10/10] drm/rcar-du/crc: Implement get_crc_sources callback

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Thursday, 12 July 2018 11:36:35 EEST Mahesh Kumar wrote:
> This patch implements get_crc_sources callback, which returns list of
> all the crc sources supported by driver in current platform.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 ++++++
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  2 +
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h  |  2 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  | 67 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.h  |  1 +
>  5 files changed, 83 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 72db1834dd50..8da064daf9c9
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -807,6 +807,16 @@ static int rcar_du_crtc_verify_crc_source(struct
> drm_crtc *crtc, return 0;
>  }
> 
> +const char *const *rcar_du_crtc_get_crc_sources(struct drm_crtc *crtc,
> +						size_t *count)
> +{
> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +
> +	*count = rcdu->sources_count;
> +	return (const char * const*)rcdu->sources;
> +}

The list of sources has to be stored per-CRTC, as in most cases planes are not 
shared between CRTCs on Gen3 hardware where CRC computation is available. On 
Gen2 hardware planes are shared between CRTCs (but in groups of two CRTCs), 
but CRC computation isn't available, so you shouldn't compute the list of 
sources in that case.

>  static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>  				       const char *source_name)
>  {
> @@ -898,6 +908,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
>  	.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,
> +	.get_crc_sources = rcar_du_crtc_get_crc_sources,
>  };
> 
>  /* ------------------------------------------------------------------------
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6cb0e53..b2c9b7e862d5
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -401,6 +401,8 @@ static int rcar_du_remove(struct platform_device *pdev)
> 
>  	drm_dev_unregister(ddev);
> 
> +	rcar_du_crc_sources_list_uninit(rcdu);

As the sources list has to be stored per-CRTC, you can move this to the CRTC 
cleanup handler.

>  	if (rcdu->fbdev)
>  		drm_fbdev_cma_fini(rcdu->fbdev);
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8e07d0..fe0e2ec724b5
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -92,6 +92,8 @@ struct rcar_du_device {
> 
>  	unsigned int dpad0_source;
>  	unsigned int vspd1_sink;
> +	char **sources;

Let's make this const.

> +	size_t sources_count;

It's not a size but a number of elements, I'd use unsigned int.

>  };
> 
>  static inline bool rcar_du_has(struct rcar_du_device *rcdu,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index f0bc7cc0e913..c74ced480c74
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -421,6 +421,70 @@ static int rcar_du_properties_init(struct
> rcar_du_device *rcdu) return 0;
>  }
> 
> +static void rcar_du_crc_sources_list_init(struct rcar_du_device *rcdu)
> +{
> +	struct drm_device *dev = rcdu->ddev;
> +	struct drm_plane *plane;
> +	char **sources;
> +	size_t count;
> +	int i = 0;

i is never negative, you can make it an unsigned int.

> +	/* reserve 1 for "auto" source */
> +	count = 1;
> +
> +	drm_for_each_plane(plane, dev)
> +		count++;
> +
> +	sources = kmalloc_array(count, sizeof(char *), GFP_KERNEL);
> +	if (!sources)
> +		goto fail;
> +
> +	sources[i] = kstrdup("auto", GFP_KERNEL);
> +	if (!sources[i])
> +		goto fail_no_mem;
> +
> +	i++;
> +	drm_for_each_plane(plane, dev) {
> +		char name[16];
> +
> +		sprintf(name, "plane%d", plane->base.id);
> +		sources[i] = kstrdup(name, GFP_KERNEL);
> +		if (!sources[i])
> +			goto fail_no_mem;
> +		i++;
> +	}
> +
> +	rcdu->sources = sources;
> +	rcdu->sources_count = count;
> +	return;
> +
> +fail_no_mem:
> +	while (i > 0) {
> +		i--;
> +		kfree(sources[i]);
> +	}
> +	kfree(sources);
> +fail:
> +	rcdu->sources = NULL;
> +	rcdu->sources_count = 0;
> +}
> +
> +void rcar_du_crc_sources_list_uninit(struct rcar_du_device *rcdu)
> +{
> +	int i = rcdu->sources_count;

i is never negative, you can make it an unsigned int.

> +	if (!rcdu->sources)
> +		return;
> +
> +	while (i > 0) {
> +		i--;
> +		kfree(rcdu->sources[i]);
> +	}

Anything wrong with a classic for loop ?

	for (i = 0; i < rcdu->sources_count; ++i)
		kfree(rcdu->sources[i]);

> +	kfree(rcdu->sources);
> +	rcdu->sources = NULL;
> +	rcdu->sources_count = 0;
> +}
> +
>  static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
>  {
>  	const struct device_node *np = rcdu->dev->of_node;
> @@ -591,6 +655,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  			return ret;
>  	}
> 
> +	/* Initialize CRC-sources list */
> +	rcar_du_crc_sources_list_init(rcdu);
> +
>  	/* Initialize the encoders. */
>  	ret = rcar_du_encoders_init(rcdu);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.h index 07951d5fe38b..29120e6b5666
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> @@ -32,6 +32,7 @@ struct rcar_du_format_info {
>  const struct rcar_du_format_info *rcar_du_format_info(u32 fourcc);
> 
>  int rcar_du_modeset_init(struct rcar_du_device *rcdu);
> +void rcar_du_crc_sources_list_uninit(struct rcar_du_device *rcdu);
> 
>  int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
>  			struct drm_mode_create_dumb *args);

-- 
Regards,

Laurent Pinchart



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

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

[Kumar, Mahesh]

Hi,

thanks for the review.


On 7/12/2018 4:38 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> Thank you for the patches.
>
> When resubmitting patch series, could you please add a version number to the
> [PATCH] prefix ? Otherwise it gets difficult to figure out which version is
> the latest. This can be done automatically with the -v argument to git-format-
> patch.
sure :), added patch prefix (now v4) and re-floated the series after 
addressing review comments in patches 03/10, 08/10 & 10/10.

-Mahesh
>
> On Thursday, 12 July 2018 11:36:25 EEST Mahesh Kumar wrote:
>> 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
>>
>> Cc: dri-devel@lists.freedesktop.org
>>
>> Mahesh Kumar (10):
>>    drm: crc: Introduce verify_crc_source callback
>>    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()"
>>    drm/rcar-du/crc: Implement get_crc_sources callback
>>
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
>>   drivers/gpu/drm/drm_debugfs_crc.c                  |  92 +++++++++-------
>>   drivers/gpu/drm/i915/intel_display.c               |   2 +
>>   drivers/gpu/drm/i915/intel_drv.h                   |   9 +-
>>   drivers/gpu/drm/i915/intel_pipe_crc.c              | 119 +++++++++++++++++-
>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  82 +++++++++++---
>>   drivers/gpu/drm/rcar-du/rcar_du_drv.c              |   2 +
>>   drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   2 +
>>   drivers/gpu/drm/rcar-du/rcar_du_kms.c              |  67 ++++++++++++
>>   drivers/gpu/drm/rcar-du/rcar_du_kms.h              |   1 +
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  26 ++++-
>>   include/drm/drm_crtc.h                             |  40 ++++++-
>>   14 files changed, 396 insertions(+), 74 deletions(-)

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

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

[Mahesh Kumar]

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

Changes Since V1:
 - avoid duplication of code

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 +++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 15dc9caa128b..7124918372f8 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -756,17 +756,66 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc)
 	rcrtc->vblank_enable = false;
 }
 
+static int rcar_du_get_plane_index(struct drm_crtc *crtc,
+				   const char *source_name,
+				   unsigned int *index)
+{
+	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+	unsigned int i;
+	int ret = 0;
+
+	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;
+
+	return ret;
+}
+
+static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
+					  const char *source_name,
+					  size_t *values_cnt)
+{
+	int ret;
+	unsigned int index;
+	/*
+	 * 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 = rcar_du_get_plane_index(crtc, source_name, &index);
+		if (ret < 0)
+			return ret;
+	} 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)
 {
-	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_crtc_state *crtc_state;
 	struct drm_atomic_state *state;
 	enum vsp1_du_crc_source source;
 	unsigned int index = 0;
-	unsigned int i;
 	int ret;
 
 	/*
@@ -781,19 +830,9 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
 	} else if (strstarts(source_name, "plane")) {
 		source = VSP1_DU_CRC_PLANE;
 
-		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
+		ret = rcar_du_get_plane_index(crtc, source_name, &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;
 	}
@@ -861,6 +900,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

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

[Laurent Pinchart]

Hi Mahesh,

On Tuesday, 10 July 2018 15:59:11 EEST Kumar, Mahesh wrote:
> On 7/10/2018 5:07 PM, Laurent Pinchart wrote:
> > On Monday, 2 July 2018 14:07:24 EEST Mahesh Kumar wrote:
> >> This patch implements "verify_crc_source" callback function for
> >> rcar drm driver.
> >> 
> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> 
> >>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 +++++++++++++++++++++++++++
> >>   1 file changed, 40 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..24eeaa7e14d7
> >> 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -756,6 +756,45 @@ 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;
> > 
> > This duplicates lots of code from the rcar_du_crtc_set_crc_source()
> > function. Could you please extract it to a shared function ?
> 
> Agree, it duplicates the code but "index" is needed by set_crc_source call
> anyway will create a wrapper to avoid duplication of code.

Thank you.

> > Could you please also implement support for the .get_crc_sources()
> > operation ? I think doing so might show limitations in the current API,
> > namely the fact that the list will need to be dynamically created for
> > this driver.
> 
> for that I think rcar_du_crtc_create function can build a dynamic list
> during initializing crtc functions, unless plane can be dynamically
> allocated to any CRTC. this is the place where I need input from maintainer.

That should indeed work. The DU hardware can, in some SoC models, share planes 
between CRTCs. In that case we'll need to ensure that the source corresponds 
to a plane currently associated with the CRTC. That check is currently missing 
and likely out of scope for this patch series, so don't worry about it.

> >> +}
> >> +
> >>   static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
> >>   				       const char *source_name,
> >>   				       size_t *values_cnt)
> >> @@ -861,6 +900,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,
> >> 
> >>   };
> >>   
> >>   /*
> >>   ----------------------------------------------------------------------
> >>   --


-- 
Regards,

Laurent Pinchart



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

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

[Kumar, Mahesh]

Hi,

thanks foe the review.

On 7/10/2018 5:07 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> Thank you for the patch.
>
> On Monday, 2 July 2018 14:07:24 EEST Mahesh Kumar wrote:
>> This patch implements "verify_crc_source" callback function for
>> rcar drm driver.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 +++++++++++++++++++++++++++++++
>>   1 file changed, 40 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..24eeaa7e14d7
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -756,6 +756,45 @@ 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;
> This duplicates lots of code from the rcar_du_crtc_set_crc_source() function.
> Could you please extract it to a shared function ?
Agree, it duplicates the code but "index" is needed by set_crc_source call
anyway will create a wrapper to avoid duplication of code.
>
> Could you please also implement support for the .get_crc_sources() operation ?
> I think doing so might show limitations in the current API, namely the fact
> that the list will need to be dynamically created for this driver.
for that I think rcar_du_crtc_create function can build a dynamic list 
during initializing crtc functions, unless plane can be dynamically 
allocated to any CRTC. this is the place where I need input from maintainer.

-Mahesh
>
>> +}
>> +
>>   static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>>   				       const char *source_name,
>>   				       size_t *values_cnt)
>> @@ -861,6 +900,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,
>>   };
>>
>>   /* ------------------------------------------------------------------------

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

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

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Monday, 2 July 2018 14:07:24 EEST Mahesh Kumar wrote:
> This patch implements "verify_crc_source" callback function for
> rcar drm driver.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 +++++++++++++++++++++++++++++++
>  1 file changed, 40 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..24eeaa7e14d7
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -756,6 +756,45 @@ 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;

This duplicates lots of code from the rcar_du_crtc_set_crc_source() function. 
Could you please extract it to a shared function ?

Could you please also implement support for the .get_crc_sources() operation ? 
I think doing so might show limitations in the current API, namely the fact 
that the list will need to be dynamically created for this driver.

> +}
> +
>  static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>  				       const char *source_name,
>  				       size_t *values_cnt)
> @@ -861,6 +900,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,
>  };
> 
>  /* ------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart



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

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

[Maarten Lankhorst]

Op 02-07-18 om 13:07 schreef Mahesh Kumar:
> This patch implements "verify_crc_source" callback function for
> rcar drm driver.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 ++++++++++++++++++++++++++++++++++
>  1 file changed, 40 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..24eeaa7e14d7 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -756,6 +756,45 @@ 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 +900,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,
>  };
>  
>  /* -----------------------------------------------------------------------------

Ack for merging this and 8/10 through drm-misc?

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

[PATCH 05/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>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 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..24eeaa7e14d7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -756,6 +756,45 @@ 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 +900,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