[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

Changes:
 - Add dri-devel in Cc

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

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

-- 
2.16.2

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

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

[Mahesh Kumar]

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

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

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

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

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

[Mahesh Kumar]

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

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

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

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

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

[Mahesh Kumar]

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

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

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

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

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

[Mahesh Kumar]

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

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c9222119767d..ea4884ac4cb0 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1138,12 +1138,32 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
 
 	return ret;
 }
+
+static int
+vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
+			   size_t *values_cnt)
+{
+	if ((source_name && strcmp(source_name, "auto") == 0) || !source_name) {
+		*values_cnt = 3;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 #else
 static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
 				   const char *source_name, size_t *values_cnt)
 {
 	return -ENODEV;
 }
+
+static int
+vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
+			   size_t *values_cnt)
+{
+	return -ENODEV;
+}
 #endif
 
 static const struct drm_crtc_funcs vop_crtc_funcs = {
@@ -1156,6 +1176,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

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

[Jani Nikula]

On Tue, 26 Jun 2018, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
> This patch implements "verify_crc_source" callback function for
> rockchip drm driver.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index c9222119767d..ea4884ac4cb0 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1138,12 +1138,32 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
>  
>  	return ret;
>  }
> +
> +static int
> +vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> +			   size_t *values_cnt)
> +{
> +	if ((source_name && strcmp(source_name, "auto") == 0) || !source_name) {

Drive-by review:

IOW,

	if (!source_name || strcmp(source_name, "auto") == 0)

Better yet, reverse the logic,

	if (source_name && strcmp(source_name, "auto") != 0)
        	return -EINVAL;

	*values_cnt = 3;

	return 0;

BR,
Jani.

> +		*values_cnt = 3;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  #else
>  static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
>  				   const char *source_name, size_t *values_cnt)
>  {
>  	return -ENODEV;
>  }
> +
> +static int
> +vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> +			   size_t *values_cnt)
> +{
> +	return -ENODEV;
> +}
>  #endif
>  
>  static const struct drm_crtc_funcs vop_crtc_funcs = {
> @@ -1156,6 +1176,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)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 04/10] drm/rockchip/crc: Implement verify_crc_source callback

[Kumar, Mahesh]

Hi,


On 6/27/2018 5:30 PM, Jani Nikula wrote:
> On Tue, 26 Jun 2018, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
>> This patch implements "verify_crc_source" callback function for
>> rockchip drm driver.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index c9222119767d..ea4884ac4cb0 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -1138,12 +1138,32 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
>>   
>>   	return ret;
>>   }
>> +
>> +static int
>> +vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>> +			   size_t *values_cnt)
>> +{
>> +	if ((source_name && strcmp(source_name, "auto") == 0) || !source_name) {
> Drive-by review:
>
> IOW,
>
> 	if (!source_name || strcmp(source_name, "auto") == 0)
>
> Better yet, reverse the logic,
>
> 	if (source_name && strcmp(source_name, "auto") != 0)
>          	return -EINVAL;
>
> 	*values_cnt = 3;
>
> 	return 0;
thanks for review,
Will reverse the logic as suggested.

-Mahesh
> BR,
> Jani.
>
>> +		*values_cnt = 3;
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>>   #else
>>   static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
>>   				   const char *source_name, size_t *values_cnt)
>>   {
>>   	return -ENODEV;
>>   }
>> +
>> +static int
>> +vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>> +			   size_t *values_cnt)
>> +{
>> +	return -ENODEV;
>> +}
>>   #endif
>>   
>>   static const struct drm_crtc_funcs vop_crtc_funcs = {
>> @@ -1156,6 +1176,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)

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

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

[Mahesh Kumar]

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

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 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 a1dd49545a5b..95aec2903add 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2565,6 +2565,7 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
 	.atomic_duplicate_state = dm_crtc_duplicate_state,
 	.atomic_destroy_state = dm_crtc_destroy_state,
 	.set_crc_source = amdgpu_dm_crtc_set_crc_source,
+	.verify_crc_source = amdgpu_dm_crtc_verify_crc_source,
 	.enable_vblank = dm_enable_vblank,
 	.disable_vblank = dm_disable_vblank,
 };
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index d5aa89ad5571..79b0a16652b9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -263,9 +263,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 06/10] drm/rcar-du/crc: Implement verify_crc_source callback

