Re: [Intel-gfx] [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function
Hi Mahesh,
Thank you for the patch.
On Thursday, 12 July 2018 11:36:33 EEST Mahesh Kumar wrote:
> This patch make changes to allocate crc-entries buffer before
> enabling CRC generation.
> It moves all the failure check early in the function before setting
> the source or memory allocation.
> Now set_crc_source takes only two variable inputs, values_cnt we
> already gets as part of verify_crc_source.
>
> Changes since V1:
> - refactor code to use single spin lock
>
> Signed-off-by: Mahesh Kumar
> Cc: dri-de...@lists.freedesktop.org
> Cc: Laurent Pinchart
> Reviewed-by: Maarten Lankhorst
> Acked-by: Leo Li
For core code and the R-Car DU driver,
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 +-
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 4 +-
> drivers/gpu/drm/drm_debugfs_crc.c | 61 +--
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_pipe_crc.c | 4 +-
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 5 +-
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c| 6 +--
> include/drm/drm_crtc.h | 3 +-
> 8 files changed, 37 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index
> e43ed064dc46..54056d180003 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -258,8 +258,7 @@ amdgpu_dm_remove_sink_from_freesync_module(struct
> drm_connector *connector);
>
> /* amdgpu_dm_crc.c */
> #ifdef CONFIG_DEBUG_FS
> -int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *src_name, -size_t *values_cnt);
> +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *src_name); int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
>const char *src_name,
>size_t *values_cnt);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c index
> dfcca594d52a..e7ad528f5853 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> @@ -62,8 +62,7 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
> const char *src_name, return 0;
> }
>
> -int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *src_name, - size_t *values_cnt)
> +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *src_name) {
> struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
> struct dc_stream_state *stream_state = crtc_state->stream;
> @@ -99,7 +98,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc,
> const char *src_name, return -EINVAL;
> }
>
> - *values_cnt = 3;
> /* Reset crc_skipped on dm state */
> crtc_state->crc_skip_count = 0;
> return 0;
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index 940b76a1ab71..7386391636e3 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -127,11 +127,9 @@ static ssize_t crc_control_write(struct file *file,
> const char __user *ubuf, if (source[len] == '\n')
> source[len] = '\0';
>
> - if (crtc->funcs->verify_crc_source) {
> - ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> - if (ret)
> - return ret;
> - }
> + ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> + if (ret)
> + return ret;
>
> spin_lock_irq(&crc->lock);
>
> @@ -197,40 +195,40 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) return ret;
> }
>
> + ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt);
> + if (ret)
> + return ret;
> +
> + if (WARN_ON(values_cnt > DRM_MAX_CRC_NR))
> + return -EINVAL;
> +
> + if (WARN_ON(values_cnt == 0))
> + return -EINVAL;
> +
> + entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
> + if (!entries)
> + return -ENOMEM;
> +
> spin_lock_irq(&crc->lock);
> - if (!crc->opened)
> + if (!crc->opened) {
> crc->opened = true;
> - else
> + crc->entries = entries;
> + crc->values_cnt = values_cnt;
> + } else {
> ret = -EBUSY;
> + }
> spin_unlock_irq(&crc->lock);
>
> - if (ret)
> + if (ret) {
> + kfree(entries);
> return ret;
> + }
>
> - ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> + ret = crtc->funcs->set_crc_source(crtc, crc->source);
> if (ret)
> goto
Re: [Intel-gfx] [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function
Hi,
Thanks for the review.
On 7/10/2018 5:19 PM, Laurent Pinchart wrote:
Hi Mahesh,
Thank you for the patch.
On Monday, 2 July 2018 14:07:27 EEST Mahesh Kumar wrote:
This patch make changes to allocate crc-entries buffer before
enabling CRC generation.
It moves all the failure check early in the function before setting
the source or memory allocation.
Now set_crc_source takes only two variable inputs, values_cnt we
already gets as part of verify_crc_source.
Signed-off-by: Mahesh Kumar
Cc: dri-de...@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst
---
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(-)
[snip]
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
b/drivers/gpu/drm/drm_debugfs_crc.c index f4d76528d24c..739a813b4b09 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -124,11 +124,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);
@@ -193,12 +191,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)
@@ -210,30 +211,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;
}
If you moved allocation before the !crc->opened check, you could group the two
code blocks protected by the crc->lock.
agree, will update in next version.
-Mahesh
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
@@ -250,7 +243,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);
@@ -262,9 +255,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);
@@ -370,7 +362,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);
[snip]
___
Intel-gfx mailing list
Intel-gfx@lists.fr
Re: [Intel-gfx] [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function
Hi Mahesh,
Thank you for the patch.
On Monday, 2 July 2018 14:07:27 EEST Mahesh Kumar wrote:
> This patch make changes to allocate crc-entries buffer before
> enabling CRC generation.
> It moves all the failure check early in the function before setting
> the source or memory allocation.
> Now set_crc_source takes only two variable inputs, values_cnt we
> already gets as part of verify_crc_source.
>
> Signed-off-by: Mahesh Kumar
> Cc: dri-de...@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst
> ---
> 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(-)
[snip]
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index f4d76528d24c..739a813b4b09 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -124,11 +124,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);
>
> @@ -193,12 +191,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)
> @@ -210,30 +211,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;
> }
If you moved allocation before the !crc->opened check, you could group the two
code blocks protected by the crc->lock.
> 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
> @@ -250,7 +243,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);
> @@ -262,9 +255,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);
> @@ -370,7 +362,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);
[snip]
--
Regards,
Laurent Pinchart
___
Intel-gfx mailing list
Intel-gfx
Re: [Intel-gfx] [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function
On 2018-07-02 07:07 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 inputs, values_cnt we
already gets as part of verify_crc_source.
Signed-off-by: Mahesh Kumar
Cc: dri-de...@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst
Acked-by: Leo Li
---
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 e43ed064dc46..54056d180003 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -258,8 +258,7 @@ amdgpu_dm_remove_sink_from_freesync_module(struct
drm_connector *connector);
/* amdgpu_dm_crc.c */
#ifdef CONFIG_DEBUG_FS
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
- size_t *values_cnt);
+int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name);
int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
const char *src_name,
size_t *values_cnt);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index dfcca594d52a..e7ad528f5853 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -62,8 +62,7 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const
char *src_name,
return 0;
}
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
- size_t *values_cnt)
+int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
{
struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
struct dc_stream_state *stream_state = crtc_state->stream;
@@ -99,7 +98,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc,
const char *src_name,
return -EINVAL;
}
- *values_cnt = 3;
/* Reset crc_skipped on dm state */
crtc_state->crc_skip_count = 0;
return 0;
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
b/drivers/gpu/drm/drm_debugfs_crc.c
index f4d76528d24c..739a813b4b09 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -124,11 +124,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);
@@ -193,12 +191,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)
@@ -210,30 +211,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;
+
