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

[Laurent Pinchart]

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

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


-- 
Regards,

Laurent Pinchart