Re: [PATCH v2 1/3] v4l: vsp1: extend VSP1 module API to allow DRM callbacks

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

Thank you for the patch.

On Saturday 04 Mar 2017 02:01:17 Kieran Bingham wrote:
> To be able to perform page flips in DRM without flicker we need to be
> able to notify the rcar-du module when the VSP has completed its
> processing.
> 
> We must not have bidirectional dependencies on the two components to
> maintain support for loadable modules, thus we extend the API to allow
> a callback to be registered within the VSP DRM interface.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> ---
> v2:
>  - vsp1_du_setup_lif() uses config structure to set callbacks
>  - vsp1_du_pipeline_frame_end() moved to interrupt section
>  - vsp1_du_pipeline_frame_end registered in vsp1_drm_init()
>    meaning of any NULL values
>  - removed unnecessary 'private data' variables
> 
>  drivers/media/platform/vsp1/vsp1_drm.c | 20 ++++++++++++++++++++
>  drivers/media/platform/vsp1/vsp1_drm.h | 10 ++++++++++
>  include/media/vsp1.h                   |  3 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 7dce55043379..85e5ebca82a5
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -36,6 +36,16 @@ void vsp1_drm_display_start(struct vsp1_device *vsp1)
>  	vsp1_dlm_irq_display_start(vsp1->drm->pipe.output->dlm);
>  }
> 
> +static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
> +{
> +	struct vsp1_drm *drm = to_vsp1_drm(pipe);
> +
> +	if (drm->du_complete && drm->du_pending) {
> +		drm->du_complete(drm->du_private);
> +		drm->du_pending = false;
> +	}
> +}
> +
>  /* ------------------------------------------------------------------------
>   * DU Driver API
>   */
> @@ -95,6 +105,7 @@ int vsp1_du_setup_lif(struct device *dev, const struct
> vsp1_du_lif_config *cfg)
> 		}
> 
>  		pipe->num_inputs = 0;
> +		vsp1->drm->du_complete = NULL;
> 
>  		vsp1_dlm_reset(pipe->output->dlm);
>  		vsp1_device_put(vsp1);
> @@ -196,6 +207,13 @@ int vsp1_du_setup_lif(struct device *dev, const struct
> vsp1_du_lif_config *cfg) if (ret < 0)
>  		return ret;
> 
> +	/*
> +	 * Register a callback to allow us to notify the DRM framework of 
frame
> +	 * completion events.
> +	 */
> +	vsp1->drm->du_complete = cfg->callback;
> +	vsp1->drm->du_private = cfg->callback_data;
> +
>  	ret = media_pipeline_start(&pipe->output->entity.subdev.entity,
>  					  &pipe->pipe);
>  	if (ret < 0) {
> @@ -504,6 +522,7 @@ void vsp1_du_atomic_flush(struct device *dev)
> 
>  	vsp1_dl_list_commit(pipe->dl);
>  	pipe->dl = NULL;
> +	vsp1->drm->du_pending = true;
> 
>  	/* Start or stop the pipeline if needed. */
>  	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
> @@ -597,6 +616,7 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
>  	pipe->lif = &vsp1->lif->entity;
>  	pipe->output = vsp1->wpf[0];
>  	pipe->output->pipe = pipe;
> +	pipe->frame_end = vsp1_du_pipeline_frame_end;
> 
>  	return 0;
>  }
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> b/drivers/media/platform/vsp1/vsp1_drm.h index 9e28ab9254ba..3a53e9a60c73
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -33,8 +33,18 @@ struct vsp1_drm {
>  		struct v4l2_rect compose;
>  		unsigned int zpos;
>  	} inputs[VSP1_MAX_RPF];
> +
> +	/* Frame syncronisation */
> +	void (*du_complete)(void *);
> +	void *du_private;

These fields need to be documented.

> +	bool du_pending;

You can remove the du_pending flag, see my comments in patch 2/3.

>  };
> 
> +static inline struct vsp1_drm *to_vsp1_drm(struct vsp1_pipeline *pipe)
> +{
> +	return container_of(pipe, struct vsp1_drm, pipe);
> +}
> +
>  int vsp1_drm_init(struct vsp1_device *vsp1);
>  void vsp1_drm_cleanup(struct vsp1_device *vsp1);
>  int vsp1_drm_create_links(struct vsp1_device *vsp1);
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index bfc701f04f3f..f6629f19f209 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -23,6 +23,9 @@ int vsp1_du_init(struct device *dev);
>  struct vsp1_du_lif_config {
>  	unsigned int width;
>  	unsigned int height;
> +
> +	void (*callback)(void *);
> +	void *callback_data;

These fields need to be documented too.

>  };
> 
>  int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config
> *cfg);

-- 
Regards,

Laurent Pinchart