[05/10] drm/rcar-du/crc: Implement verify_crc_source callback

Submitted by Mahesh Kumar on July 12, 2018, 8:36 a.m.

Details

Message ID 20180712083635.7472-6-mahesh1.kumar@intel.com
State New
Headers show
Series "Improve crc-core driver interface" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Mahesh Kumar July 12, 2018, 8:36 a.m.
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(-)

Patch hide | download patch | download mbox

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,
 };
 
 /* -----------------------------------------------------------------------------

Comments

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);
> +		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,
>  };
> 
>  /* ------------------------------------------------------------------------