[Mahesh Kumar]

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

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 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

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

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

[Kumar, Mahesh]

Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>


On 6/26/2018 11:52 AM, 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
> ---
>   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,
>   };
>   
>   /* -----------------------------------------------------------------------------

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

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

[Mahesh Kumar]

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

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 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 3709fa1b6318..c0eb752b0901 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12893,6 +12893,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 578346b8d7e2..64e13adad9f5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2154,10 +2154,13 @@ int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
+int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
+				 const char *source_name, size_t *values_cnt);
 void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
 void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
+#define intel_crtc_verify_crc_source NULL
 static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
 {
 }
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 39a4e4edda07..a37521380f94 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -913,6 +913,114 @@ int intel_pipe_crc_create(struct drm_minor *minor)
 	return 0;
 }
 
+static int i8xx_crc_source_valid(struct drm_i915_private *dev_priv,
+				 const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int i9xx_crc_source_valid(struct drm_i915_private *dev_priv,
+				 const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_TV:
+	case INTEL_PIPE_CRC_SOURCE_DP_B:
+	case INTEL_PIPE_CRC_SOURCE_DP_C:
+	case INTEL_PIPE_CRC_SOURCE_DP_D:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vlv_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_DP_B:
+	case INTEL_PIPE_CRC_SOURCE_DP_C:
+	case INTEL_PIPE_CRC_SOURCE_DP_D:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ilk_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_PLANE1:
+	case INTEL_PIPE_CRC_SOURCE_PLANE2:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ivb_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_PLANE1:
+	case INTEL_PIPE_CRC_SOURCE_PLANE2:
+	case INTEL_PIPE_CRC_SOURCE_PF:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int
+intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
+			  const enum intel_pipe_crc_source source)
+{
+	if (IS_GEN2(dev_priv))
+		return i8xx_crc_source_valid(dev_priv, source);
+	else if (INTEL_GEN(dev_priv) < 5)
+		return i9xx_crc_source_valid(dev_priv, source);
+	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		return vlv_crc_source_valid(dev_priv, source);
+	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
+		return ilk_crc_source_valid(dev_priv, source);
+	else
+		return ivb_crc_source_valid(dev_priv, source);
+}
+
+int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
+				 size_t *values_cnt)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum intel_pipe_crc_source source;
+
+	if (display_crc_ctl_parse_source(source_name, &source) < 0) {
+		DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
+		return -EINVAL;
+	}
+
+	if (source == INTEL_PIPE_CRC_SOURCE_AUTO ||
+	    intel_is_valid_crc_source(dev_priv, source) == 0) {
+		*values_cnt = 5;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt)
 {
-- 
2.16.2

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

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

[Mahesh Kumar]

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c0eb752b0901..93b4be2eee32 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12894,6 +12894,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 64e13adad9f5..17735cafdd72 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2156,11 +2156,13 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
 				 const char *source_name, size_t *values_cnt);
+void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc);
 void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
 void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
 #define intel_crtc_verify_crc_source NULL
+#define intel_crtc_get_crc_sources NULL
 static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
 {
 }
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index a37521380f94..d6807155a237 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -1001,6 +1001,18 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
 		return ivb_crc_source_valid(dev_priv, source);
 }
 
+void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum intel_pipe_crc_source source;
+
+	seq_puts(m, "[");
+	for (source = 0; source < INTEL_PIPE_CRC_SOURCE_MAX; source++)
+		if (intel_is_valid_crc_source(dev_priv, source) == 0)
+			seq_printf(m, "%s,", pipe_crc_source_name(source));
+	seq_puts(m, "auto] ");
+}
+
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 				 size_t *values_cnt)
 {
-- 
2.16.2

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

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

[Daniel Vetter]

On Tue, Jun 26, 2018 at 11:52:57AM +0530, Mahesh Kumar wrote:
> This patch implements get_crc_sources callback, which returns list of
> all the valid crc sources supported by driver in current platform.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  1 +
>  drivers/gpu/drm/i915/intel_drv.h      |  2 ++
>  drivers/gpu/drm/i915/intel_pipe_crc.c | 12 ++++++++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c0eb752b0901..93b4be2eee32 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12894,6 +12894,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 64e13adad9f5..17735cafdd72 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2156,11 +2156,13 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			      size_t *values_cnt);
>  int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
>  				 const char *source_name, size_t *values_cnt);
> +void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc);
>  void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
>  void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
>  #else
>  #define intel_crtc_set_crc_source NULL
>  #define intel_crtc_verify_crc_source NULL
> +#define intel_crtc_get_crc_sources NULL
>  static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
>  {
>  }
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index a37521380f94..d6807155a237 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -1001,6 +1001,18 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
>  		return ivb_crc_source_valid(dev_priv, source);
>  }
>  
> +void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	enum intel_pipe_crc_source source;
> +
> +	seq_puts(m, "[");
> +	for (source = 0; source < INTEL_PIPE_CRC_SOURCE_MAX; source++)
> +		if (intel_is_valid_crc_source(dev_priv, source) == 0)
> +			seq_printf(m, "%s,", pipe_crc_source_name(source));
> +	seq_puts(m, "auto] ");

This seems to be a very quirky interface ... Can't you instead return a
real array, and then also push validation into the core? Same we do with
e.g. the per-plane drm_fourcc format support. Something like an array of
const char * pointers should be good.
-Daniel

> +}
> +
>  int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  				 size_t *values_cnt)
>  {
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Kumar, Mahesh]

Hi,

thanks for review.
On 6/26/2018 1:59 PM, Daniel Vetter wrote:
> On Tue, Jun 26, 2018 at 11:52:57AM +0530, Mahesh Kumar wrote:
>> This patch implements get_crc_sources callback, which returns list of
>> all the valid crc sources supported by driver in current platform.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c  |  1 +
>>   drivers/gpu/drm/i915/intel_drv.h      |  2 ++
>>   drivers/gpu/drm/i915/intel_pipe_crc.c | 12 ++++++++++++
>>   3 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index c0eb752b0901..93b4be2eee32 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12894,6 +12894,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 64e13adad9f5..17735cafdd72 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -2156,11 +2156,13 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>   			      size_t *values_cnt);
>>   int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
>>   				 const char *source_name, size_t *values_cnt);
>> +void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc);
>>   void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
>>   void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
>>   #else
>>   #define intel_crtc_set_crc_source NULL
>>   #define intel_crtc_verify_crc_source NULL
>> +#define intel_crtc_get_crc_sources NULL
>>   static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
>>   {
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> index a37521380f94..d6807155a237 100644
>> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
>> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> @@ -1001,6 +1001,18 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
>>   		return ivb_crc_source_valid(dev_priv, source);
>>   }
>>   
>> +void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>> +	enum intel_pipe_crc_source source;
>> +
>> +	seq_puts(m, "[");
>> +	for (source = 0; source < INTEL_PIPE_CRC_SOURCE_MAX; source++)
>> +		if (intel_is_valid_crc_source(dev_priv, source) == 0)
>> +			seq_printf(m, "%s,", pipe_crc_source_name(source));
>> +	seq_puts(m, "auto] ");
> This seems to be a very quirky interface ... Can't you instead return a
> real array, and then also push validation into the core? Same we do with
> e.g. the per-plane drm_fourcc format support. Something like an array of
> const char * pointers should be good.
sounds good, will  make the changes and return constant pointer to an array.
Will also push validation to the crc-core
-Mahesh
> -Daniel
>
>> +}
>> +
>>   int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>>   				 size_t *values_cnt)
>>   {
>> -- 
>> 2.16.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Daniel Vetter]

On Wed, Jun 27, 2018 at 07:41:05PM +0530, Kumar, Mahesh wrote:
> Hi,
> 
> thanks for review.
> On 6/26/2018 1:59 PM, Daniel Vetter wrote:
> > On Tue, Jun 26, 2018 at 11:52:57AM +0530, Mahesh Kumar wrote:
> > > This patch implements get_crc_sources callback, which returns list of
> > > all the valid crc sources supported by driver in current platform.
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_display.c  |  1 +
> > >   drivers/gpu/drm/i915/intel_drv.h      |  2 ++
> > >   drivers/gpu/drm/i915/intel_pipe_crc.c | 12 ++++++++++++
> > >   3 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index c0eb752b0901..93b4be2eee32 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12894,6 +12894,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 64e13adad9f5..17735cafdd72 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -2156,11 +2156,13 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> > >   			      size_t *values_cnt);
> > >   int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
> > >   				 const char *source_name, size_t *values_cnt);
> > > +void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc);
> > >   void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
> > >   void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
> > >   #else
> > >   #define intel_crtc_set_crc_source NULL
> > >   #define intel_crtc_verify_crc_source NULL
> > > +#define intel_crtc_get_crc_sources NULL
> > >   static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
> > >   {
> > >   }
> > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > index a37521380f94..d6807155a237 100644
> > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > @@ -1001,6 +1001,18 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
> > >   		return ivb_crc_source_valid(dev_priv, source);
> > >   }
> > > +void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > +	enum intel_pipe_crc_source source;
> > > +
> > > +	seq_puts(m, "[");
> > > +	for (source = 0; source < INTEL_PIPE_CRC_SOURCE_MAX; source++)
> > > +		if (intel_is_valid_crc_source(dev_priv, source) == 0)
> > > +			seq_printf(m, "%s,", pipe_crc_source_name(source));
> > > +	seq_puts(m, "auto] ");
> > This seems to be a very quirky interface ... Can't you instead return a
> > real array, and then also push validation into the core? Same we do with
> > e.g. the per-plane drm_fourcc format support. Something like an array of
> > const char * pointers should be good.
> sounds good, will  make the changes and return constant pointer to an array.
> Will also push validation to the crc-core

If you NULL terminate the array you can even skip the array_size. NULL
terminating arrays is the usual linux pattern - we can't do it in a few
places in drm (e.g. modifiers) because we don't have a canonical invalid
value. Could/should probably fix that, but oh well.
-Daniel

> -Mahesh
> > -Daniel
> > 
> > > +}
> > > +
> > >   int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> > >   				 size_t *values_cnt)
> > >   {
> > > -- 
> > > 2.16.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

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

[Mahesh Kumar]

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

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

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 79b0a16652b9..a7a5551fee1b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -261,8 +261,7 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
 
 /* amdgpu_dm_crc.c */
 #ifdef CONFIG_DEBUG_FS
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
-				  size_t *values_cnt);
+int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name);
 int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
 				     const char *src_name,
 				     size_t *values_cnt);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index dfcca594d52a..e7ad528f5853 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -62,8 +62,7 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
 	return 0;
 }
 
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
-			   size_t *values_cnt)
+int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 {
 	struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
 	struct dc_stream_state *stream_state = crtc_state->stream;
@@ -99,7 +98,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 			return -EINVAL;
 	}
 
