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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

-- 
Regards,

Laurent Pinchart



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx