Hi Kieran,
Thank you for the patch.
On Wednesday 01 Mar 2017 13:12:55 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.
>
> To synchronise the page flip events for userspace, we move the required
> event through the VSP to track the data flow. When the frame is
> completed, the event can be returned back to the originator through the
> registered callback.
>
> 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@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 2 +-
> drivers/media/platform/vsp1/vsp1_drm.c | 42 +++++++++++++++++++++++++--
> drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++++++-
> include/media/vsp1.h | 6 +++-
> 4 files changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b5bfbe50bd87..71e70e1e0881
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -81,7 +81,7 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
>
> void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
> {
> - vsp1_du_atomic_flush(crtc->vsp->vsp);
> + vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
> }
>
> /* Keep the two tables in sync. */
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 8e2aa3f8e52f..743cbce48d0c
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -52,6 +52,40 @@ int vsp1_du_init(struct device *dev)
> EXPORT_SYMBOL_GPL(vsp1_du_init);
>
> /**
> + * vsp1_du_register_callback - Register VSP completion notifier callback
> + *
> + * Allow the DRM framework to register a callback with us to notify the end
> of + * processing each frame. This allows synchronisation for page
> flipping. + *
> + * @dev: the VSP device
> + * @callback: the callback function to notify the DU module
> + * @private: private structure data to pass with the callback
> + *
> + */
> +void vsp1_du_register_callback(struct device *dev,
> + void (*callback)(void *, void *),
> + void *private)
> +{
> + struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> +
> + vsp1->drm->du_complete = callback;
> + vsp1->drm->du_private = private;
> +}
> +EXPORT_SYMBOL_GPL(vsp1_du_register_callback);
As they're not supposed to change at runtime while the display is running, how
about passing the callback and private data pointer to the vsp1_du_setup_lif()
function ? Feel free to create a structure for all the parameters passed to
the function if you think we'll have too much (which would, as a side effect,
made updates to the API easier in the future as changes to the two subsystems
will be easier to decouple).
> +static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
> +{
> + struct vsp1_drm *drm = to_vsp1_drm(pipe);
> +
> + if (drm->du_complete && drm->active_data)
> + drm->du_complete(drm->du_private, drm->active_data);
> +
> + /* The pending frame is now active */
> + drm->active_data = drm->pending_data;
> + drm->pending_data = NULL;
> +}
I would move this function to the "Interrupt Handling" section.
> +/**
> * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
> * @dev: the VSP device
> * @width: output frame width in pixels
> @@ -99,7 +133,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
> width, }
>
> pipe->num_inputs = 0;
> -
> + pipe->frame_end = NULL;
You can drop this if ...
> + vsp1->drm->du_complete = NULL;
> vsp1_dlm_reset(pipe->output->dlm);
> vsp1_device_put(vsp1);
>
> @@ -196,6 +231,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
> width, if (ret < 0)
> return ret;
>
> + pipe->frame_end = vsp1_du_pipeline_frame_end;
> +
... you move this to vsp1_drm_init().
> ret = media_entity_pipeline_start(&pipe->output->entity.subdev.entity,
> &pipe->pipe);
> if (ret < 0) {
> @@ -420,7 +457,7 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1,
> struct vsp1_rwpf *rpf) * vsp1_du_atomic_flush - Commit an atomic update
> * @dev: the VSP device
> */
> -void vsp1_du_atomic_flush(struct device *dev)
> +void vsp1_du_atomic_flush(struct device *dev, void *data)
> {
> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
> @@ -504,6 +541,7 @@ void vsp1_du_atomic_flush(struct device *dev)
>
> vsp1_dl_list_commit(pipe->dl);
> pipe->dl = NULL;
> + vsp1->drm->pending_data = data;
>
> /* Start or stop the pipeline if needed. */
> if (!vsp1->drm->num_inputs && pipe->num_inputs) {
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> b/drivers/media/platform/vsp1/vsp1_drm.h index 9e28ab9254ba..fde19e5948a0
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -33,8 +33,20 @@ struct vsp1_drm {
> struct v4l2_rect compose;
> unsigned int zpos;
> } inputs[VSP1_MAX_RPF];
> +
> + /* Frame syncronisation */
> + void (*du_complete)(void *, void *);
> + void *du_private;
> + void *pending_data;
> + void *active_data;
> };
>
> +static inline struct vsp1_drm *
> +to_vsp1_drm(struct vsp1_pipeline *pipe)
No need for a line split.
> +{
> + 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 458b400373d4..f82fbab01f21 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -20,6 +20,10 @@ struct device;
>
> int vsp1_du_init(struct device *dev);
>
> +void vsp1_du_register_callback(struct device *dev,
> + void (*callback)(void *, void *),
> + void *private);
> +
> int vsp1_du_setup_lif(struct device *dev, unsigned int width,
> unsigned int height);
>
> @@ -36,6 +40,6 @@ struct vsp1_du_atomic_config {
> void vsp1_du_atomic_begin(struct device *dev);
> int vsp1_du_atomic_update(struct device *dev, unsigned int rpf,
> const struct vsp1_du_atomic_config *cfg);
> -void vsp1_du_atomic_flush(struct device *dev);
> +void vsp1_du_atomic_flush(struct device *dev, void *data);
>
> #endif /* __MEDIA_VSP1_H__ */
--
Regards,
Laurent Pinchart