-	*values_cnt = 3;
 	/* Reset crc_skipped on dm state */
 	crtc_state->crc_skip_count = 0;
 	return 0;
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 2751d124387d..834bc7ee5550 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -109,11 +109,9 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
-	if (crtc->funcs->verify_crc_source) {
-		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
-		if (ret)
-			return ret;
-	}
+	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+	if (ret)
+		return ret;
 
 	spin_lock_irq(&crc->lock);
 
@@ -178,12 +176,15 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 			return ret;
 	}
 
-	if (crtc->funcs->verify_crc_source) {
-		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
-						     &values_cnt);
-		if (ret)
-			return ret;
-	}
+	ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt);
+	if (ret)
+		return ret;
+
+	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR))
+		return -EINVAL;
+
+	if (WARN_ON(values_cnt == 0))
+		return -EINVAL;
 
 	spin_lock_irq(&crc->lock);
 	if (!crc->opened)
@@ -195,30 +196,22 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	if (ret)
 		return ret;
 
-	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
-	if (ret)
-		goto err;
-
-	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
-		ret = -EINVAL;
-		goto err_disable;
-	}
-
-	if (WARN_ON(values_cnt == 0)) {
-		ret = -EINVAL;
-		goto err_disable;
-	}
-
 	entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
 	if (!entries) {
 		ret = -ENOMEM;
-		goto err_disable;
+		goto err;
 	}
 
 	spin_lock_irq(&crc->lock);
 	crc->entries = entries;
 	crc->values_cnt = values_cnt;
