[PATCH v6 11/18] media: vsp1: drm: Implement writeback support
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Mar 13 11:42:34 UTC 2019
Hi Laurent,
On 13/03/2019 00:05, Laurent Pinchart wrote:
> Extend the vsp1_du_atomic_flush() API with writeback support by adding
> format, pitch and memory addresses of the writeback framebuffer.
> Writeback completion is reported through the existing frame completion
> callback with a new VSP1_DU_STATUS_WRITEBACK status flag.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>
> ---
> drivers/media/platform/vsp1/vsp1_dl.c | 14 ++++++++++++++
> drivers/media/platform/vsp1/vsp1_dl.h | 3 ++-
> drivers/media/platform/vsp1/vsp1_drm.c | 25 ++++++++++++++++++++++++-
> include/media/vsp1.h | 15 +++++++++++++++
> 4 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> index ed7cda4130f2..104b6f514536 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -958,6 +958,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
> *
> * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
> * became active had been queued with the internal notification flag.
> + *
> + * The VSP1_DL_FRAME_END_WRITEBACK flag indicates that the previously active
> + * display list had been queued with the writeback flag.
How does this interact with the possibility of the writeback being
disabled by the WPF in the event of it failing to get a DL.
It's only a small corner case, but will the 'writeback' report back as
though it succeeded? (without writing to memory, and thus giving an
unmodified buffer back?)
> */
> unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> {
> @@ -995,6 +998,17 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> if (status & VI6_STATUS_FLD_STD(dlm->index))
> goto done;
>
> + /*
> + * If the active display list has the writeback flag set, the frame
> + * completion marks the end of the writeback capture. Return the
> + * VSP1_DL_FRAME_END_WRITEBACK flag and reset the display list's
> + * writeback flag.
> + */
> + if (dlm->active && (dlm->active->flags & VSP1_DL_FRAME_END_WRITEBACK)) {
> + flags |= VSP1_DL_FRAME_END_WRITEBACK;
> + dlm->active->flags &= ~VSP1_DL_FRAME_END_WRITEBACK;
> + }
> +
> /*
> * The device starts processing the queued display list right after the
> * frame end interrupt. The display list thus becomes active.
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
> index e0fdb145e6ed..4d7bcfdc9bd9 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -18,7 +18,8 @@ struct vsp1_dl_list;
> struct vsp1_dl_manager;
>
> #define VSP1_DL_FRAME_END_COMPLETED BIT(0)
> -#define VSP1_DL_FRAME_END_INTERNAL BIT(1)
> +#define VSP1_DL_FRAME_END_WRITEBACK BIT(1)
So below BIT(2) (code above) the flags match the externally exposed
bitfield for the VSP1_DU_STATUS_
While above (code below), are 'private' bitfields.
Should this requirement be documented here somehow? especially the
mapping of FRAME_END_{COMPLETED,WRITEBACK} to
DU_STATUS_{COMPLETED,WRITEBACK}.
> +#define VSP1_DL_FRAME_END_INTERNAL BIT(2)
>
> /**
> * struct vsp1_dl_ext_cmd - Extended Display command
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 0367f88135bf..16826bf184c7 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -37,7 +37,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
>
> if (drm_pipe->du_complete) {
> struct vsp1_entity *uif = drm_pipe->uif;
> - unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
> + unsigned int status = completion
> + & (VSP1_DU_STATUS_COMPLETE |
> + VSP1_DU_STATUS_WRITEBACK);
> u32 crc;
>
> crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> @@ -541,6 +543,8 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
>
> if (drm_pipe->force_brx_release)
> dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
> + if (pipe->output->writeback)
> + dl_flags |= VSP1_DL_FRAME_END_WRITEBACK;
>
> dl = vsp1_dl_list_get(pipe->output->dlm);
> dlb = vsp1_dl_list_get_body0(dl);
> @@ -870,12 +874,31 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> struct vsp1_pipeline *pipe = &drm_pipe->pipe;
> + int ret;
>
> drm_pipe->crc = cfg->crc;
>
> mutex_lock(&vsp1->drm->lock);
> +
> + if (pipe->output->has_writeback && cfg->writeback.pixelformat) {
Is pipe->output->has_writeback necessary here? Can
cfg->writeback.pixelformat be set if pipe->output->has_writeback is false?
Hrm ... actually - Perhaps it is useful. It validates both sides of the
system.
pipe->output->has_writeback is a capability of the VSP, where as
cfg->writeback.pixelformat is a 'request' from the DU.
> + const struct vsp1_du_writeback_config *wb_cfg = &cfg->writeback;
> +
> + ret = vsp1_du_pipeline_set_rwpf_format(vsp1, pipe->output,
> + wb_cfg->pixelformat,
> + wb_cfg->pitch);
> + if (WARN_ON(ret < 0))
> + goto done;
> +
> + pipe->output->mem.addr[0] = wb_cfg->mem[0];
> + pipe->output->mem.addr[1] = wb_cfg->mem[1];
> + pipe->output->mem.addr[2] = wb_cfg->mem[2];
> + pipe->output->writeback = true;
> + }
> +
> vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> vsp1_du_pipeline_configure(pipe);
> +
> +done:
> mutex_unlock(&vsp1->drm->lock);
> }
> EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 877496936487..cc1b0d42ce95 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -18,6 +18,7 @@ struct device;
> int vsp1_du_init(struct device *dev);
>
> #define VSP1_DU_STATUS_COMPLETE BIT(0)
> +#define VSP1_DU_STATUS_WRITEBACK BIT(1)
>
> /**
> * struct vsp1_du_lif_config - VSP LIF configuration
> @@ -83,12 +84,26 @@ struct vsp1_du_crc_config {
> unsigned int index;
> };
>
> +/**
> + * struct vsp1_du_writeback_config - VSP writeback configuration parameters
> + * @pixelformat: plane pixel format (V4L2 4CC)
> + * @pitch: line pitch in bytes for the first plane
> + * @mem: DMA memory address for each plane of the frame buffer
> + */
> +struct vsp1_du_writeback_config {
> + u32 pixelformat;
> + unsigned int pitch;
> + dma_addr_t mem[3];
> +};
> +
> /**
> * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
> * @crc: CRC computation configuration
> + * @writeback: writeback configuration
> */
> struct vsp1_du_atomic_pipe_config {
> struct vsp1_du_crc_config crc;
> + struct vsp1_du_writeback_config writeback;
> };
>
> void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
>
--
Regards
--
Kieran
More information about the dri-devel
mailing list