[Intel-gfx] [PATCH 02/10] drm: crc: Introduce get_crc_sources callback

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Monday, 2 July 2018 14:07:21 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 print list of available CRC sources in seq_file privided
> as an input to the callback.

The commit message seems to be outdated, the callback doesn't take a seq_file 
anymore.

> Changes Since V1: (Daniel)
>  - return const pointer to an array of crc sources list
>  - do validation of sources in CRC-core
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
> Cc: dri-devel at lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++-
>  include/drm/drm_crtc.h            | 16 ++++++++++++++++
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -67,9 +67,27 @@
>  static int crc_control_show(struct seq_file *m, void *data)
>  {
>  	struct drm_crtc *crtc = m->private;
> +	size_t count;

Count it only used within the if () {} block, you can declare it there.

> +
> +	if (crtc->funcs->get_crc_sources) {
> +		const char *const *sources = crtc->funcs->get_crc_sources(crtc,
> +									&count);
> +		size_t values_cnt;
> +		int i;

I only takes positive values, it can be an unsigned int.

> +
> +		if (count <= 0 || !sources)

count is a size_t, it can't be negative.

The .get_crc_sources() documentation doesn't clearly specify whether sources 
should always be NULL when count is zero. I advise updating the documentation, 
and possibly updating this check accordingly.

> +			goto out;
> +
> +		seq_puts(m, "[");
> +		for (i = 0; i < count; i++)
> +			if (!crtc->funcs->verify_crc_source(crtc, sources[i],
> +							    &values_cnt))

I assume that you verify sources one by one here to avoid having to create a 
list of sources dynamically in the .get_crc_sources() callback ? If so, I 
think the .get_crc_sources() callback should document that.

You should also document that .verify_crc_source() is required when 
get_crc_sources() is provided.

> +				seq_printf(m, "%s ", sources[i]);
> +		seq_puts(m, "] ");

This assumes that source names can't include a space. Isn't that too 
restrictive ? Shouldn't a different separator be used ? How about one source 
name per line ?

Additionally, shouldn't the active source be marked ?

> +	}
> 
> +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 1a6dcbf91744..ffaec138aeee 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -676,6 +676,22 @@ 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 is optional if the driver does not support exporting of
> +	 * possible CRC sources list. CRC-core does the verification of sources.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * a constant character pointer to the list of all the available CRC
> +	 * sources
> +	 */
> +	const char *const *(*get_crc_sources)(struct drm_crtc *crtc,
> +					      size_t *count);
> 
>  	/**
>  	 * @atomic_print_state:

-- 
Regards,

Laurent Pinchart