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

Commit Message

Kumar, Mahesh July 2, 2018, 11:07 a.m. UTC

This patch implements "verify_crc_source" callback function for
rcar drm driver.

Signed-off-by: Mahesh Kumar <[e-mail]>
Cc: [e-mail]
Reviewed-by: Maarten Lankhorst <[e-mail]>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Comments

Maarten Lankhorst July 3, 2018, 10:20 a.m. UTC | #1

Op 02-07-18 om 13:07 schreef Mahesh Kumar:
> This patch implements "verify_crc_source" callback function for
> rcar drm driver.
>
> Signed-off-by: Mahesh Kumar <[e-mail]>
> Cc: [e-mail]
> Reviewed-by: Maarten Lankhorst <[e-mail]>
> ---
>  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;
> +}
> +
>  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,
>  };
>  
>  /* -----------------------------------------------------------------------------

Ack for merging this and 8/10 through drm-misc?

Laurent Pinchart July 10, 2018, 11:37 a.m. UTC | #2

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 <[e-mail]>
> Cc: [e-mail]
> Reviewed-by: Maarten Lankhorst <[e-mail]>
> ---
>  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 ?

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.

> +}
> +
>  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,
>  };
> 
>  /* ------------------------------------------------------------------------

Kumar, Mahesh July 10, 2018, 12:59 p.m. UTC | #3

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 <[e-mail]>
>> Cc: [e-mail]
>> Reviewed-by: Maarten Lankhorst <[e-mail]>
>> ---
>>   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,
>>   };
>>
>>   /* ------------------------------------------------------------------------

Laurent Pinchart July 11, 2018, 10:45 a.m. UTC | #4

Hi Mahesh,

On Tuesday, 10 July 2018 15:59:11 EEST Kumar, Mahesh wrote:
> On 7/10/2018 5:07 PM, Laurent Pinchart wrote:
> > 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 <[e-mail]>
> >> Cc: [e-mail]
> >> Reviewed-by: Maarten Lankhorst <[e-mail]>
> >> ---
> >> 
> >>   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.

Thank you.

> > 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.

That should indeed work. The DU hardware can, in some SoC models, share planes 
between CRTCs. In that case we'll need to ensure that the source corresponds 
to a plane currently associated with the CRTC. That check is currently missing 
and likely out of scope for this patch series, so don't worry about it.

> >> +}
> >> +
> >>   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,
> >> 
> >>   };
> >>   
> >>   /*
> >>   ----------------------------------------------------------------------
> >>   --

Patch

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;
+}
+
 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,
 };
 
 /* -----------------------------------------------------------------------------