[PATCH 0/4] Improve crc-core driver interface

[PATCH 0/4] 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.

Changes:
 - Rebase on top of VKMS driver
 - Include pre_crc_read patch 

Cc: dri-devel@lists.freedesktop.org

Mahesh Kumar (4):
  drm/vkms/crc: Implement verify_crc_source callback
  drm/crc: Cleanup crtc_crc_open function
  Revert "drm: crc: Wait for a frame before returning from open()"
  drm/crc: add pre_crc_read function

 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                  | 79 ++++++++++------------
 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             |  4 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  6 +-
 drivers/gpu/drm/vkms/vkms_crc.c                    | 41 ++++++++---
 drivers/gpu/drm/vkms/vkms_crtc.c                   |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h                    |  5 +-
 include/drm/drm_crtc.h                             | 17 ++++-
 11 files changed, 93 insertions(+), 74 deletions(-)

-- 
2.16.2

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

[PATCH V3 1/4] drm/vkms/crc: Implement verify_crc_source callback

[Mahesh Kumar]

This patch implements "verify_crc_source" callback function for
Virtual KMS drm driver.

Changes Since V1:
- update values_cnt in verify_crc_source
Changes Since V2:
- don't return early from set_crc_source to keep behavior same (Haneen)

Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Reviewed-by: Haneen Mohammed <hamohammed.sa@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_crc.c  | 36 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/vkms/vkms_crtc.c |  1 +
 drivers/gpu/drm/vkms/vkms_drv.h  |  2 ++
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 37d717f38e3c..68421d3d809a 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -70,6 +70,37 @@ void vkms_crc_work_handle(struct work_struct *work)
 	drm_crtc_add_crc_entry(crtc, true, crtc_state->n_frame, &crc32);
 }
 
+static int vkms_crc_parse_source(const char *src_name, bool *enabled)
+{
+	int ret = 0;
+
+	if (!src_name) {
+		*enabled = false;
+	} else if (strcmp(src_name, "auto") == 0) {
+		*enabled = true;
+	} else {
+		*enabled = false;
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
+			   size_t *values_cnt)
+{
+	bool enabled;
+
+	if (vkms_crc_parse_source(src_name, &enabled) < 0) {
+		DRM_DEBUG_DRIVER("unknown source %s\n", src_name);
+		return -EINVAL;
+	}
+
+	*values_cnt = 1;
+
+	return 0;
+}
+
 int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 			size_t *values_cnt)
 {
@@ -78,10 +109,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 	unsigned long flags;
 	int ret = 0;
 
-	if (src_name && strcmp(src_name, "auto") == 0)
-		enabled = true;
-	else if (src_name)
-		ret = -EINVAL;
+	ret = vkms_crc_parse_source(src_name, &enabled);
 
 	*values_cnt = 1;
 
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index bfe6e0312cc4..9d0b1a325a78 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -140,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
 	.set_crc_source		= vkms_set_crc_source,
+	.verify_crc_source	= vkms_verify_crc_source,
 };
 
 static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index f156c930366a..090c5e4f5544 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -125,6 +125,8 @@ void vkms_gem_vunmap(struct drm_gem_object *obj);
 /* CRC Support */
 int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 			size_t *values_cnt);
+int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
+			   size_t *values_cnt);
 void vkms_crc_work_handle(struct work_struct *work);
 
 #endif /* _VKMS_DRV_H_ */
-- 
2.16.2

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

[PATCH V5 2/4] 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
Changes since V2:
 - rebase
Changes since V3:
 - rebase on top of VKMS driver

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (V2)
Acked-by: Leo Li <sunpeng.li@amd.com> (V2)
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (V3)
---
 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             |  4 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  6 +--
 drivers/gpu/drm/vkms/vkms_crc.c                    |  5 +-
 drivers/gpu/drm/vkms/vkms_drv.h                    |  3 +-
 include/drm/drm_crtc.h                             |  3 +-
 10 files changed, 39 insertions(+), 57 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 16b679faadbd..01fc5717b657 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 d7e626331eca..3e0a2cfaa35c 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 529e72bcc5ef..50a51e73cc91 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2187,8 +2187,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 57db868da4fe..8a9e5e6f16b4 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -887,8 +887,7 @@ const char *const *rcar_du_crtc_get_crc_sources(struct drm_crtc *crtc,
 }
 
 static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
-				       const char *source_name,
-				       size_t *values_cnt)
+				       const char *source_name)
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct drm_modeset_acquire_ctx ctx;
@@ -903,7 +902,6 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
 		return ret;
 
 	index = ret;
-	*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 c9a5ea38e86b..38f8cae7ef51 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1111,7 +1111,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;
@@ -1121,8 +1121,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)
@@ -1146,7 +1144,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/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 68421d3d809a..ed47d67cecd6 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -101,8 +101,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
 	return 0;
 }
 
