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

[Kumar, Mahesh]

Hi,

thanks for the review.
On 7/10/2018 4:52 PM, Laurent Pinchart wrote:
> 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.
ops, will update.
>
>> 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.
agree.
>
>> +
>> +	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.
ok
>
>> +
>> +		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.
ok will update.
>
>> +			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.
ok sure.
>
>> +				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 ?
what about comma separated as I'm putting names inside square-brackets?
>
> Additionally, shouldn't the active source be marked ?
active source is again printed by the code in next few lines. output 
will be of following format.
[space separated list of valid sources] active_source

-Mahesh
>
>> +	}
>>
>> +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: