Re: [PATCH v4 10/10] drm/rcar-du/crc: Implement get_crc_sources callback

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

Hi Mahesh,

Thank you for the patch.

On Friday, 13 July 2018 16:59:42 EEST Mahesh Kumar wrote:
> This patch implements get_crc_sources callback, which returns list of
> all the crc sources supported by driver in current platform.
> 
> Changes Since V1:
>  - move sources list per-crtc
>  - init sources-list only for gen3
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 96 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  3 ++
>  2 files changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 6a29055a4ab0..bbe417e93fe3
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -691,6 +691,79 @@ static const struct drm_crtc_helper_funcs
> crtc_helper_funcs = { .atomic_disable = rcar_du_crtc_atomic_disable,
>  };
> 
> +static void rcar_du_crtc_crc_sources_list_init(struct rcar_du_crtc *rcrtc)
> +{
> +	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +	struct drm_device *dev = rcrtc->crtc.dev;
> +	struct drm_crtc *crtc = &rcrtc->crtc;
> +	struct drm_plane *plane;
> +	unsigned int count;
> +	const char **sources;
> +	u32 plane_mask;
> +	int i = 0;

i never takes negative values, it can be an unsigned int.

> +	/* CRC available only on Gen3 HW */

Please capitalize sentences and end them with a period in comments to match 
the driver's style. This applies to other locations in this patch.

> +	if (rcdu->info->gen < 3)
> +		goto fail;

You can just return here, sources_count and sources are initialized to 0 when 
the rcar_du_crtc structure is allocated.

> +	drm_for_each_plane(plane, dev) {
> +		if (drm_crtc_mask(crtc) & plane->possible_crtcs) {
> +			count++;
> +			plane_mask |= drm_plane_mask(plane);
> +		}
> +	}

You can instead iterate over the planes of the associated VSP (hardware 
composer).

	/* Reserve 1 for "auto" source. */
	count = rcrtc->vsp->num_planes + 1;

and get rid of plane_mask.

> +	/* reserve 1 for "auto" source */
> +	count += 1;
> +	sources = kmalloc_array(count, sizeof(char *), GFP_KERNEL);

s/sizeof(char *)/sizeof(*sources)/

> +	if (!sources)
> +		goto fail;
> +
> +	sources[i] = kstrdup("auto", GFP_KERNEL);
> +	if (!sources[i])
> +		goto fail_no_mem;
> +
> +	i++;
> +	drm_for_each_plane_mask(plane, dev, plane_mask) {
> +		char name[16];
> +
> +		sprintf(name, "plane%d", plane->base.id);

The ID is an unsigned integer, you should use %u.

> +		sources[i] = kstrdup(name, GFP_KERNEL);
> +		if (!sources[i])
> +			goto fail_no_mem;

As there will be a single error label, you can just name it "error".

> +		i++;
> +	}

You can iterate over the VSP planes here too.

	for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
		struct drm_plane *plane = &rcrtc->vsp->planes[i].plane;
		char name[16];

		sprintf(name, "plane%u", plane->base.id);
		sources[i+1] = kstrdup(name, GFP_KERNEL);
		if (!sources[i+1])
			goto error;
	}

> +	rcrtc->sources = sources;
> +	rcrtc->sources_count = count;
> +	return;
> +
> +fail_no_mem:
> +	while (i > 0) {
> +		i--;
> +		kfree(sources[i]);
> +	}

You'll have to adapt it based on the code above.

> +	kfree(sources);
> +fail:
> +	rcrtc->sources = NULL;
> +	rcrtc->sources_count = 0;
> +}
> +
> +static void rcar_du_crtc_crc_sources_list_uninit(struct rcar_du_crtc
> *rcrtc)
> +{
> +	unsigned int i;
> +
> +	if (!rcrtc->sources)
> +		return;
> +
> +	for (i = 0; i < rcrtc->sources_count; i++)
> +		kfree(rcrtc->sources[i]);
> +	kfree(rcrtc->sources);
> +
> +	rcrtc->sources = NULL;
> +	rcrtc->sources_count = 0;
> +}
> +
>  static struct drm_crtc_state *
>  rcar_du_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
>  {
> @@ -717,6 +790,15 @@ static void rcar_du_crtc_atomic_destroy_state(struct
> drm_crtc *crtc, kfree(to_rcar_crtc_state(state));
>  }
> 
> +static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
> +{
> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +
> +	rcar_du_crtc_crc_sources_list_uninit(rcrtc);
> +
> +	return drm_crtc_cleanup(crtc);
> +}
> +
>  static void rcar_du_crtc_reset(struct drm_crtc *crtc)
>  {
>  	struct rcar_du_crtc_state *state;
> @@ -811,6 +893,15 @@ static int rcar_du_crtc_verify_crc_source(struct
> drm_crtc *crtc, return 0;
>  }
> 
> +const char *const *rcar_du_crtc_get_crc_sources(struct drm_crtc *crtc,
> +						size_t *count)
> +{
> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +
> +	*count = rcrtc->sources_count;
> +	return (const char * const*)rcrtc->sources;

Shouldn't you declare rcrtc->sources as a const char * const * field instead 
of casting it here ?

> +}
> +
>  static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>  				       const char *source_name)
>  {
> @@ -881,7 +972,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen2 = {
> 
>  static const struct drm_crtc_funcs crtc_funcs_gen3 = {
>  	.reset = rcar_du_crtc_reset,
> -	.destroy = drm_crtc_cleanup,
> +	.destroy = rcar_du_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
>  	.atomic_duplicate_state = rcar_du_crtc_atomic_duplicate_state,
> @@ -890,6 +981,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
>  	.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,
> +	.get_crc_sources = rcar_du_crtc_get_crc_sources,
>  };
> 
>  /*
> ---------------------------------------------------------------------------
> -- @@ -1028,5 +1120,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
> unsigned int swindex, return ret;
>  	}
> 
> +	rcar_du_crtc_crc_sources_list_init(rcrtc);
> +
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index 7680cb2636c8..0cd0c1655beb
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -67,6 +67,9 @@ struct rcar_du_crtc {
>  	struct rcar_du_group *group;
>  	struct rcar_du_vsp *vsp;
>  	unsigned int vsp_pipe;
> +
> +	const char **sources;
> +	unsigned int sources_count;
>  };
> 
>  #define to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc, crtc)

-- 
Regards,

Laurent Pinchart



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