Re: [PATCH v2 8/8] drm: rcar-du: Add support for CRC computation

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9421 bytes --]

Hi Laurent,

On 22/04/18 23:34, Laurent Pinchart wrote:
> Implement CRC computation configuration and reporting through the DRM
> debugfs-based CRC API. The CRC source can be configured to any input
> plane or the pipeline output.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I don't think I have any actual blocking questions here, so feel free to add a

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

I'll not be in distress if the CRC structures remain duplicated (although I see
from your other mail you've considered defining the structure non-anonymously

--
Kieran



> ---
> Changes since v1:
> 
> - Format the source names using plane IDs instead of plane indices
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 156 +++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  19 ++++
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  |   7 ++
>  3 files changed, 176 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index c4420538ec85..d71d709fe3d9 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -691,6 +691,52 @@ static const struct drm_crtc_helper_funcs crtc_helper_funcs = {
>  	.atomic_disable = rcar_du_crtc_atomic_disable,
>  };
>  
> +static struct drm_crtc_state *
> +rcar_du_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
> +{
> +	struct rcar_du_crtc_state *state;
> +	struct rcar_du_crtc_state *copy;
> +
> +	if (WARN_ON(!crtc->state))
> +		return NULL;
> +
> +	state = to_rcar_crtc_state(crtc->state);
> +	copy = kmemdup(state, sizeof(*state), GFP_KERNEL);
> +	if (copy == NULL)
> +		return NULL;
> +
> +	__drm_atomic_helper_crtc_duplicate_state(crtc, &copy->state);
> +
> +	return &copy->state;
> +}
> +
> +static void rcar_du_crtc_atomic_destroy_state(struct drm_crtc *crtc,
> +					      struct drm_crtc_state *state)
> +{
> +	__drm_atomic_helper_crtc_destroy_state(state);
> +	kfree(to_rcar_crtc_state(state));
> +}
> +
> +static void rcar_du_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct rcar_du_crtc_state *state;
> +
> +	if (crtc->state) {
> +		rcar_du_crtc_atomic_destroy_state(crtc, crtc->state);
> +		crtc->state = NULL;
> +	}
> +
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (state == NULL)
> +		return;
> +
> +	state->crc.source = VSP1_DU_CRC_NONE;
> +	state->crc.index = 0;
> +
> +	crtc->state = &state->state;
> +	crtc->state->crtc = crtc;
> +}
> +
>  static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> @@ -710,15 +756,111 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc)
>  	rcrtc->vblank_enable = false;
>  }
>  
> -static const struct drm_crtc_funcs crtc_funcs = {
> -	.reset = drm_atomic_helper_crtc_reset,
> +static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
> +				       const char *source_name,
> +				       size_t *values_cnt)
> +{
> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_atomic_state *state;
> +	enum vsp1_du_crc_source source;
> +	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) {
> +		source = VSP1_DU_CRC_NONE;
> +	} else if (!strcmp(source_name, "auto")) {
> +		source = VSP1_DU_CRC_OUTPUT;
> +	} else if (strstarts(source_name, "plane")) {
> +		source = VSP1_DU_CRC_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;
> +	}
> +
> +	*values_cnt = 1;
> +
> +	/* Perform an atomic commit to set the CRC source. */
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	state = drm_atomic_state_alloc(crtc->dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (!IS_ERR(crtc_state)) {
> +		struct rcar_du_crtc_state *rcrtc_state;
> +
> +		rcrtc_state = to_rcar_crtc_state(crtc_state);
> +		rcrtc_state->crc.source = source;
> +		rcrtc_state->crc.index = index;
> +
> +		ret = drm_atomic_commit(state);

Does this 'cost' a vblank ? (as in - does this action being performed from
userspace have the capability to cause flicker, or loss of frame?)

> +	} else {
> +		ret = PTR_ERR(crtc_state);
> +	}
> +
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(&ctx);
> +		goto retry;

Not knowing what the -EDEADLK represents yet, this isn't an infinite loop
opportunity is it ? (I assume the state_clear(),backoff() clean up and prevent
that.)

> +	}
> +
> +	drm_atomic_state_put(state);
> +
> +unlock:
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return 0;
> +}
> +
> +static const struct drm_crtc_funcs crtc_funcs_gen2 = {
> +	.reset = rcar_du_crtc_reset,
> +	.destroy = drm_crtc_cleanup,
> +	.set_config = drm_atomic_helper_set_config,
> +	.page_flip = drm_atomic_helper_page_flip,
> +	.atomic_duplicate_state = rcar_du_crtc_atomic_duplicate_state,
> +	.atomic_destroy_state = rcar_du_crtc_atomic_destroy_state,
> +	.enable_vblank = rcar_du_crtc_enable_vblank,
> +	.disable_vblank = rcar_du_crtc_disable_vblank,
> +};
> +
> +static const struct drm_crtc_funcs crtc_funcs_gen3 = {
> +	.reset = rcar_du_crtc_reset,
>  	.destroy = drm_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
> -	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +	.atomic_duplicate_state = rcar_du_crtc_atomic_duplicate_state,
> +	.atomic_destroy_state = rcar_du_crtc_atomic_destroy_state,
>  	.enable_vblank = rcar_du_crtc_enable_vblank,
>  	.disable_vblank = rcar_du_crtc_disable_vblank,
> +	.set_crc_source = rcar_du_crtc_set_crc_source,
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -821,8 +963,10 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
>  	else
>  		primary = &rgrp->planes[index % 2].plane;
>  
> -	ret = drm_crtc_init_with_planes(rcdu->ddev, crtc, primary,
> -					NULL, &crtc_funcs, NULL);
> +	ret = drm_crtc_init_with_planes(rcdu->ddev, crtc, primary, NULL,
> +					rcdu->info->gen <= 2 ?
> +					&crtc_funcs_gen2 : &crtc_funcs_gen3,
> +					NULL);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index fdc2bf99bda1..518ee2c60eb8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -21,6 +21,8 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  
> +#include <media/vsp1.h>
> +
>  struct rcar_du_group;
>  struct rcar_du_vsp;
>  
> @@ -69,6 +71,23 @@ struct rcar_du_crtc {
>  
>  #define to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc, crtc)
>  
> +/**
> + * struct rcar_du_crtc_state - Driver-specific CRTC state
> + * @state: base DRM CRTC state
> + * @crc.source: source for CRC calculation
> + * @crc.index: index of the CRC source plane (when crc.source is set to plane)
> + */
> +struct rcar_du_crtc_state {
> +	struct drm_crtc_state state;
> +
> +	struct {
> +		enum vsp1_du_crc_source source;
> +		unsigned int index;
> +	} crc;

Another definition of this structure ... (is this the third?) do we need to
replicate it each time ? (I know it's small ... but I love to keep things DRY)

> +};
> +
> +#define to_rcar_crtc_state(s) container_of(s, struct rcar_du_crtc_state, state)
> +
>  enum rcar_du_output {
>  	RCAR_DU_OUTPUT_DPAD0,
>  	RCAR_DU_OUTPUT_DPAD1,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index bdcec201591f..ce19b883ad16 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -40,6 +40,8 @@ static void rcar_du_vsp_complete(void *private, bool completed, u32 crc)
>  
>  	if (completed)
>  		rcar_du_crtc_finish_page_flip(crtc);
> +
> +	drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc);
>  }
>  
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
> @@ -103,6 +105,11 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>  {
>  	struct vsp1_du_atomic_pipe_config cfg = { { 0, } };
> +	struct rcar_du_crtc_state *state;
> +
> +	state = to_rcar_crtc_state(crtc->crtc.state);
> +	cfg.crc.source = state->crc.source;
> +	cfg.crc.index = state->crc.index;
>  
>  	vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
>  }
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]