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

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@intel.com Cc: dri-devel@lists.freedesktop.org Reviewed-by: Maarten Lankhorst maarten.lankhorst@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, };

  /* ------------------------------------------------------------------------