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+rene...@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+rene...@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, ©->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); > } >
signature.asc
Description: OpenPGP digital signature