-int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
-			size_t *values_cnt)
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 {
 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
 	bool enabled = false;
@@ -111,8 +110,6 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 
 	ret = vkms_crc_parse_source(src_name, &enabled);
 
-	*values_cnt = 1;
-
 	/* make sure nothing is scheduled on crtc workq */
 	flush_workqueue(out->crc_workq);
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 090c5e4f5544..2017a2ccc43d 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -123,8 +123,7 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
 /* CRC Support */
-int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
-			size_t *values_cnt);
+int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name);
 int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 			   size_t *values_cnt);
 void vkms_crc_work_handle(struct work_struct *work);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f2dd180a867a..b21437bc95bf 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -744,8 +744,7 @@ struct drm_crtc_funcs {
 	 *
 	 * 0 on success or a negative error code on failure.
 	 */
-	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
-			      size_t *values_cnt);
+	int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
 	/**
 	 * @verify_crc_source:
 	 *
-- 
2.16.2

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

[PATCH V4 3/4] 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 3e0a2cfaa35c..00e743153e94 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

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

[PATCH V2 4/4] drm/crc: add pre_crc_read function

[Mahesh Kumar]

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

Changes Since V1:
 - rebase

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

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

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

✓ Fi.CI.BAT: success for Improve crc-core driver interface (rev14)

[Patchwork]

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from CI_DRM_4692 -> Patchwork_9983 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_guc:
      fi-skl-guc:         NOTRUN -> DMESG-WARN (fdo#107175, fdo#107258)

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#107362)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-kbl-7567u:       PASS -> FAIL (fdo#103191)

    {igt@kms_psr@primary_page_flip}:
      {fi-icl-u}:         NOTRUN -> FAIL (fdo#107383) +3

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

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107175 https://bugs.freedesktop.org/show_bug.cgi?id=107175
  fdo#107258 https://bugs.freedesktop.org/show_bug.cgi?id=107258
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107383 https://bugs.freedesktop.org/show_bug.cgi?id=107383


== Participating hosts (51 -> 48) ==

  Additional (2): fi-skl-guc fi-icl-u 
  Missing    (5): fi-byt-j1900 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4692 -> Patchwork_9983

  CI_DRM_4692: d53f119472fc7daa532e46ea77098e9e9db2ac10 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4607: 6e0b3e7a2d241af36f8c6b1cc335aa1db3532d29 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9983: 38c9740da78420b23bcd1f1a9754b5a090a92026 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

38c9740da784 drm/crc: add pre_crc_read function
76187455beb4 Revert "drm: crc: Wait for a frame before returning from open()"
996e35e156d5 drm/crc: Cleanup crtc_crc_open function
50ea6d9b4ac1 drm/vkms/crc: Implement verify_crc_source callback

== Logs ==

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

✓ Fi.CI.IGT: success for Improve crc-core driver interface (rev14)

[Patchwork]

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from CI_DRM_4692_full -> Patchwork_9983_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_await@wide-contexts:
      shard-glk:          PASS -> FAIL (fdo#105900)

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106023)

    igt@gem_workarounds@suspend-resume:
      shard-glk:          PASS -> FAIL (fdo#103375)

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103313)

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_suspend@shrink:
      shard-glk:          FAIL (fdo#106886) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-mmap-wc:
      shard-glk:          FAIL (fdo#103167) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103313 https://bugs.freedesktop.org/show_bug.cgi?id=103313
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4692 -> Patchwork_9983

  CI_DRM_4692: d53f119472fc7daa532e46ea77098e9e9db2ac10 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4607: 6e0b3e7a2d241af36f8c6b1cc335aa1db3532d29 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9983: 38c9740da78420b23bcd1f1a9754b5a090a92026 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH V2 4/4] drm/crc: add pre_crc_read function

[Rodrigo Vivi]

On Tue, Aug 21, 2018 at 02:08:58PM +0530, Mahesh Kumar wrote:
> This patch implements a callback function which will be called before
> crc read. In this function driver can implement any preparation work
> required for successfully reading CRC data.
> 
> Changes Since V1:
>  - rebase
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
>  include/drm/drm_crtc.h            | 14 ++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 00e743153e94..e303c194c080 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -274,6 +274,14 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
>  		return 0;
>  	}
>  
> +	if (crtc->funcs->pre_crc_read) {
> +		ret = crtc->funcs->pre_crc_read(crtc);
> +		if (ret) {
> +			spin_unlock_irq(&crc->lock);
> +			return ret;
> +		}
> +	}
> +
>  	/* Nothing to read? */
>  	while (crtc_crc_data_count(crc) == 0) {
>  		if (filep->f_flags & O_NONBLOCK) {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b21437bc95bf..19c2bf63935e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -761,6 +761,20 @@ struct drm_crtc_funcs {
>  	 */
>  	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
>  				 size_t *values_cnt);
> +	/**
> +	 * @pre_crc_read:
> +	 *
> +	 * Driver callback for performing any preparation work required by
> +	 * driver before reading CRC
> +	 *
> +	 * This callback is optional if the driver does not support CRC
> +	 * generation or no prework is required before reading the crc
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or a negative error code on failure.
> +	 */
> +	int (*pre_crc_read)(struct drm_crtc *crtc);
>  	/**
>  	 * @get_crc_sources:
>  	 *
> -- 
> 2.16.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V2 4/4] drm/crc: add pre_crc_read function

[Maarten Lankhorst]

Op 21-08-18 om 10:38 schreef Mahesh Kumar:
> This patch implements a callback function which will be called before
> crc read. In this function driver can implement any preparation work
> required for successfully reading CRC data.
>
> Changes Since V1:
>  - rebase
>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
After some discussion with David on whether this can be upstreamed, he NACK'd it.
Because there are no upstream users for this callback, it cannot be included upstream.
It would be removed by the first person to notice there are no in-kernel users using it.

For the first 3 patches:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>  drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
>  include/drm/drm_crtc.h            | 14 ++++++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 00e743153e94..e303c194c080 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -274,6 +274,14 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
>  		return 0;
>  	}
>  
> +	if (crtc->funcs->pre_crc_read) {
> +		ret = crtc->funcs->pre_crc_read(crtc);
> +		if (ret) {
> +			spin_unlock_irq(&crc->lock);
> +			return ret;
> +		}
> +	}
> +
>  	/* Nothing to read? */
>  	while (crtc_crc_data_count(crc) == 0) {
>  		if (filep->f_flags & O_NONBLOCK) {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b21437bc95bf..19c2bf63935e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -761,6 +761,20 @@ struct drm_crtc_funcs {
>  	 */
>  	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
>  				 size_t *values_cnt);
> +	/**
> +	 * @pre_crc_read:
> +	 *
> +	 * Driver callback for performing any preparation work required by
> +	 * driver before reading CRC
> +	 *
> +	 * This callback is optional if the driver does not support CRC
> +	 * generation or no prework is required before reading the crc
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or a negative error code on failure.
> +	 */
> +	int (*pre_crc_read)(struct drm_crtc *crtc);
>  	/**
>  	 * @get_crc_sources:
>  	 *


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

Re: [PATCH V2 4/4] drm/crc: add pre_crc_read function

[Rodrigo Vivi]

On Wed, Aug 22, 2018 at 12:37:05PM +0200, Maarten Lankhorst wrote:
> Op 21-08-18 om 10:38 schreef Mahesh Kumar:
> > This patch implements a callback function which will be called before
> > crc read. In this function driver can implement any preparation work
> > required for successfully reading CRC data.
> >
> > Changes Since V1:
> >  - rebase
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> After some discussion with David on whether this can be upstreamed, he NACK'd it.
> Because there are no upstream users for this callback, it cannot be included upstream.
> It would be removed by the first person to notice there are no in-kernel users using it.

It makes sense.... After I posted the review yesterday I started thinking exactly
about this case... if would get easily removed in a clean-up for obvious reasons...

> For the first 3 patches:
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

thanks. pushed all 3 to drm-misc-next.

> >  drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
> >  include/drm/drm_crtc.h            | 14 ++++++++++++++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> > index 00e743153e94..e303c194c080 100644
> > --- a/drivers/gpu/drm/drm_debugfs_crc.c
> > +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> > @@ -274,6 +274,14 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
> >  		return 0;
> >  	}
> >  
> > +	if (crtc->funcs->pre_crc_read) {
> > +		ret = crtc->funcs->pre_crc_read(crtc);
> > +		if (ret) {
> > +			spin_unlock_irq(&crc->lock);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	/* Nothing to read? */
> >  	while (crtc_crc_data_count(crc) == 0) {
> >  		if (filep->f_flags & O_NONBLOCK) {
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index b21437bc95bf..19c2bf63935e 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -761,6 +761,20 @@ struct drm_crtc_funcs {
> >  	 */
> >  	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
> >  				 size_t *values_cnt);
> > +	/**
> > +	 * @pre_crc_read:
> > +	 *
> > +	 * Driver callback for performing any preparation work required by
> > +	 * driver before reading CRC
> > +	 *
> > +	 * This callback is optional if the driver does not support CRC
> > +	 * generation or no prework is required before reading the crc
> > +	 *
> > +	 * RETURNS:
> > +	 *
> > +	 * 0 on success or a negative error code on failure.
> > +	 */
> > +	int (*pre_crc_read)(struct drm_crtc *crtc);
> >  	/**
> >  	 * @get_crc_sources:
> >  	 *
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx