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

[Kumar, Mahesh]

Hi,


On 7/10/2018 5:39 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> On Tuesday, 10 July 2018 15:01:38 EEST Kumar, Mahesh wrote:
>> On 7/10/2018 4:52 PM, Laurent Pinchart wrote:
>>> Hi Mahesh,
>>> 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
> I had missed that, my bad.
>
> The proposed format seems a bit hackish to me, in the sense that it forbids
> both spaces and brackets in source names. One source per line would fix both
> and be easy to parse. We would then need to mark the active source, which
> could be done by adding a marker to the corresponding line (maybe a * at the
> end of the line ?).
sounds good, will do that.
-Mahesh
>
>>>> +	}
>>>>
>>>> +out:
>>>>    	seq_printf(m, "%s\n", crtc->crc.source);
>>>> -
>>>>    	return 0;
>>>>    }
> [snip]
>