From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function
Date: Thu, 12 Jul 2018 14:39:55 +0300 [thread overview]
Message-ID: <2218246.iABgPZ8dVG@avalon> (raw)
In-Reply-To: <20180712083635.7472-9-mahesh1.kumar@intel.com>
Hi Mahesh,
Thank you for the patch.
On Thursday, 12 July 2018 11:36:33 EEST Mahesh Kumar wrote:
> This patch make changes to allocate crc-entries buffer before
> enabling CRC generation.
> It moves all the failure check early in the function before setting
> the source or memory allocation.
> Now set_crc_source takes only two variable inputs, values_cnt we
> already gets as part of verify_crc_source.
>
> Changes since V1:
> - refactor code to use single spin lock
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Acked-by: Leo Li <sunpeng.li@amd.com>
For core code and the R-Car DU driver,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 +-
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 4 +-
> drivers/gpu/drm/drm_debugfs_crc.c | 61 +++++++++----------
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_pipe_crc.c | 4 +-
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 5 +-
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 6 +--
> include/drm/drm_crtc.h | 3 +-
> 8 files changed, 37 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index
> e43ed064dc46..54056d180003 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -258,8 +258,7 @@ amdgpu_dm_remove_sink_from_freesync_module(struct
> drm_connector *connector);
>
> /* amdgpu_dm_crc.c */
> #ifdef CONFIG_DEBUG_FS
> -int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *src_name, - size_t *values_cnt);
> +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *src_name); int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
> const char *src_name,
> size_t *values_cnt);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c index
> dfcca594d52a..e7ad528f5853 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> @@ -62,8 +62,7 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
> const char *src_name, return 0;
> }
>
> -int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *src_name, - size_t *values_cnt)
> +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *src_name) {
> struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
> struct dc_stream_state *stream_state = crtc_state->stream;
> @@ -99,7 +98,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc,
> const char *src_name, return -EINVAL;
> }
>
> - *values_cnt = 3;
> /* Reset crc_skipped on dm state */
> crtc_state->crc_skip_count = 0;
> return 0;
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index 940b76a1ab71..7386391636e3 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -127,11 +127,9 @@ static ssize_t crc_control_write(struct file *file,
> const char __user *ubuf, if (source[len] == '\n')
> source[len] = '\0';
>
> - if (crtc->funcs->verify_crc_source) {
> - ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> - if (ret)
> - return ret;
> - }
> + ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> + if (ret)
> + return ret;
>
> spin_lock_irq(&crc->lock);
>
> @@ -197,40 +195,40 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) return ret;
> }
>
> + ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt);
> + if (ret)
> + return ret;
> +
> + if (WARN_ON(values_cnt > DRM_MAX_CRC_NR))
> + return -EINVAL;
> +
> + if (WARN_ON(values_cnt == 0))
> + return -EINVAL;
> +
> + entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
> + if (!entries)
> + return -ENOMEM;
> +
> spin_lock_irq(&crc->lock);
> - if (!crc->opened)
> + if (!crc->opened) {
> crc->opened = true;
> - else
> + crc->entries = entries;
> + crc->values_cnt = values_cnt;
> + } else {
> ret = -EBUSY;
> + }
> spin_unlock_irq(&crc->lock);
>
> - if (ret)
> + if (ret) {
> + kfree(entries);
> return ret;
> + }
>
> - ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> + ret = crtc->funcs->set_crc_source(crtc, crc->source);
> if (ret)
> goto err;
>
> - if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
> - ret = -EINVAL;
> - goto err_disable;
> - }
> -
> - if (WARN_ON(values_cnt == 0)) {
> - ret = -EINVAL;
> - goto err_disable;
> - }
> -
> - entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
> - if (!entries) {
> - ret = -ENOMEM;
> - goto err_disable;
> - }
> -
> spin_lock_irq(&crc->lock);
> - crc->entries = entries;
> - crc->values_cnt = values_cnt;
> -
> /*
> * Only return once we got a first frame, so userspace doesn't have to
> * guess when this particular piece of HW will be ready to start
> @@ -247,7 +245,7 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) return 0;
>
> err_disable:
> - crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> + crtc->funcs->set_crc_source(crtc, NULL);
> err:
> spin_lock_irq(&crc->lock);
> crtc_crc_cleanup(crc);
> @@ -259,9 +257,8 @@ static int crtc_crc_release(struct inode *inode, struct
> file *filep) {
> struct drm_crtc *crtc = filep->f_inode->i_private;
> struct drm_crtc_crc *crc = &crtc->crc;
> - size_t values_cnt;
>
> - crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> + crtc->funcs->set_crc_source(crtc, NULL);
>
> spin_lock_irq(&crc->lock);
> crtc_crc_cleanup(crc);
> @@ -367,7 +364,7 @@ int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> {
> struct dentry *crc_ent, *ent;
>
> - if (!crtc->funcs->set_crc_source)
> + if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source)
> return 0;
>
> crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h index 29b054de9e13..814f6f3e1595 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2155,8 +2155,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon
> *lspcon);
>
> /* intel_pipe_crc.c */
> #ifdef CONFIG_DEBUG_FS
> -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *source_name, - size_t *values_cnt);
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *source_name); int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
> const char *source_name, size_t *values_cnt);
> const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/intel_pipe_crc.c index 83f9ade0cd81..f3c9010e332a
> 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -583,8 +583,7 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
> const char *source_name, return -EINVAL;
> }
>
> -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *source_name, - size_t *values_cnt)
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> *source_name) {
> struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> @@ -623,7 +622,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc,
> const char *source_name, }
>
> pipe_crc->skipped = 0;
> - *values_cnt = 5;
>
> out:
> intel_display_power_put(dev_priv, power_domain);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 7124918372f8..72db1834dd50
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -808,8 +808,7 @@ static int rcar_du_crtc_verify_crc_source(struct
> drm_crtc *crtc, }
>
> static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
> - const char *source_name,
> - size_t *values_cnt)
> + const char *source_name)
> {
> struct drm_modeset_acquire_ctx ctx;
> struct drm_crtc_state *crtc_state;
> @@ -837,8 +836,6 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc
> *crtc, return -EINVAL;
> }
>
> - *values_cnt = 1;
> -
> /* Perform an atomic commit to set the CRC source. */
> drm_modeset_acquire_init(&ctx, 0);
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index
> 77e91b15ddb4..657372306623 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1117,7 +1117,7 @@ static struct drm_connector
> *vop_get_edp_connector(struct vop *vop) }
>
> static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
> - const char *source_name, size_t *values_cnt)
> + const char *source_name)
> {
> struct vop *vop = to_vop(crtc);
> struct drm_connector *connector;
> @@ -1127,8 +1127,6 @@ static int vop_crtc_set_crc_source(struct drm_crtc
> *crtc, if (!connector)
> return -EINVAL;
>
> - *values_cnt = 3;
> -
> if (source_name && strcmp(source_name, "auto") == 0)
> ret = analogix_dp_start_crc(connector);
> else if (!source_name)
> @@ -1152,7 +1150,7 @@ vop_crtc_verify_crc_source(struct drm_crtc *crtc,
> const char *source_name,
>
> #else
> static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
> - const char *source_name, size_t *values_cnt)
> + const char *source_name)
> {
> return -ENODEV;
> }
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 69886025e628..1013d6596408 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -682,8 +682,7 @@ struct drm_crtc_funcs {
> *
> * 0 on success or a negative error code on failure.
> */
> - int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
> - size_t *values_cnt);
> + int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
> /**
> * @verify_crc_source:
> *
--
Regards,
Laurent Pinchart
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2018-07-12 11:39 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-12 8:36 [PATCH 00/10] Improve crc-core driver interface Mahesh Kumar
2018-07-12 8:36 ` [PATCH 01/10] drm: crc: Introduce verify_crc_source callback Mahesh Kumar
2018-07-12 11:06 ` Laurent Pinchart
2018-07-12 8:36 ` [PATCH 02/10] drm: crc: Introduce get_crc_sources callback Mahesh Kumar
2018-07-12 11:28 ` Laurent Pinchart
2018-07-12 8:36 ` [PATCH 03/10] drm/rockchip/crc: Implement verify_crc_source callback Mahesh Kumar
2018-07-12 8:36 ` [PATCH 04/10] drm/amdgpu_dm/crc: " Mahesh Kumar
2018-07-12 8:36 ` [PATCH 05/10] drm/rcar-du/crc: " Mahesh Kumar
2018-07-12 11:37 ` Laurent Pinchart
2018-07-12 8:36 ` [PATCH 06/10] drm/i915/crc: implement " Mahesh Kumar
2018-07-12 8:36 ` [PATCH 07/10] drm/i915/crc: implement get_crc_sources callback Mahesh Kumar
2018-07-12 8:36 ` [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function Mahesh Kumar
2018-07-12 11:39 ` Laurent Pinchart [this message]
2018-07-12 8:36 ` [PATCH 09/10] Revert "drm: crc: Wait for a frame before returning from open()" Mahesh Kumar
2018-07-12 8:36 ` [PATCH 10/10] drm/rcar-du/crc: Implement get_crc_sources callback Mahesh Kumar
2018-07-12 11:46 ` Laurent Pinchart
2018-07-12 9:22 ` ✗ Fi.CI.SPARSE: warning for Improve crc-core driver interface (rev6) Patchwork
2018-07-12 9:35 ` ✗ Fi.CI.BAT: failure " Patchwork
2018-07-12 11:08 ` [PATCH 00/10] Improve crc-core driver interface Laurent Pinchart
2018-07-16 14:24 ` Kumar, Mahesh
-- strict thread matches above, loose matches on Subject: below --
2018-07-11 15:11 Mahesh Kumar
2018-07-11 15:11 ` [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function Mahesh Kumar
2018-07-02 11:07 [PATCH 00/10] Improve crc-core driver interface Mahesh Kumar
2018-07-02 11:07 ` [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function Mahesh Kumar
2018-07-03 13:05 ` Leo Li
2018-07-10 11:49 ` Laurent Pinchart
2018-07-10 13:18 ` Kumar, Mahesh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2218246.iABgPZ8dVG@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=mahesh1.kumar@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.