[Intel-gfx] [PATCH 05/10] drm/rcar-du/crc: Implement verify_crc_source callback

[Kumar, Mahesh]

Hi,

thanks foe the review.

On 7/10/2018 5:07 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> Thank you for the patch.
>
> On Monday, 2 July 2018 14:07:24 EEST Mahesh Kumar wrote:
>> This patch implements "verify_crc_source" callback function for
>> rcar drm driver.
>>
>> 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/rcar-du/rcar_du_crtc.c | 40 +++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 15dc9caa128b..24eeaa7e14d7
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -756,6 +756,45 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc
>> *crtc) rcrtc->vblank_enable = false;
>>   }
>>
>> +static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
>> +					  const char *source_name,
>> +					  size_t *values_cnt)
>> +{
>> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>> +	unsigned int index = 0;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	/*
>> +	 * Parse the source name. Supported values are "plane%u" to compute the
>> +	 * CRC on an input plane (%u is the plane ID), and "auto" to compute the
>> +	 * CRC on the composer (VSP) output.
>> +	 */
>> +	if (!source_name || !strcmp(source_name, "auto")) {
>> +		goto out;
>> +	} else if (strstarts(source_name, "plane")) {
>> +		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
>> +			if (index == rcrtc->vsp->planes[i].plane.base.id) {
>> +				index = i;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (i >= rcrtc->vsp->num_planes)
>> +			return -EINVAL;
>> +	} else {
>> +		return -EINVAL;
>> +	}
>> +
>> +out:
>> +	*values_cnt = 1;
>> +	return 0;
> This duplicates lots of code from the rcar_du_crtc_set_crc_source() function.
> Could you please extract it to a shared function ?
Agree, it duplicates the code but "index" is needed by set_crc_source call
anyway will create a wrapper to avoid duplication of code.
>
> Could you please also implement support for the .get_crc_sources() operation ?
> I think doing so might show limitations in the current API, namely the fact
> that the list will need to be dynamically created for this driver.
for that I think rcar_du_crtc_create function can build a dynamic list 
during initializing crtc functions, unless plane can be dynamically 
allocated to any CRTC. this is the place where I need input from maintainer.

-Mahesh
>
>> +}
>> +
>>   static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>>   				       const char *source_name,
>>   				       size_t *values_cnt)
>> @@ -861,6 +900,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
>>   	.enable_vblank = rcar_du_crtc_enable_vblank,
>>   	.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,
>>   };
>>
>>   /* ------------------------------------------------------------------------