+	spin_unlock_irq(&crc->lock);
 
+	ret = crtc->funcs->set_crc_source(crtc, crc->source);
+	if (ret)
+		goto err;
+
+	spin_lock_irq(&crc->lock);
 	/*
 	 * Only return once we got a first frame, so userspace doesn't have to
 	 * guess when this particular piece of HW will be ready to start
@@ -235,7 +228,7 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	return 0;
 
 err_disable:
-	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+	crtc->funcs->set_crc_source(crtc, NULL);
 err:
 	spin_lock_irq(&crc->lock);
 	crtc_crc_cleanup(crc);
@@ -247,9 +240,8 @@ static int crtc_crc_release(struct inode *inode, struct file *filep)
 {
 	struct drm_crtc *crtc = filep->f_inode->i_private;
 	struct drm_crtc_crc *crc = &crtc->crc;
-	size_t values_cnt;
 
-	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+	crtc->funcs->set_crc_source(crtc, NULL);
 
 	spin_lock_irq(&crc->lock);
 	crtc_crc_cleanup(crc);
@@ -363,7 +355,7 @@ int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
 {
 	struct dentry *crc_ent, *ent;
 
-	if (!crtc->funcs->set_crc_source)
+	if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source)
 		return 0;
 
 	crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 17735cafdd72..ba3f1c5e6a1e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2152,8 +2152,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
 /* intel_pipe_crc.c */
 int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
