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
Cc: dri-devel@lists.freedesktop.org
Mahesh Kumar (10): drm: crc: Introduce verify_crc_source callback 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()" drm/rcar-du/crc: Implement get_crc_sources callback
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 | 92 +++++++++------- drivers/gpu/drm/i915/intel_display.c | 2 + drivers/gpu/drm/i915/intel_drv.h | 9 +- drivers/gpu/drm/i915/intel_pipe_crc.c | 119 ++++++++++++++++++++- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 82 +++++++++++--- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 + drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 67 ++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_kms.h | 1 + drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 26 ++++- include/drm/drm_crtc.h | 40 ++++++- 14 files changed, 396 insertions(+), 74 deletions(-)
This patch adds a new callback function "verify_crc_source" which will be used during setting the crc source in control node. This will help in avoiding setting of wrong string for source.
Changes since V1: - do not yet verify_crc_source during open.
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 --- drivers/gpu/drm/drm_debugfs_crc.c | 8 ++++++++ include/drm/drm_crtc.h | 15 +++++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index 99961192bf03..1956acb61cc8 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) { diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 17f4f93340b8..3ce5d2fe133d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -684,6 +684,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:
Hi Mahesh,
Thank you for the patch.
On Thursday, 12 July 2018 11:36:26 EEST Mahesh Kumar wrote:
This patch adds a new callback function "verify_crc_source" which will be used during setting the crc source in control node. This will help in avoiding setting of wrong string for source.
Changes since V1:
- do not yet verify_crc_source during open.
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
drivers/gpu/drm/drm_debugfs_crc.c | 8 ++++++++ include/drm/drm_crtc.h | 15 +++++++++++++++ 2 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index 99961192bf03..1956acb61cc8 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;
No need to initialize ret to 0 here.
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) {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 17f4f93340b8..3ce5d2fe133d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -684,6 +684,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.
Please also document whether the source parameter can be NULL.
It would also be nice to document values_cnt, but as that's not documented for the .set_crc_source() operation at the moment, I won't block the patch for this.
Apart from that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
*
* 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:
This patch introduce 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 return a constant pointer to an array of crc sources list and update count according to the number of source in the list.
Changes Since V1: (Daniel) - return const pointer to an array of crc sources list - do validation of sources in CRC-core Changes Since V2: - update commit message - update callback documentation - print one source name per line
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 --- drivers/gpu/drm/drm_debugfs_crc.c | 23 ++++++++++++++++++++++- include/drm/drm_crtc.h | 22 ++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index 1956acb61cc8..940b76a1ab71 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -68,8 +68,29 @@ static int crc_control_show(struct seq_file *m, void *data) { struct drm_crtc *crtc = m->private;
- seq_printf(m, "%s\n", crtc->crc.source); + if (crtc->funcs->get_crc_sources) { + size_t count; + const char *const *sources = crtc->funcs->get_crc_sources(crtc, + &count); + size_t values_cnt; + int i; + + if (count == 0 || !sources) + goto out; + + for (i = 0; i < count; i++) + if (!crtc->funcs->verify_crc_source(crtc, sources[i], + &values_cnt)) { + if (strcmp(sources[i], crtc->crc.source)) + seq_printf(m, "%s\n", sources[i]); + else + seq_printf(m, "%s*\n", sources[i]); + } + } + return 0;
+out: + 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 3ce5d2fe133d..69886025e628 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -699,6 +699,28 @@ struct drm_crtc_funcs { */ int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, size_t *values_cnt); + /** + * @get_crc_sources: + * + * Driver callback for getting a list of all the available sources for + * CRC generation. This callback depends upon verify_crc_source, So + * verify_crc_source callback should be implemented before implementing + * this. Driver can pass full list of available crc sources, this + * callback does the verification on each crc-source before passing it + * to userspace. + * + * This callback is optional if the driver does not support exporting of + * possible CRC sources list. + * + * RETURNS: + * + * a constant character pointer to the list of all the available CRC + * sources. On failure driver should return NULL. count should be + * updated with number of sources in list. if zero we don't process any + * source from the list. + */ + const char *const *(*get_crc_sources)(struct drm_crtc *crtc, + size_t *count);
/** * @atomic_print_state:
Hi Mahesh,
Thank you for the patch.
On Thursday, 12 July 2018 11:36:27 EEST Mahesh Kumar wrote:
This patch introduce 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 return a constant pointer to an array of crc sources list and update count according to the number of source in the list.
Changes Since V1: (Daniel)
- return const pointer to an array of crc sources list
- do validation of sources in CRC-core
Changes Since V2:
- update commit message
- update callback documentation
- print one source name per line
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
drivers/gpu/drm/drm_debugfs_crc.c | 23 ++++++++++++++++++++++- include/drm/drm_crtc.h | 22 ++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index 1956acb61cc8..940b76a1ab71 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -68,8 +68,29 @@ static int crc_control_show(struct seq_file *m, void *data) { struct drm_crtc *crtc = m->private;
- seq_printf(m, "%s\n", crtc->crc.source);
- if (crtc->funcs->get_crc_sources) {
size_t count;
const char *const *sources = crtc->funcs->get_crc_sources(crtc,
&count);
size_t values_cnt;
int i;
i only takes positive values, you can make it an unsigned int.
if (count == 0 || !sources)
goto out;
for (i = 0; i < count; i++)
if (!crtc->funcs->verify_crc_source(crtc, sources[i],
&values_cnt)) {
if (strcmp(sources[i], crtc->crc.source))
seq_printf(m, "%s\n", sources[i]);
else
seq_printf(m, "%s*\n", sources[i]);
}
- }
- return 0;
+out:
- 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 3ce5d2fe133d..69886025e628 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -699,6 +699,28 @@ struct drm_crtc_funcs { */ int (*verify_crc_source)(struct drm_crtc *crtc, const char *source, size_t *values_cnt);
- /**
* @get_crc_sources:
*
* Driver callback for getting a list of all the available sources for
* CRC generation. This callback depends upon verify_crc_source, So
s/So/so/
* verify_crc_source callback should be implemented before implementing
* this. Driver can pass full list of available crc sources, this
* callback does the verification on each crc-source before passing it
* to userspace.
*
* This callback is optional if the driver does not support exporting of
* possible CRC sources list.
*
* RETURNS:
*
* a constant character pointer to the list of all the available CRC
* sources. On failure driver should return NULL. count should be
* updated with number of sources in list. if zero we don't process any
s/if zero/If zero/
Apart from that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
* source from the list.
*/
const char *const *(*get_crc_sources)(struct drm_crtc *crtc,
size_t *count);
/**
- @atomic_print_state:
This patch implements "verify_crc_source" callback function for rockchip drm driver.
Changes since V1: - simplify the verification (Jani N)
Signed-off-by: Mahesh Kumar mahesh1.kumar@intel.com Cc: dri-devel@lists.freedesktop.org Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Heiko Stuebner heiko@sntech.de --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index effecbed2d11..77e91b15ddb4 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1138,12 +1138,31 @@ 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) + return -EINVAL; + + *values_cnt = 3; + return 0; +} + #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 +1175,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)
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 Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Leo Li sunpeng.li@amd.com --- 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 f0fc3067ab5d..be127ba7e604 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -2608,6 +2608,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 a29dc35954c9..e43ed064dc46 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -260,9 +260,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) {
This patch implements "verify_crc_source" callback function for rcar drm driver.
Changes Since V1: - avoid duplication of code
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 --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 +++++++++++++++++++++++++++------- 1 file changed, 53 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 15dc9caa128b..7124918372f8 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -756,17 +756,66 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc) rcrtc->vblank_enable = false; }
+static int rcar_du_get_plane_index(struct drm_crtc *crtc, + const char *source_name, + unsigned int *index) +{ + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); + unsigned int i; + int ret = 0; + + 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; + + return ret; +} + +static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc, + const char *source_name, + size_t *values_cnt) +{ + int ret; + unsigned int index; + /* + * 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 = rcar_du_get_plane_index(crtc, source_name, &index); + if (ret < 0) + return ret; + } 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) { - struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); struct drm_modeset_acquire_ctx ctx; struct drm_crtc_state *crtc_state; struct drm_atomic_state *state; enum vsp1_du_crc_source source; unsigned int index = 0; - unsigned int i; int ret;
/* @@ -781,19 +830,9 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc, } else if (strstarts(source_name, "plane")) { source = VSP1_DU_CRC_PLANE;
- ret = kstrtouint(source_name + strlen("plane"), 10, &index); + ret = rcar_du_get_plane_index(crtc, source_name, &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; } @@ -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, };
/* -----------------------------------------------------------------------------
Hi Mahesh,
Thank you for the patch.
On Thursday, 12 July 2018 11:36:30 EEST Mahesh Kumar wrote:
This patch implements "verify_crc_source" callback function for rcar drm driver.
Changes Since V1:
- avoid duplication of code
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
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 ++++++++++++++++++++++++------- 1 file changed, 53 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 15dc9caa128b..7124918372f8 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -756,17 +756,66 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc) rcrtc->vblank_enable = false; }
+static int rcar_du_get_plane_index(struct drm_crtc *crtc,
Please pass a struct rcar_du_crtc pointer to this function, as callers should already have it (or can easily compute it).
const char *source_name,
unsigned int *index)
I think you can simplify this by returning the index, which is always positive, or a negative error code in case of error.
I think you could share even more code if you named this function rcar_du_crtc_parse_crc_source() and returned both the index and the vsp1_du_crc_source (one of them through a pointer).
+{
- struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
- unsigned int i;
- int ret = 0;
No need to initialize ret to 0.
- ret = kstrtouint(source_name + strlen("plane"), 10, index);
This assumes that the caller will always ensure that source_nam starts with "plane", otherwise you could access the string beyond its end. This patch is fine, but it would be easy to use the function erroneously in the future. How about moving the strstarts(source_name, "plane") check at the beginning of this function ?
- 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;
- return ret;
+}
+static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
const char *source_name,
size_t *values_cnt)
+{
- int ret;
- unsigned int index;
Please sort variable declarations roughly by line length to match the style of the driver.
You need a blank line here.
- /*
* 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")) {
Can source_name be NULL here ?
goto out;
- } else if (strstarts(source_name, "plane")) {
ret = rcar_du_get_plane_index(crtc, source_name, &index);
if (ret < 0)
return ret;
- } 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) {
struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc); struct drm_modeset_acquire_ctx ctx; struct drm_crtc_state *crtc_state; struct drm_atomic_state *state; enum vsp1_du_crc_source source; unsigned int index = 0;
unsigned int i; int ret;
/*
@@ -781,19 +830,9 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc, } else if (strstarts(source_name, "plane")) { source = VSP1_DU_CRC_PLANE;
ret = kstrtouint(source_name + strlen("plane"), 10, &index);
if (ret < 0) return ret;ret = rcar_du_get_plane_index(crtc, source_name, &index);
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)
} else { return -EINVAL; }return -EINVAL;
@@ -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,
};
/* ------------------------------------------------------------------------
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 Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- 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 8f3199b06d1f..07c1eff77e7d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12895,6 +12895,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 61e715ddd0d5..eaea10a7a600 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2157,10 +2157,13 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon); #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 849e1b69ba73..03025c50cb38 100644 --- a/drivers/gpu/drm/i915/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c @@ -468,6 +468,114 @@ void intel_display_crc_init(struct drm_i915_private *dev_priv) } }
+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) {
This patch implements get_crc_sources callback, which returns list of all the valid crc sources supported by driver in current platform.
Changes since V1: - Return array of crc sources
Signed-off-by: Mahesh Kumar mahesh1.kumar@intel.com Cc: dri-devel@lists.freedesktop.org Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +++ drivers/gpu/drm/i915/intel_pipe_crc.c | 7 +++++++ 3 files changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 07c1eff77e7d..4f2a75f9c15d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12896,6 +12896,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 eaea10a7a600..29b054de9e13 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2159,11 +2159,14 @@ 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); +const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc, + size_t *count); 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 03025c50cb38..83f9ade0cd81 100644 --- a/drivers/gpu/drm/i915/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c @@ -556,6 +556,13 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv, return ivb_crc_source_valid(dev_priv, source); }
+const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc, + size_t *count) +{ + *count = ARRAY_SIZE(pipe_crc_sources); + return pipe_crc_sources; +} + int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name, size_t *values_cnt) {
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 --- 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: *
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) {
return ret;kfree(entries);
- }
- 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:
This reverts commit e8fa5671183c80342d520ad81d14fa79a9d4a680.
Don't wait for first CRC during crtc_crc_open. It avoids one frame wait during open. If application want to wait after read call, it can use poll/read blocking read() call.
Suggested-by: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Mahesh Kumar mahesh1.kumar@intel.com Cc: dri-devel@lists.freedesktop.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/drm_debugfs_crc.c | 16 ---------------- 1 file changed, 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c index 7386391636e3..973b3968ad17 100644 --- a/drivers/gpu/drm/drm_debugfs_crc.c +++ b/drivers/gpu/drm/drm_debugfs_crc.c @@ -228,24 +228,8 @@ static int crtc_crc_open(struct inode *inode, struct file *filep) if (ret) goto err;
- spin_lock_irq(&crc->lock); - /* - * Only return once we got a first frame, so userspace doesn't have to - * guess when this particular piece of HW will be ready to start - * generating CRCs. - */ - ret = wait_event_interruptible_lock_irq(crc->wq, - crtc_crc_data_count(crc), - crc->lock); - spin_unlock_irq(&crc->lock); - - if (ret) - goto err_disable; - return 0;
-err_disable: - crtc->funcs->set_crc_source(crtc, NULL); err: spin_lock_irq(&crc->lock); crtc_crc_cleanup(crc);
Hi Mahesh,
Thank you for the patches.
When resubmitting patch series, could you please add a version number to the [PATCH] prefix ? Otherwise it gets difficult to figure out which version is the latest. This can be done automatically with the -v argument to git-format- patch.
On Thursday, 12 July 2018 11:36:25 EEST 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
Cc: dri-devel@lists.freedesktop.org
Mahesh Kumar (10): drm: crc: Introduce verify_crc_source callback 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()" drm/rcar-du/crc: Implement get_crc_sources callback
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 | 92 +++++++++------- drivers/gpu/drm/i915/intel_display.c | 2 + drivers/gpu/drm/i915/intel_drv.h | 9 +- drivers/gpu/drm/i915/intel_pipe_crc.c | 119 +++++++++++++++++- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 82 +++++++++++--- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 + drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 67 ++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_kms.h | 1 + drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 26 ++++- include/drm/drm_crtc.h | 40 ++++++- 14 files changed, 396 insertions(+), 74 deletions(-)
Hi,
thanks for the review.
On 7/12/2018 4:38 PM, Laurent Pinchart wrote:
Hi Mahesh,
Thank you for the patches.
When resubmitting patch series, could you please add a version number to the [PATCH] prefix ? Otherwise it gets difficult to figure out which version is the latest. This can be done automatically with the -v argument to git-format- patch.
sure :), added patch prefix (now v4) and re-floated the series after addressing review comments in patches 03/10, 08/10 & 10/10.
-Mahesh
On Thursday, 12 July 2018 11:36:25 EEST 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
Cc: dri-devel@lists.freedesktop.org
Mahesh Kumar (10): drm: crc: Introduce verify_crc_source callback 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()" drm/rcar-du/crc: Implement get_crc_sources callback
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 | 92 +++++++++------- drivers/gpu/drm/i915/intel_display.c | 2 + drivers/gpu/drm/i915/intel_drv.h | 9 +- drivers/gpu/drm/i915/intel_pipe_crc.c | 119 +++++++++++++++++- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 82 +++++++++++--- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 + drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 + drivers/gpu/drm/rcar-du/rcar_du_kms.c | 67 ++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_kms.h | 1 + drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 26 ++++- include/drm/drm_crtc.h | 40 ++++++- 14 files changed, 396 insertions(+), 74 deletions(-)
dri-devel@lists.freedesktop.org