Re: [PATCH 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 Thursday, 12 July 2018 11:36:35 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.
> 
> 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 | 11 ++++++
>  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 +
>  5 files changed, 83 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 72db1834dd50..8da064daf9c9
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -807,6 +807,16 @@ 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);
> +	struct rcar_du_device *rcdu = rcrtc->group->dev;
> +
> +	*count = rcdu->sources_count;
> +	return (const char * const*)rcdu->sources;
> +}

The list of sources has to be stored per-CRTC, as in most cases planes are not 
shared between CRTCs on Gen3 hardware where CRC computation is available. On 
Gen2 hardware planes are shared between CRTCs (but in groups of two CRTCs), 
but CRC computation isn't available, so you shouldn't compute the list of 
sources in that case.

>  static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>  				       const char *source_name)
>  {
> @@ -898,6 +908,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,
>  };
> 
>  /* ------------------------------------------------------------------------
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6cb0e53..b2c9b7e862d5
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -401,6 +401,8 @@ static int rcar_du_remove(struct platform_device *pdev)
> 
>  	drm_dev_unregister(ddev);
> 
> +	rcar_du_crc_sources_list_uninit(rcdu);

As the sources list has to be stored per-CRTC, you can move this to the CRTC 
cleanup handler.

>  	if (rcdu->fbdev)
>  		drm_fbdev_cma_fini(rcdu->fbdev);
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8e07d0..fe0e2ec724b5
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -92,6 +92,8 @@ struct rcar_du_device {
> 
>  	unsigned int dpad0_source;
>  	unsigned int vspd1_sink;
> +	char **sources;

Let's make this const.

> +	size_t sources_count;

It's not a size but a number of elements, I'd use unsigned int.

>  };
> 
>  static inline bool rcar_du_has(struct rcar_du_device *rcdu,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index f0bc7cc0e913..c74ced480c74
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -421,6 +421,70 @@ static int rcar_du_properties_init(struct
> rcar_du_device *rcdu) return 0;
>  }
> 
> +static void rcar_du_crc_sources_list_init(struct rcar_du_device *rcdu)
> +{
> +	struct drm_device *dev = rcdu->ddev;
> +	struct drm_plane *plane;
> +	char **sources;
> +	size_t count;
> +	int i = 0;

i is never negative, you can make it an unsigned int.

> +	/* reserve 1 for "auto" source */
> +	count = 1;
> +
> +	drm_for_each_plane(plane, dev)
> +		count++;
> +
> +	sources = kmalloc_array(count, sizeof(char *), GFP_KERNEL);
> +	if (!sources)
> +		goto fail;
> +
> +	sources[i] = kstrdup("auto", GFP_KERNEL);
> +	if (!sources[i])
> +		goto fail_no_mem;
> +
> +	i++;
> +	drm_for_each_plane(plane, dev) {
> +		char name[16];
> +
> +		sprintf(name, "plane%d", plane->base.id);
> +		sources[i] = kstrdup(name, GFP_KERNEL);
> +		if (!sources[i])
> +			goto fail_no_mem;
> +		i++;
> +	}
> +
> +	rcdu->sources = sources;
> +	rcdu->sources_count = count;
> +	return;
> +
> +fail_no_mem:
> +	while (i > 0) {
> +		i--;
> +		kfree(sources[i]);
> +	}
> +	kfree(sources);
> +fail:
> +	rcdu->sources = NULL;
> +	rcdu->sources_count = 0;
> +}
> +
> +void rcar_du_crc_sources_list_uninit(struct rcar_du_device *rcdu)
> +{
> +	int i = rcdu->sources_count;

i is never negative, you can make it an unsigned int.

> +	if (!rcdu->sources)
> +		return;
> +
> +	while (i > 0) {
> +		i--;
> +		kfree(rcdu->sources[i]);
> +	}

Anything wrong with a classic for loop ?

	for (i = 0; i < rcdu->sources_count; ++i)
		kfree(rcdu->sources[i]);

> +	kfree(rcdu->sources);
> +	rcdu->sources = NULL;
> +	rcdu->sources_count = 0;
> +}
> +
>  static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
>  {
>  	const struct device_node *np = rcdu->dev->of_node;
> @@ -591,6 +655,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  			return ret;
>  	}
> 
> +	/* Initialize CRC-sources list */
> +	rcar_du_crc_sources_list_init(rcdu);
> +
>  	/* Initialize the encoders. */
>  	ret = rcar_du_encoders_init(rcdu);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.h index 07951d5fe38b..29120e6b5666
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> @@ -32,6 +32,7 @@ struct rcar_du_format_info {
>  const struct rcar_du_format_info *rcar_du_format_info(u32 fourcc);
> 
>  int rcar_du_modeset_init(struct rcar_du_device *rcdu);
> +void rcar_du_crc_sources_list_uninit(struct rcar_du_device *rcdu);
> 
>  int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
>  			struct drm_mode_create_dumb *args);

-- 
Regards,

Laurent Pinchart



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