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 <mahesh1.ku...@intel.com>
> Cc: dri-de...@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 +-
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 4 +-
> drivers/gpu/drm/drm_debugfs_crc.c | 52 ++++++++-----------
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_pipe_crc.c | 4 +-
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 5 +--
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 6 +--
> include/drm/drm_crtc.h | 3 +-
> 8 files changed, 30 insertions(+), 50 deletions(-)
[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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