-int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
-			      size_t *values_cnt);
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name);
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
 				 const char *source_name, size_t *values_cnt);
 void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index d6807155a237..50bdb56afc79 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -1033,8 +1033,7 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 	return -EINVAL;
 }
 
-int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
-			      size_t *values_cnt)
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
@@ -1073,7 +1072,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 	}
 
 	pipe_crc->skipped = 0;
-	*values_cnt = 5;
 
 out:
 	intel_display_power_put(dev_priv, power_domain);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 24eeaa7e14d7..829c644addba 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -796,8 +796,7 @@ static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
 }
 
 static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
-				       const char *source_name,
-				       size_t *values_cnt)
+				       const char *source_name)
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct drm_modeset_acquire_ctx ctx;
@@ -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 ea4884ac4cb0..ed7f8a2c7208 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)
@@ -1153,7 +1151,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 b3824f92c190..257b3d738d94 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -659,8 +659,7 @@ struct drm_crtc_funcs {
 	 *
 	 * 0 on success or a negative error code on failure.
 	 */
-	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
-			      size_t *values_cnt);
+	int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
 	/**
 	 * @verify_crc_source:
 	 *
-- 
2.16.2

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

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

[Kumar, Mahesh]

Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>


On 6/26/2018 11:52 AM, 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 input, values_cnt we
> already gets as part of verify_crc_source.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |  3 +-
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  4 +-
>   drivers/gpu/drm/drm_debugfs_crc.c                  | 52 +++++++++-------------
>   drivers/gpu/drm/i915/intel_drv.h                   |  3 +-
>   drivers/gpu/drm/i915/intel_pipe_crc.c              |  4 +-
>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  5 +--
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  6 +--
>   include/drm/drm_crtc.h                             |  3 +-
>   8 files changed, 30 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 79b0a16652b9..a7a5551fee1b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -261,8 +261,7 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
>   
>   /* amdgpu_dm_crc.c */
>   #ifdef CONFIG_DEBUG_FS
> -int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> -				  size_t *values_cnt);
> +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name);
>   int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
>   				     const char *src_name,
>   				     size_t *values_cnt);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> index dfcca594d52a..e7ad528f5853 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> @@ -62,8 +62,7 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
>   	return 0;
>   }
>   
> -int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> -			   size_t *values_cnt)
> +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>   {
>   	struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
>   	struct dc_stream_state *stream_state = crtc_state->stream;
> @@ -99,7 +98,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
>   			return -EINVAL;
>   	}
>   
> -	*values_cnt = 3;
>   	/* Reset crc_skipped on dm state */
>   	crtc_state->crc_skip_count = 0;
>   	return 0;
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 2751d124387d..834bc7ee5550 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -109,11 +109,9 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>   	if (source[len] == '\n')
>   		source[len] = '\0';
>   
> -	if (crtc->funcs->verify_crc_source) {
> -		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> +	if (ret)
> +		return ret;
>   
>   	spin_lock_irq(&crc->lock);
>   
> @@ -178,12 +176,15 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>   			return ret;
>   	}
>   
> -	if (crtc->funcs->verify_crc_source) {
> -		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
> -						     &values_cnt);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt);
> +	if (ret)
> +		return ret;
> +
> +	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR))
> +		return -EINVAL;
> +
> +	if (WARN_ON(values_cnt == 0))
> +		return -EINVAL;
>   
>   	spin_lock_irq(&crc->lock);
>   	if (!crc->opened)
> @@ -195,30 +196,22 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>   	if (ret)
>   		return ret;
>   
> -	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> -	if (ret)
> -		goto err;
> -
> -	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
> -		ret = -EINVAL;
> -		goto err_disable;
> -	}
> -
> -	if (WARN_ON(values_cnt == 0)) {
> -		ret = -EINVAL;
> -		goto err_disable;
> -	}
> -
>   	entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
>   	if (!entries) {
>   		ret = -ENOMEM;
> -		goto err_disable;
> +		goto err;
>   	}
>   
>   	spin_lock_irq(&crc->lock);
>   	crc->entries = entries;
>   	crc->values_cnt = values_cnt;
> +	spin_unlock_irq(&crc->lock);
>   
> +	ret = crtc->funcs->set_crc_source(crtc, crc->source);
> +	if (ret)
> +		goto err;
> +
> +	spin_lock_irq(&crc->lock);
>   	/*
>   	 * Only return once we got a first frame, so userspace doesn't have to
>   	 * guess when this particular piece of HW will be ready to start
> @@ -235,7 +228,7 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>   	return 0;
>   
>   err_disable:
> -	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> +	crtc->funcs->set_crc_source(crtc, NULL);
>   err:
>   	spin_lock_irq(&crc->lock);
>   	crtc_crc_cleanup(crc);
> @@ -247,9 +240,8 @@ static int crtc_crc_release(struct inode *inode, struct file *filep)
>   {
>   	struct drm_crtc *crtc = filep->f_inode->i_private;
>   	struct drm_crtc_crc *crc = &crtc->crc;
> -	size_t values_cnt;
>   
> -	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> +	crtc->funcs->set_crc_source(crtc, NULL);
>   
>   	spin_lock_irq(&crc->lock);
>   	crtc_crc_cleanup(crc);
> @@ -363,7 +355,7 @@ int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>   {
>   	struct dentry *crc_ent, *ent;
>   
> -	if (!crtc->funcs->set_crc_source)
> +	if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source)
>   		return 0;
>   
>   	crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 17735cafdd72..ba3f1c5e6a1e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2152,8 +2152,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
>   /* intel_pipe_crc.c */
>   int intel_pipe_crc_create(struct drm_minor *minor);
>   #ifdef CONFIG_DEBUG_FS
> -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> -			      size_t *values_cnt);
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name);
>   int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
>   				 const char *source_name, size_t *values_cnt);
>   void intel_crtc_get_crc_sources(struct seq_file *m, struct drm_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index d6807155a237..50bdb56afc79 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -1033,8 +1033,7 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>   	return -EINVAL;
>   }
>   
> -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> -			      size_t *values_cnt)
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>   	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> @@ -1073,7 +1072,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>   	}
>   
>   	pipe_crc->skipped = 0;
> -	*values_cnt = 5;
>   
>   out:
>   	intel_display_power_put(dev_priv, power_domain);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 24eeaa7e14d7..829c644addba 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -796,8 +796,7 @@ static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
>   }
>   
>   static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
> -				       const char *source_name,
> -				       size_t *values_cnt)
> +				       const char *source_name)
>   {
>   	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>   	struct drm_modeset_acquire_ctx ctx;
> @@ -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 ea4884ac4cb0..ed7f8a2c7208 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)
> @@ -1153,7 +1151,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 b3824f92c190..257b3d738d94 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -659,8 +659,7 @@ struct drm_crtc_funcs {
>   	 *
>   	 * 0 on success or a negative error code on failure.
>   	 */
> -	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
> -			      size_t *values_cnt);
> +	int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
>   	/**
>   	 * @verify_crc_source:
>   	 *

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

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

[Mahesh Kumar]

This reverts commit e8fa5671183c80342d520ad81d14fa79a9d4a680.

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

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 16 ----------------
 1 file changed, 16 deletions(-)

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

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

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

[Kumar, Mahesh]

[-- Attachment #1.1: Type: text/plain, Size: 2294 bytes --]

Cc:dri-devel@lists.freedesktop.org


On 6/26/2018 11:52 AM, 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
>
> Changes:
>   - Add dri-devel in Cc
>
> Mahesh Kumar (10):
>    drm: crc: Introduce verify_crc_source callback
>    drm: crc: Introduce pre_crc_read function
>    drm: crc: Introduce get_crc_sources callback
>    drm/rockchip/crc: Implement verify_crc_source callback
>    drm/amdgpu_dm/crc: Implement verify_crc_source callback
>    drm/rcar-du/crc: Implement verify_crc_source callback
>    drm/i915/crc: implement verify_crc_source callback
>    drm/i915/crc: implement get_crc_sources callback
>    drm/crc: Cleanup crtc_crc_open function
>    Revert "drm: crc: Wait for a frame before returning from open()"
>
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
>   drivers/gpu/drm/drm_debugfs_crc.c                  |  62 ++++++-----
>   drivers/gpu/drm/i915/intel_display.c               |   2 +
>   drivers/gpu/drm/i915/intel_drv.h                   |   8 +-
>   drivers/gpu/drm/i915/intel_pipe_crc.c              | 124 ++++++++++++++++++++-
>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  45 +++++++-
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  27 ++++-
>   include/drm/drm_crtc.h                             |  42 ++++++-
>   10 files changed, 288 insertions(+), 50 deletions(-)
>


[-- Attachment #1.2: Type: text/html, Size: 2677 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from CI_DRM_4376 -> Patchwork_9420 =

== Summary - FAILURE ==

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-r:           PASS -> DMESG-WARN

    igt@kms_pipe_crc_basic@bad-source:
      fi-skl-6770hq:      PASS -> FAIL +6
      fi-bwr-2160:        PASS -> FAIL +4
      fi-hsw-4770r:       PASS -> FAIL +6
      fi-glk-j4005:       PASS -> FAIL
      fi-cfl-8700k:       PASS -> FAIL +6
      {fi-kbl-x1275}:     PASS -> FAIL
      fi-kbl-guc:         PASS -> FAIL
      fi-glk-dsi:         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 +5
      fi-blb-e6850:       PASS -> FAIL +4
      fi-byt-j1900:       PASS -> FAIL +4

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-ilk-650:         PASS -> FAIL +4
      fi-elk-e7500:       PASS -> FAIL +4
      fi-byt-n2820:       PASS -> FAIL +4
      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-bxt-j4205:       PASS -> FAIL +6
      fi-kbl-7560u:       PASS -> FAIL +6
      fi-whl-u:           PASS -> FAIL +6
      fi-kbl-r:           PASS -> FAIL +6
      fi-bsw-n3050:       PASS -> FAIL +2

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-ilk-650:         PASS -> DMESG-WARN (fdo#106387)

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       PASS -> FAIL (fdo#106744)

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

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

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-skl-6600u:       DMESG-WARN -> PASS

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

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


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

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4376 -> Patchwork_9420

  CI_DRM_4376: b64deb6f5bec9732aff68734c2e2382f044403f2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9420: 0d30485d448b621bb51059babc574168332173b7 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

0d30485d448b Revert "drm: crc: Wait for a frame before returning from open()"
83b4a77edbb8 drm/crc: Cleanup crtc_crc_open function
18a314fc3ea7 drm/i915/crc: implement get_crc_sources callback
c4857112d0d3 drm/i915/crc: implement verify_crc_source callback
5ce40cb4b3cb drm/rcar-du/crc: Implement verify_crc_source callback
f05ec26c1589 drm/amdgpu_dm/crc: Implement verify_crc_source callback
373bd2222294 drm/rockchip/crc: Implement verify_crc_source callback
e4807850a12e drm: crc: Introduce get_crc_sources callback
e38fd22995f8 drm: crc: Introduce pre_crc_read function
2647676817bf drm: crc: Introduce verify_crc_source callback

== Logs ==

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