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@xxxxxxxxxxxxxxxx>
I don't think I have any actual blocking questions here, so feel free to add a
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
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, ©->state);
> +
> + return ©->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:
signature.asc
Description: OpenPGP digital signature
